Skip to content

Conversation

@yorhodes
Copy link
Member

@yorhodes yorhodes commented Oct 27, 2025

Description

Removes inheritance from AbstractMessageIdAuthorizedIsm to get under maximum bytecode size

Error: some contracts exceed the runtime size limit (EIP-170: 24576 bytes)

$ forge build --sizes --json | jq '.TokenBridgeCctpV1, .TokenBridgeCctpV2'
{
  "runtime_size": 23744,
  "init_size": 25183,
  "runtime_margin": 832,
  "init_margin": 23969
}
{
  "runtime_size": 24293,
  "init_size": 25927,
  "runtime_margin": 283,
  "init_margin": 23225
}

Backward compatibility

Yes

Testing

Unit Tests

Summary by CodeRabbit

  • Refactor

    • Optimized message verification mechanism to use message identifiers instead of full message payloads, improving efficiency.
    • Simplified error messages for unauthorized message transmitter calls.
    • Streamlined domain mapping configurations for cross-chain operations.
  • Bug Fixes

    • Enhanced message delivery validation to prevent duplicate processing.

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2025

⚠️ No Changeset found

Latest commit: 9249e9b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.69%. Comparing base (3fc7383) to head (9249e9b).
⚠️ Report is 1 commits behind head on audit-q3-2025.

Additional details and impacted files
@@                Coverage Diff                 @@
##           audit-q3-2025    #7265       +/-   ##
==================================================
+ Coverage           0.00%   76.69%   +76.69%     
==================================================
  Files                  1      123      +122     
  Lines                 14     2712     +2698     
  Branches               0      252      +252     
==================================================
+ Hits                   0     2080     +2080     
- Misses                14      614      +600     
- Partials               0       18       +18     
Components Coverage Δ
core 87.80% <ø> (∅)
hooks 72.89% <ø> (∅)
isms 80.19% <ø> (∅)
token 87.57% <66.66%> (∅)
middlewares 84.98% <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, how complex is the contract? Where is the meat of it?

@yorhodes
Copy link
Member Author

Damn, how complex is the contract? Where is the meat of it?

What claude thinks:

⏺ Based on my analysis, the majority of the bytecode size in the CCTP contracts comes from:

Main Contributors to Bytecode Size

  1. Triple Inheritance (~40-50% of size)

TokenBridgeCctpBase inherits from three base contracts:

  • TokenRouter → GasRouter → MailboxClient → Router (entire token routing stack)
  • AbstractCcipReadIsm → OwnableUpgradeable + CCIP-read offchain lookup logic
  • AbstractPostDispatchHook → Hook metadata handling and refund logic
  1. Complex Verification Logic (~20-30%)

The verify() function in TokenBridgeCctpBase.sol:249-288 contains:

  • CCTP message parsing and validation
  • Token message validation (_validateTokenMessage)
  • Hook message validation (_validateHookMessage)
  • Attestation handling via messageTransmitter.receiveMessage
  • Multiple conditional branches for different message types
  1. Domain Mapping Infrastructure (~10-15%)
  • Two bidirectional mappings (_hyperlaneDomainMap, _circleDomainMap)
  • Conversion functions with validation
  • addDomain() and addDomains() management functions
  1. Library Usage (~10-15%)
  • TypedMemView operations
  • Message parsing (v1 and v2 formats)
  • TokenMessage formatting
  • CctpMessage/BurnMessage parsing
  • TypeCasts utilities
  1. ERC20 Transfer Logic (~5-10%)
  • SafeERC20 operations
  • Token approval in initialization
  • Custom _transferFromSender and _transferTo overrides

@yorhodes yorhodes marked this pull request as ready for review October 28, 2025 18:11
@yorhodes
Copy link
Member Author

noting that the storage incompatibility is only against unreleased version
this is compatible with the deployed version (as the merge to main will reflect)

diff --unified solidity/base-storage/TokenBridgeCctpV1-layout.tsv solidity/HEAD-storage/TokenBridgeCctpV1-layout.tsv
--- solidity/base-storage/TokenBridgeCctpV1-layout.tsv	2025-10-27 18:30:12.116754367 +0000
+++ solidity/HEAD-storage/TokenBridgeCctpV1-layout.tsv	2025-10-27 18:29:48.177477173 +0000
@@ -13,6 +13,5 @@
 203	0	__MOVABLE_COLLATERAL_GAP
 207	0	_urls
 208	0	_hyperlaneDomainMap
-209	0	verifiedMessages
-210	0	authorizedHook
-211	0	_circleDomainMap
+209	0	_circleDomainMap
+210	0	isVerified
diff --unified solidity/base-storage/TokenBridgeCctpV2-layout.tsv solidity/HEAD-storage/TokenBridgeCctpV2-layout.tsv
--- solidity/base-storage/TokenBridgeCctpV2-layout.tsv	2025-10-27 18:30:08.879679880 +0000
+++ solidity/HEAD-storage/TokenBridgeCctpV2-layout.tsv	2025-10-27 18:29:32.021414404 +0000
@@ -13,6 +13,5 @@
 203	0	__MOVABLE_COLLATERAL_GAP
 207	0	_urls
 208	0	_hyperlaneDomainMap
-209	0	verifiedMessages
-210	0	authorizedHook
-211	0	_circleDomainMap
+209	0	_circleDomainMap
+210	0	isVerified
Detected storage removals in diff. Failing job.

Base automatically changed from cctp-patch to audit-q3-2025 October 28, 2025 18:53
@yorhodes yorhodes merged commit 0faa783 into audit-q3-2025 Oct 28, 2025
32 of 49 checks passed
@yorhodes yorhodes deleted the cctp-bytecode-size branch October 28, 2025 18:58
@github-project-automation github-project-automation bot moved this from In Review to Done in Hyperlane Tasks Oct 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

This change peels back layers of abstraction by inlining TokenBridgeCctpBase and removing AbstractMessageIdAuthorizedIsm coupling. It introduces direct domain mappings, replaces message-based verification with messageId-based tracking, and adds sender authorization checks at the _receiveMessageId level instead of through inheritance hooks.

Changes

Cohort / File(s) Summary
Message Verification Logic Refactor
solidity/contracts/token/TokenBridgeCctpBase.sol
Removed AbstractMessageIdAuthorizedIsm dependency; added public isVerified mapping keyed by messageId; introduced _hyperlaneDomainMap and _circleDomainMap for direct domain lookups; updated verify() signature to external with bool return; replaced pre-verification with post-reception messageId marking; added sender authorization check in _receiveMessageId requiring caller to be messageTransmitter.
Test Verification Updates
solidity/test/token/TokenBridgeCctp.t.sol
Updated verification assertions to check isVerified[messageId] instead of raw message payloads; changed revert error messages from "AbstractMessageIdAuthorizedIsm: sender is not the hook" to "Not message transmitter"; removed duplicate message delivery tests and related assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas requiring extra attention:

  • Verification flow changes in _receiveMessageId — ensure messageId is properly set before any subsequent logic depends on it
  • Domain mapping initialization — verify both _hyperlaneDomainMap and _circleDomainMap are correctly populated during setup
  • Test removals (duplicate message delivery tests) — confirm the old behavior is intentionally simplified rather than accidentally lost
  • Sender authorization enforcement — ensure messageTransmitter is the only valid caller and all existing code paths respect this

Possibly related PRs

Suggested reviewers

  • tkporter
  • ltyu

Poem

🧅 Layers stripped, onions simplified,
messageIds now verified,
Domain maps laid bare and plain,
Authorization flows refrained—
Cleaner contracts, less to drain.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cctp-bytecode-size

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3fc7383 and 9249e9b.

📒 Files selected for processing (2)
  • solidity/contracts/token/TokenBridgeCctpBase.sol (4 hunks)
  • solidity/test/token/TokenBridgeCctp.t.sol (9 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants