From 4e9fe32722356a823c614eb331907cbc0ad19902 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 15 Aug 2022 12:08:48 -0700 Subject: [PATCH 1/8] nft: L-13 Duplicated code --- .../contracts/L1/L1ERC721Bridge.sol | 138 +------------- .../contracts/L2/L2ERC721Bridge.sol | 156 +--------------- .../universal/op-erc721/ERC721Bridge.sol | 172 ++++++++++++++++++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 2 +- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 2 +- 5 files changed, 182 insertions(+), 288 deletions(-) create mode 100644 packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 947b85ebdaae2..c26e21c3535cd 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; import { CrossDomainEnabled } from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; @@ -8,7 +9,6 @@ import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.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"; @@ -18,51 +18,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv * make it possible to transfer ERC721 tokens between Optimism and Ethereum. 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, OwnableUpgradeable { /** * @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. @@ -82,82 +38,6 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { 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. - * - * @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. - */ - 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 - ); - } - /************************* * Cross-chain Functions * *************************/ @@ -200,17 +80,7 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { } /** - * @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 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. + * @inheritdoc ERC721Bridge */ function _initiateBridgeERC721( address _localToken, @@ -220,7 +90,7 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { uint256 _tokenId, uint32 _minGasLimit, bytes calldata _extraData - ) internal { + ) internal override { // Construct calldata for _l2Token.finalizeBridgeERC721(_to, _tokenId) bytes memory message = abi.encodeWithSelector( L2ERC721Bridge.finalizeBridgeERC721.selector, diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 237dcdd4de9cb..5746e4242201f 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; import { CrossDomainEnabled } from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; @@ -8,7 +9,6 @@ import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.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"; @@ -20,69 +20,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv * 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. */ -contract L2ERC721Bridge 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 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; - +contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { /** * @custom:semver 0.0.1 * @@ -96,82 +34,6 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { 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. - * - * @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. - */ - 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 Completes an ERC721 bridge from the other domain and sends the ERC721 token to the * recipient on this domain. @@ -237,17 +99,7 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { } /** - * @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 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. + * @inheritdoc ERC721Bridge */ function _initiateBridgeERC721( address _localToken, @@ -257,7 +109,7 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { uint256 _tokenId, uint32 _minGasLimit, bytes calldata _extraData - ) internal { + ) internal override { // Check that the withdrawal is being initiated by the NFT owner require( _from == IOptimismMintableERC721(_localToken).ownerOf(_tokenId), 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..b52745dfdb592 --- /dev/null +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import { + CrossDomainEnabled +} from "@eth-optimism/contracts/contracts/libraries/bridge/CrossDomainEnabled.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, CrossDomainEnabled { + /** + * @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; + + /** + * @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; + } + + /** + * @notice Initiates a bridge of an NFT to the caller's account on the other domain. + * + * @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 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 the other domain. + * + * @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 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/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index e0f2046f0fcdb..39bdfd5daa2e6 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -222,7 +222,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', () => { diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 687a12806fc0f..8fc870f1a19a6 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -262,7 +262,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 () => { From bd4e51bf17a82647e29d0189243499c5b855678e Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 26 Aug 2022 12:23:52 -0400 Subject: [PATCH 2/8] nft: M-08 Upgradeability inconsistencies --- .../contracts/L1/L1ERC721Bridge.sol | 10 ++++++++ .../contracts/L2/L2ERC721Bridge.sol | 10 ++++++++ .../universal/op-erc721/ERC721Bridge.sol | 23 ++++++++++++++----- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index c26e21c3535cd..46ad4c96a8ac2 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -38,6 +38,16 @@ contract L1ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { initialize(_messenger, _otherBridge); } + /** + * @notice Initializer. + * + * @param _messenger Address of the L1CrossDomainMessenger. + * @param _otherBridge Address of the L2ERC721Bridge. + */ + function initialize(address _messenger, address _otherBridge) public initializer { + __ERC721Bridge_init(_messenger, _otherBridge); + } + /************************* * Cross-chain Functions * *************************/ diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 5746e4242201f..8ac8cab0987c1 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -34,6 +34,16 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { initialize(_messenger, _otherBridge); } + /** + * @notice Initializer. + * + * @param _messenger Address of the L2CrossDomainMessenger. + * @param _otherBridge Address of the L1ERC721Bridge. + */ + function initialize(address _messenger, address _otherBridge) public initializer { + __ERC721Bridge_init(_messenger, _otherBridge); + } + /** * @notice Completes an ERC721 bridge from the other domain and sends the ERC721 token to the * recipient on this domain. diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index b52745dfdb592..788012848b848 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -75,13 +75,9 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled { address public otherBridge; /** - * @param _messenger Address of the CrossDomainMessenger on this network. - * @param _otherBridge Address of the ERC721 bridge on the other network. + * @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades. */ - function initialize(address _messenger, address _otherBridge) public initializer { - messenger = _messenger; - otherBridge = _otherBridge; - } + uint256[48] private __gap; /** * @notice Initiates a bridge of an NFT to the caller's account on the other domain. @@ -147,6 +143,21 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled { ); } + /** + * @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 = _messenger; + otherBridge = _otherBridge; + } + /** * @notice Internal function for initiating a token bridge to the other domain. * From fad822efa92aaefe730a26e798ced4891c3c6313 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 2 Sep 2022 14:20:59 -0700 Subject: [PATCH 3/8] fix(ctp): change `CrossDomainEnabled` to `CrossDomainMessenger` --- .../contracts/L1/L1ERC721Bridge.sol | 12 +++------ .../contracts/L2/L2ERC721Bridge.sol | 14 +++------- .../universal/op-erc721/ERC721Bridge.sol | 26 +++++++++++++++---- .../test/contracts/L1/L1ERC721Bridge.spec.ts | 9 +++---- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 9 +++---- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 46ad4c96a8ac2..6955d02776eec 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -2,9 +2,6 @@ pragma solidity 0.8.15; import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; -import { - CrossDomainEnabled -} from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; @@ -31,10 +28,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { * @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(0, 0, 1) { initialize(_messenger, _otherBridge); } @@ -72,7 +66,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { address _to, uint256 _tokenId, bytes calldata _extraData - ) external onlyFromCrossDomainAccount(otherBridge) { + ) external onlyOtherBridge { // Checks that the L1/L2 token pair has a token ID that is escrowed in the L1 Bridge require( deposits[_localToken][_remoteToken][_tokenId] == true, @@ -117,7 +111,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver, 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 8ac8cab0987c1..f78297e2440cd 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -2,9 +2,6 @@ pragma solidity 0.8.15; import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; -import { - CrossDomainEnabled -} from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol"; import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; @@ -27,10 +24,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { * @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(0, 0, 1) { initialize(_messenger, _otherBridge); } @@ -64,7 +58,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { address _to, uint256 _tokenId, bytes calldata _extraData - ) external onlyFromCrossDomainAccount(otherBridge) { + ) external onlyOtherBridge { // Check the target token is compliant and verify the deposited token on L1 matches the L2 // deposited token representation. if ( @@ -101,7 +95,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { // 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); @@ -151,7 +145,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, 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 index 788012848b848..49f0ce1edd824 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -2,8 +2,8 @@ pragma solidity 0.8.15; import { - CrossDomainEnabled -} from "@eth-optimism/contracts/contracts/libraries/bridge/CrossDomainEnabled.sol"; + 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"; @@ -11,7 +11,12 @@ 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, CrossDomainEnabled { +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. * @@ -77,7 +82,18 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled { /** * @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades. */ - uint256[48] private __gap; + 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 Initiates a bridge of an NFT to the caller's account on the other domain. @@ -154,7 +170,7 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled { internal onlyInitializing { - messenger = _messenger; + messenger = CrossDomainMessenger(_messenger); otherBridge = _otherBridge; } diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index 39bdfd5daa2e6..e429b18276520 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) ) @@ -291,7 +290,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 +306,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', () => { diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 8fc870f1a19a6..c448ed9d4243f 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 = @@ -88,7 +87,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 +107,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 () => { From 1677a7bfe9aee38b31506d685e4db9d3e343f3af Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 15 Sep 2022 16:21:27 -0400 Subject: [PATCH 4/8] fix(ctp): make `otherBridge` and `messenger` in nft bridge contracts immutable --- .../contracts/L1/L1ERC721Bridge.sol | 17 +++------ .../contracts/L2/L2ERC721Bridge.sol | 17 +++------ .../universal/op-erc721/ERC721Bridge.sol | 36 ++++++++----------- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 12 ------- 4 files changed, 23 insertions(+), 59 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 6955d02776eec..c814323b7992d 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -28,19 +28,10 @@ contract L1ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { * @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) { - initialize(_messenger, _otherBridge); - } - - /** - * @notice Initializer. - * - * @param _messenger Address of the L1CrossDomainMessenger. - * @param _otherBridge Address of the L2ERC721Bridge. - */ - function initialize(address _messenger, address _otherBridge) public initializer { - __ERC721Bridge_init(_messenger, _otherBridge); - } + constructor(address _messenger, address _otherBridge) + Semver(0, 0, 1) + ERC721Bridge(_messenger, _otherBridge) + {} /************************* * Cross-chain Functions * diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index f78297e2440cd..bd764dc0b00b7 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -24,19 +24,10 @@ contract L2ERC721Bridge is ERC721Bridge, Semver, OwnableUpgradeable { * @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) { - initialize(_messenger, _otherBridge); - } - - /** - * @notice Initializer. - * - * @param _messenger Address of the L2CrossDomainMessenger. - * @param _otherBridge Address of the L1ERC721Bridge. - */ - function initialize(address _messenger, address _otherBridge) public initializer { - __ERC721Bridge_init(_messenger, _otherBridge); - } + constructor(address _messenger, address _otherBridge) + Semver(0, 0, 1) + ERC721Bridge(_messenger, _otherBridge) + {} /** * @notice Completes an ERC721 bridge from the other domain and sends the ERC721 token to the diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 49f0ce1edd824..2c03a98982b65 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -12,11 +12,6 @@ import { Address } from "@openzeppelin/contracts/utils/Address.sol"; * @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. * @@ -74,10 +69,15 @@ abstract contract ERC721Bridge is Initializable { bytes extraData ); + /** + * @notice Messenger contract on this domain. + */ + CrossDomainMessenger public immutable messenger; + /** * @notice Address of the bridge on the other network. */ - address public otherBridge; + address public immutable otherBridge; /** * @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades. @@ -95,6 +95,15 @@ abstract contract ERC721Bridge is Initializable { _; } + /** + * @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) { + messenger = CrossDomainMessenger(_messenger); + otherBridge = _otherBridge; + } + /** * @notice Initiates a bridge of an NFT to the caller's account on the other domain. * @@ -159,21 +168,6 @@ abstract contract ERC721Bridge is Initializable { ); } - /** - * @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. * diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index c448ed9d4243f..caf62f94cf495 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -8,7 +8,6 @@ import { toRpcHexString } from '@eth-optimism/core-utils' 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_X_DOMAIN_MESSAGE = 'ERC721Bridge: function can only be called from the other bridge' const DUMMY_L1BRIDGE_ADDRESS: string = @@ -64,17 +63,6 @@ describe('L2ERC721Bridge', () => { ) }) - describe('initialize', () => { - it('Should only be callable once', async () => { - await expect( - L2ERC721Bridge.initialize( - Fake__L2CrossDomainMessenger.address, - DUMMY_L1BRIDGE_ADDRESS - ) - ).to.be.revertedWith(ERR_ALREADY_INITIALIZED) - }) - }) - // test the transfer flow of moving a token from L1 to L2 describe('finalizeBridgeERC721', () => { it('onlyFromCrossDomainAccount: should revert on calls from a non-crossDomainMessenger L2 account', async () => { From cf5a60fcf2682068d1f6531155f50e1834f57799 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 15 Sep 2022 22:06:14 -0400 Subject: [PATCH 5/8] docs(ctp): update `ERC721Bridge` bridging function docs --- .../universal/op-erc721/ERC721Bridge.sol | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 2c03a98982b65..098be3fd5bdb1 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -105,15 +105,19 @@ abstract contract ERC721Bridge is Initializable { } /** - * @notice Initiates a bridge of an NFT to the caller's account on the other domain. + * @notice Initiates a bridge of an NFT to the caller's account on the other chain. 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. Also note that the current owner of the token on this chain must + * approve this contract to operate the NFT before it can be bridged. * * @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. + * @param _extraData Optional data to forward to the other chain. Data supplied here will not + * be used to execute any code on the other chain and is only emitted as + * extra data for the convenience of off-chain tooling. */ function bridgeERC721( address _localToken, @@ -122,8 +126,12 @@ abstract contract ERC721Bridge is Initializable { 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. + // 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( @@ -138,16 +146,18 @@ abstract contract ERC721Bridge is Initializable { } /** - * @notice Initiates a bridge of an NFT to some recipient's account on the other domain. + * @notice Initiates a bridge of an NFT to some recipient's account on the other 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. * * @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. + * @param _extraData Optional data to forward to the other chain. Data supplied here will not + * be used to execute any code on the other chain and is only emitted as + * extra data for the convenience of off-chain tooling. */ function bridgeERC721To( address _localToken, From 9d8009c091ce103fa0f80b112e7144218f2e0d03 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 16 Sep 2022 12:47:23 -0400 Subject: [PATCH 6/8] re-order `OptimismMintableERC721Factory` event args --- .../universal/op-erc721/OptimismMintableERC721Factory.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol index ea5315dc41db3..57e613b603bc9 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol @@ -15,10 +15,10 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { /** * @notice Emitted whenever a new OptimismMintableERC721 contract is created. * - * @param remoteToken Address of the token on the remote domain. * @param localToken Address of the token on the this domain. + * @param remoteToken Address of the token on the remote domain. */ - event OptimismMintableERC721Created(address indexed remoteToken, address indexed localToken); + event OptimismMintableERC721Created(address indexed localToken, address indexed remoteToken); /** * @notice Address of the ERC721 bridge on this network. @@ -88,6 +88,6 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { ); isStandardOptimismMintableERC721[address(localToken)] = true; - emit OptimismMintableERC721Created(_remoteToken, address(localToken)); + emit OptimismMintableERC721Created(address(localToken), _remoteToken); } } From 9a2e8609971d83887d38abcca529129746b139a0 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 16 Sep 2022 14:52:07 -0400 Subject: [PATCH 7/8] remove unnecessary `Initializable` import in nft bridges --- packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol | 1 - packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol | 1 - .../contracts/universal/op-erc721/ERC721Bridge.sol | 3 +-- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 1379ae30a89b5..cc64aa10a6463 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -5,7 +5,6 @@ import { ERC721Bridge } from "../universal/op-erc721/ERC721Bridge.sol"; import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.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 diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 3a752c4ca0dd0..a7bce88e9c7a3 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -6,7 +6,6 @@ import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC16 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 diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 098be3fd5bdb1..c29861ba30b8d 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -4,14 +4,13 @@ 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 { +abstract contract ERC721Bridge { /** * @notice Emitted when an ERC721 bridge to the other network is initiated. * From 6e06b59780020ff59557d5e7c434e089543d3625 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Tue, 20 Sep 2022 19:01:11 -0400 Subject: [PATCH 8/8] changed legacy `CrossDomainEnabled` call to `CrossDomainMessenger --- .../contracts/L1/L1ERC721Bridge.sol | 2 +- .../universal/op-erc721/ERC721Bridge.sol | 4 ++-- .../test/contracts/L1/L1ERC721Bridge.spec.ts | 20 +------------------ .../test/contracts/L2/L2ERC721Bridge.spec.ts | 9 +++++++++ 4 files changed, 13 insertions(+), 22 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 28c8a9564e6bc..444926bc756cc 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -90,7 +90,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { // Send the message to the L2 bridge. // slither-disable-next-line reentrancy-events - sendCrossDomainMessage(otherBridge, 600_000, message); + messenger.sendMessage(otherBridge, message, 600_000); // slither-disable-next-line reentrancy-events emit ERC721BridgeFailed(_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 index 0500d5d1b62e5..10473b32d7083 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -111,7 +111,7 @@ abstract contract ERC721Bridge { ); _; } - + /** * @notice Ensures that the caller is this contract. */ @@ -201,7 +201,7 @@ abstract contract ERC721Bridge { bytes calldata _extraData ) external { require(_to != address(0), "ERC721Bridge: nft recipient cannot be address(0)"); - + _initiateBridgeERC721( _localToken, _remoteToken, diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index c3b5c70c21e82..6c05d18eed302 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -83,25 +83,7 @@ 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)') - }) - + describe('constructor', async () => { it('initializes correctly', async () => { expect(await L1ERC721Bridge.messenger()).equals( Fake__L1CrossDomainMessenger.address diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index abc6fcf56ebb9..f46fb134b5380 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -65,6 +65,15 @@ describe('L2ERC721Bridge', () => { ) }) + describe('constructor', async () => { + 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 describe('finalizeBridgeERC721', () => { it('onlyFromCrossDomainAccount: should revert on calls from a non-crossDomainMessenger L2 account', async () => {