From 8603864482eb734cbeb79e7b5fc994f16685b577 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 15 Aug 2022 12:08:48 -0700 Subject: [PATCH 1/3] 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 0ee2d260ae709..4de84f6e3db34 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/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 05b8ddc06ee2c..53dd3631272b0 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/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 844ec04d8c874da46ce27d32e1256471e866fe18 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 26 Aug 2022 12:23:52 -0400 Subject: [PATCH 2/3] 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 4de84f6e3db34..d320b43c8c540 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 53dd3631272b0..0b485844b6eab 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 cd12a58c036ee2e05bd02c685c2cdf5b9f6d4133 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 2 Sep 2022 14:20:59 -0700 Subject: [PATCH 3/3] 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 d320b43c8c540..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/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 0b485844b6eab..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/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 () => {