diff --git a/.changeset/breezy-carrots-flow.md b/.changeset/breezy-carrots-flow.md new file mode 100644 index 0000000000000..4dfdb59d098fd --- /dev/null +++ b/.changeset/breezy-carrots-flow.md @@ -0,0 +1,6 @@ +--- +'@eth-optimism/contracts-periphery': minor +'@eth-optimism/integration-tests': patch +--- + +Fixes NFT bridge related contracts in response to the OpenZeppelin audit. Updates tests to support these changes, including integration tests. diff --git a/integration-tests/contracts/FakeOptimismMintableERC721.sol b/integration-tests/contracts/FakeOptimismMintableERC721.sol index 2108dbbfc373b..6aad566010e77 100644 --- a/integration-tests/contracts/FakeOptimismMintableERC721.sol +++ b/integration-tests/contracts/FakeOptimismMintableERC721.sol @@ -2,6 +2,8 @@ pragma solidity ^0.8.9; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; +import { IOptimismMintableERC721 } from "@eth-optimism/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol"; +import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; contract FakeOptimismMintableERC721 is ERC721 { @@ -13,10 +15,24 @@ contract FakeOptimismMintableERC721 is ERC721 { bridge = _bridge; } - function mint(address to, uint256 tokenId) public { - _mint(to, tokenId); + function safeMint(address to, uint256 tokenId) public { + _safeMint(to, tokenId); } // Burn will be called by the L2 Bridge to burn the NFT we are bridging to L1 - function burn(address, uint256) external {} + function burn(address, uint256 tokenId) external { + _burn(tokenId); + } + + // Returns true when queried with the interface ID for OptimismMintableERC721. + function supportsInterface(bytes4 _interfaceId) + public + pure + override + returns (bool) + { + bytes4 iface1 = type(IERC165).interfaceId; + bytes4 iface2 = type(IOptimismMintableERC721).interfaceId; + return _interfaceId == iface1 || _interfaceId == iface2; + } } diff --git a/integration-tests/test/nft-bridge.spec.ts b/integration-tests/test/nft-bridge.spec.ts index 830ccc3785431..4a24227fd3d33 100644 --- a/integration-tests/test/nft-bridge.spec.ts +++ b/integration-tests/test/nft-bridge.spec.ts @@ -3,6 +3,7 @@ import { Contract, ContractFactory, utils, Wallet } from 'ethers' import { ethers } from 'hardhat' import { getChainId } from '@eth-optimism/core-utils' import { predeploys } from '@eth-optimism/contracts' +import { MessageLike } from '@eth-optimism/sdk' import Artifact__TestERC721 from '@eth-optimism/contracts-periphery/artifacts/contracts/testing/helpers/TestERC721.sol/TestERC721.json' import Artifact__L1ERC721Bridge from '@eth-optimism/contracts-periphery/artifacts/contracts/L1/L1ERC721Bridge.sol/L1ERC721Bridge.json' import Artifact__L2ERC721Bridge from '@eth-optimism/contracts-periphery/artifacts/contracts/L2/L2ERC721Bridge.sol/L2ERC721Bridge.json' @@ -15,8 +16,11 @@ import { OptimismEnv } from './shared/env' import { withdrawalTest } from './shared/utils' const TOKEN_ID: number = 1 -const FINALIZATION_GAS: number = 1_200_000 +const FINALIZATION_GAS: number = 600_000 const NON_NULL_BYTES: string = '0x1111' +const DUMMY_L1ERC721_ADDRESS: string = ethers.utils.getAddress( + '0x' + 'acdc'.repeat(10) +) describe('ERC721 Bridge', () => { let env: OptimismEnv @@ -129,7 +133,6 @@ describe('ERC721 Bridge', () => { Artifact__OptimismMintableERC721.abi, erc721CreatedEvent.args.localToken ) - await OptimismMintableERC721.deployed() // Mint an L1 ERC721 to Bob on L1 const tx2 = await L1ERC721.mint(bobAddress, TOKEN_ID) @@ -278,7 +281,10 @@ describe('ERC721 Bridge', () => { await FakeOptimismMintableERC721.deployed() // Use the fake contract to mint Alice an NFT with the same token ID - const tx = await FakeOptimismMintableERC721.mint(aliceAddress, TOKEN_ID) + const tx = await FakeOptimismMintableERC721.safeMint( + aliceAddress, + TOKEN_ID + ) await tx.wait() // Check that Alice owns the NFT from the fake ERC721 contract @@ -303,4 +309,57 @@ describe('ERC721 Bridge', () => { expect(await L1ERC721.ownerOf(TOKEN_ID)).to.equal(L1ERC721Bridge.address) } ) + + withdrawalTest( + 'should refund an L2 NFT that fails to be finalized on l1', + async () => { + // Deploy an L2 native NFT, which: + // - Mimics the interface of an OptimismMintableERC721. + // - Allows anyone to mint tokens. + // - Has a `remoteToken` state variable that returns the address of a non-existent L1 ERC721. + // This will cause the bridge to fail on L1, triggering a refund on L2. + const L2NativeNFT = await ( + await ethers.getContractFactory( + 'FakeOptimismMintableERC721', + aliceWalletL2 + ) + ).deploy(DUMMY_L1ERC721_ADDRESS, L2ERC721Bridge.address) + await L2NativeNFT.deployed() + + // Alice mints an NFT from the L2 native ERC721 contract + const tx = await L2NativeNFT.safeMint(aliceAddress, TOKEN_ID) + await tx.wait() + + // Check that Alice owns the L2 NFT + expect(await L2NativeNFT.ownerOf(TOKEN_ID)).to.equal(aliceAddress) + + // Alice bridges her L2 native NFT to L1, which burns the L2 NFT. + const withdrawalTx = await L2ERC721Bridge.connect( + aliceWalletL2 + ).bridgeERC721( + L2NativeNFT.address, + DUMMY_L1ERC721_ADDRESS, + TOKEN_ID, + FINALIZATION_GAS, + NON_NULL_BYTES + ) + await withdrawalTx.wait() + + // Check that the token was burnt on L2 (pre-refund). + await expect(L2NativeNFT.ownerOf(TOKEN_ID)).to.be.revertedWith( + 'ERC721: owner query for nonexistent token' + ) + + // Relay the cross-domain transaction to L1, which initiates an L1 -> L2 message to refund + // Alice her L2 NFT. + await env.relayXDomainMessages(withdrawalTx) + + // Wait for the L1 -> L2 message to finalize on L2 + const txPair = await env.waitForXDomainTransaction(withdrawalTx) + await env.messenger.waitForMessageReceipt(txPair.remoteTx as MessageLike) + + // Check that the L2 NFT has been refunded to Alice. + expect(await L2NativeNFT.ownerOf(TOKEN_ID)).to.equal(aliceAddress) + } + ) }) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 947b85ebdaae2..5480e84ea0723 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -1,68 +1,19 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; -import { - CrossDomainEnabled -} from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; -import { - OwnableUpgradeable -} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; -import { Address } from "@openzeppelin/contracts/utils/Address.sol"; import { L2ERC721Bridge } from "../L2/L2ERC721Bridge.sol"; import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semver.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; /** * @title L1ERC721Bridge * @notice The L1 ERC721 bridge is a contract which works together with the L2 ERC721 bridge to - * make it possible to transfer ERC721 tokens between Optimism and Ethereum. This contract + * make it possible to transfer ERC721 tokens from Ethereum to Optimism. This contract * acts as an escrow for ERC721 tokens deposted into L2. */ -contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { - /** - * @notice Emitted when an ERC721 bridge to the other network is initiated. - * - * @param localToken Address of the token on this domain. - * @param remoteToken Address of the token on the remote domain. - * @param from Address that initiated bridging action. - * @param to Address to receive the token. - * @param tokenId ID of the specific token deposited. - * @param extraData Extra data for use on the client-side. - */ - event ERC721BridgeInitiated( - address indexed localToken, - address indexed remoteToken, - address indexed from, - address to, - uint256 tokenId, - bytes extraData - ); - - /** - * @notice Emitted when an ERC721 bridge from the other network is finalized. - * - * @param localToken Address of the token on this domain. - * @param remoteToken Address of the token on the remote domain. - * @param from Address that initiated bridging action. - * @param to Address to receive the token. - * @param tokenId ID of the specific token deposited. - * @param extraData Extra data for use on the client-side. - */ - event ERC721BridgeFinalized( - address indexed localToken, - address indexed remoteToken, - address indexed from, - address to, - uint256 tokenId, - bytes extraData - ); - - /** - * @notice Address of the bridge on the other network. - */ - address public otherBridge; - - // Maps L1 token to L2 token to token ID to a boolean indicating if the token is deposited +contract L1ERC721Bridge is ERC721Bridge, Semver { /** * @notice Mapping of L1 token to L2 token to ID to boolean, indicating if the given L1 token * by ID was deposited for a given L2 token. @@ -70,92 +21,25 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { mapping(address => mapping(address => mapping(uint256 => bool))) public deposits; /** - * @custom:semver 0.0.1 + * @custom:semver 1.0.0 * * @param _messenger Address of the CrossDomainMessenger on this network. * @param _otherBridge Address of the ERC721 bridge on the other network. */ - constructor(address _messenger, address _otherBridge) - Semver(0, 0, 1) - CrossDomainEnabled(address(0)) - { + constructor(address _messenger, address _otherBridge) Semver(1, 0, 0) { initialize(_messenger, _otherBridge); } /** - * @param _messenger Address of the CrossDomainMessenger on this network. - * @param _otherBridge Address of the ERC721 bridge on the other network. - */ - function initialize(address _messenger, address _otherBridge) public initializer { - messenger = _messenger; - otherBridge = _otherBridge; - - // Initialize upgradable OZ contracts - __Ownable_init(); - } - - /** - * @notice Initiates a bridge of an NFT to the caller's account on L2. - * - * @param _localToken Address of the ERC721 on this domain. - * @param _remoteToken Address of the ERC721 on the remote domain. - * @param _tokenId Token ID to bridge. - * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. - * @param _extraData Optional data to forward to L2. Data supplied here will not be used to - * execute any code on L2 and is only emitted as extra data for the - * convenience of off-chain tooling. - */ - function bridgeERC721( - address _localToken, - address _remoteToken, - uint256 _tokenId, - uint32 _minGasLimit, - bytes calldata _extraData - ) external { - // Modifier requiring sender to be EOA. This check could be bypassed by a malicious - // contract via initcode, but it takes care of the user error we want to avoid. - require(!Address.isContract(msg.sender), "L1ERC721Bridge: account is not externally owned"); - - _initiateBridgeERC721( - _localToken, - _remoteToken, - msg.sender, - msg.sender, - _tokenId, - _minGasLimit, - _extraData - ); - } - - /** - * @notice Initiates a bridge of an NFT to some recipient's account on L2. + * @notice Initializer. * - * @param _localToken Address of the ERC721 on this domain. - * @param _remoteToken Address of the ERC721 on the remote domain. - * @param _to Address to receive the token on the other domain. - * @param _tokenId Token ID to bridge. - * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. - * @param _extraData Optional data to forward to L2. Data supplied here will not be used to - * execute any code on L2 and is only emitted as extra data for the - * convenience of off-chain tooling. + * @param _messenger Address of the L1CrossDomainMessenger. + * @param _otherBridge Address of the L2ERC721Bridge. */ - function bridgeERC721To( - address _localToken, - address _remoteToken, - address _to, - uint256 _tokenId, - uint32 _minGasLimit, - bytes calldata _extraData - ) external { - _initiateBridgeERC721( - _localToken, - _remoteToken, - msg.sender, - _to, - _tokenId, - _minGasLimit, - _extraData - ); + function initialize(address _messenger, address _otherBridge) public initializer { + require(_messenger != address(0), "ERC721Bridge: messenger cannot be address(0)"); + require(_otherBridge != address(0), "ERC721Bridge: other bridge cannot be address(0)"); + __ERC721Bridge_init(_messenger, _otherBridge); } /************************* @@ -182,21 +66,73 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { address _to, uint256 _tokenId, bytes calldata _extraData - ) external onlyFromCrossDomainAccount(otherBridge) { - // Checks that the L1/L2 token pair has a token ID that is escrowed in the L1 Bridge + ) external onlyOtherBridge { + try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { + if (_from == otherBridge) { + // The _from address is the address of the remote bridge if a transfer fails to be + // finalized on the remote chain. + // slither-disable-next-line reentrancy-events + emit ERC721Refunded(_localToken, _remoteToken, _to, _tokenId, _extraData); + } else { + // slither-disable-next-line reentrancy-events + emit ERC721BridgeFinalized( + _localToken, + _remoteToken, + _from, + _to, + _tokenId, + _extraData + ); + } + } catch { + // If the token ID for this L1/L2 NFT pair is not escrowed in the L1 Bridge or if + // another error occurred during finalization, we initiate a cross-domain message to + // send the NFT back to its original owner on L2. This can happen if an L2 native NFT is + // bridged to L1, or if a user mistakenly entered an incorrect L1 ERC721 address. + bytes memory message = abi.encodeWithSelector( + L2ERC721Bridge.finalizeBridgeERC721.selector, + _remoteToken, + _localToken, + address(this), // Set the new _from address to be this contract since the NFT was + // never transferred to the recipient on this chain. + _from, // Refund the NFT to the original owner on the remote chain. + _tokenId, + _extraData + ); + + // Send the message to the L2 bridge. + // slither-disable-next-line reentrancy-events + sendCrossDomainMessage(otherBridge, 600_000, message); + + // slither-disable-next-line reentrancy-events + emit ERC721BridgeFailed(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + } + } + + /** + * @inheritdoc ERC721Bridge + */ + function completeOutboundTransfer( + address _localToken, + address _remoteToken, + address _to, + uint256 _tokenId + ) external onlySelf { + // Checks that the L1/L2 NFT pair has a token ID that is escrowed in the L1 Bridge. Without + // this check, an attacker could steal a legitimate L1 NFT by supplying an arbitrary L2 NFT + // that maps to the L1 NFT. require( deposits[_localToken][_remoteToken][_tokenId] == true, - "Token ID is not escrowed in the L1 Bridge" + "L1ERC721Bridge: token ID is not escrowed in l1 bridge for this l1/l2 nft pair" ); + // Mark that the token ID for this L1/L2 token pair is no longer escrowed in the L1 + // Bridge. deposits[_localToken][_remoteToken][_tokenId] = false; - // When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the withdrawer - // slither-disable-next-line reentrancy-events - IERC721(_localToken).transferFrom(address(this), _to, _tokenId); - - // slither-disable-next-line reentrancy-events - emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + // When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the + // withdrawer. + IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId); } /** @@ -220,7 +156,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { uint256 _tokenId, uint32 _minGasLimit, bytes calldata _extraData - ) internal { + ) internal override { + require(_remoteToken != address(0), "ERC721Bridge: remote token cannot be address(0)"); + // Construct calldata for _l2Token.finalizeBridgeERC721(_to, _tokenId) bytes memory message = abi.encodeWithSelector( L2ERC721Bridge.finalizeBridgeERC721.selector, @@ -237,7 +175,7 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { IERC721(_localToken).transferFrom(_from, address(this), _tokenId); // Send calldata into L2 - sendCrossDomainMessage(otherBridge, _minGasLimit, message); + messenger.sendMessage(otherBridge, message, _minGasLimit); emit ERC721BridgeInitiated(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); } } diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 237dcdd4de9cb..1af4dad8a2a3f 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -1,175 +1,45 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; -import { - CrossDomainEnabled -} from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; -import { - OwnableUpgradeable -} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; -import { Address } from "@openzeppelin/contracts/utils/Address.sol"; import { L1ERC721Bridge } from "../L1/L1ERC721Bridge.sol"; import { IOptimismMintableERC721 } from "../universal/op-erc721/IOptimismMintableERC721.sol"; import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semver.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; /** * @title L2ERC721Bridge * @notice The L2 ERC721 bridge is a contract which works together with the L1 ERC721 bridge to - * make it possible to transfer ERC721 tokens between Optimism and Ethereum. This contract + * make it possible to transfer ERC721 tokens from Ethereum to Optimism. This contract * acts as a minter for new tokens when it hears about deposits into the L1 ERC721 bridge. * This contract also acts as a burner for tokens being withdrawn. + * **WARNING**: Do not bridge an ERC721 that was originally deployed on Optimism. This + * bridge ONLY supports ERC721s originally deployed on Ethereum. Users will need to + * wait for the one-week challenge period to elapse before their Optimism-native NFT + * can be refunded on L2. */ -contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { +contract L2ERC721Bridge is ERC721Bridge, Semver { /** - * @notice Emitted when an ERC721 bridge to the other network is initiated. - * - * @param localToken Address of the token on this domain. - * @param remoteToken Address of the token on the remote domain. - * @param from Address that initiated bridging action. - * @param to Address to receive the token. - * @param tokenId ID of the specific token deposited. - * @param extraData Extra data for use on the client-side. - */ - event ERC721BridgeInitiated( - address indexed localToken, - address indexed remoteToken, - address indexed from, - address to, - uint256 tokenId, - bytes extraData - ); - - /** - * @notice Emitted when an ERC721 bridge from the other network is finalized. - * - * @param localToken Address of the token on this domain. - * @param remoteToken Address of the token on the remote domain. - * @param from Address that initiated bridging action. - * @param to Address to receive the token. - * @param tokenId ID of the specific token deposited. - * @param extraData Extra data for use on the client-side. - */ - event ERC721BridgeFinalized( - address indexed localToken, - address indexed remoteToken, - address indexed from, - address to, - uint256 tokenId, - bytes extraData - ); - - /** - * @notice Emitted when an ERC721 bridge from the other network fails. - * - * @param localToken Address of the token on this domain. - * @param remoteToken Address of the token on the remote domain. - * @param from Address that initiated bridging action. - * @param to Address to receive the token. - * @param tokenId ID of the specific token deposited. - * @param extraData Extra data for use on the client-side. - */ - event ERC721BridgeFailed( - address indexed localToken, - address indexed remoteToken, - address indexed from, - address to, - uint256 tokenId, - bytes extraData - ); - - /** - * @notice Address of the bridge on the other network. - */ - address public otherBridge; - - /** - * @custom:semver 0.0.1 + * @custom:semver 1.0.0 * * @param _messenger Address of the CrossDomainMessenger on this network. * @param _otherBridge Address of the ERC721 bridge on the other network. */ - constructor(address _messenger, address _otherBridge) - Semver(0, 0, 1) - CrossDomainEnabled(address(0)) - { + constructor(address _messenger, address _otherBridge) Semver(1, 0, 0) { initialize(_messenger, _otherBridge); } /** - * @param _messenger Address of the CrossDomainMessenger on this network. - * @param _otherBridge Address of the ERC721 bridge on the other network. - */ - function initialize(address _messenger, address _otherBridge) public initializer { - messenger = _messenger; - otherBridge = _otherBridge; - - // Initialize upgradable OZ contracts - __Ownable_init(); - } - - /** - * @notice Initiates a bridge of an NFT to the caller's account on L1. - * - * @param _localToken Address of the ERC721 on this domain. - * @param _remoteToken Address of the ERC721 on the remote domain. - * @param _tokenId Token ID to bridge. - * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. - * @param _extraData Optional data to forward to L1. Data supplied here will not be used to - * execute any code on L1 and is only emitted as extra data for the - * convenience of off-chain tooling. - */ - function bridgeERC721( - address _localToken, - address _remoteToken, - uint256 _tokenId, - uint32 _minGasLimit, - bytes calldata _extraData - ) external { - // Modifier requiring sender to be EOA. This check could be bypassed by a malicious - // contract via initcode, but it takes care of the user error we want to avoid. - require(!Address.isContract(msg.sender), "L2ERC721Bridge: account is not externally owned"); - - _initiateBridgeERC721( - _localToken, - _remoteToken, - msg.sender, - msg.sender, - _tokenId, - _minGasLimit, - _extraData - ); - } - - /** - * @notice Initiates a bridge of an NFT to some recipient's account on L1. + * @notice Initializer. * - * @param _localToken Address of the ERC721 on this domain. - * @param _remoteToken Address of the ERC721 on the remote domain. - * @param _to Address to receive the token on the other domain. - * @param _tokenId Token ID to bridge. - * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. - * @param _extraData Optional data to forward to L1. Data supplied here will not be used to - * execute any code on L1 and is only emitted as extra data for the - * convenience of off-chain tooling. + * @param _messenger Address of the L2CrossDomainMessenger. + * @param _otherBridge Address of the L1ERC721Bridge. */ - function bridgeERC721To( - address _localToken, - address _remoteToken, - address _to, - uint256 _tokenId, - uint32 _minGasLimit, - bytes calldata _extraData - ) external { - _initiateBridgeERC721( - _localToken, - _remoteToken, - msg.sender, - _to, - _tokenId, - _minGasLimit, - _extraData - ); + function initialize(address _messenger, address _otherBridge) public initializer { + require(_messenger != address(0), "ERC721Bridge: messenger cannot be address(0)"); + require(_otherBridge != address(0), "ERC721Bridge: other bridge cannot be address(0)"); + __ERC721Bridge_init(_messenger, _otherBridge); } /** @@ -192,26 +62,28 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { address _to, uint256 _tokenId, bytes calldata _extraData - ) external onlyFromCrossDomainAccount(otherBridge) { - // Check the target token is compliant and verify the deposited token on L1 matches the L2 - // deposited token representation. - if ( - // slither-disable-next-line reentrancy-events - ERC165Checker.supportsInterface( - _localToken, - type(IOptimismMintableERC721).interfaceId - ) && _remoteToken == IOptimismMintableERC721(_localToken).remoteToken() - ) { - // When a deposit is finalized, we give the NFT with the same tokenId to the account - // on L2. - // slither-disable-next-line reentrancy-events - IOptimismMintableERC721(_localToken).mint(_to, _tokenId); - // slither-disable-next-line reentrancy-events - emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); - } else { + ) external onlyOtherBridge { + try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { + if (_from == otherBridge) { + // The _from address is the address of the remote bridge if a transfer fails to be + // finalized on the remote chain. + // slither-disable-next-line reentrancy-events + emit ERC721Refunded(_localToken, _remoteToken, _to, _tokenId, _extraData); + } else { + // slither-disable-next-line reentrancy-events + emit ERC721BridgeFinalized( + _localToken, + _remoteToken, + _from, + _to, + _tokenId, + _extraData + ); + } + } catch { // Either the L2 token which is being deposited-into disagrees about the correct address // of its L1 token, or does not support the correct interface. - // This should only happen if there is a malicious L2 token, or if a user somehow + // This should only happen if there is a malicious L2 token, or if a user somehow // specified the wrong L2 token address to deposit into. // In either case, we stop the process here and construct a withdrawal // message so that users can get their NFT out in some cases. @@ -221,15 +93,16 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { L1ERC721Bridge.finalizeBridgeERC721.selector, _remoteToken, _localToken, - _to, // switched the _to and _from here to bounce back the deposit to the sender - _from, + address(this), // Set the new _from address to be this contract since the NFT was + // never transferred to the recipient on this chain. + _from, // Refund the NFT to the original owner on the remote chain. _tokenId, _extraData ); // Send message up to L1 bridge // slither-disable-next-line reentrancy-events - sendCrossDomainMessage(otherBridge, 0, message); + messenger.sendMessage(otherBridge, message, 0); // slither-disable-next-line reentrancy-events emit ERC721BridgeFailed(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); @@ -237,17 +110,40 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { } /** - * @notice Internal function for initiating a token bridge to the other domain. + * @notice Completes an outbound token transfer. Public function, but can only be called by + * this contract. It's security critical that there be absolutely no way for anyone to + * trigger this function, except by explicit trigger within this contract. Used as a + * simple way to be able to try/catch any type of revert that could occur during an + * ERC721 mint/transfer. * - * @param _localToken Address of the ERC721 on this domain. - * @param _remoteToken Address of the ERC721 on the remote domain. - * @param _from Address of the sender on this domain. - * @param _to Address to receive the token on the other domain. - * @param _tokenId Token ID to bridge. - * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. - * @param _extraData Optional data to forward to L1. Data supplied here will not be used to - * execute any code on L1 and is only emitted as extra data for the - * convenience of off-chain tooling. + * @param _localToken Address of the ERC721 on this chain. + * @param _remoteToken Address of the corresponding token on the remote chain. + * @param _to Address of the receiver. + * @param _tokenId ID of the token being deposited. + */ + function completeOutboundTransfer( + address _localToken, + address _remoteToken, + address _to, + uint256 _tokenId + ) external onlySelf { + require( + // slither-disable-next-line reentrancy-events + ERC165Checker.supportsInterface(_localToken, type(IOptimismMintableERC721).interfaceId), + "L2ERC721Bridge: local token interface is not compliant" + ); + require( + _remoteToken == IOptimismMintableERC721(_localToken).remoteToken(), + "L2ERC721Bridge: wrong remote token for Optimism Mintable ERC721 local token" + ); + + // When a deposit is finalized, we give the NFT with the same tokenId to the account + // on L2. + IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId); + } + + /** + * @inheritdoc ERC721Bridge */ function _initiateBridgeERC721( address _localToken, @@ -257,18 +153,15 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { uint256 _tokenId, uint32 _minGasLimit, bytes calldata _extraData - ) internal { + ) internal override { + require(_remoteToken != address(0), "ERC721Bridge: remote token cannot be address(0)"); + // Check that the withdrawal is being initiated by the NFT owner require( _from == IOptimismMintableERC721(_localToken).ownerOf(_tokenId), "Withdrawal is not being initiated by NFT owner" ); - // When a withdrawal is initiated, we burn the withdrawer's NFT to prevent subsequent L2 - // usage - // slither-disable-next-line reentrancy-events - IOptimismMintableERC721(_localToken).burn(_from, _tokenId); - // Construct calldata for l1ERC721Bridge.finalizeBridgeERC721(_to, _tokenId) // slither-disable-next-line reentrancy-events address remoteToken = IOptimismMintableERC721(_localToken).remoteToken(); @@ -277,6 +170,11 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { "L2ERC721Bridge: remote token does not match given value" ); + // When a withdrawal is initiated, we burn the withdrawer's NFT to prevent subsequent L2 + // usage + // slither-disable-next-line reentrancy-events + IOptimismMintableERC721(_localToken).burn(_from, _tokenId); + bytes memory message = abi.encodeWithSelector( L1ERC721Bridge.finalizeBridgeERC721.selector, remoteToken, @@ -289,7 +187,7 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { // Send message to L1 bridge // slither-disable-next-line reentrancy-events - sendCrossDomainMessage(otherBridge, _minGasLimit, message); + messenger.sendMessage(otherBridge, message, _minGasLimit); // slither-disable-next-line reentrancy-events emit ERC721BridgeInitiated(_localToken, remoteToken, _from, _to, _tokenId, _extraData); diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol new file mode 100644 index 0000000000000..25dc87f8001fd --- /dev/null +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -0,0 +1,243 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import { + CrossDomainMessenger +} from "@eth-optimism/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol"; +import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import { Address } from "@openzeppelin/contracts/utils/Address.sol"; + +/** + * @title ERC721Bridge + * @notice ERC721Bridge is a base contract for the L1 and L2 ERC721 bridges. + */ +abstract contract ERC721Bridge is Initializable { + /** + * @notice Messenger contract on this domain. + */ + CrossDomainMessenger public messenger; + + /** + * @notice Emitted when an ERC721 bridge to the other network is initiated. + * + * @param localToken Address of the token on this domain. + * @param remoteToken Address of the token on the remote domain. + * @param from Address that initiated bridging action. + * @param to Address to receive the token. + * @param tokenId ID of the specific token deposited. + * @param extraData Extra data for use on the client-side. + */ + event ERC721BridgeInitiated( + address indexed localToken, + address indexed remoteToken, + address indexed from, + address to, + uint256 tokenId, + bytes extraData + ); + + /** + * @notice Emitted when an ERC721 bridge from the other network is finalized. + * + * @param localToken Address of the token on this domain. + * @param remoteToken Address of the token on the remote domain. + * @param from Address that initiated bridging action. + * @param to Address to receive the token. + * @param tokenId ID of the specific token deposited. + * @param extraData Extra data for use on the client-side. + */ + event ERC721BridgeFinalized( + address indexed localToken, + address indexed remoteToken, + address indexed from, + address to, + uint256 tokenId, + bytes extraData + ); + + /** + * @notice Emitted when an ERC721 bridge from the other network fails. + * + * @param localToken Address of the token on this domain. + * @param remoteToken Address of the token on the remote domain. + * @param from Address that initiated bridging action. + * @param to Address to receive the token. + * @param tokenId ID of the specific token deposited. + * @param extraData Extra data for use on the client-side. + */ + event ERC721BridgeFailed( + address indexed localToken, + address indexed remoteToken, + address indexed from, + address to, + uint256 tokenId, + bytes extraData + ); + + /** + * @notice Emitted when an NFT is refunded to the owner after an ERC721 bridge from the other + * chain fails. + * + * @param localToken Address of the token on this domain. + * @param remoteToken Address of the token on the remote domain. + * @param to Address to receive the refunded token. + * @param tokenId ID of the specific token being refunded. + * @param extraData Extra data for use on the client-side. + */ + event ERC721Refunded( + address indexed localToken, + address indexed remoteToken, + address indexed to, + uint256 tokenId, + bytes extraData + ); + + /** + * @notice Address of the bridge on the other network. + */ + address public otherBridge; + + /** + * @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades. + */ + uint256[49] private __gap; + + /** + * @notice Ensures that the caller is a cross-chain message from the other bridge. + */ + modifier onlyOtherBridge() { + require( + msg.sender == address(messenger) && messenger.xDomainMessageSender() == otherBridge, + "ERC721Bridge: function can only be called from the other bridge" + ); + _; + } + + /** + * @notice Ensures that the caller is this contract. + */ + modifier onlySelf() { + require(msg.sender == address(this), "ERC721Bridge: function can only be called by self"); + _; + } + + /** + * @notice Initiates a bridge of an NFT to the caller's account on the remote chain. Note that + * the current owner of the token on this chain must approve this contract to operate + * the NFT before it can be bridged. Also note that this function can only be called by + * EOAs. Smart contract wallets should use the `bridgeERC721To` function after ensuring + * that the recipient address on the remote chain exists. + * **WARNING**: Do not bridge an ERC721 that was originally deployed on Optimism. This + * bridge only supports ERC721s originally deployed on Ethereum. Users will need to + * wait for the one-week challenge period to elapse before their Optimism-native NFT + * can be refunded on L2. + * + * @param _localToken Address of the ERC721 on this domain. + * @param _remoteToken Address of the ERC721 on the remote domain. + * @param _tokenId Token ID to bridge. + * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. + * @param _extraData Optional data to forward to the other domain. Data supplied here will + * not be used to execute any code on the other domain and is only emitted + * as extra data for the convenience of off-chain tooling. + */ + function bridgeERC721( + address _localToken, + address _remoteToken, + uint256 _tokenId, + uint32 _minGasLimit, + bytes calldata _extraData + ) external { + // Modifier requiring sender to be EOA. This prevents against a user error that would occur + // if the sender is a smart contract wallet that has a different address on the remote chain + // (or doesn't have an address on the remote chain at all). The user would fail to receive + // the NFT if they use this function because it sends the NFT to the same address as the + // caller. This check could be bypassed by a malicious contract via initcode, but it takes + // care of the user error we want to avoid. + require(!Address.isContract(msg.sender), "ERC721Bridge: account is not externally owned"); + + _initiateBridgeERC721( + _localToken, + _remoteToken, + msg.sender, + msg.sender, + _tokenId, + _minGasLimit, + _extraData + ); + } + + /** + * @notice Initiates a bridge of an NFT to some recipient's account on L2. Note that the current + * owner of the token on this chain must approve this contract to operate the NFT before + * it can be bridged. + * **WARNING**: Do not bridge an ERC721 that was originally deployed on Optimism. This + * bridge only supports ERC721s originally deployed on Ethereum. Users will need to + * wait for the one-week challenge period to elapse before their Optimism-native NFT + * can be refunded on L2. + * + * @param _localToken Address of the ERC721 on this domain. + * @param _remoteToken Address of the ERC721 on the remote domain. + * @param _to Address to receive the token on the other domain. + * @param _tokenId Token ID to bridge. + * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. + * @param _extraData Optional data to forward to the other domain. Data supplied here will + * not be used to execute any code on the other domain and is only emitted + * as extra data for the convenience of off-chain tooling. + */ + function bridgeERC721To( + address _localToken, + address _remoteToken, + address _to, + uint256 _tokenId, + uint32 _minGasLimit, + bytes calldata _extraData + ) external { + _initiateBridgeERC721( + _localToken, + _remoteToken, + msg.sender, + _to, + _tokenId, + _minGasLimit, + _extraData + ); + } + + /** + * @notice Initializer. + * + * @param _messenger Address of the CrossDomainMessenger on this network. + * @param _otherBridge Address of the ERC721 bridge on the other network. + */ + // solhint-disable-next-line func-name-mixedcase + function __ERC721Bridge_init(address _messenger, address _otherBridge) + internal + onlyInitializing + { + messenger = CrossDomainMessenger(_messenger); + otherBridge = _otherBridge; + } + + /** + * @notice Internal function for initiating a token bridge to the other domain. + * + * @param _localToken Address of the ERC721 on this domain. + * @param _remoteToken Address of the ERC721 on the remote domain. + * @param _from Address of the sender on this domain. + * @param _to Address to receive the token on the other domain. + * @param _tokenId Token ID to bridge. + * @param _minGasLimit Minimum gas limit for the bridge message on the other domain. + * @param _extraData Optional data to forward to the other domain. Data supplied here will + * not be used to execute any code on the other domain and is only emitted + * as extra data for the convenience of off-chain tooling. + */ + function _initiateBridgeERC721( + address _localToken, + address _remoteToken, + address _from, + address _to, + uint256 _tokenId, + uint32 _minGasLimit, + bytes calldata _extraData + ) internal virtual; +} diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol index c801cc79b272d..a37dbd3d6ac58 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol @@ -30,25 +30,26 @@ interface IOptimismMintableERC721 is IERC721Enumerable { /** * @notice Chain ID of the chain where the remote token is deployed. */ - function remoteChainId() external returns (uint256); + function remoteChainId() external view returns (uint256); /** * @notice Address of the token on the remote domain. */ - function remoteToken() external returns (address); + function remoteToken() external view returns (address); /** * @notice Address of the ERC721 bridge on this network. */ - function bridge() external returns (address); + function bridge() external view returns (address); /** - * @notice Mints some token ID for a user. + * @notice Mints some token ID for a user, checking first that contract recipients + * are aware of the ERC721 protocol to prevent tokens from being forever locked. * * @param _to Address of the user to mint the token for. * @param _tokenId Token ID to mint. */ - function mint(address _to, uint256 _tokenId) external; + function safeMint(address _to, uint256 _tokenId) external; /** * @notice Burns a token ID from a user. diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol index de8d1cc7de266..0307a8f580a17 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol @@ -50,6 +50,13 @@ contract OptimismMintableERC721 is ERC721Enumerable, IOptimismMintableERC721 { string memory _name, string memory _symbol ) ERC721(_name, _symbol) { + require(_bridge != address(0), "OptimismMintableERC721: bridge cannot be address(0)"); + require(_remoteChainId != 0, "OptimismMintableERC721: remote chain id cannot be zero"); + require( + _remoteToken != address(0), + "OptimismMintableERC721: remote token cannot be address(0)" + ); + remoteChainId = _remoteChainId; remoteToken = _remoteToken; bridge = _bridge; @@ -78,8 +85,8 @@ contract OptimismMintableERC721 is ERC721Enumerable, IOptimismMintableERC721 { /** * @inheritdoc IOptimismMintableERC721 */ - function mint(address _to, uint256 _tokenId) external virtual onlyBridge { - _mint(_to, _tokenId); + function safeMint(address _to, uint256 _tokenId) external virtual onlyBridge { + _safeMint(_to, _tokenId); emit Mint(_to, _tokenId); } diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol index ea5315dc41db3..ddff77b324c6d 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol @@ -1,9 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; -import { - OwnableUpgradeable -} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; import { OptimismMintableERC721 } from "./OptimismMintableERC721.sol"; import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semver.sol"; @@ -11,7 +9,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv * @title OptimismMintableERC721Factory * @notice Factory contract for creating OptimismMintableERC721 contracts. */ -contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { +contract OptimismMintableERC721Factory is Semver, Initializable { /** * @notice Emitted whenever a new OptimismMintableERC721 contract is created. * @@ -36,11 +34,11 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { mapping(address => bool) public isStandardOptimismMintableERC721; /** - * @custom:semver 0.0.1 + * @custom:semver 1.0.0 * * @param _bridge Address of the ERC721 bridge on this network. */ - constructor(address _bridge, uint256 _remoteChainId) Semver(0, 0, 1) { + constructor(address _bridge, uint256 _remoteChainId) Semver(1, 0, 0) { initialize(_bridge, _remoteChainId); } @@ -50,11 +48,17 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { * @param _bridge Address of the ERC721 bridge on this network. */ function initialize(address _bridge, uint256 _remoteChainId) public initializer { + require( + _bridge != address(0), + "OptimismMintableERC721Factory: bridge cannot be address(0)" + ); + require( + _remoteChainId != 0, + "OptimismMintableERC721Factory: remote chain id cannot be zero" + ); + bridge = _bridge; remoteChainId = _remoteChainId; - - // Initialize upgradable OZ contracts - __Ownable_init(); } /** diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index e0f2046f0fcdb..c3b5c70c21e82 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -13,9 +13,8 @@ import ICrossDomainMessenger from '@eth-optimism/contracts/artifacts/contracts/l import { NON_NULL_BYTES32, NON_ZERO_ADDRESS } from '../../helpers' import { expect } from '../../setup' -const ERR_INVALID_MESSENGER = 'OVM_XCHAIN: messenger contract unauthenticated' -const ERR_INVALID_X_DOMAIN_MSG_SENDER = - 'OVM_XCHAIN: wrong sender of cross-domain message' +const ERR_INVALID_X_DOMAIN_MESSAGE = + 'ERC721Bridge: function can only be called from the other bridge' const DUMMY_L2_ERC721_ADDRESS = ethers.utils.getAddress( '0x' + 'abba'.repeat(10) ) @@ -23,7 +22,7 @@ const DUMMY_L2_BRIDGE_ADDRESS = ethers.utils.getAddress( '0x' + 'acdc'.repeat(10) ) -const FINALIZATION_GAS = 1_200_000 +const FINALIZATION_GAS = 600_000 describe('L1ERC721Bridge', () => { // init signers @@ -59,6 +58,7 @@ describe('L1ERC721Bridge', () => { let L1ERC721: MockContract let L1ERC721Bridge: Contract let Fake__L1CrossDomainMessenger: FakeContract + let Factory__L1ERC721Bridge: ContractFactory beforeEach(async () => { // Get a new mock L1 messenger Fake__L1CrossDomainMessenger = await smock.fake( @@ -67,9 +67,11 @@ describe('L1ERC721Bridge', () => { ) // Deploy the contract under test - L1ERC721Bridge = await ( - await ethers.getContractFactory('L1ERC721Bridge') - ).deploy(Fake__L1CrossDomainMessenger.address, DUMMY_L2_BRIDGE_ADDRESS) + Factory__L1ERC721Bridge = await ethers.getContractFactory('L1ERC721Bridge') + L1ERC721Bridge = await Factory__L1ERC721Bridge.deploy( + Fake__L1CrossDomainMessenger.address, + DUMMY_L2_BRIDGE_ADDRESS + ) L1ERC721 = await Factory__L1ERC721.deploy('L1ERC721', 'ERC') @@ -81,11 +83,50 @@ describe('L1ERC721Bridge', () => { }) }) + describe('initialize', async () => { + it('reverts if messenger is address(0)', async () => { + await expect( + Factory__L1ERC721Bridge.deploy( + constants.AddressZero, + DUMMY_L2_BRIDGE_ADDRESS + ) + ).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)') + }) + + it('reverts if otherBridge is address(0)', async () => { + await expect( + Factory__L1ERC721Bridge.deploy( + Fake__L1CrossDomainMessenger.address, + constants.AddressZero + ) + ).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)') + }) + + it('initializes correctly', async () => { + expect(await L1ERC721Bridge.messenger()).equals( + Fake__L1CrossDomainMessenger.address + ) + expect(await L1ERC721Bridge.otherBridge()).equals(DUMMY_L2_BRIDGE_ADDRESS) + }) + }) + describe('ERC721 deposits', () => { beforeEach(async () => { await L1ERC721.connect(alice).approve(L1ERC721Bridge.address, tokenId) }) + it('bridgeERC721() reverts if remote token is address(0)', async () => { + await expect( + L1ERC721Bridge.connect(alice).bridgeERC721( + L1ERC721.address, + constants.AddressZero, + tokenId, + FINALIZATION_GAS, + NON_NULL_BYTES32 + ) + ).to.be.revertedWith('ERC721Bridge: remote token cannot be address(0)') + }) + it('bridgeERC721() escrows the deposit and sends the correct deposit message', async () => { // alice calls deposit on the bridge and the L1 bridge calls transferFrom on the token. // emits an ERC721BridgeInitiated event with the correct arguments. @@ -147,6 +188,19 @@ describe('L1ERC721Bridge', () => { ).to.equal(true) }) + it('bridgeERC721To() reverts if NFT receiver is address(0)', async () => { + await expect( + L1ERC721Bridge.connect(alice).bridgeERC721To( + L1ERC721.address, + DUMMY_L2_ERC721_ADDRESS, + constants.AddressZero, + tokenId, + FINALIZATION_GAS, + NON_NULL_BYTES32 + ) + ).to.be.revertedWith('ERC721Bridge: nft recipient cannot be address(0)') + }) + it('bridgeERC721To() escrows the deposited NFT and sends the correct deposit message', async () => { // depositor calls deposit on the bridge and the L1 bridge calls transferFrom on the token. // emits an ERC721BridgeInitiated event with the correct arguments. @@ -222,7 +276,7 @@ describe('L1ERC721Bridge', () => { FINALIZATION_GAS, NON_NULL_BYTES32 ) - ).to.be.revertedWith('L1ERC721Bridge: account is not externally owned') + ).to.be.revertedWith('ERC721Bridge: account is not externally owned') }) describe('Handling ERC721.transferFrom() failures that revert', () => { @@ -291,7 +345,7 @@ describe('L1ERC721Bridge', () => { tokenId, NON_NULL_BYTES32 ) - ).to.be.revertedWith(ERR_INVALID_MESSENGER) + ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE) }) it('onlyFromCrossDomainAccount: should revert on calls from the right crossDomainMessenger, but wrong xDomainMessageSender (ie. not the L2DepositedERC721)', async () => { @@ -307,7 +361,7 @@ describe('L1ERC721Bridge', () => { from: Fake__L1CrossDomainMessenger.address, } ) - ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MSG_SENDER) + ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE) }) describe('withdrawal attempts that pass the onlyFromCrossDomainAccount check', () => { @@ -331,23 +385,57 @@ describe('L1ERC721Bridge', () => { ) }) - it('should revert if the l1/l2 token pair has a token ID that has not been escrowed in the l1 bridge', async () => { + it('should refund an L2 NFT that fails to be finalized on l1', async () => { + const RANDOM_L1_ERC721_ADDRESS = ethers.utils.getAddress( + '0x' + 'cdbc'.repeat(10) + ) + // alice sends bob an nft that has an incorrect l1 erc721 address await expect( L1ERC721Bridge.finalizeBridgeERC721( - L1ERC721.address, - DUMMY_L2_BRIDGE_ADDRESS, // incorrect l2 token address - constants.AddressZero, - constants.AddressZero, + RANDOM_L1_ERC721_ADDRESS, // incorrect address for the l1 erc721 + DUMMY_L2_ERC721_ADDRESS, + aliceAddress, + bobsAddress, tokenId, NON_NULL_BYTES32, { from: Fake__L1CrossDomainMessenger.address, } ) - ).to.be.revertedWith('Token ID is not escrowed in the L1 Bridge') + ) + .to.emit(L1ERC721Bridge, 'ERC721BridgeFailed') + .withArgs( + RANDOM_L1_ERC721_ADDRESS, + DUMMY_L2_ERC721_ADDRESS, + aliceAddress, + bobsAddress, + tokenId, + NON_NULL_BYTES32 + ) + + // Get the second call from `Fake__L1CrossDomainMessenger` because the first call is `finalizeBridgeERC721`. + const depositCallToMessenger = + Fake__L1CrossDomainMessenger.sendMessage.getCall(1) + + // Check the correct cross-chain call was sent: + // Message should be sent to the L2 bridge + expect(depositCallToMessenger.args[0]).to.equal(DUMMY_L2_BRIDGE_ADDRESS) + // Message data should be a call telling the L2DepositedERC721 to finalize the deposit + expect(depositCallToMessenger.args[1]).to.equal( + IL2ERC721Bridge.encodeFunctionData('finalizeBridgeERC721', [ + DUMMY_L2_ERC721_ADDRESS, + RANDOM_L1_ERC721_ADDRESS, + L1ERC721Bridge.address, // the 'from' address is the l1 bridge to indicate that the l1 recipient never controlled the nft. + aliceAddress, + tokenId, + NON_NULL_BYTES32, + ]) + ) + // Gas limit is 0 + expect(depositCallToMessenger.args[2]).to.equal(FINALIZATION_GAS) }) - it('should credit funds to the withdrawer and not use too much gas', async () => { + it('should credit funds to the withdrawer to finalize withdrawal', async () => { // finalizing the withdrawal emits an ERC721BridgeFinalized event with the correct arguments. await expect( L1ERC721Bridge.finalizeBridgeERC721( @@ -382,6 +470,54 @@ describe('L1ERC721Bridge', () => { ) ).to.equal(false) }) + + it('should credit funds to the withdrawer to finalize refund', async () => { + // finalizing a refund emits an ERC721Refunded event with the correct arguments. + await expect( + L1ERC721Bridge.finalizeBridgeERC721( + L1ERC721.address, + DUMMY_L2_ERC721_ADDRESS, + DUMMY_L2_BRIDGE_ADDRESS, // _from is the l2 bridge for refunds + NON_ZERO_ADDRESS, + tokenId, + NON_NULL_BYTES32, + { from: Fake__L1CrossDomainMessenger.address } + ) + ) + .to.emit(L1ERC721Bridge, 'ERC721Refunded') + .withArgs( + L1ERC721.address, + DUMMY_L2_ERC721_ADDRESS, + NON_ZERO_ADDRESS, + tokenId, + NON_NULL_BYTES32 + ) + + // NFT is transferred to new owner + expect(await L1ERC721.ownerOf(tokenId)).to.equal(NON_ZERO_ADDRESS) + + // deposits state variable is updated + expect( + await L1ERC721Bridge.deposits( + L1ERC721.address, + DUMMY_L2_ERC721_ADDRESS, + tokenId + ) + ).to.equal(false) + }) + }) + }) + + describe('completeOutboundTransfer', async () => { + it('reverts if caller is not L1 bridge', async () => { + await expect( + L1ERC721Bridge.completeOutboundTransfer( + L1ERC721.address, + DUMMY_L2_ERC721_ADDRESS, + bobsAddress, + tokenId + ) + ).to.be.revertedWith('ERC721Bridge: function can only be called by self') }) }) }) diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 687a12806fc0f..344e59e103fd9 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -9,9 +9,8 @@ import { NON_NULL_BYTES32, NON_ZERO_ADDRESS } from '../../helpers' import { expect } from '../../setup' const ERR_ALREADY_INITIALIZED = 'Initializable: contract is already initialized' -const ERR_INVALID_MESSENGER = 'OVM_XCHAIN: messenger contract unauthenticated' -const ERR_INVALID_X_DOMAIN_MSG_SENDER = - 'OVM_XCHAIN: wrong sender of cross-domain message' +const ERR_INVALID_X_DOMAIN_MESSAGE = + 'ERC721Bridge: function can only be called from the other bridge' const DUMMY_L1BRIDGE_ADDRESS: string = '0x1234123412341234123412341234123412341234' const DUMMY_L1ERC721_ADDRESS: string = @@ -20,6 +19,8 @@ const ERR_INVALID_WITHDRAWAL: string = 'Withdrawal is not being initiated by NFT owner' const TOKEN_ID: number = 10 +const FINALIZATION_GAS = 600_000 + describe('L2ERC721Bridge', () => { let alice: Signer let aliceAddress: string @@ -74,6 +75,31 @@ describe('L2ERC721Bridge', () => { ) ).to.be.revertedWith(ERR_ALREADY_INITIALIZED) }) + + it('reverts if messenger is address(0)', async () => { + await expect( + Factory__L1ERC721Bridge.deploy( + constants.AddressZero, + DUMMY_L1BRIDGE_ADDRESS + ) + ).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)') + }) + + it('reverts if otherBridge is address(0)', async () => { + await expect( + Factory__L1ERC721Bridge.deploy( + Fake__L2CrossDomainMessenger.address, + constants.AddressZero + ) + ).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)') + }) + + it('initializes correctly', async () => { + expect(await L2ERC721Bridge.messenger()).equals( + Fake__L2CrossDomainMessenger.address + ) + expect(await L2ERC721Bridge.otherBridge()).equals(DUMMY_L1BRIDGE_ADDRESS) + }) }) // test the transfer flow of moving a token from L1 to L2 @@ -88,7 +114,7 @@ describe('L2ERC721Bridge', () => { TOKEN_ID, NON_NULL_BYTES32 ) - ).to.be.revertedWith(ERR_INVALID_MESSENGER) + ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE) }) it('onlyFromCrossDomainAccount: should revert on calls from the right crossDomainMessenger, but wrong xDomainMessageSender (ie. not the L1ERC721Bridge)', async () => { @@ -108,7 +134,7 @@ describe('L2ERC721Bridge', () => { from: Fake__L2CrossDomainMessenger.address, } ) - ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MSG_SENDER) + ).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE) }) it('should initialize a withdrawal if the L2 token is not compliant', async () => { @@ -157,7 +183,7 @@ describe('L2ERC721Bridge', () => { [ DUMMY_L1ERC721_ADDRESS, NonCompliantERC721.address, - bobsAddress, + L2ERC721Bridge.address, // the 'from' address is the l2 bridge to indicate that the l2 recipient never controlled the nft. aliceAddress, TOKEN_ID, NON_NULL_BYTES32, @@ -213,6 +239,54 @@ describe('L2ERC721Bridge', () => { const tokenIdOwner = await L2ERC721.ownerOf(TOKEN_ID) tokenIdOwner.should.equal(bobsAddress) }) + + it('should credit funds to the depositer to finalize refund', async () => { + Fake__L2CrossDomainMessenger.xDomainMessageSender.returns( + DUMMY_L1BRIDGE_ADDRESS + ) + + // Assert that nobody owns the L2 token initially + await expect(L2ERC721.ownerOf(TOKEN_ID)).to.be.revertedWith( + 'ERC721: owner query for nonexistent token' + ) + + // finalizing a refund emits an ERC721Refunded event with the correct arguments. + await expect( + L2ERC721Bridge.connect(l2MessengerImpersonator).finalizeBridgeERC721( + L2ERC721.address, + DUMMY_L1ERC721_ADDRESS, + DUMMY_L1BRIDGE_ADDRESS, // _from is the l1 bridge for refunds + aliceAddress, + TOKEN_ID, + NON_NULL_BYTES32, + { from: Fake__L2CrossDomainMessenger.address } + ) + ) + .to.emit(L2ERC721Bridge, 'ERC721Refunded') + .withArgs( + L2ERC721.address, + DUMMY_L1ERC721_ADDRESS, + aliceAddress, + TOKEN_ID, + NON_NULL_BYTES32 + ) + + // NFT is transferred to new owner + expect(await L2ERC721.ownerOf(TOKEN_ID)).to.equal(aliceAddress) + }) + }) + + describe('completeOutboundTransfer', async () => { + it('reverts if caller is not L2 bridge', async () => { + await expect( + L2ERC721Bridge.completeOutboundTransfer( + L2ERC721.address, + DUMMY_L1ERC721_ADDRESS, + bobsAddress, + TOKEN_ID + ) + ).to.be.revertedWith('ERC721Bridge: function can only be called by self') + }) }) describe('withdrawals', () => { @@ -238,7 +312,19 @@ describe('L2ERC721Bridge', () => { ]) const signer = await ethers.getSigner(L2ERC721Bridge.address) - await L2Token.connect(signer).mint(aliceAddress, TOKEN_ID) + await L2Token.connect(signer).safeMint(aliceAddress, TOKEN_ID) + }) + + it('bridgeERC721() reverts if remote token is address(0)', async () => { + await expect( + L2ERC721Bridge.connect(alice).bridgeERC721( + L2Token.address, + constants.AddressZero, + TOKEN_ID, + FINALIZATION_GAS, + NON_NULL_BYTES32 + ) + ).to.be.revertedWith('ERC721Bridge: remote token cannot be address(0)') }) it('bridgeERC721() reverts when called by non-owner of nft', async () => { @@ -262,7 +348,7 @@ describe('L2ERC721Bridge', () => { 0, NON_NULL_BYTES32 ) - ).to.be.revertedWith('L2ERC721Bridge: account is not externally owned') + ).to.be.revertedWith('ERC721Bridge: account is not externally owned') }) it('bridgeERC721() burns and sends the correct withdrawal message', async () => { @@ -331,6 +417,19 @@ describe('L2ERC721Bridge', () => { expect(withdrawalCallToMessenger.args[2]).to.equal(0) }) + it('bridgeERC721To() reverts if NFT receiver is address(0)', async () => { + await expect( + L2ERC721Bridge.connect(alice).bridgeERC721To( + L2Token.address, + DUMMY_L1ERC721_ADDRESS, + constants.AddressZero, + TOKEN_ID, + 0, + NON_NULL_BYTES32 + ) + ).to.be.revertedWith('ERC721Bridge: nft recipient cannot be address(0)') + }) + it('bridgeERC721To() reverts when called by non-owner of nft', async () => { await expect( L2ERC721Bridge.connect(bob).bridgeERC721To( diff --git a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts index e54296dd170cf..19ac72d6f5619 100644 --- a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts +++ b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts @@ -20,6 +20,7 @@ describe('OptimismMintableERC721', () => { let baseUri: string const remoteChainId = 100 + let Factory__OptimismMintableERC721 before(async () => { ;[l2BridgeImpersonator, alice] = await ethers.getSigners() l2BridgeImpersonatorAddress = await l2BridgeImpersonator.getAddress() @@ -33,15 +34,15 @@ describe('OptimismMintableERC721', () => { '/tokenURI?uint256=' ) - OptimismMintableERC721 = await ( - await ethers.getContractFactory('OptimismMintableERC721') - ).deploy( + Factory__OptimismMintableERC721 = await ethers.getContractFactory( + 'OptimismMintableERC721' + ) + OptimismMintableERC721 = await Factory__OptimismMintableERC721.deploy( l2BridgeImpersonatorAddress, remoteChainId, DUMMY_L1ERC721_ADDRESS, 'L2ERC721', - 'ERC', - { gasLimit: 4_000_000 } // Necessary to avoid an out-of-gas error + 'ERC' ) // Get a new fake L2 bridge @@ -52,7 +53,7 @@ describe('OptimismMintableERC721', () => { ) // mint an nft to alice - await OptimismMintableERC721.connect(l2BridgeImpersonator).mint( + await OptimismMintableERC721.connect(l2BridgeImpersonator).safeMint( aliceAddress, TOKEN_ID, { @@ -62,6 +63,48 @@ describe('OptimismMintableERC721', () => { }) describe('constructor', () => { + it('should revert if bridge is address(0)', async () => { + await expect( + Factory__OptimismMintableERC721.deploy( + ethers.constants.AddressZero, + remoteChainId, + DUMMY_L1ERC721_ADDRESS, + 'L2ERC721', + 'ERC' + ) + ).to.be.revertedWith( + 'OptimismMintableERC721: bridge cannot be address(0)' + ) + }) + + it('should revert if remote chain id is address(0)', async () => { + await expect( + Factory__OptimismMintableERC721.deploy( + l2BridgeImpersonatorAddress, + 0, + DUMMY_L1ERC721_ADDRESS, + 'L2ERC721', + 'ERC' + ) + ).to.be.revertedWith( + 'OptimismMintableERC721: remote chain id cannot be zero' + ) + }) + + it('should revert if remote token is address(0)', async () => { + await expect( + Factory__OptimismMintableERC721.deploy( + l2BridgeImpersonatorAddress, + remoteChainId, + ethers.constants.AddressZero, + 'L2ERC721', + 'ERC' + ) + ).to.be.revertedWith( + 'OptimismMintableERC721: remote token cannot be address(0)' + ) + }) + it('should be able to create a standard ERC721 contract with the correct parameters', async () => { expect(await OptimismMintableERC721.bridge()).to.equal( l2BridgeImpersonatorAddress @@ -83,7 +126,7 @@ describe('OptimismMintableERC721', () => { describe('mint and burn', () => { it('should not allow anyone but the L2 bridge to mint and burn', async () => { await expect( - OptimismMintableERC721.connect(alice).mint(aliceAddress, 100) + OptimismMintableERC721.connect(alice).safeMint(aliceAddress, 100) ).to.be.revertedWith( 'OptimismMintableERC721: only bridge can call this function' ) @@ -102,7 +145,7 @@ describe('OptimismMintableERC721', () => { .true // OptimismMintableERC721 - expect(await OptimismMintableERC721.supportsInterface(0x051e4975)).to.be + expect(await OptimismMintableERC721.supportsInterface(0xe49bc7f8)).to.be .true // ERC721 diff --git a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts index 5e88038541e8e..b685e4d094f92 100644 --- a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts +++ b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721Factory.spec.ts @@ -22,6 +22,7 @@ describe('OptimismMintableERC721Factory', () => { let baseURI: string const remoteChainId = 100 + let Factory__OptimismMintableERC721Factory: ContractFactory beforeEach(async () => { ;[signer] = await ethers.getSigners() @@ -31,9 +32,14 @@ describe('OptimismMintableERC721Factory', () => { ) L1ERC721 = await Factory__L1ERC721.deploy('L1ERC721', 'ERC') - OptimismMintableERC721Factory = await ( - await ethers.getContractFactory('OptimismMintableERC721Factory') - ).deploy(DUMMY_L2_BRIDGE_ADDRESS, remoteChainId) + Factory__OptimismMintableERC721Factory = await ethers.getContractFactory( + 'OptimismMintableERC721Factory' + ) + OptimismMintableERC721Factory = + await Factory__OptimismMintableERC721Factory.deploy( + DUMMY_L2_BRIDGE_ADDRESS, + remoteChainId + ) baseURI = ''.concat( 'ethereum:', @@ -44,6 +50,25 @@ describe('OptimismMintableERC721Factory', () => { ) }) + it('should revert if bridge is initialized as address(0)', async () => { + await expect( + Factory__OptimismMintableERC721Factory.deploy( + ethers.constants.AddressZero, + remoteChainId + ) + ).to.be.revertedWith( + 'OptimismMintableERC721Factory: bridge cannot be address(0)' + ) + }) + + it('should revert if remote chain id is initialized as zero', async () => { + await expect( + Factory__OptimismMintableERC721Factory.deploy(DUMMY_L2_BRIDGE_ADDRESS, 0) + ).to.be.revertedWith( + 'OptimismMintableERC721Factory: remote chain id cannot be zero' + ) + }) + it('should be deployed with the correct constructor argument', async () => { expect(await OptimismMintableERC721Factory.bridge()).to.equal( DUMMY_L2_BRIDGE_ADDRESS