fix: prevent token message verifying GMP recipient #7261
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## audit-q3-2025 #7261 +/- ##
=================================================
- Coverage 76.80% 76.77% -0.03%
=================================================
Files 123 123
Lines 2712 2713 +1
Branches 250 251 +1
=================================================
Hits 2083 2083
- Misses 611 612 +1
Partials 18 18
🚀 New features to boost your workflow:
|
|
📝 WalkthroughWalkthroughThis PR extends the internal Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TokenBridgeCctpBase
participant _bridgeViaCircle
Caller->>TokenBridgeCctpBase: transferRemote()
activate TokenBridgeCctpBase
TokenBridgeCctpBase->>_bridgeViaCircle: _bridgeViaCircle(_destination, _recipient, _amount, externalFee)
activate _bridgeViaCircle
_bridgeViaCircle->>_bridgeViaCircle: depositForBurn(..., _maxFee)
deactivate _bridgeViaCircle
deactivate TokenBridgeCctpBase
sequenceDiagram
participant Mailbox
participant TokenBridgeCctpBase as Bridge verify()
participant Validation as Recipient Check
participant Response
Mailbox->>TokenBridgeCctpBase: verify(message)
activate TokenBridgeCctpBase
alt CCTP Message for Token Messenger
TokenBridgeCctpBase->>Validation: Check message.recipientAddress() == address(this)
activate Validation
alt Recipient is valid (this contract)
Validation-->>TokenBridgeCctpBase: ✓ Pass
else Recipient is invalid
Validation-->>TokenBridgeCctpBase: ✗ Revert "Invalid token message recipient"
end
deactivate Validation
end
deactivate TokenBridgeCctpBase
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
solidity/contracts/token/TokenBridgeCctpBase.sol (1)
252-295: Critical security checks are missing from the mitigation.The recipient validation at lines 275-280 is good and addresses part of the vulnerability, but according to the PR objectives, the mitigation should also include:
Origin domain validation: The Hyperlane message's
originDomainshould be validated to ensure it matches the expected source. Currently, there's no check that_hyperlaneMessage.origin()matches the CCTP source domain.Message body length validation: The PR description states we should "check the message body length to ensure no unexpected metadata." The token message body should have a fixed expected length, but there's no validation to prevent extra metadata from being appended.
Without these additional checks, an attacker might still be able to exploit edge cases by:
- Sending messages from unexpected origin domains
- Appending malicious metadata to the message body
Apply these additional validations:
// check if CCTP message is a USDC burn message if (circleRecipient == address(tokenMessenger)) { // prevent hyperlane message recipient configured with CCTP ISM // from verifying and handling token messages require( _hyperlaneMessage.recipientAddress() == address(this), "Invalid token message recipient" ); + + // validate origin domain matches CCTP source + uint32 expectedOrigin = circleDomainToHyperlaneDomain( + cctpMessage._sourceDomain() + ); + require( + _hyperlaneMessage.origin() == expectedOrigin, + "Invalid token message origin" + ); + + // validate message body length to prevent unexpected metadata + bytes calldata messageBody = _hyperlaneMessage.body(); + require( + messageBody.length == TokenMessage.FORMAT_LENGTH, + "Invalid token message body length" + ); + _validateTokenMessage(_hyperlaneMessage, cctpMessage); }Note: You'll need to verify that
TokenMessage.FORMAT_LENGTHis defined or calculate the expected length based on the TokenMessage format (typically 32 bytes recipient + 32 bytes amount + any metadata length).
🧹 Nitpick comments (1)
solidity/contracts/token/TokenBridgeCctpV2.sol (1)
172-187: Parameter naming could be clearer.The
_maxFeeparameter name might be a bit confusing here since it's receiving the actual external fee amount (fromexternalFeein the base contract), not a maximum fee or basis points value. In CCTP contexts, "maxFee" often refers to basis points or a cap, but this is the calculated fee amount being passed through.Consider whether
_feeAmountor_externalFeewould make the intent clearer, or add a comment explaining that this is the actual fee amount calculated by_externalFeeAmount().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
solidity/contracts/token/TokenBridgeCctpBase.sol(3 hunks)solidity/contracts/token/TokenBridgeCctpV1.sol(1 hunks)solidity/contracts/token/TokenBridgeCctpV2.sol(1 hunks)solidity/test/token/TokenBridgeCctp.t.sol(6 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/TokenBridgeCctpV2.solsolidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/TokenBridgeCctpV1.sol
solidity/**/*.sol
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Solidity code with solhint via yarn --cwd solidity lint
Files:
solidity/contracts/token/TokenBridgeCctpV2.solsolidity/contracts/token/TokenBridgeCctpBase.solsolidity/contracts/token/TokenBridgeCctpV1.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: env-test-matrix (mainnet3, inevm, core)
- GitHub Check: env-test-matrix (mainnet3, inevm, igp)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: cli-e2e-matrix (warp-extend-config)
- GitHub Check: cli-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-e2e-matrix (warp-init)
- GitHub Check: cli-e2e-matrix (warp-read)
- GitHub Check: cli-e2e-matrix (core-deploy)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: cli-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-e2e-matrix (warp-send)
- GitHub Check: cli-e2e-matrix (warp-check)
- GitHub Check: cli-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-e2e-matrix (warp-deploy)
- GitHub Check: cli-e2e-matrix (relay)
- GitHub Check: cli-e2e-matrix (warp-bridge-2)
- 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-read)
- GitHub Check: cli-e2e-matrix (core-apply)
- GitHub Check: cli-e2e-matrix (core-init)
- GitHub Check: coverage-run
- GitHub Check: yarn-test-run
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: cli-install-test-run
- GitHub Check: diff-check
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: test-rs
- GitHub Check: lint-prettier
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: agent-configs (testnet4)
- GitHub Check: slither
🔇 Additional comments (8)
solidity/contracts/token/TokenBridgeCctpBase.sol (2)
134-165: Good implementation of fee handling.The change correctly passes the calculated
externalFeeto_bridgeViaCircle, allowing CCTP V2 to know exactly how much fee is being charged. TheburnAmountincludes both the desired recipient amount and the fee, which aligns with the "minimum amount out" approach described in the V2 contract comments.
375-380: Signature update looks good.The addition of the
_maxFeeparameter to the internal virtual function is consistent with the changes in V1 and V2 implementations. V1 ignores it (with a comment explaining it's not used), while V2 uses it for the fee parameter.solidity/contracts/token/TokenBridgeCctpV1.sol (1)
108-120: Compatibility parameter handled correctly.The unused
_maxFeeparameter is properly added for signature compatibility with the base contract. The comment clearly indicates it's not used for CCTP V1, and the V1depositForBurncall remains unchanged, which is the correct approach.solidity/test/token/TokenBridgeCctp.t.sol (5)
27-27: Import needed for new test.The TokenMessage import is necessary for the new test that validates invalid token message recipients.
726-727: Domain configuration correctly added.After the upgrade, the domain mapping needs to be configured for the test to work properly. The addition of
ism.addDomain(origin, 6)ensures the upgraded contract has the necessary domain mappings.
1315-1315: Test assertion correctly updated.The change from
maxFeetofastFeecorrectly reflects that the deposit expects the calculated fee amount, not the fee basis points constant.
1445-1445: Test expectation correctly updated.The
fastFeevariable represents the calculated external fee amount, which is what gets passed todepositForBurnin the V2 implementation.
1493-1493: Test assertion matches implementation.Using
fastFeehere is correct since it represents the actual fee amount calculated for this specific transfer.
|
patch coverage is wrong |
Description
Introduced constraint on hyperlane message recipient for CCTP token messages. The metadata (nonce), originDomain, and message body length checks are excluded as noted the 1:1 pairing is unenforceable.
Drive-by changes
Backward compatibility
Yes
Testing
Unit Tests
Summary by CodeRabbit
Bug Fixes
Tests