ERC-7786 based crosschain bridge for ERC-20 tokens #5914
Conversation
Co-authored-by: Ernesto García <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
…BridgeERC20Bridgeable and ERC20Crosschain
|
WalkthroughAdds a cross-chain ERC-20 bridging stack built on ERC-7786. Introduces BridgeCore (link management, messaging, authorization), BridgeERC20 (token-agnostic transfer flow with send/receive events), and two implementations: BridgeERC20Bridgeable (ERC-7802 mint/burn) and BridgeERC20Custodial (custody transfers). Adds ERC20Crosschain extension that combines ERC20 with BridgeERC20 using burn/mint for lock/unlock and allowance via crosschainTransferFrom. Updates ERC20BridgeableMock constructor and adds internal _setBridge. Introduces comprehensive tests for bridges and ERC20Crosschain, plus a helper tweak for account impersonation. Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Reviewer's GuideThis PR introduces a generic ERC-7786 cross-chain messaging framework and builds upon it to implement ERC20 bridging (custodial, bridgeable, and integrated token variants), accompanied by mocks, utilities, and extensive tests, along with minor helper and interface refinements. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Amxx
changed the title
Amxx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
contracts/crosschain/bridges/BridgeERC20Bridgeable.sol (1)
14-16: Consider adding zero-address validation.While the constructor correctly assigns the token reference, consider adding a zero-address check for defense in depth:
constructor(IERC7802 token_) { + require(address(token_) != address(0), "BridgeERC20Bridgeable: zero address"); _token = token_; }This will fail during deployment/testing anyway if a zero address is passed, but an explicit check provides clearer error messaging.
contracts/token/ERC20/extensions/ERC20Crosschain.sol (1)
8-13: Add comprehensive NatSpec documentation for the public function.The public function
crosschainTransferFromlacks full NatSpec documentation. Consider adding@notice,@param, and@returntags to document the function's purpose, parameters, and return value for external integrators.Apply this diff:
+ /** + * @notice Transfer tokens cross-chain using ERC20 allowance. + * @param from The address from which tokens are transferred. + * @param to The cross-chain recipient address (ERC-7930 encoded). + * @param amount The amount of tokens to transfer. + * @return The message ID of the cross-chain transfer. + */ - /// @dev TransferFrom variant of {crosschainTransferFrom}, using ERC20 allowance from the sender to the caller. function crosschainTransferFrom(address from, bytes memory to, uint256 amount) public virtual returns (bytes32) {test/crosschain/BridgeERC20.behavior.js (1)
128-155: LGTM! Consider adding explicit gateway validation.The reconfiguration tests correctly validate link updates, override protection, and invalid gateway rejection.
The test on line 153 expects a revert without reason when an EOA is used as a gateway. This works because the gateway validation likely attempts a contract call that fails on an EOA. However, this is fragile—consider adding explicit contract existence checks or custom errors in the implementation for clearer failure modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
contracts/crosschain/bridges/BridgeCore.sol(1 hunks)contracts/crosschain/bridges/BridgeERC20.sol(1 hunks)contracts/crosschain/bridges/BridgeERC20Bridgeable.sol(1 hunks)contracts/crosschain/bridges/BridgeERC20Custodial.sol(1 hunks)contracts/mocks/token/ERC20BridgeableMock.sol(1 hunks)contracts/token/ERC20/extensions/ERC20Crosschain.sol(1 hunks)test/crosschain/BridgeERC20.behavior.js(1 hunks)test/crosschain/BridgeERC20.test.js(1 hunks)test/helpers/account.js(1 hunks)test/token/ERC20/extensions/ERC20Crosschain.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/token/ERC20/extensions/ERC20Crosschain.test.js (3)
test/crosschain/BridgeERC20.behavior.js (3)
require(1-1)require(2-2)require(3-3)test/crosschain/BridgeERC20.test.js (14)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)chain(11-11)accounts(12-12)gateway(15-15)gatewayAsEOA(16-16)tokenA(19-19)bridgeA(20-20)tokenB(23-23)bridgeB(24-24)test/helpers/account.js (1)
impersonate(7-12)
test/crosschain/BridgeERC20.test.js (4)
test/crosschain/BridgeERC20.behavior.js (3)
require(1-1)require(2-2)require(3-3)test/token/ERC20/extensions/ERC20Crosschain.test.js (14)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)chain(11-11)accounts(12-12)gateway(15-15)gatewayAsEOA(16-16)tokenA(19-19)bridgeA(20-20)tokenB(23-23)bridgeB(24-24)test/crosschain/ERC7786Recipient.test.js (1)
getLocalChain(15-15)test/helpers/account.js (1)
impersonate(7-12)
test/crosschain/BridgeERC20.behavior.js (2)
test/crosschain/BridgeERC20.test.js (6)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)test/token/ERC20/extensions/ERC20Crosschain.test.js (6)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)
⏰ 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). (5)
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: halmos
🔇 Additional comments (36)
test/crosschain/BridgeERC20.test.js (5)
1-8: LGTM!The imports are appropriate for the test setup and all are utilized in the file.
10-36: LGTM!The fixture correctly sets up a two-chain bridge scenario with appropriate token and bridge configurations. The custodial bridge (chain A) uses token custody, while the bridgeable variant (chain B) requires the token to be aware of its bridge via
$_setBridge, which is correctly configured on line 33.
38-41: LGTM!The test suite setup follows Hardhat best practices by using
loadFixturefor efficient test state management.
43-46: LGTM!The token getter test appropriately verifies bridge-token associations before executing the shared behavior tests.
48-48: LGTM!The behavior test is correctly parameterized with
chainAIsCustodial: true, matching the fixture's use of a custodial bridge on chain A.test/helpers/account.js (1)
7-12: LGTM! Enhanced helper flexibility.The updated implementation correctly handles multiple account object shapes (ethers v6 contracts with
.target, wallet objects with.address, and plain address strings), making the helper more robust across different test scenarios.contracts/mocks/token/ERC20BridgeableMock.sol (2)
13-15: LGTM! Improved testability.The refactored constructor delegates to the internal
_setBridgefunction, enabling tests to reconfigure the bridge reference via the exposed$_setBridgehelper.
17-19: LGTM! Clean extraction.The internal
_setBridgefunction correctly encapsulates bridge assignment, improving code organization and test flexibility.contracts/crosschain/bridges/BridgeERC20Bridgeable.sol (3)
1-7: LGTM!The file header and imports are appropriate for the bridge implementation.
8-11: LGTM!The contract is appropriately declared as abstract and the documentation clearly describes its purpose.
18-21: LGTM!The token getter is correctly implemented with appropriate visibility, modifiers, and documentation.
contracts/token/ERC20/extensions/ERC20Crosschain.sol (3)
1-7: LGTM!License, pragma, and imports are appropriate for the cross-chain extension.
15-18: LGTM!The burn-based locking mechanism is correct for the burn/mint bridge model.
20-23: LGTM!The mint-based unlocking mechanism is correct for the burn/mint bridge model.
contracts/crosschain/bridges/BridgeERC20Custodial.sol (3)
1-7: LGTM!License, pragma, and imports are appropriate. SafeERC20 usage ensures safe token transfers.
26-29: LGTM! Ensure users approve the bridge before transfers.The custody lock correctly uses
safeTransferFrom. Users must approve the bridge contract before initiating cross-chain transfers, or the transaction will revert.
31-34: LGTM!The custody release correctly uses
safeTransferto return tokens to users.test/token/ERC20/extensions/ERC20Crosschain.test.js (2)
1-8: LGTM!Imports are correct and follow the established test patterns.
38-44: LGTM!The test structure correctly uses the fixture pattern and reuses comprehensive behavior tests. The default parameters (non-custodial on both chains) match the burn/mint model used by ERC20Crosschain and BridgeERC20Bridgeable.
test/crosschain/BridgeERC20.behavior.js (6)
1-6: LGTM!Imports and test constants are correctly defined.
7-15: LGTM!The behavior function signature and payload encoding helper are correct. The encoding properly handles cross-chain source addresses (ERC-7930) and local destination addresses.
17-26: LGTM!The bridge setup test correctly verifies bidirectional link configuration.
28-73: LGTM!The bidirectional cross-chain transfer test comprehensively validates:
- Token locking (custody or burn) on the source chain
- Cross-chain message relay via the gateway
- Token unlocking (custody release or mint) on the destination chain
- Correct event emissions for both custodial and non-custodial (burn/mint) models
The conditional event assertions based on
chainAIsCustodialandchainBIsCustodialflags correctly handle both bridge variants.
75-126: LGTM!The restriction tests correctly validate:
- Gateway authorization for message relay
- Remote authorization for cross-chain message senders
- Replay protection for message IDs
The tests comprehensively cover access control and security constraints.
158-160: LGTM!The export correctly exposes the shared behavior function for reuse across test files.
contracts/crosschain/bridges/BridgeERC20.sol (4)
20-24: LGTM! Well-structured events.The contract declaration and events are properly designed with appropriate indexed parameters for efficient filtering and tracking of cross-chain transfers.
26-29: LGTM! Clean public entry point.The delegation pattern is straightforward and appropriate for a public API. The bytes-encoded destination enables flexible cross-chain addressing.
65-69: LGTM! Clean abstraction for token-specific logic.The abstract hooks provide clear extension points for concrete implementations to handle token custody, minting, or burning as appropriate for their use case.
31-47: parseV1 handles malformed destinations with a clear custom error (InteroperableAddressParsingError), andformatEvmV1is explicitly documented as EVM-only by design; no changes required.contracts/crosschain/bridges/BridgeCore.sol (7)
20-28: LGTM! Clean data structures.The Link struct and mapping provide appropriate encapsulation for gateway and remote tracking per chain.
30-32: LGTM! Appropriate event and error definitions.The event and error provide clear information for link registration tracking and debugging.
40-44: LGTM! Clean accessor function.The link getter provides appropriate read access to gateway and remote configuration for each chain.
46-59: LGTM! Robust link registration with safety checks.The function correctly validates the gateway interface and provides protection against accidental link overwrites. The supportsAttribute check at line 50 ensures the gateway is a valid ERC-7786 implementation.
61-69: LGTM! Clean message delegation.The function correctly resolves the gateway and remote for the target chain and delegates to the ERC-7786 gateway. Will fail fast if the link is not configured.
71-78: LGTM! Security-critical authorization check.The authorization logic correctly verifies both the gateway instance and the sender address match the registered link, preventing unauthorized message injection.
80-83: LGTM! Clean helper for chain extraction.The function correctly extracts the chain identifier from an interoperable address by parsing and reformatting without the address component.
| /// @inheritdoc ERC7786Recipient | ||
| function _processMessage( | ||
| address /*gateway*/, | ||
| bytes32 receiveId, | ||
| bytes calldata /*sender*/, | ||
| bytes calldata payload | ||
| ) internal virtual override { | ||
| // split payload | ||
| (bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256)); | ||
| address to = address(bytes20(toBinary)); | ||
|
|
||
| _unlock(to, amount); | ||
|
|
||
| emit CrossChainTransferReceived(receiveId, from, to, amount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify address extraction safety.
The payload decoding and address extraction at line 58 using address(bytes20(toBinary)) assumes toBinary contains at least 20 bytes. If the remote bridge sends a shorter or differently formatted address, this could silently produce an incorrect address (zero-padded).
Consider validating that toBinary is exactly 20 bytes for EVM addresses, or document the expected format and ensure remote bridges comply.
🏁 Script executed:
#!/bin/bash
# Description: Verify how addresses are encoded in the sending logic and decoded here
# Find formatEvmV1 implementation to understand the address encoding
ast-grep --pattern $'function formatEvmV1($$$) {
$$$
}'
# Check if there are any address length validations in the codebase
rg -n "bytes20|address.*length" --type solidity -C 2Length of output: 1274
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' contracts/crosschain/bridges/BridgeERC20.solLength of output: 3066
🏁 Script executed:
#!/bin/bash
# Show parseV1 implementation in InteroperableAddress
grep -R "function parseV1" -n contracts/utils/draft-InteroperableAddress.sol -C5Length of output: 989
🏁 Script executed:
#!/bin/bash
grep -R "function tryParseV1" -n contracts/utils/draft-InteroperableAddress.sol -C5Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Display the body of tryParseV1 to inspect how `addr` is extracted
sed -n '90,150p' contracts/utils/draft-InteroperableAddress.solLength of output: 3077
Enforce 20-byte EVM address in _processMessage before casting.
Insert at the top of the function, before the address(bytes20(toBinary)) line:
(bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256));
+ require(toBinary.length == 20, "BridgeERC20: invalid address length");
address to = address(bytes20(toBinary));This guarantees the bytes20 conversion can’t under- or over-read the payload.
📝 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.
| /// @inheritdoc ERC7786Recipient | |
| function _processMessage( | |
| address /*gateway*/, | |
| bytes32 receiveId, | |
| bytes calldata /*sender*/, | |
| bytes calldata payload | |
| ) internal virtual override { | |
| // split payload | |
| (bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256)); | |
| address to = address(bytes20(toBinary)); | |
| _unlock(to, amount); | |
| emit CrossChainTransferReceived(receiveId, from, to, amount); | |
| } | |
| /// @inheritdoc ERC7786Recipient | |
| function _processMessage( | |
| address /*gateway*/, | |
| bytes32 receiveId, | |
| bytes calldata /*sender*/, | |
| bytes calldata payload | |
| ) internal virtual override { | |
| // split payload | |
| (bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256)); | |
| require(toBinary.length == 20, "BridgeERC20: invalid address length"); | |
| address to = address(bytes20(toBinary)); | |
| _unlock(to, amount); | |
| emit CrossChainTransferReceived(receiveId, from, to, amount); | |
| } |
🤖 Prompt for AI Agents
In contracts/crosschain/bridges/BridgeERC20.sol around lines 49 to 63, the
function _processMessage currently casts toBinary to address without validating
its length; add a sanity check at the top of the function to require that
toBinary.length == 20 (reverting with a clear message like "BridgeERC20: invalid
to address length") before converting bytes20(toBinary) to address, then proceed
with the existing cast, unlock, and emit logic.
There was a problem hiding this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the discussion from the referenced comment URL
# The comment ID is r2401871066
gh api graphql -f query='
query {
node(id: "r2401871066") {
... on PullRequestReviewComment {
body
path
position
author {
login
}
replies(first: 10) {
nodes {
body
author {
login
}
}
}
}
}
}'
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Try fetching the comment using REST API
# Extract the numeric ID from the review comment ID
# The URL format is: /pull/5914/files#r2401871066
# We need to fetch review comments for PR 5914 and find the one with this ID
gh api repos/OpenZeppelin/openzeppelin-contracts/pulls/5914/comments | jq '.[] | select(.id == 2401871066) | {body: .body, path: .path, user: .user.login}'
Length of output: 752
@Amxx, thank you for the reference. I've reviewed the discussion, and you're absolutely right.
As you explained in that thread, adding a require check here would create a cross-chain inconsistency risk: if the validation fails, tokens would remain locked/burned on the source chain but never minted on the destination chain, leaving funds in limbo.
The best-effort approach with address(bytes20(toBinary)) is the correct design choice. Since the message origin has been verified through the gateway, and toBinary originates from the encoded addr parameter on line 40, the extraction will work correctly for well-formed addresses. If a malformed address is provided due to user error, the tokens will be minted to the derived address, and recovery may be possible through admin controls rather than having funds permanently locked across chains.
My suggestion to add the length validation was inappropriate for this cross-chain bridging context. Would you like me to resolve this comment thread?
🧠 Learnings used
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.638Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
| (bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256)); | ||
| address to = address(bytes20(toBinary)); |
There was a problem hiding this comment.
Unsafe address extraction from bytes.
The conversion address(bytes20(toBinary)) assumes toBinary is exactly 20 bytes in the expected position. However, addr from the sender could be in various interoperable address formats (not just raw EVM addresses). This creates a mismatch:
- Sending side (line 40): encodes
addrdirectly fromparseV1()output - Receiving side (line 58): blindly casts to
bytes20
If toBinary is not a raw 20-byte EVM address or has encoding wrapper bytes, this will either revert or extract the wrong address.
Consider using a proper parser or validating that toBinary is an EVM address format before casting:
(bytes memory from, bytes memory toBinary, uint256 amount) = abi.decode(payload, (bytes, bytes, uint256));
- address to = address(bytes20(toBinary));
+ // Parse the destination address properly
+ (, , bytes memory addrBytes) = toBinary.parseV1();
+ require(addrBytes.length == 20, "Invalid EVM address");
+ address to = address(bytes20(addrBytes));Or ensure that the sending side always encodes a raw 20-byte address rather than a full interoperable address structure.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/crosschain/bridges/BridgeERC20.sol around lines 57-58, the code
unsafely casts toBinary to an address via address(bytes20(toBinary)) which
assumes toBinary is exactly a raw 20-byte EVM address; instead, validate the
payload format before casting (e.g., require(toBinary.length == 20) and revert
with a clear error if not) or implement a parser that accepts interoperable
address encodings (strip known wrappers or handle 32-byte padded addresses by
selecting the correct 20-byte slice) so you only convert when you have a
confirmed raw 20-byte address; alternatively, ensure the sender always encodes a
raw 20-byte address and document/validate that invariant at decode time.
There was a problem hiding this comment.
The origin of the message has been verified, so we have a guarantee that bytes memory toBinary was encoded on line 40 and corresponds to addr that was parsed on line 35.
So we have a guarantee that this is the "address" part of the to ERC-7930 parameter of _crosschainTransfer.
What if this is not properly formated? Reverting here means the the tokens were locked/burn on the source chain, and never minted on this side, creating an inconsistency. Not doing the check means we mint to whatever match the to parameter as best as possible. If the token is administered, there may be a way to recover these funds using a force transfer.
There was a problem hiding this comment.
Note: toBinary is NOT a full ERC-7930 address. Its only the address section, so we shouldn't parse it.
There was a problem hiding this comment.
@Amxx, you're absolutely right. I apologize for the misleading suggestion. In a cross-chain bridge context, reverting on malformed addresses would create an irrecoverable inconsistency where tokens are locked/burned on the source chain but never minted on the destination chain.
The current best-effort approach is the correct design:
- The message origin has been verified, so
toBinaryis guaranteed to come from a trusted source (line 40 encoding) - If the user provided a malformed
toparameter, that's user error at the point of initiating the transfer - Minting to the best interpretation of the address preserves cross-chain consistency
- For administered tokens, there's a recovery path via force transfer
- Reverting would permanently lock funds with no recovery mechanism
The current implementation correctly prioritizes cross-chain consistency over local validation. Thank you for the clarification!
✏️ Learnings added
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.638Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@Amxx, thank you for the clarification! I understand now—toBinary is just the raw address bytes extracted from the ERC-7930 address structure (the addr component from parseV1()), not a complete ERC-7930 formatted address. That's why the direct cast to address(bytes20(toBinary)) is appropriate here without needing to parse it again.
I appreciate you taking the time to ensure I understood the data structure correctly.
🧠 Learnings used
Learnt from: Amxx
PR: OpenZeppelin/openzeppelin-contracts#5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.638Z
Learning: In cross-chain bridge contracts like BridgeERC20, when processing incoming messages in _processMessage, avoid validation checks that revert on malformed addresses. Reverting would create cross-chain inconsistency where tokens are locked/burned on the source chain but never minted on the destination. Instead, use best-effort address extraction (e.g., address(bytes20(toBinary))) to maintain atomicity across chains. If tokens are minted to an incorrect address due to user error, recovery may be possible through admin controls rather than leaving funds permanently locked.
| constructor(IERC20 token_) { | ||
| _token = token_; | ||
| } |
There was a problem hiding this comment.
Validate token address in constructor.
The constructor does not validate that token_ is non-zero. If deployed with address(0), all custody operations (_lock and _unlock) will fail, rendering the bridge unusable.
Apply this diff to add validation:
+ error InvalidToken();
+
constructor(IERC20 token_) {
+ if (address(token_) == address(0)) revert InvalidToken();
_token = token_;
}📝 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.
| constructor(IERC20 token_) { | |
| _token = token_; | |
| } | |
| error InvalidToken(); | |
| constructor(IERC20 token_) { | |
| if (address(token_) == address(0)) revert InvalidToken(); | |
| _token = token_; | |
| } |
🤖 Prompt for AI Agents
In contracts/crosschain/bridges/BridgeERC20Custodial.sol around lines 17 to 19,
the constructor currently assigns token_ without validation which allows
deploying with address(0); add a require that address(token_) != address(0) (or
token_ != IERC20(address(0))) with a clear revert message (e.g., "token cannot
be zero") before assigning _token to ensure a non-zero ERC20 address is
provided.
| async function fixture() { | ||
| const chain = await getLocalChain(); | ||
| const accounts = await ethers.getSigners(); | ||
|
|
||
| // Mock gateway | ||
| const gateway = await ethers.deployContract('$ERC7786GatewayMock'); | ||
| const gatewayAsEOA = await impersonate(gateway); | ||
|
|
||
| // Chain A: legacy ERC20 with bridge | ||
| const tokenA = await ethers.deployContract('$ERC20Crosschain', ['Token1', 'T1', []]); | ||
| const bridgeA = tokenA; // self bridge | ||
|
|
||
| // Chain B: ERC7802 with bridge | ||
| const tokenB = await ethers.deployContract('$ERC20BridgeableMock', ['Token2', 'T2', ethers.ZeroAddress]); | ||
| const bridgeB = await ethers.deployContract('$BridgeERC20Bridgeable', [[], tokenB]); | ||
|
|
||
| // deployment check + remote setup | ||
| await expect(bridgeA.$_setLink(gateway, chain.toErc7930(bridgeB), false)) | ||
| .to.emit(bridgeA, 'RemoteRegistered') | ||
| .withArgs(gateway, chain.toErc7930(bridgeB)); | ||
| await expect(bridgeB.$_setLink(gateway, chain.toErc7930(bridgeA), false)) | ||
| .to.emit(bridgeB, 'RemoteRegistered') | ||
| .withArgs(gateway, chain.toErc7930(bridgeA)); | ||
| await tokenB.$_setBridge(bridgeB); | ||
|
|
||
| return { chain, accounts, gateway, gatewayAsEOA, tokenA, tokenB, bridgeA, bridgeB }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify test coverage for crosschainTransferFrom.
The fixture correctly sets up the cross-chain bridge scenario, and the shared behavior tests cover crosschainTransfer. However, crosschainTransferFrom (the allowance-based variant introduced in ERC20Crosschain) may not be explicitly tested by the shared behavior.
Run the following script to verify whether crosschainTransferFrom is tested:
🏁 Script executed:
#!/bin/bash
# Search for tests of crosschainTransferFrom in test files
rg -nC3 'crosschainTransferFrom' --type=js -g 'test/**'Length of output: 0
Add tests for crosschainTransferFrom
The fixture correctly establishes the cross-chain bridge, but there are no explicit tests covering the allowance-based crosschainTransferFrom variant introduced in ERC20Crosschain. Add tests in test/token/ERC20/extensions/ERC20Crosschain.test.js (or your shared behavior suite) to verify crosschainTransferFrom success and failure scenarios.
🤖 Prompt for AI Agents
In test/token/ERC20/extensions/ERC20Crosschain.test.js around lines 10 to 36,
add unit tests exercising the allowance-based crosschainTransferFrom API
introduced in ERC20Crosschain: write a success case where an owner mints or
holds tokens on tokenA, grants allowance to a spender (approve), then the
spender calls crosschainTransferFrom to send tokens to tokenB via the
established bridge and assert emitted events, allowance decrease, and final
balances on source and destination; also add failure cases asserting revert when
allowance is insufficient, when no approval exists, and when amount exceeds
sender balance. Use the existing fixture to get
chain/accounts/gateway/tokenA/tokenB/bridgeA/bridgeB, impersonate the spender as
needed, and reuse the RemoteRegistered setup; keep tests concise and include
both positive assertions (events, balances, allowance change) and negative
revert checks for incorrect allowance or balances.
Based of #5904
Fixes #5900
PR Checklist
npx changeset add)Summary by Sourcery
Implement cross-chain ERC20 bridging using the ERC-7786 standard by introducing core messaging and bridge contracts, token-specific bridge variants, and extensive tests and mocks.
New Features:
Enhancements:
Documentation:
Tests:
Chores: