Fix claim proofs#120
Conversation
WalkthroughSwitches to an external self-call for signature verification in L1 bridge to enable mocking in tests, updates tests to mock signature verification and use a local commitment hash and new mock Merkle root, and adjusts StarkNet signature verification to reduce the message hash modulo EC_ORDER before elliptic curve multiplication. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Test
participant L1 as ZeroXBridgeL1
participant SN as Starknet.verifyStarknetSignature (external self-call)
U->>L1: unlockFundsWithProof(proof, sig, ...)
L1->>SN: verifyStarknetSignature(messageHash, sig, pubKey)
Note over SN: In tests, mocked to return true
SN-->>L1: bool ok
alt ok
L1->>L1: validate commitment hash, verify Merkle root, replay-protect
L1->>U: transfer/unlock funds
else not ok
L1->>U: revert "Invalid signature"
end
sequenceDiagram
autonumber
participant T as Test
participant L1 as ZeroXBridgeL1
Note over T,L1: Starknet.sol change
T->>L1: verifyStarknetSignature(messageHash, sig, pubKey)
activate L1
Note over L1: Compute msgHash = messageHash % EC_ORDER<br/>Use msgHash in EC multiply
L1-->>T: bool result
deactivate L1
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(None) 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
contracts/L1/src/ZeroXBridgeL1.sol (2)
278-279: Do not ship with relayer auth disabled.Re-enable the relayer gate; update tests to call as an approved relayer.
Apply:
- // require(approvedRelayers[msg.sender], "ZeroXBridge: Only approved relayers can submit proofs"); + require(approvedRelayers[msg.sender], "ZeroXBridge: Only approved relayers can submit proofs");
304-306: Guard against unregistered pubkeys burning funds.If starkPubKey isn’t registered, user is address(0) and transfers will burn. Add a check.
// get user address from starknet pubkey address user = starkPubKeyRecord[starknetPubKey]; + require(user != address(0), "ZeroXBridge: Unknown Starknet pubkey");contracts/L1/utils/Starknet.sol (2)
244-245:wrange check can reject valid signatures.
w = s^{-1} mod nis in [1, n-1]. Enforcingw < 2^251may wrongly fail whenw ∈ [2^251, n). Check againstEC_ORDERinstead or drop the check.- require(w >= 1 && w < (1 << N_ELEMENT_BITS_ECDSA), "w out of range"); + require(w >= 1 && w < EC_ORDER, "w out of range");
262-263: Compare X-coordinate modulo group order.ECDSA requires
(X.x mod n) == r. Direct equality may spuriously fail whenX.x ≥ n.- isValid = res_x == r; + isValid = (res_x % EC_ORDER) == r;contracts/L1/test/ZeroXBridgeL1.t.sol (1)
282-287: Bug in helper: signing digest uses the global instead of the param.This can silently mismatch if
_starknetPubKeydiffers.- bytes32 digest = keccak256(abi.encodePacked("UserRegistration", _user, starknetPubKey)); + bytes32 digest = keccak256(abi.encodePacked("UserRegistration", _user, _starknetPubKey));
🧹 Nitpick comments (4)
contracts/L1/src/ZeroXBridgeL1.sol (3)
313-317: Amount back-conversion formula is correct; consider removing TODO and using mulDiv for precision.Current math matches the deposit path: amount = usd_amount * 10**(dec-10) / price. Prefer full-precision mulDiv to minimize rounding.
- // 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-10) / price + uint256 amount = (usd_amount * 10 ** dec) / (price * 10 ** 10);Or, with OZ Math (requires
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";):- uint256 amount = (usd_amount * 10 ** dec) / (price * 10 ** 10); + uint256 amount = Math.mulDiv(usd_amount, 10 ** dec, price * 10 ** 10);
142-149: Price feed hygiene: add stale/zero checks and honor feed decimals.Relying on 8 decimals is brittle. Read
decimals()and enforce freshness.- AggregatorV3Interface priceFeed = AggregatorV3Interface(feedAddress); - (, int256 priceInt,,,) = priceFeed.latestRoundData(); - require(priceInt > 0, "Invalid price from feed"); - // e.g. 1 ETH = 3000.12345678 => 300012345678 - return uint256(priceInt); + AggregatorV3Interface priceFeed = AggregatorV3Interface(feedAddress); + (uint80 roundId, int256 answer,, uint256 updatedAt, uint80 ansRound) = priceFeed.latestRoundData(); + require(answer > 0, "Invalid price from feed"); + require(updatedAt != 0 && ansRound >= roundId, "Stale price feed"); + uint8 feedDec = priceFeed.decimals(); + // Normalize to 8 decimals to match existing maths + if (feedDec > 8) return uint256(answer) / (10 ** (feedDec - 8)); + if (feedDec < 8) return uint256(answer) * (10 ** (8 - feedDec)); + return uint256(answer);
10-10: Drop debug/unused imports.
console.solandusing ECDSA for bytes32;are unused in this contract.-import "forge-std/console.sol"; ... - using ECDSA for bytes32;Also applies to: 17-17
contracts/L1/test/ZeroXBridgeL1.t.sol (1)
1061-1062: Nit: unify mock merkle root literal.Use a single literal across tests to avoid confusion.
- uint256 merkleRoot = uint256(keccak256("mock_merkle")); + uint256 merkleRoot = uint256(keccak256("mock merkle"));(or vice versa; update all tests consistently)
📜 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 ignored due to path filters (1)
contracts/L1/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contracts/L1/src/ZeroXBridgeL1.sol(1 hunks)contracts/L1/test/ZeroXBridgeL1.t.sol(5 hunks)contracts/L1/utils/Starknet.sol(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/L1/utils/Starknet.sol (1)
contracts/L1/scripts/generateSignature.ts (2)
signKeccakHash(12-24)main(26-64)
contracts/L1/test/ZeroXBridgeL1.t.sol (1)
contracts/L1/scripts/generateSignature.ts (1)
main(26-64)
🔇 Additional comments (6)
contracts/L1/src/ZeroXBridgeL1.sol (1)
292-295: External self-call for signature check is fine; enables vm.mockCall.Good change for testability. Minor gas overhead is acceptable.
contracts/L1/utils/Starknet.sol (1)
239-241: Reducing the message hash mod EC_ORDER is the right fix.This prevents out-of-range scalar issues before ecMul and aligns with Stark/ECDSA practice.
Also applies to: 257-257
contracts/L1/test/ZeroXBridgeL1.t.sol (4)
924-931: Mocking the external verify path correctly.Selector/args match the external self-call; this reliably bypasses curve signing in tests.
933-934: Future-proof tests for relayer gate.Once the relayer check is re-enabled, call as an approved relayer.
- bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash_, dummySig); + vm.prank(relayer); + bridge.unlockFundsWithProof(ZeroXBridgeL1.AssetType.ERC20, address(dai), proofdata, commitmentHash_, dummySig);
984-985: USDC claim test updates look good.Commitment hash source, proof registration, and signature mocking align with contract expectations.
Also applies to: 989-989, 997-1004, 1007-1008
1058-1059: ETH claim: good use of commitmentHashLocal and mocked verify; relayer context included.All consistent with the external self-call change.
Also applies to: 1077-1077, 1083-1083
1 participant
Fix L1 Failing Tests: normalize Starknet msgHash, allow signature mocking, align commitment hashes
Tests failed due to:
What Changed
Starknet.sol
messageHash % EC_ORDERand use it in ecMul; keep existing range checks for r/s; validate pubkey as beforeZeroXBridgeL1.sol
this.verifyStarknetSignature(...)so tests canvm.mockCallthe checkZeroXBridgeL1.t.sol
commitmentHashaskeccak256(abi.encodePacked(starknetPubKey, usdValue, nonce, timestamp))foundry.lock
Why
Risks/Compatibility
Acceptance Criteria
Closes #113. Related: #112.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
Notes