fix: prevent CCTP v2 nonce manipulation #7209
Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR replaces nonce/source gating with message‑ID ISM attestations, adds bidirectional circle↔hyperlane domain maps, shifts bridge hooks to non‑returning deposits, makes mocks version‑aware for CCTP V1/V2 parsing/dispatch, and updates tests and docs accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Relayer
participant ISM as TokenBridgeCctpBase (ISM)
participant MT as MessageTransmitter
participant Handler as IMessageHandler / IMessageHandlerV2
Note right of ISM: verify(metadata, hyperlaneMessage) entry
Relayer->>ISM: verify(metadata, hyperlaneMessage)
ISM->>ISM: if isVerified(_hyperlaneMessage) → return true
ISM->>MT: messageTransmitter.receiveMessage(cctpMessage, metadata)
activate MT
MT->>MT: parse CCTP V1/V2, check idempotency, extract (source,sender,body,messageId)
MT->>Handler: handleReceive*(source, sender, body)
activate Handler
Handler->>ISM: _receiveMessageId(circleSource, circleSender, messageId)
ISM->>ISM: domain resolution & _isAuthorized() check
Handler-->>MT: return true
deactivate Handler
MT-->>ISM: return true
deactivate MT
ISM-->>Relayer: return true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
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 (1)
solidity/test/token/TokenBridgeCctp.t.sol (1)
93-95: Default hook set on origin twice — destination likely intended.You set mailboxOrigin’s default hook to igpOrigin and then immediately overwrite it with igpDestination. That smells off; mailboxDestination should get igpDestination.
Apply:
- mailboxOrigin.setDefaultHook(address(igpOrigin)); - mailboxOrigin.setDefaultHook(address(igpDestination)); + mailboxOrigin.setDefaultHook(address(igpOrigin)); + mailboxDestination.setDefaultHook(address(igpDestination));
🧹 Nitpick comments (2)
solidity/contracts/mock/MockCircleMessageTransmitter.sol (1)
80-97: Optional: require handler success to mirror prod behavior.Currently a non‑reverting false from the handler still “burns” the nonce. Consider requiring success to avoid accidental lockout.
Patch:
- if (version == 0) { - // V1: Call handleReceiveMessage - success = IMessageHandler(recipient).handleReceiveMessage( - sourceDomain, - sender, - messageBody - ); - } else { - // V2: Call handleReceiveUnfinalizedMessage - success = IMessageHandlerV2(recipient) - .handleReceiveUnfinalizedMessage( - sourceDomain, - sender, - 1000, // mock finality threshold - messageBody - ); - } + if (version == 0) { + success = IMessageHandler(recipient).handleReceiveMessage( + sourceDomain, sender, messageBody + ); + } else { + success = IMessageHandlerV2(recipient).handleReceiveUnfinalizedMessage( + sourceDomain, sender, 1000, messageBody + ); + } + require(success, "handler returned false");solidity/contracts/token/TokenBridgeCctpV1.sol (1)
83-89: Light auth hint (optional).You rely on AbstractMessageIdAuthorizedIsm inside preVerifyMessage for auth. That’s fine. If you want earlier failure, add a cheap guard here.
Example:
function handleReceiveMessage( uint32 sourceDomain, bytes32 sender, bytes calldata body ) external override returns (bool) { - return _receiveCircleMessage(sourceDomain, sender, body); + // if (_isAuthorized()) { return _receiveCircleMessage(sourceDomain, sender, body); } + // revert("sender is not the hook"); + return _receiveCircleMessage(sourceDomain, sender, body); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
solidity/contracts/mock/MockCircleMessageTransmitter.sol(4 hunks)solidity/contracts/token/TokenBridgeCctpBase.sol(11 hunks)solidity/contracts/token/TokenBridgeCctpV1.sol(1 hunks)solidity/contracts/token/TokenBridgeCctpV2.sol(3 hunks)solidity/test/token/TokenBridgeCctp.t.sol(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
solidity/contracts/token/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Place token bridge contracts (e.g., HypERC20, HypERC20Collateral) under solidity/contracts/token/
Files:
solidity/contracts/token/TokenBridgeCctpV1.solsolidity/contracts/token/TokenBridgeCctpV2.solsolidity/contracts/token/TokenBridgeCctpBase.sol
solidity/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Solidity code with solhint via yarn --cwd solidity lint
Files:
solidity/contracts/token/TokenBridgeCctpV1.solsolidity/contracts/mock/MockCircleMessageTransmitter.solsolidity/contracts/token/TokenBridgeCctpV2.solsolidity/contracts/token/TokenBridgeCctpBase.solsolidity/test/token/TokenBridgeCctp.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (40)
- GitHub Check: infra-test
- GitHub Check: cli-e2e-matrix (warp-init)
- GitHub Check: cli-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-e2e-matrix (warp-send)
- GitHub Check: cli-e2e-matrix (relay)
- GitHub Check: cli-e2e-matrix (core-read)
- GitHub Check: cli-e2e-matrix (warp-bridge-2)
- GitHub Check: cli-e2e-matrix (warp-read)
- GitHub Check: cli-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-e2e-matrix (warp-check)
- GitHub Check: cli-e2e-matrix (warp-extend-config)
- GitHub Check: cli-e2e-matrix (core-init)
- GitHub Check: cli-e2e-matrix (warp-apply)
- GitHub Check: cli-e2e-matrix (warp-bridge-1)
- GitHub Check: cli-e2e-matrix (core-check)
- GitHub Check: cli-e2e-matrix (warp-deploy)
- GitHub Check: cli-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-e2e-matrix (core-apply)
- GitHub Check: cli-e2e-matrix (core-deploy)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: env-test-matrix (mainnet3, inevm, igp)
- GitHub Check: env-test-matrix (mainnet3, inevm, core)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: coverage-run
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: cli-install-test-run
- GitHub Check: yarn-test-run
- GitHub Check: lint-prettier
- GitHub Check: diff-check
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: agent-configs (testnet4)
- GitHub Check: slither
🔇 Additional comments (15)
solidity/test/token/TokenBridgeCctp.t.sol (4)
590-596: Switch to IRelayer.sendMessage looks right.Expectation matches the new dispatch path and message-body encoding of id. No swamp gas here.
626-630: Good: ensure mapping before forging CCTP message.Adding the domain mapping up front prevents brittle fork tests.
883-1010: Solid coverage for direct-delivery path.You prove preVerify via handleReceiveMessage and then verify() short‑circuits on isVerified with empty metadata. Crisp.
1011-1041: Token message replay guard test is on point.Simulating Circle consumption via receiveMessage and asserting verify() reverts hardens the nonce‑reuse edge.
solidity/contracts/token/TokenBridgeCctpV1.sol (2)
74-80: Hook message check is tight and minimal.Comparing embedded circle messageId to hyperlane id is the right onion layer now that sender auth moved to the callback path.
103-114: Bridge call wiring looks correct for V1.depositForBurn args match V1 ordering; no return plumbing needed anymore. Smooth.
solidity/contracts/token/TokenBridgeCctpV2.sol (3)
75-81: Fee inversion formula LGTM.The reverse-fee calc matches the docstring reasoning. Rounding down is acceptable; tests cover it.
129-136: Correct delegation to base on both finalized and unfinalized paths.Ignoring finalityThresholdExecuted at the interface edge is fine for the mock/test flow.
Also applies to: 139-146
153-160: Allow‑any relayer via destinationCaller=0 is intentional.Matches the PR’s “anyone can relay” stance while still authorizing by messageId. All good.
solidity/contracts/token/TokenBridgeCctpBase.sol (6)
142-148: Intentional: token message encodes burnAmount (amount + externalFee).This keeps destination validation aligned with Circle’s minted amount. Just noting for future readers: it’s not the user-facing “amount”.
177-185: Bidirectional domain maps: good.Storing both directions avoids zero‑sentinel ambiguity and makes lookups O(1). Nice and tidy.
241-279: verify() flow is safer now.
- Short‑circuit on isVerified avoids wasting nonces.
- Recipient gating to {tokenMessenger | this} closes odd recipients.
- Final receiveMessage call pushes attestation verification to Circle.
No ogres lurking.
298-301: Auth hook is minimal and correct.Only the messageTransmitter may call preVerify. Clean.
321-337: postDispatch path aligns with new ISM pattern.You gate by dispatched id, send id over CCTP, and refund last. Looks sharp.
1-1: Manual solhint verification required—sandbox environment limitation.The TokenBridgeCctpBase.sol file exists in the correct location and has a proper SPDX header. I ran the bonus check for lingering
sendMessageWithCallerexpectations and found none—those function references only show up in the legitimate interface and mock definitions, which is fine.However, I can't actually execute
yarn --cwd solidity lintin this sandbox (no node_modules environment to run the linter). The coding guidelines explicitly require solhint verification for Solidity files, so this needs to be validated in your CI/CD pipeline or locally before merge. Your typical yarn lint command should catch any issues—just make sure those solhint warnings stay happy.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
solidity/contracts/token/TokenBridgeCctpBase.sol (1)
362-367: Rename _destination to circleDomain for precision.This param is a Circle domain, not Hyperlane. Aligning the name avoids head‑scratching.
🧹 Nitpick comments (9)
solidity/contracts/token/TokenBridgeCctpV1.sol (2)
74-80: Add a tiny guard before slicing the message id.index(0, 32) assumes at least 32 bytes. Add a quick length check to ogre‑proof this.
function _validateHookMessage( bytes calldata hyperlaneMessage, bytes29 cctpMessage ) internal pure override { - bytes32 circleMessageId = cctpMessage._messageBody().index(0, 32); + bytes29 bodyView = cctpMessage._messageBody(); + require(bodyView.len() >= 32, "Invalid hook body length"); + bytes32 circleMessageId = bodyView.index(0, 32); require(circleMessageId == hyperlaneMessage.id(), "Invalid message id"); }
84-94: Gate the handler by MessageTransmitter for clarity (preVerify already enforces it).preVerifyMessage will revert if msg.sender isn’t the transmitter, but an explicit require here makes intent obvious and saves gas on bad calls.
function handleReceiveMessage( uint32 sourceDomain, bytes32 sender, bytes calldata body ) external override returns (bool) { + require(msg.sender == address(messageTransmitter), "Unauthorized caller"); return _receiveMessageId( sourceDomain, sender, abi.decode(body, (bytes32)) ); }solidity/contracts/token/TokenBridgeCctpV2.sol (3)
120-126: Same guard before slicing the message id.Let’s not poke at shorter bodies. Quick length check keeps the swamp calm.
function _validateHookMessage( bytes calldata hyperlaneMessage, bytes29 cctpMessage ) internal pure override { - bytes32 circleMessageId = cctpMessage._getMessageBody().index(0, 32); + bytes29 bodyView = cctpMessage._getMessageBody(); + require(bodyView.len() >= 32, "Invalid hook body length"); + bytes32 circleMessageId = bodyView.index(0, 32); require(circleMessageId == hyperlaneMessage.id(), "Invalid message id"); }
130-141: Gate both handlers by MessageTransmitter.Like V1: add an explicit require to make the contract’s front door rules crystal clear.
function handleReceiveFinalizedMessage( uint32 sourceDomain, bytes32 sender, uint32 /*finalityThresholdExecuted*/, bytes calldata messageBody ) external override returns (bool) { + require(msg.sender == address(messageTransmitter), "Unauthorized caller"); return _receiveMessageId( sourceDomain, sender, abi.decode(messageBody, (bytes32)) ); } function handleReceiveUnfinalizedMessage( uint32 sourceDomain, bytes32 sender, uint32 /*finalityThresholdExecuted*/, bytes calldata messageBody ) external override returns (bool) { + require(msg.sender == address(messageTransmitter), "Unauthorized caller"); return _receiveMessageId( sourceDomain, sender, abi.decode(messageBody, (bytes32)) ); }Also applies to: 145-156
75-81: Round fees up to honor “minimum amount out.”Integer division floors; recipients can end up 1–few wei short. Use ceilDiv to keep promises.
- return (amount * maxFeeBps) / (10_000 - maxFeeBps); + uint256 denom = 10_000 - maxFeeBps; + unchecked { + // ceil((amount * maxFeeBps) / denom) + uint256 num = amount * maxFeeBps; + return (num + denom - 1) / denom; + }Consider a quick test where amount % denom != 0.
solidity/contracts/token/TokenBridgeCctpBase.sol (4)
65-72: Enforce one‑to‑one domain mapping to avoid foot‑guns.Currently, addDomain silently overwrites either side. Add collision checks so pairs stay bijective and mistakes don’t linger.
function addDomain( uint32 _hyperlaneDomain, uint32 _circleDomain ) public onlyOwner { + Domain memory h = _hyperlaneDomainMap[_hyperlaneDomain]; + require(h.hyperlane == 0 && h.circle == 0, "Hyperlane already mapped"); + Domain memory c = _circleDomainMap[_circleDomain]; + require(c.hyperlane == 0 && c.circle == 0, "Circle already mapped"); _hyperlaneDomainMap[_hyperlaneDomain] = Domain( _hyperlaneDomain, _circleDomain ); _circleDomainMap[_circleDomain] = Domain( _hyperlaneDomain, _circleDomain ); emit DomainAdded(_hyperlaneDomain, _circleDomain); }If domain 0 is ever valid, store an extra “isSet” flag instead of sentinel 0s.
Also applies to: 178-186
284-299: Add explicit transmitter gate in _receiveMessageId.preVerify already enforces this via _isAuthorized(), but a direct require gives earlier, cheaper failures and documents intent.
function _receiveMessageId( uint32 circleSource, bytes32 circleSender, bytes32 messageId ) internal returns (bool) { + require(msg.sender == address(messageTransmitter), "Unauthorized caller"); // ensure that the message was sent from the hook on the origin chain uint32 origin = circleDomainToHyperlaneDomain(circleSource); require( _mustHaveRemoteRouter(origin) == circleSender, "Unauthorized circle sender" ); preVerifyMessage(messageId, 0); return true; }
4-5: Drop the duplicate TokenRouter import.One import is plenty — keeps lint lintless.
-import {TokenRouter} from "./libs/TokenRouter.sol"; ... -import {TokenRouter} from "./libs/TokenRouter.sol";Also applies to: 18-19
325-341: Minor naming nit: “ism” here is the remote router.Consider renaming the local variable to remoteRouter for clarity. No behavior change, just fewer onion layers to peel when reading.
- bytes32 ism = _mustHaveRemoteRouter(destination); + bytes32 remoteRouter = _mustHaveRemoteRouter(destination); - _sendMessageIdToIsm(circleDestination, ism, id); + _sendMessageIdToIsm(circleDestination, remoteRouter, id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
solidity/contracts/token/CCTP.md(1 hunks)solidity/contracts/token/TokenBridgeCctpBase.sol(11 hunks)solidity/contracts/token/TokenBridgeCctpV1.sol(1 hunks)solidity/contracts/token/TokenBridgeCctpV2.sol(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- solidity/contracts/token/CCTP.md
🧰 Additional context used
📓 Path-based instructions (2)
solidity/contracts/token/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Place token bridge contracts (e.g., HypERC20, HypERC20Collateral) under solidity/contracts/token/
Files:
solidity/contracts/token/TokenBridgeCctpV1.solsolidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/TokenBridgeCctpV2.sol
solidity/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Solidity code with solhint via yarn --cwd solidity lint
Files:
solidity/contracts/token/TokenBridgeCctpV1.solsolidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/TokenBridgeCctpV2.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (41)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: cli-e2e-matrix (relay)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: cli-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-e2e-matrix (warp-extend-config)
- GitHub Check: env-test-matrix (mainnet3, inevm, igp)
- GitHub Check: env-test-matrix (mainnet3, inevm, core)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: cli-e2e-matrix (warp-send)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: cli-e2e-matrix (core-deploy)
- GitHub Check: cli-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-e2e-matrix (warp-read)
- GitHub Check: cli-e2e-matrix (core-init)
- GitHub Check: cli-e2e-matrix (core-read)
- GitHub Check: cli-e2e-matrix (warp-init)
- GitHub Check: cli-e2e-matrix (warp-check)
- GitHub Check: cli-e2e-matrix (warp-bridge-2)
- GitHub Check: cli-e2e-matrix (warp-deploy)
- GitHub Check: cli-e2e-matrix (warp-apply)
- GitHub Check: cli-e2e-matrix (warp-bridge-1)
- GitHub Check: cli-e2e-matrix (core-check)
- GitHub Check: cli-e2e-matrix (core-apply)
- GitHub Check: yarn-test-run
- GitHub Check: cli-install-test-run
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: coverage-run
- GitHub Check: Cursor Bugbot
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lander-coverage
- GitHub Check: slither
- GitHub Check: lint-prettier
- GitHub Check: yarn-install
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: agent-configs (testnet4)
- GitHub Check: diff-check
🔇 Additional comments (7)
solidity/contracts/token/TokenBridgeCctpV1.sol (2)
101-106: Confirm the V1 send API choice.Swapping sendMessageWithCaller -> sendMessage removes the destinationCaller constraint. That matches CCTP V1’s interface, but just sanity‑check we don’t rely on caller restriction on the destination in V1. If we do, we’ll need an alternate guard downstream.
112-119: V1 depositForBurn arguments look correct.(amount, circleDomain, mintRecipient, burnToken) ordering matches V1. No nits here.
solidity/contracts/token/TokenBridgeCctpV2.sol (2)
158-170: “Allow anyone to relay” — double‑check the threat model.bytes32(0) for caller is intentional per comment. Confirm downstream relies solely on AbstractMessageIdAuthorizedIsm + circle sender/domain checks so open relays can’t grief or escalate. If any environment expects caller pinning, surface a config switch.
172-186: V2 depositForBurn args and flags look consistent.Relay open, fee and finality threaded through. Looks good.
solidity/contracts/token/TokenBridgeCctpBase.sol (3)
20-20: Nice integration of AbstractMessageIdAuthorizedIsm.This closes the nonce‑waste hole and ties preVerify to the transmitter. Clean.
Also applies to: 43-45
145-149: Confirm TokenMessage.amount = burnAmount is intended for V2 as well.Base encodes burnAmount (amount + externalFee). In V2, Circle mints (burnAmount − fee). Your _validateTokenMessage compares TokenMessage.amount to the Circle burn message’s amount; depending on CCTP V2 semantics, that may be burn or mint amount. If Circle’s burn message reports the pre‑fee amount, we’re good. If it reports post‑fee (mint) amount, V2 should encode amount instead or override transferRemote.
Happy to draft a V2-specific override if needed.
242-283: Always calling receiveMessage fixes the reused‑nonce bypass.Short‑circuit on isVerified, then defer to Circle for attestation/nonce checks — this is the right shape. Ogre‑thumbs up.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
solidity/test/token/TokenBridgeCctp.t.sol (4)
426-426: UseassertTrueinstead ofassertfor consistency.The rest of the test suite uses Foundry's assertion helpers like
assertTrue. Stick with that pattern for better error messages and consistency.- assert(recipient.verify(metadata, message)); + assertTrue(recipient.verify(metadata, message));
1006-1036: Consider asserting a specific revert message.The test expects a generic revert when the CCTP nonce is already consumed, but it would be more robust to verify the specific error from Circle's MessageTransmitter (e.g., "Nonce already used" or similar).
If the mock doesn't provide a specific message, consider adding one or at least documenting what the expected Circle behavior is.
1532-1535: Remove commented code or usevm.skip(true)pattern.Since V2 doesn't need this test (nonces aren't validated the same way), either delete the commented lines entirely or use the
vm.skip(true)pattern you're already using for other overridden tests (liketestFork_verify_upgradeat line 1537-1539).- // function test_verify_revertsWhen_invalidNonce() public override { - // vm.skip(true); - // // cannot assert nonce in v2 - // } + function test_verify_revertsWhen_invalidNonce() public override { + vm.skip(true); + // V2 uses hash-based nonces; cannot assert specific nonce values + }
1853-1883: Token message already-delivered test looks good.Like the V1 version, this expects a generic revert when the CCTP nonce is consumed. Same suggestion as before: if possible, assert the specific Circle error message for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
solidity/contracts/token/TokenBridgeCctpBase.sol(11 hunks)solidity/test/token/TokenBridgeCctp.t.sol(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
solidity/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Solidity code with solhint via yarn --cwd solidity lint
Files:
solidity/test/token/TokenBridgeCctp.t.solsolidity/contracts/token/TokenBridgeCctpBase.sol
solidity/contracts/token/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Place token bridge contracts (e.g., HypERC20, HypERC20Collateral) under solidity/contracts/token/
Files:
solidity/contracts/token/TokenBridgeCctpBase.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Cursor Bugbot
- GitHub Check: slither
- GitHub Check: test-rs
- GitHub Check: lint-rs
- GitHub Check: lander-coverage
- GitHub Check: diff-check
🔇 Additional comments (17)
solidity/test/token/TokenBridgeCctp.t.sol (10)
590-606: LGTM! API change correctly reflected.The switch from
sendMessageWithCallertosendMessagematches the updated CCTP interface. Test expectations are spot on.
626-629: Good addition - ensures domain mapping exists.Adding the domain mapping setup here makes the test more robust and aligns with the new bidirectional mapping architecture.
649-649: LGTM! Using bytes32(0) to allow any relayer.Setting destinationCaller to bytes32(0) lets any relayer deliver the message, which is correct for GMP messages where the ISM validates the sender.
691-693: Good documentation of the validation flow change.These comments accurately reflect that sender validation now happens inside the
receiveMessagecallback via_authenticateCircleSender, which is central to the nonce manipulation fix.Also applies to: 819-822
813-813: Correct message ID embedding for hook messages.Encoding the message ID here aligns with the authorized message ID pattern where GMP messages carry their ID in the CCTP body.
878-1004: Excellent test coverage for V1 handleReceiveMessage.These tests comprehensively cover the callback flow: success path, authorization checks (sender and caller), domain configuration, router enrollment, and idempotency. Well structured.
1368-1368: LGTM! V2 correctly uses address(0) for any-relayer delivery.Like the V1 tests, using
address(0).addressToBytes32()fordestinationCallerallows any relayer to deliver GMP messages, which is the intended behavior for V2.Also applies to: 1491-1491
1560-1677: Strong V2 test coverage for handleReceiveFinalizedMessage.The V2 callback tests mirror the V1 structure but correctly use
handleReceiveFinalizedMessagewith thefinalityThresholdparameter. All error paths (unauthorized sender/caller, unconfigured domain, unenrolled router) are covered.
1679-1769: Comprehensive coverage for handleReceiveUnfinalizedMessage.These tests parallel the finalized message tests but exercise the unfinalized path. Good symmetry and coverage of both V2 callback variants.
1771-1851: Good idempotency and post-delivery verification tests.These tests verify that messages can't be delivered twice and that
verify()works correctly after direct delivery via callbacks. The coverage of both finalized (lines 1797-1823) and unfinalized (lines 1825-1851) paths is thorough.solidity/contracts/token/TokenBridgeCctpBase.sol (7)
20-20: Well-structured storage layout and inheritance chain.The intermediate storage contracts and bidirectional domain mappings are thoughtfully designed. Using a struct with both domains avoids ambiguity when domain 0 is involved, and the careful storage slot management prevents upgrade issues.
Also applies to: 39-60, 75-78
152-153: Cleaner _bridgeViaCircle signature.Removing the return value and passing
burnAmountdirectly makes the control flow more explicit. Good refactor.Also applies to: 373-373
185-192: Bidirectional mapping correctly implemented.Populating both
_hyperlaneDomainMapand_circleDomainMapwith the full Domain struct enables efficient lookups in both directions, which is essential for the callback-driven validation flow.
206-225: Domain conversion functions look solid.Using the struct comparison (
domain.hyperlane == _hyperlaneDomain) to verify the domain is configured prevents silent failures when domain 0 is involved. Both conversion directions are properly implemented.
252-289: Core vulnerability fix: always process CCTP messages, check isVerified first.This is the heart of the nonce manipulation fix:
- Line 261: Early return if message already verified prevents bypass
- Line 288: Always calls
receiveMessage()to validate attestations (no more nonce skipping)- Token messages may revert if nonce consumed; GMP messages succeed because ISM is the only valid caller
The flow is now secure and correctly documented.
291-310: Proper authorization and message ID registration.The callback functions are well-protected:
_receiveMessageId(lines 291-306): Validates the circle sender against enrolled routers, then registers the message ID viapreVerifyMessage_isAuthorized(lines 308-310): Ensures only Circle's MessageTransmitter can trigger callbacksThis prevents attackers from directly calling verify() or pre-registering arbitrary message IDs.
332-333: Good documentation of the routing pattern.The comment clearly explains that this mirrors
AbstractMessageIdAuthHook._postDispatchbut leverages the Router table instead of direct hook-ISM coupling. Helpful for understanding the architecture.
Description
Fixes a critical vulnerability where malicious relayers could manipulate Hyperlane messages by reusing already-consumed CCTP nonces.
Vulnerability
Previously,
CctpISM.verify()would skip CCTP verification if a nonce was already used but still returntrue. This allowed attackers to:Mailbox.process()with an arbitrary message + the already-used CCTP attestationSolution
Always process CCTP messages: Remove nonce checks and always call
messageTransmitter.receiveMessage()to validate attestations.Authorized message ID pattern: Inherit from
AbstractMessageIdAuthorizedIsmto prevent directverify()calls that would waste nonces:_receiveCircleMessage()preVerifyMessage()verify()Changes
AbstractMessageIdAuthorizedIsm, bidirectional domain mapping, implement_receiveCircleMessage()callbackBackward Compatibility
Breaking change - requires redeployment of CCTP ISM v1 and v2.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests