Conversation
WalkthroughThis PR removes MerkleStateManager and its tests, updates ZeroXBridgeL1 to emit leavesCount in DepositEvent and to return leavesCount from appendDepositHash, and expands ZeroXBridgeL1 tests to cover unlock flows (ETH/USDC). It also deletes an unused ZeroXBridgeProof test suite. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ZeroXBridgeL1 as ZeroXBridgeL1
participant MMR as Merkle/MMR Append Logic
rect rgba(230,245,255,0.6)
note over User,ZeroXBridgeL1: Deposit flow
User->>ZeroXBridgeL1: deposit(asset, amount, ...)
ZeroXBridgeL1->>MMR: appendDepositHash(commitment)
MMR-->>ZeroXBridgeL1: newRoot, leavesCount, elementCount
ZeroXBridgeL1-->>User: emit DepositEvent(..., commitmentHash, leavesCount, newRoot, elementCount)
end
sequenceDiagram
autonumber
actor Relayer
participant ZeroXBridgeL1 as ZeroXBridgeL1
participant Token as ERC20/ETH Reserve
rect rgba(240,255,240,0.6)
note over Relayer,ZeroXBridgeL1: Unlock with proof (tests exercise path)
Relayer->>ZeroXBridgeL1: unlockFundsWithProof(proof, sig, params)
alt Proof valid
ZeroXBridgeL1->>Token: transfer/unlock to recipient
ZeroXBridgeL1-->>Relayer: emit FundsUnlocked(...)
else Proof invalid
ZeroXBridgeL1-->>Relayer: revert
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/L1/src/ZeroXBridgeL1.sol (4)
235-243: Require the target ‘user’ to be registered before minting a commitmentdepositAsset builds the commitment with userRecord[user] but does not require that user has registered. Deposits to an unregistered user will commit a zero pubkey and can later burn funds during unlock (see comment on Lines 300–306).
Apply this guard before computing the commitment:
// Get the next nonce for this user uint256 nonce = nextDepositNonce[user]; nextDepositNonce[user] = nonce + 1; + // Ensure the L2 identity is bound for the receiver + require(userRecord[user] != 0, "ZeroXBridge: User not registered");
295-306: Replay-protection keyed by root blocks multi-claim batches; also accept-zero risk
- You mark proofs as used by verifiedRoot. In a batch, multiple leaves share the same root; after the first claim, all remaining leaves will revert.
- If the registry returns 0 for an unknown commitment, the current code will set verifiedProofs[0] = true and brick subsequent unlocks, even after a legit root is registered.
Use per-leaf replay protection and reject zero roots.
- // Track verified proofs to prevent replay attacks - mapping(uint256 => bool) public verifiedProofs; + // Track claimed leaves to prevent per-leaf replay attacks + mapping(bytes32 => bool) public claimedLeaves;- uint256 verifiedRoot = proofRegistry.getVerifiedMerkleRoot(commitmentHash); - // assert root hasn't been used - require(!verifiedProofs[verifiedRoot], "ZeroXBridge: Proof has already been used"); + uint256 verifiedRoot = proofRegistry.getVerifiedMerkleRoot(commitmentHash); + require(verifiedRoot != 0, "ZeroXBridge: Unverified merkle root"); + bytes32 leaf = bytes32(commitmentHash); + require(!claimedLeaves[leaf], "ZeroXBridge: Leaf already claimed");- // Store the proof hash to prevent replay attacks - verifiedProofs[verifiedRoot] = true; + // Mark leaf as claimed + claimedLeaves[leaf] = true;If you still need root-level idempotency, keep a separate mapping for finalized batches, but don’t conflate the two.
300-306: Avoid burning funds when Starknet pubkey is unknownstarkPubKeyRecord[starknetPubKey] can be address(0) if the user never registered. Sending ETH/ERC20 to address(0) either burns funds or locks them forever.
Add a guard:
// get user address from starknet pubkey address user = starkPubKeyRecord[starknetPubKey]; + require(user != address(0), "ZeroXBridge: Unknown Starknet pub key");Also consider requiring registration at deposit time (see Lines 235–243).
278-280: Re-enable relayer gating before productionThe call to
unlockFundsWithProofcurrently has the relayer check commented out, leaving it open to any caller and significantly expanding the attack surface. Before deploying to production, restore this authorization guard.• File: contracts/L1/src/ZeroXBridgeL1.sol
• Location:unlockFundsWithProof(around lines 277–281)- // require(approvedRelayers[msg.sender], "ZeroXBridge: Only approved relayers can submit proofs"); + require(approvedRelayers[msg.sender], "ZeroXBridge: Only approved relayers can submit proofs");If your test suite relies on permissionless access, wrap this check in a test‐only hook or enable it via a constructor flag.
♻️ Duplicate comments (1)
contracts/L1/test/ZeroXBridgeL1.t.sol (1)
1006-1009: USDC unlock test shares the same signature issueSame fix as above; also ensure you register the proof and pass the exact commitment used to sign.
🧹 Nitpick comments (10)
contracts/L1/src/ZeroXBridgeL1.sol (5)
75-85: Event schema change adds leavesCount; ensure indexers and UIs are updatedAdding leavesCount and reordering fields is a breaking change for off-chain consumers. Confirm downstream indexers (The Graph, log parsers, analytics) are updated to the new ABI and argument order. Also consider whether commitmentHash or depositId should be indexed to improve queryability (you still have one free indexed slot).
247-258: Event emission aligns with new ABI; minor nits
- Casting bytes32 commitmentHash to uint256 is fine, but consider keeping bytes32 in the event to avoid downstream casting assumptions.
- If you keep uint256, document endianness for off-chain decoders.
311-314: Amount reconstruction formula is correct; use full-precision math and remove TODOThe scaling matches your deposit path: amount = usd * 10^dec / (price * 10^10). Use Math.mulDiv to avoid overflow/precision loss, and delete the lingering TODO.
- // uint256 amount = (usd_amount * (10 ** dec)) / price; - uint256 amount = (usd_amount * 10 ** dec) / (price * 10 ** 10); //TODO Check if Calculation is correct + // amount = usd_amount * 10^dec / (price * 10^10) + uint256 amount = Math.mulDiv(usd_amount, 10 ** dec, price * 10 ** 10);Remember to import OpenZeppelin’s Math.
142-152: Do not assume 8-decimal Chainlink feedsSome feeds use 8 decimals, others differ. Query decimals() and normalize instead of hardcoding 1e8 in price math, and propagate that value to deposit/unlock conversions.
Example:
- (, int256 priceInt,,,) = priceFeed.latestRoundData(); + (, int256 priceInt,,,) = priceFeed.latestRoundData(); + uint8 feedDec = priceFeed.decimals(); require(priceInt > 0, "Invalid price from feed"); - return uint256(priceInt); + return uint256(priceInt); // return alongside feedDec or store per tokenAnd replace divisions by 1e8 with the dynamic 10**feedDec.
316-336: ETH transfer reentrancy is low risk but add a guard for defense-in-depthState is updated before the external call, so you’re safe, but marking unlockFundsWithProof nonReentrant prevents future regressions or complex reentrancy paths when logic evolves.
contracts/L1/test/ZeroXBridgeL1.t.sol (5)
7-11: Duplicate import of OwnableOwnable is imported twice. Remove the duplicate to keep the test tidy.
-import "@openzeppelin/contracts/access/Ownable.sol"; -import "@openzeppelin/contracts/access/Ownable.sol"; +import "@openzeppelin/contracts/access/Ownable.sol";
483-501: Calculate expected leavesCount instead of hardcodingYou left a hint comment but didn’t compute it. If leavesCount is tracked separately from elementCount, tests should read current leaves and add 1 for the next append to avoid brittleness across test order.
- function calculateExpectedMmrValues(bytes32 _commitmentHash) + function calculateExpectedMmrValues(bytes32 _commitmentHash) internal view returns (bytes32 expectedNewRoot, uint256 expectedElementCount) { // ... - (uint256 nextElementsCount, bytes32 nextRoot,) = + (uint256 nextElementsCount, bytes32 nextRoot,) = StatelessMmr.appendWithPeaksRetrieval(_commitmentHash, currentPeaks, currentElementsCount, currentRoot); - - // leavesCount += 1; + return (nextRoot, nextElementsCount); }Then, when emitting expectations below, derive expectedLeavesCount = bridge.getLeavesCount() + 1 (assuming MerkleManager exposes this).
526-538: Don’t hardcode leavesCount = 1If any prior test appends to the MMR, this assertion becomes flaky. Compute expectedLeavesCount from contract state.
- uint256(expectedCommitmentHash), - 1, + uint256(expectedCommitmentHash), + bridge.getLeavesCount() + 1, expectedNewRoot, expectedElementCountIf getLeavesCount() isn’t exposed, either expose it on MerkleManager for tests or parse the event and assert the rest of the fields instead.
572-584: Same issue: hardcoded leavesCount = 1Repeat of the previous comment for ERC20.
- uint256(expectedCommitmentHash), - 1, + uint256(expectedCommitmentHash), + bridge.getLeavesCount() + 1, expectedNewRoot, expectedElementCount
1069-1076: vm.mockCall won’t intercept internal solidity callsverifyStarknetSignature is invoked internally, so vm.mockCall against address(bridge) is ineffective. Use the harness approach above or make the function external and call this.verifyStarknetSignature(...) to enable mocking (test-only).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
contracts/L1/src/MerkleStateManager.sol(0 hunks)contracts/L1/src/ZeroXBridgeL1.sol(2 hunks)contracts/L1/test/MerkleStateManager.t.sol(0 hunks)contracts/L1/test/ZeroXBridgeL1.t.sol(5 hunks)contracts/L1/test/ZeroXBridgeProof.t.sol(0 hunks)
💤 Files with no reviewable changes (3)
- contracts/L1/test/ZeroXBridgeProof.t.sol
- contracts/L1/src/MerkleStateManager.sol
- contracts/L1/test/MerkleStateManager.t.sol
🔇 Additional comments (3)
contracts/L1/src/ZeroXBridgeL1.sol (1)
244-245: Confirm appendDepositHash return orderingYou’re capturing (newRoot, leavesCount, elementCount). Double-check this matches MerkleManager.appendDepositHash’s latest signature; a mismatch in tuple ordering will silently corrupt emitted values and tests relying on them.
contracts/L1/test/ZeroXBridgeL1.t.sol (2)
43-55: Event ABI update mirrored correctly in testsDepositEvent includes leavesCount in the correct position.
98-143: TVL tests now correctly reflect tracked reserves modelThese changes assert that direct transfers don’t affect TVL, which matches the contract’s tokenReserves-based accounting.
| bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash, starknetSig); | ||
|
|
||
| // Step 9: Assertions | ||
| assertEq(bridge.tokenReserves(address(dai)), 0, "tokenReserves should be reduced after unlock"); | ||
| assertEq(dai.balanceOf(user), depositAmount, "User should receive full unlocked amount"); | ||
| } |
There was a problem hiding this comment.
Bug: wrong variable passed as commitmentHash to unlock
You compute commitmentHash_ but pass commitmentHash (from setUp, different format). This will always fail the commitment equality check.
- bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash, starknetSig);
+ bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash_, starknetSig);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash, starknetSig); | |
| // Step 9: Assertions | |
| assertEq(bridge.tokenReserves(address(dai)), 0, "tokenReserves should be reduced after unlock"); | |
| assertEq(dai.balanceOf(user), depositAmount, "User should receive full unlocked amount"); | |
| } | |
| bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash_, starknetSig); | |
| // Step 9: Assertions | |
| assertEq(bridge.tokenReserves(address(dai)), 0, "tokenReserves should be reduced after unlock"); | |
| assertEq(dai.balanceOf(user), depositAmount, "User should receive full unlocked amount"); |
🤖 Prompt for AI Agents
In contracts/L1/test/ZeroXBridgeL1.t.sol around lines 931 to 936, the test calls
bridge.unlockFundsWithProof with commitmentHash (the setup variable in a
different format) but the correct value computed for this unlock is
commitmentHash_; replace the passed commitmentHash with commitmentHash_ so the
proof verification compares the intended computed commitment; ensure
commitmentHash_ is in scope at this call site (or rename/move the computation)
so the test uses the exact computed commitment for the equality check.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests