Open
Conversation
📝 WalkthroughWalkthroughThe changes update a ZkSync message verification snapshot value and modify the ParentToChildProver contract to use a configurable L1 messenger address instead of a hardcoded one, converting the internal function from pure to view to access the new constant. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/contracts/provers/zksync/ParentToChildProver.sol (4)
266-282:⚠️ Potential issue | 🟡 MinorStale
@devon_l2MessageToLog— key is nowbroadcaster, not the message sender.The comment "The message sender is encoded as the key" is no longer accurate; the key is now fixed to
broadcaster.📝 Proposed fix
/// `@dev` Transforms an L2Message into the L2Log format used for Merkle tree hashing. /// Uses fixed values for shard ID (0), service flag (true), and sender address -/// (the ZkSync system contract address). The message sender is encoded as the key, -/// and the message data hash is used as the value. +/// (the L1Messenger system contract). The broadcaster address is encoded as the key, +/// and the message data hash is used as the value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 266 - 282, The NatSpec `@dev` for _l2MessageToLog is stale because the key is no longer the message sender but the fixed broadcaster; update the comment to reflect that the key field is bytes32(uint256(uint160(broadcaster))) and not derived from _message.sender, clarifying that sender remains L1_MESSENGER and value is keccak256(_message.data); edit the `@dev` block in the _l2MessageToLog function to describe these exact mappings (fixed shard ID 0, isService true, sender L1_MESSENGER, key = broadcaster, value = hash of _message.data).
170-175:⚠️ Potential issue | 🟡 MinorStale
@param inputdescription still mentionssenderas anL2Messagefield.Line 174 says the message "contains txNumberInBatch, sender, and data" but
senderis no longer a field ofL2Message.📝 Proposed fix
-/// - message: The L2 message to be verified (contains txNumberInBatch, sender, and data). +/// - message: The L2 message to be verified (contains txNumberInBatch and data).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 170 - 175, The NatSpec for the `input` parameter is stale: it says the `L2Message` contains "txNumberInBatch, sender, and data" but `L2Message` no longer has `sender`. Update the `@param input` description in ParentToChildProver.sol to reflect the actual fields of the `L2Message` struct (refer to the `L2Message` definition) — remove the `sender` mention and list the current fields (e.g., `txNumberInBatch` and `data` or whatever `L2Message` currently contains) so the comment matches the code.
102-116:⚠️ Potential issue | 🟠 MajorMissing zero-address guard for
_broadcaster.If
address(0)is passed,broadcasteris zero,keybecomesbytes32(0), and every call toverifyStorageSlotwill revert withBatchSettlementRootMismatch— no descriptive error is surfaced. A deployed prover would be permanently broken with no on-chain indication of the root cause.🛡️ Proposed fix
constructor( address _gatewayZkChain, uint256 _l2LogsRootHashSlot, uint256 _childChainId, uint256 _gatewayChainId, uint256 _homeChainId, address _broadcaster ) { + require(_broadcaster != address(0), "broadcaster cannot be zero"); gatewayZkChain = IZkChain(_gatewayZkChain); l2LogsRootHashSlot = _l2LogsRootHashSlot; childChainId = _childChainId; gatewayChainId = _gatewayChainId; homeChainId = _homeChainId; broadcaster = _broadcaster; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 102 - 116, The constructor leaves broadcaster writable to the zero address which causes key to be bytes32(0) and makes verifyStorageSlot revert with BatchSettlementRootMismatch; add a zero-address guard in the constructor by requiring _broadcaster != address(0) and revert with a clear message (e.g., "broadcaster cannot be zero") before assigning broadcaster so deployments cannot be initialized in a broken state (update the ParentToChildProver constructor that sets broadcaster and related state).
33-41:⚠️ Potential issue | 🟡 MinorStale
@param senderinL2MessageNatSpec.The
senderfield was removed from the struct but the@param senderdoc comment remains.📝 Proposed fix
/// `@notice` An arbitrary length message passed from L2 to L1. /// `@dev` Under the hood it is an `L2Log` sent from the special system L2 contract. /// `@param` txNumberInBatch The L2 transaction number in a batch, in which the message was sent. -/// `@param` sender The address of the L2 account from which the message was passed. /// `@param` data An arbitrary length message data. struct L2Message { uint16 txNumberInBatch; bytes data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 33 - 41, The NatSpec for the L2Message struct is stale: it still documents a removed `sender` field. Update the comment block above the `struct L2Message` to remove the `@param sender` line and, if needed, adjust the remaining `@param` tags to match the current struct fields (`txNumberInBatch` and `data`) so the documentation accurately reflects the `L2Message` definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/contracts/provers/zksync/ParentToChildProver.sol`:
- Around line 266-282: The NatSpec `@dev` for _l2MessageToLog is stale because the
key is no longer the message sender but the fixed broadcaster; update the
comment to reflect that the key field is bytes32(uint256(uint160(broadcaster)))
and not derived from _message.sender, clarifying that sender remains
L1_MESSENGER and value is keccak256(_message.data); edit the `@dev` block in the
_l2MessageToLog function to describe these exact mappings (fixed shard ID 0,
isService true, sender L1_MESSENGER, key = broadcaster, value = hash of
_message.data).
- Around line 170-175: The NatSpec for the `input` parameter is stale: it says
the `L2Message` contains "txNumberInBatch, sender, and data" but `L2Message` no
longer has `sender`. Update the `@param input` description in
ParentToChildProver.sol to reflect the actual fields of the `L2Message` struct
(refer to the `L2Message` definition) — remove the `sender` mention and list the
current fields (e.g., `txNumberInBatch` and `data` or whatever `L2Message`
currently contains) so the comment matches the code.
- Around line 102-116: The constructor leaves broadcaster writable to the zero
address which causes key to be bytes32(0) and makes verifyStorageSlot revert
with BatchSettlementRootMismatch; add a zero-address guard in the constructor by
requiring _broadcaster != address(0) and revert with a clear message (e.g.,
"broadcaster cannot be zero") before assigning broadcaster so deployments cannot
be initialized in a broken state (update the ParentToChildProver constructor
that sets broadcaster and related state).
- Around line 33-41: The NatSpec for the L2Message struct is stale: it still
documents a removed `sender` field. Update the comment block above the `struct
L2Message` to remove the `@param sender` line and, if needed, adjust the
remaining `@param` tags to match the current struct fields (`txNumberInBatch`
and `data`) so the documentation accurately reflects the `L2Message` definition.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/provers/zksync/ParentToChildProver.sol`:
- Around line 106-114: The contract assigns broadcaster = _broadcaster in
ParentToChildProver without guarding against address(0), which lets
_l2MessageToLog produce bytes32(0) keys and break authenticity; add a
zero-address check in the constructor (or initializer) that sets broadcaster —
e.g., require(_broadcaster != address(0)) or revert with your project's custom
error (e.g., ZeroAddressBroadcaster()) — so broadcaster, _broadcaster and the
ParentToChildProver constructor/initializer reject address(0) inputs before
assignment.
Comment on lines
106
to
114
| uint256 _homeChainId, | ||
| address _broadcaster | ||
| ) { | ||
| gatewayZkChain = IZkChain(_gatewayZkChain); | ||
| l2LogsRootHashSlot = _l2LogsRootHashSlot; | ||
| childChainId = _childChainId; | ||
| gatewayChainId = _gatewayChainId; | ||
| homeChainId = _homeChainId; | ||
| broadcaster = _broadcaster; |
There was a problem hiding this comment.
Missing zero-address guard for _broadcaster.
If _broadcaster is address(0) — whether by mistake or misconfiguration — _l2MessageToLog will produce key = bytes32(0) for every log. Any message whose L2 log happens to carry a zero key would pass the Merkle inclusion check without a valid broadcaster, silently breaking message authenticity guarantees.
🛡️ Proposed fix
constructor(
address _gatewayZkChain,
uint256 _l2LogsRootHashSlot,
uint256 _childChainId,
uint256 _gatewayChainId,
uint256 _homeChainId,
address _broadcaster
) {
+ require(_broadcaster != address(0), "broadcaster cannot be zero address");
gatewayZkChain = IZkChain(_gatewayZkChain);
l2LogsRootHashSlot = _l2LogsRootHashSlot;
childChainId = _childChainId;
gatewayChainId = _gatewayChainId;
homeChainId = _homeChainId;
broadcaster = _broadcaster;
}Or, if the project already uses a custom error pattern:
+ /// `@notice` Error thrown when a required address argument is the zero address.
+ error ZeroAddress();
constructor(...) {
+ if (_broadcaster == address(0)) revert ZeroAddress();
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/provers/zksync/ParentToChildProver.sol` around lines 106 - 114,
The contract assigns broadcaster = _broadcaster in ParentToChildProver without
guarding against address(0), which lets _l2MessageToLog produce bytes32(0) keys
and break authenticity; add a zero-address check in the constructor (or
initializer) that sets broadcaster — e.g., require(_broadcaster != address(0))
or revert with your project's custom error (e.g., ZeroAddressBroadcaster()) — so
broadcaster, _broadcaster and the ParentToChildProver constructor/initializer
reject address(0) inputs before assignment.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/contracts/provers/zksync/ParentToChildProver.sol (1)
268-268: Changeinternal viewtointernal pure.
_l2MessageToLogonly reads the constant state variableL1_MESSENGER(line 65), which is inlined at compile-time. In Solidity 0.8, readingconstantvariables does not violate thepuremutability constraint, since constants are not stored in contract state. The function should remainpureto accurately reflect that it performs no state reads.♻️ Proposed fix
- function _l2MessageToLog(L2Message memory _message) internal view returns (L2Log memory) { + function _l2MessageToLog(L2Message memory _message) internal pure returns (L2Log memory) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/provers/zksync/ParentToChildProver.sol` at line 268, The function _l2MessageToLog currently declared as "internal view" should be changed to "internal pure" because it only uses the constant L1_MESSENGER (a compile-time constant) and performs no state reads; update the function signature from internal view to internal pure (ensure _l2MessageToLog and any helpers it calls do not access storage or non-constant state) so the mutability accurately reflects it is a pure function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/contracts/provers/zksync/ParentToChildProver.sol`:
- Line 268: The function _l2MessageToLog currently declared as "internal view"
should be changed to "internal pure" because it only uses the constant
L1_MESSENGER (a compile-time constant) and performs no state reads; update the
function signature from internal view to internal pure (ensure _l2MessageToLog
and any helpers it calls do not access storage or non-constant state) so the
mutability accurately reflects it is a pure function.
pepebndc
approved these changes
Feb 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the ParentToChild contract of the ZKsync Era rollup, several fields of the constructed L2Log rely on values that are effectively constant at the protocol level but are either hardcoded directly or implicitly derived from user input.
Specifically:
The sender field of the L2Log is hardcoded to the address of the L1Messenger contract.
The key field is derived from user input (_message.sender), even though, due to the construction logic in the L1Messenger contract, this value is expected in practice to be constant and effectively equal to the address of the ZKSyncBroadcaster contract.
Encoding protocol-level constants in this way reduces code clarity and obscures the intended semantics of the L2Log structure. In particular, sourcing an effectively constant value from user input may be misleading and increases the risk of confusion or incorrect usage, even if the current execution path constrains the input.
This PR adds the
L1_MESSENGERas a constant but keeps the broadcaster as an user input. This is done like this because although the prover will mostly be used to verify messages sent by the broadcaster, it is intended to be more open and the broadcaster logic is used at the receiver level, not the prover level.Summary by CodeRabbit
New Features
Changes
Tests