-
Notifications
You must be signed in to change notification settings - Fork 12.4k
ERC-7786 based crosschain bridge for ERC-721 tokens #6259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
9535015
769398e
adddbc4
e52226c
4c6ee7e
42ffea0
7a0db4e
07f5812
d756c5c
551a3c7
9f3a5e4
4355ab0
6c50eaf
f490550
88faa8e
86ef787
ade7c92
bbbfa0b
09aebc7
f8ff779
6cf8faf
7f6d544
86c41a5
be7ab96
9a53ca2
981a262
d2cef0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.26; | ||
|
|
||
| import {IERC721} from "../../interfaces/IERC721.sol"; | ||
| import {IERC721Receiver} from "../../interfaces/IERC721Receiver.sol"; | ||
| import {IERC721Errors} from "../../interfaces/draft-IERC6093.sol"; | ||
| import {BridgeERC721Core} from "./BridgeERC721Core.sol"; | ||
|
|
||
| /** | ||
| * @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 | ||
| abstract contract BridgeERC721 is IERC721Receiver, BridgeERC721Core { | ||
| IERC721 private immutable _token; | ||
|
|
||
| error BridgeERC721Unauthorized(address caller); | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| constructor(IERC721 token_) { | ||
| _token = token_; | ||
| } | ||
|
|
||
| /// @dev Return the address of the ERC721 token this bridge operates on. | ||
| function token() public view virtual returns (IERC721) { | ||
| return _token; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Transfer `amount` tokens to a crosschain receiver. | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * | ||
| * Note: The `to` parameter is the full InteroperableAddress (chain ref + address). | ||
| */ | ||
| function crosschainTransferFrom(address from, bytes memory to, uint256 tokenId) public virtual returns (bytes32) { | ||
| // Permission is handled using the ERC721's allowance system. This check replicates `ERC721._isAuthorized`. | ||
| address spender = _msgSender(); | ||
| require( | ||
| from == spender || token().isApprovedForAll(from, spender) || token().getApproved(tokenId) == spender, | ||
ernestognw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| IERC721Errors.ERC721InsufficientApproval(spender, tokenId) | ||
| ); | ||
|
|
||
| // This call verifies that `from` is the owner of `tokenId` | ||
| // Note: do not use safeTransferFrom here! Using it would trigger the `onERC721Received` which we don't want. | ||
| token().transferFrom(from, address(this), tokenId); | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Perform the crosschain transfer and return the handler | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return _crosschainTransfer(from, to, tokenId); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Transfer a token received using an ERC-721 safeTransferFrom | ||
| * | ||
| * Note: The `data` must contain the `to` as a full InteroperableAddress (chain ref + address). | ||
| */ | ||
| function onERC721Received( | ||
| address /*operator*/, | ||
| address from, | ||
| uint256 tokenId, | ||
| bytes calldata data // this is the to | ||
| ) public virtual override returns (bytes4) { | ||
| // TODO: should this consider _msgSender() ? | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| require(msg.sender == address(_token), BridgeERC721Unauthorized(msg.sender)); | ||
| _crosschainTransfer(from, data, tokenId); | ||
| return IERC721Receiver.onERC721Received.selector; | ||
| } | ||
|
|
||
| /// @dev "Locking" tokens is done by taking custody | ||
| function _onSend(address from, uint256 tokenId) internal virtual override { | ||
| // Do nothing, token movement is handled by `crosschainTransfer` and `onERC721Received` | ||
frangio marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * @dev "Unlocking" tokens is done by releasing custody | ||
| * | ||
| * NOTE: `safeTransferFrom` will revert if the receiver is a contract that doesn't implement {IERC721Receiver} | ||
| * This can be retried by at the ERC-7786 gateway level. | ||
| */ | ||
| function _onReceive(address to, uint256 tokenId) internal virtual override { | ||
| token().safeTransferFrom(address(this), to, tokenId); | ||
frangio marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.26; | ||
|
|
||
| import {InteroperableAddress} from "../../utils/draft-InteroperableAddress.sol"; | ||
| import {Context} from "../../utils/Context.sol"; | ||
| import {ERC7786Recipient} from "../ERC7786Recipient.sol"; | ||
| import {CrosschainLinked} from "../CrosschainLinked.sol"; | ||
|
|
||
| /** | ||
| * @dev Base contract for bridging ERC-721 between chains using an ERC-7786 gateway. | ||
| * | ||
| * In order to use this contract, two functions must be implemented to link it to the token: | ||
| * * {_onSend}: called when a crosschain transfer is going out. Must take the sender tokens or revert. | ||
| * * {_onReceive}: called when a crosschain transfer is coming in. Must give tokens to the receiver. | ||
| * | ||
| * This base contract is used by the {BridgeERC721}, which interfaces with legacy ERC-721 tokens. It is also used by | ||
| * the {ERC721Crosschain} extension, which embeds the bridge logic directly in the token contract. | ||
| */ | ||
| abstract contract BridgeERC721Core is Context, CrosschainLinked { | ||
| using InteroperableAddress for bytes; | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| event CrosschainERC721TransferSent(bytes32 indexed sendId, address indexed from, bytes to, uint256 tokenId); | ||
| event CrosschainERC721TransferReceived(bytes32 indexed receiveId, bytes from, address indexed to, uint256 tokenId); | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * @dev Internal crosschain transfer function. | ||
| * | ||
| * Note: The `to` parameter is the full InteroperableAddress (chain ref + address). | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| */ | ||
| function _crosschainTransfer(address from, bytes memory to, uint256 tokenId) internal virtual returns (bytes32) { | ||
| _onSend(from, tokenId); | ||
|
|
||
| (bytes2 chainType, bytes memory chainReference, bytes memory addr) = to.parseV1(); | ||
| bytes memory chain = InteroperableAddress.formatV1(chainType, chainReference, hex""); | ||
|
|
||
| bytes32 sendId = _sendMessageToCounterpart( | ||
| chain, | ||
| abi.encode(InteroperableAddress.formatEvmV1(block.chainid, from), addr, tokenId), | ||
| new bytes[](0) | ||
| ); | ||
|
|
||
| emit CrosschainERC721TransferSent(sendId, from, to, tokenId); | ||
|
|
||
| return sendId; | ||
| } | ||
|
|
||
| /// @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 tokenId) = abi.decode(payload, (bytes, bytes, uint256)); | ||
|
||
| address to = address(bytes20(toBinary)); | ||
|
|
||
| _onReceive(to, tokenId); | ||
|
|
||
| emit CrosschainERC721TransferReceived(receiveId, from, to, tokenId); | ||
| } | ||
|
|
||
| /// @dev Virtual function: implementation is required to handle token being burnt or locked on the source chain. | ||
| function _onSend(address from, uint256 tokenId) internal virtual; | ||
|
|
||
| /// @dev Virtual function: implementation is required to handle token being minted or unlocked on the destination chain. | ||
| function _onReceive(address to, uint256 tokenId) internal virtual; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.26; | ||
|
|
||
| import {ERC721} from "../ERC721.sol"; | ||
| import {BridgeERC721Core} from "../../../crosschain/bridges/BridgeERC721Core.sol"; | ||
|
|
||
| /** | ||
| * @dev Extension of {ERC721} that makes it natively cross-chain using the ERC-7786 based {BridgeERC721Core}. | ||
| * | ||
| * This extension makes the token compatible with: | ||
| * * {ERC721Crosschain} instances on other chains, | ||
| * * {ERC721} instances on other chains that are bridged using {BridgeERC721}, | ||
| */ | ||
| // slither-disable-next-line locked-ether | ||
| abstract contract ERC721Crosschain is ERC721, BridgeERC721Core { | ||
| /// @dev TransferFrom variant of {crosschainTransferFrom}, using ERC721 allowance from the sender to the caller. | ||
Amxx marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| function crosschainTransferFrom(address from, bytes memory to, uint256 tokenId) public virtual returns (bytes32) { | ||
| // operator (_msgSender) permission over `from` is checked in `_onSend` | ||
| return _crosschainTransfer(from, to, tokenId); | ||
| } | ||
|
|
||
| /// @dev "Locking" tokens is achieved through burning | ||
| function _onSend(address from, uint256 tokenId) internal virtual override { | ||
| address previousOwner = _update(address(0), tokenId, _msgSender()); | ||
frangio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (previousOwner == address(0)) { | ||
| revert ERC721NonexistentToken(tokenId); | ||
| } else if (previousOwner != from) { | ||
| revert ERC721IncorrectOwner(from, tokenId, previousOwner); | ||
| } | ||
| } | ||
|
|
||
| /// @dev "Unlocking" tokens is achieved through minting | ||
| function _onReceive(address to, uint256 tokenId) internal virtual override { | ||
| _mint(to, tokenId); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Options 2 we do a
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets discuss that on our weekly call.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to an internal My preference would be for the simpler approach that just does |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this line required? I see no relation with its documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have it, slither will be unhappy. This is because the
IERC7786Recipient.receiveMessageis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I'm curious why slither is not unhappy about the
ERC7786Recipientcontract since it also implements thereceiveMessagepayable. Maybe it's becauseBridgeERC721implementsprocessMessagewithout handling value, so there are no other functions left to handle the native value received inreceiveMessage.I agree BridgeERC721 shouldn't handle value at all, but, maybe we should restrict value sent in
receiveMessage? E.g.:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its because
ERC7786Recipient(andBridgeERC721) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.