ERC-7786 based crosschain bridge for ERC-721 tokens #6259
Conversation
🦋 Changeset detectedLatest commit: 551a3c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR introduces cross-chain bridging support for ERC-721 tokens across multiple chains. It adds three new Solidity contracts: BridgeERC721Core (abstract base providing core cross-chain transfer and message processing logic), BridgeERC721 (custody-based bridge implementation with token locking/unlocking), and ERC721Crosschain (ERC721 extension enabling native cross-chain transfers). The implementation leverages ERC-7786 gateway infrastructure for message passing and includes internal hooks for handling token state transitions on send and receive. Test files provide comprehensive coverage including behavior specification, permission validation, and end-to-end transfer scenarios in both directions. Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 6
Fix all issues with AI Agents 🤖
In @contracts/crosschain/bridges/BridgeERC721.sol:
- Line 35: Fix the typo in the comment that currently reads "Permission is
handeled using the ERC721's allowance system. This check replicates
`ERC721._isAuthorized`." — change "handeled" to "handled" in the comment near
the BridgeERC721 contract (the line containing that sentence) so the comment
correctly reads "Permission is handled...".
In @contracts/crosschain/bridges/BridgeERC721Core.sol:
- Line 18: Fix the NatSpec typo in the contract documentation where the phrase
"the {ERC721Crosschain}extension" appears: insert a space so it reads "the
{ERC721Crosschain} extension" (update the comment containing {ERC721Crosschain}
to include the space before "extension").
In @test/crosschain/BridgeERC721.behavior.js:
- Line 198: The test title string passed to the it(...) call is misspelled:
replace 'cannot override configuration is "allowOverride" is false' with 'cannot
override configuration if "allowOverride" is false' by updating the it(...)
description (the string literal) in the test case so the wording uses "if"
instead of "is".
In @test/crosschain/BridgeERC721.test.js:
- Line 71: Fix the typo in the test comment that currently reads "bridge on
custodial chain releases mints the token" by updating it to a grammatically
correct phrase such as "bridge on custodial chain releases the token" (or
"bridge on custodial chain mints the token" if minting is intended); edit the
comment in BridgeERC721.test.js where that string appears so it clearly reflects
the test behavior.
- Line 39: The test suite is misnamed: change the describe title from
"CrosschainBridgeERC20" to "CrosschainBridgeERC721" so it accurately reflects
the tests in BridgeERC721.test.js; locate the describe(...) call with the string
"CrosschainBridgeERC20" and update that literal to "CrosschainBridgeERC721"
(ensure any other occurrences in this file that reference the ERC20 name are
also updated for consistency).
In @test/token/ERC721/extensions/ERC721Crosschain.test.js:
- Line 38: The test suite name is wrong: rename the describe block currently
labeled 'CrosschainBridgeERC20' to 'CrosschainBridgeERC721' to match the file
and the used behavior test shouldBehaveLikeBridgeERC721; update the describe
string in the test file so the suite accurately reflects ERC721 tests and any
related test output logs.
🧹 Nitpick comments (1)
contracts/crosschain/bridges/BridgeERC721.sol (1)
61-62: Resolve TODO:msg.senderis correct here.Using
msg.senderdirectly is the correct choice for this check because it validates the actual calling contract address. Using_msgSender()could allow meta-transaction forwarders to bypass this security check. Consider removing the TODO and optionally adding a brief comment explaining whymsg.senderis used intentionally.🔎 Proposed fix
- // TODO: should this consider _msgSender() ? - require(msg.sender == address(_token), BridgeERC721Unauthorized(msg.sender)); + // Note: Using msg.sender intentionally (not _msgSender) to validate the actual caller contract + require(msg.sender == address(_token), BridgeERC721Unauthorized(msg.sender));
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/crosschain/bridges/BridgeERC721.solcontracts/crosschain/bridges/BridgeERC721Core.solcontracts/token/ERC721/extensions/ERC721Crosschain.soltest/crosschain/BridgeERC20.behavior.jstest/crosschain/BridgeERC721.behavior.jstest/crosschain/BridgeERC721.test.jstest/token/ERC721/extensions/ERC721Crosschain.test.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
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.
📚 Learning: 2025-10-03T13:14:57.679Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5914
File: contracts/crosschain/bridges/BridgeERC20.sol:57-58
Timestamp: 2025-10-03T13:14:57.679Z
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.
Applied to files:
test/crosschain/BridgeERC721.test.jstest/token/ERC721/extensions/ERC721Crosschain.test.jstest/crosschain/BridgeERC20.behavior.jstest/crosschain/BridgeERC721.behavior.jscontracts/crosschain/bridges/BridgeERC721.solcontracts/token/ERC721/extensions/ERC721Crosschain.solcontracts/crosschain/bridges/BridgeERC721Core.sol
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
test/crosschain/BridgeERC721.test.jstest/crosschain/BridgeERC20.behavior.jstest/crosschain/BridgeERC721.behavior.jscontracts/crosschain/bridges/BridgeERC721.solcontracts/crosschain/bridges/BridgeERC721Core.sol
📚 Learning: 2025-10-15T02:52:05.027Z
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5891
File: test/account/modules/ERC7579Module.behavior.js:56-61
Timestamp: 2025-10-15T02:52:05.027Z
Learning: In ERC7579 validator tests for `isValidSignatureWithSender`, using `this.mock` (not bound to a specific account) is valid when testing signature validation with any arbitrary sender, while `this.mockFromAccount` is used when testing account-specific validation scenarios.
Applied to files:
test/crosschain/BridgeERC721.test.js
🧬 Code graph analysis (3)
test/crosschain/BridgeERC721.test.js (4)
test/crosschain/BridgeERC721.behavior.js (7)
require(1-1)require(2-2)require(3-3)tokenId(5-5)tokenId(73-73)tokenId(85-85)tokenId(104-104)test/token/ERC721/extensions/ERC721Crosschain.test.js (7)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)chain(11-11)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/BridgeERC721.behavior.js (6)
from(160-160)alice(30-30)alice(72-72)alice(84-84)alice(103-103)id(162-162)test/token/ERC20/extensions/ERC20Crosschain.test.js (4)
amount(46-46)amount(74-74)alice(45-45)alice(73-73)
test/crosschain/BridgeERC721.behavior.js (2)
test/token/ERC721/extensions/ERC721Crosschain.test.js (6)
require(1-1)require(2-2)require(3-3)require(5-5)require(6-6)require(8-8)test/crosschain/BridgeERC721.test.js (7)
require(1-1)require(2-2)require(3-3)require(4-4)require(6-6)require(7-7)require(9-9)
🪛 GitHub Check: codespell
contracts/crosschain/bridges/BridgeERC721.sol
[failure] 35-35:
handeled ==> handled, handheld
⏰ 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). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: tests-upgradeable
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: tests-foundry
- GitHub Check: slither
- GitHub Check: halmos
🔇 Additional comments (20)
test/crosschain/BridgeERC20.behavior.js (5)
9-16: LGTM!The encodePayload helper is well-designed with consistent address encoding via
toErc7930()and defensive handling for thetoparameter. The structure aligns with ERC-7786 message format requirements.
18-27: LGTM!The bridge setup test correctly validates bidirectional link configuration between chains. The assertion properly checks both gateway and counterpart addresses in the expected format.
29-74: LGTM!The crosschain send test is comprehensive and well-structured. It correctly validates bidirectional transfers with proper event sequencing and custody-model-aware assertions. The use of
anyValuefor message IDs is appropriate, and the test flows logically through a realistic token transfer scenario.
76-127: LGTM!The restrictions tests comprehensively validate access control and replay protection. The error assertions correctly target specific custom errors, and the test setup with pre-minted tokens enables proper message processing. The replay protection test appropriately uses ZeroHash for deterministic testing.
129-159: LGTM!The reconfiguration tests properly validate link management with event validation, immutability protection, and gateway interface validation. The revert-without-reason in the invalid gateway test suggests an interface check, which is a reasonable approach for validating contract types.
test/token/ERC721/extensions/ERC721Crosschain.test.js (1)
10-36: LGTM!The fixture correctly sets up a cross-chain test scenario with two ERC721Crosschain tokens, a mock gateway, and proper link registration with event verification.
test/crosschain/BridgeERC721.test.js (1)
11-48: LGTM!The fixture correctly sets up a heterogeneous cross-chain scenario with a legacy ERC721 using a custody-based bridge on chain A and an ERC721Crosschain on chain B. The
chainAIsCustodial: trueparameter appropriately configures the shared behavior suite.test/crosschain/BridgeERC721.behavior.js (5)
1-16: LGTM!The shared behavior suite setup is well-structured with a flexible configuration pattern (
chainAIsCustodial,chainBIsCustodial) and a clear payload encoding helper.
29-68: LGTM!The bidirectional cross-chain transfer test correctly validates custody semantics based on the
chainAIsCustodial/chainBIsCustodialconfiguration, with appropriate event assertions for each direction.
70-124: LGTM!The allowance tests comprehensively cover owner transfers, universal operator approval, and token-specific approval, including proper negative test cases for unauthorized transfers.
126-181: LGTM!The restriction tests properly validate gateway authorization, counterpart verification, and replay protection. The conditional minting in the replay test correctly handles the custodial configuration.
183-213: LGTM!The reconfiguration tests properly validate link updates, override prevention, and gateway validation.
contracts/token/ERC721/extensions/ERC721Crosschain.sol (3)
16-21: LGTM!The
crosschainTransferFromcorrectly delegates to_crosschainTransfer, and the comment appropriately notes that permission checks occur in_onSend.
24-31: LGTM!The
_onSendimplementation correctly burns the token using_update(address(0), tokenId, _msgSender()), which internally validates operator approval. The subsequent checks properly handle non-existent tokens and ownership verification.
34-36: LGTM!The
_onReceiveimplementation correctly uses a best-effort approach by minting without additional validation, which aligns with cross-chain bridge design principles to avoid inconsistency when tokens are already locked/burned on the source chain. Based on learnings from similar cross-chain bridge patterns.contracts/crosschain/bridges/BridgeERC721Core.sol (2)
31-46: LGTM!The
_crosschainTransferimplementation correctly sequences the operations: first calls_onSendto lock/burn tokens, then encodes and sends the cross-chain message, and finally emits the event with the message ID.
49-62: LGTM!The
_processMessagecorrectly uses best-effort address extraction (address(bytes20(toBinary))) without validation that could revert, which maintains cross-chain atomicity. This aligns with the established pattern for cross-chain bridges where reverting would leave tokens locked on the source chain with no recourse.contracts/crosschain/bridges/BridgeERC721.sol (3)
34-48: LGTM!The
crosschainTransferFromcorrectly replicates ERC721 authorization checks, usestransferFrom(notsafeTransferFrom) to avoid re-entrancy intoonERC721Received, and properly sequences custody acquisition before initiating the cross-chain transfer.
55-65: LGTM!The
onERC721Receivedhook correctly enablessafeTransferFrom-based cross-chain transfers with proper validation that only the bridged token contract can trigger it.
67-80: LGTM!The hook implementations are correctly designed for the custody-based model. The
_onSendno-op is appropriate since custody is handled earlier, and_onReceiveusingsafeTransferFromprovides recipient safety with the documented retry mechanism at the gateway level for failed transfers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nzeppelin-contracts into crosschain/erc721bridge
There was a problem hiding this comment.
Diff is messy, but the only change is actually to wrap all the content of shouldBehaveLikeBridgeERC20 in a describe. Make sure you enable "hide whitespace" in the diff settings.
| // the `onERC721Received` which we don't want. | ||
| // | ||
| // slither-disable-next-line arbitrary-send-erc20 | ||
| token().transferFrom(from, address(this), tokenId); |
There was a problem hiding this comment.
Here we could do a safeTransferFrom with the to as data, and rely on the onERC721Received to do the _crosschainTransfer ... but then we would have to store the bytes32 in transiant storage (in onERC721Received and read it here for the return value. Not sure if that would be any better.
|
|
||
| /// @dev "Locking" tokens is achieved through burning | ||
| function _onSend(address from, uint256 tokenId) internal virtual override { | ||
| address previousOwner = _update(address(0), tokenId, _msgSender()); |
There was a problem hiding this comment.
that _msgSender() is the "operator". It makes sens to check that when doing a public crosschainTransferFrom, but is there any issue with this being called during an internal _crosschainTransfer ?
There was a problem hiding this comment.
Alternative is to move that logic to the crosschainTransferFrom function, and leave the _onSend empty.
1 participant
Followup to #5914
Fixes #5901
PR Checklist
npx changeset add)