ERC-7786 based crosschain bridge for ERC-721 tokens#6259
ERC-7786 based crosschain bridge for ERC-721 tokens#6259Amxx merged 27 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: d2cef0a 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
| ); | ||
|
|
||
| // This call verifies that `from` is the owner of `tokenId`, and the previous checks ensure that `spender` is | ||
| // allowed to move tokenId on behalf of `from`. |
There was a problem hiding this comment.
Isn't this something that's checked by the token?
There was a problem hiding this comment.
this was updated to
// This call verifies that `from` is the owner of `tokenId` (in `_onSend`), and the previous checks ensure
// that `spender` is allowed to move tokenId on behalf of `from`.
//
// Perform the crosschain transfer and return the handler
The first part is indeed checked by the token. That is what I'm saying (maybe it could be more explicit).
The second part is not checked by the token. The token check that the spender he sees is allowed. That is the bridge itself. So the token will check that the owner authorised the bridge, and the bridge will do an additional check that whoever did the crosschainTransferFrom is also authorised by the owner of token. That last check is not (and cannot) be done by the token itself.
There was a problem hiding this comment.
I see so this allows authorized ERC-721 spenders to also do cross-chain transfers.
This needs to be documented in the interface of crosschainTransferFrom.
Co-authored-by: Francisco Giordano <fg@frang.io>
| * @dev This is a variant of {BridgeERC721Core} that implements the bridge logic for ERC-721 tokens that do not expose | ||
| * a crosschain mint and burn mechanism. Instead, it takes custody of bridged assets. | ||
| */ | ||
| // slither-disable-next-line locked-ether |
There was a problem hiding this comment.
Why is this line required? I see no relation with its documentation
There was a problem hiding this comment.
If we don't have it, slither will be unhappy. This is because the IERC7786Recipient.receiveMessage is payable (to accomodate gateways that want to send native token.
In our case, we could teoretically get ether in, and there would be no way of getting it out. Slither is unhappy about that, and this is how we silent it.
There was a problem hiding this comment.
Interesting. I'm curious why slither is not unhappy about the ERC7786Recipient contract since it also implements the receiveMessage payable. Maybe it's because BridgeERC721 implements processMessage without handling value, so there are no other functions left to handle the native value received in receiveMessage.
I agree BridgeERC721 shouldn't handle value at all, but, maybe we should restrict value sent in receiveMessage? E.g.:
function _processMessage(
address gateway,
bytes32 receiveId,
bytes calldata sender,
bytes calldata payload
) internal virtual override {
require(msg.value < 1);
super._processMessage(gateway, receiveId, sender, payload);
}There was a problem hiding this comment.
I think its because ERC7786Recipient (and BridgeERC721) have unimplemented functions. Slither doesn't know what they do, and if they are going to move ether out, so I guess its not flagging them.
There was a problem hiding this comment.
maybe we should restrict value sent in receiveMessage
If we do that, and if the gateway ever decides to send some value, then we would not be processing the message, and the ERC-721 NFT would not be minted/released. Just becaused we receive either we don't know how to handle, we would be posibly bricking a token. Sounds like a disaster waiting to happen.
Also, what if someone wants to override the bridge to add more logic that deals with ethers. Some admin may add a drain, and that is fine. Other may want to add more code that deals with the token as a payment, and automatically move it somewhere or something.
I wouldn't add a check here.
There was a problem hiding this comment.
Yeah, got it. So the receiving side may require value to process the message (e.g. charging the gas for passing the message).
Let's not add the payable check, but I still think Slither has a good point, and we should document that if the underlying bridge sends value to receiveMessage, such value will indeed be stuck.
| /// @dev "Locking" tokens is done by taking custody | ||
| function _onSend(address from, uint256 tokenId) internal virtual override { | ||
| // slither-disable-next-line arbitrary-send-erc20 | ||
| token().transferFrom(from, address(this), tokenId); |
There was a problem hiding this comment.
Shouldn't this contract implement onERC721Received? How's the bridge able to receive the tokens?
There was a problem hiding this comment.
This is an unsafe transfer, to it will work without the onERC721Received hook present in the bridge.
We used to have a onERC721Received hook that allowed anyone to do a safeTransferFrom to the the bridge, and pass in data that contained the to (in ERC-7830 format). That way you were able to do a crosschain send in just one operation, without the approval+transfer pattern.
We decided to remove that because of security issues. Not on the contract side, but on the wallet side. Basically, if an app/UI asks your wallet to do a safeTransferFrom to the bridge, you'll see you are sending the token to the bridge (which is good), but its likelly you won't see the data part of the transfer. If the app/UI puts the wrong data (either maliciously or by mistake) you are going to send you token to some unexpected recipient, and the wallet won't show that information to you.
There was a problem hiding this comment.
Basically, if an app/UI asks your wallet to do a safeTransferFrom to the bridge, you'll see you are sending the token to the bridge (which is good), but its likelly you won't see the data part of the transfer.
I see. So one thing is actually using the data argument to allow one-step crosschain transfers, but another is guaranteeing that the recipient can indeed receive the token. One thing that may happen is that the message is processed and the token is effectively sent to an address that can't transfer it out. Wouldn't be better that the bridge keeps custody in those cases?
There was a problem hiding this comment.
Its not really about "guaranteeing that the recipient can receive the token". Its about guaranteeing that the sender sees (and can verify) who he is sending the token to. I believe the sender should not be "blindly" signing the crosschain recipient. If the wallet doesn't display the "data" section that his happening.
In particular, there is no way for a bridge to distinguish between:
- the EOA of the (valid) receiver
- the EOA of an attacker that injected malicious "data", that the user was not able to review because the wallet hides it
- an invalid address that has no code, and that doesn't have a (known) private key.
There was a problem hiding this comment.
One thing that may happen is that the message is processed and the token is effectively sent to an address that can't transfer it out. Wouldn't be better that the bridge keeps custody in those cases?
We briefly discussed this before and landed on transferFrom because it seems to require fewer assumptions for recovery. The bridge keeping custody is only better if the message can be retried after the receiver has been upgraded with the receive handler. If you do transferFrom, you just need the receiver to be upgradeable.
An alternative path for recovery would be if the failure to deliver the message could be reported back in the source chain. But these are strong assumptions that 7786 doesn't guarantee at all.
Edit: Just saw this was discussed in another thread #6259 (comment)
| bytes calldata payload | ||
| ) internal virtual override { | ||
| // split payload | ||
| (bytes memory from, bytes memory toBinary, uint256 tokenId) = abi.decode(payload, (bytes, bytes, uint256)); |
There was a problem hiding this comment.
It would be easy to decode this in calldata afaik
require(payload.length > 9f); // fromOffset + toBinaryOffset + tokenId + fromLength + toBinaryLength
uint256 fromOffset = payload[:0x20];
uint256 toBinaryOffset = payload[0x20:0x40];
require(fromOffset > 0x5f && fromOffset <= payload.length && toBinaryOffset > 0x7f && toBinaryOffset <= payload.length);
uint256 tokenId = payload[0x40:0x60];
uint256 fromLength = payload[fromOffset:fromOffset + 0x20];
uint256 toBinaryLength = payload[toBinaryOffset:toBinaryOffset + 0x20];
require(fromOffset + fromLength <= payload.length && toBinaryOffset + toBinaryLength <= payload.length)So we don't allocate extra memory
There was a problem hiding this comment.
Readability is not great. I think in the context of a "we are moving tokens" operation, the extra gas cost here is not that bad.
If we really want to micro optimize this, I would first merge the "simple solidity" version, and then do small PR that incrementally optimise. That way we can leverage our tools for gas comparison.
|
|
||
| /// @dev "Unlocking" tokens is achieved through minting | ||
| function _onReceive(address to, uint256 tokenId) internal virtual override { | ||
| _mint(to, tokenId); |
There was a problem hiding this comment.
I think we used to, and we had a long discussion about that with @frangio.
Basically, if the recipient is not "equiped" to receive tokens, what do we want to happen ?
Option 1
We do a _safeMint (on ERC721Crosschain) / a safeTransferFrom (on BridgeERC721). This causes the ERC7786.receiveMessage to fail. This likelly goes back to the gateway. This token is not moved. To fix the situation, you need to upgrade the receiver to add the required logic, and the re-do the call at the gateway level.
- if you cannot upgrade the receiver, the token is lost forever
- If for some reason the upgrade took to long, and some expiry thing on the gateway prevents you from replaying, the token is lost forever.
Options 2
we do a _mint (on ERC721Crosschain) / a transferFrom (on BridgeERC721). This forces the token transfer. From a gateway point of view, everything is fine. If the receiver doesn't know how to handle the token it just received, you need to do an upgrade to had that functionnality
- if you cannot upgrade the receiver, the token is lost forever
In both cases, an upgrade is needed. But option 1 also adds more logic to replay the message after the upgrade. @frangio and myself thought option 2 is probably better.
There was a problem hiding this comment.
I was thinking a 3rd option is to include a "reverse" function (or similar) that cancels out the message delivery if it's not executed in X time. While I agree it's an opinionated decision and may not be the best default, I feel the BridgeERC721 is incomplete if we either suggest a workaround or implement one by default.
To be clear, I overall agree that option 2 is simpler give the case of a stuck token, but may be worth considering a default functionality that lets developers handle it properly. For example, see that ERC721Wrapper has a _recover internal function.
There was a problem hiding this comment.
Option 3 comes with its own set of problem.
It could be implemented as an extension, but I don't think this complexity should be in the default.
There was a problem hiding this comment.
Lets discuss that on our weekly call.
There was a problem hiding this comment.
I'm open to an internal _recover function that maybe gets unlocked for a token id once a transfer fails.
My preference would be for the simpler approach that just does transferFrom. But this is not an option in ERC-1155 (#6281).
Co-authored-by: Ernesto García <ernestognw@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>
3 participants
Followup to #5914
Fixes #5901
PR Checklist
npx changeset add)