From 2aa3c87a6a0f166d21e0201009c62ec81178a61a Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 22 Aug 2022 15:14:12 -0700 Subject: [PATCH 01/16] feat(ctp): fix erc721 bridge related contracts --- .changeset/breezy-carrots-flow.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/breezy-carrots-flow.md 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. From ba1416951e6c952b67ead49ebd9a3d78474a06d0 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 29 Aug 2022 00:11:13 -0400 Subject: [PATCH 02/16] nft: add view keyword to functions in `IOptimismMintableERC721` --- .../universal/op-erc721/IOptimismMintableERC721.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol index c801cc79b272d..e30e7ba89dc82 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol @@ -30,17 +30,17 @@ 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. From 01224fb6f452018376fe73d429c3b1fd9d20bdb8 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 18 Aug 2022 14:37:29 -0700 Subject: [PATCH 03/16] nft: N-09 Typographical errors --- packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 0ee2d260ae709..584ecdeba0e12 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -16,7 +16,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv * @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 - * acts as an escrow for ERC721 tokens deposted into L2. + * acts as an escrow for ERC721 tokens deposited into L2. */ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { /** From 5676eea47be2dd22b4df9b4fbd6c07997e9a8c58 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 11 Aug 2022 14:49:42 -0700 Subject: [PATCH 04/16] nft: N-11 Unused inherited contract --- .../contracts-periphery/contracts/L1/L1ERC721Bridge.sol | 9 ++------- .../contracts-periphery/contracts/L2/L2ERC721Bridge.sol | 9 ++------- .../op-erc721/OptimismMintableERC721Factory.sol | 9 ++------- 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 584ecdeba0e12..4385ad647d6e4 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -4,13 +4,11 @@ pragma solidity 0.8.15; import { CrossDomainEnabled } from "@eth-optimism/contracts/contracts/libraries/bridge/CrossDomainEnabled.sol"; -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"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; /** * @title L1ERC721Bridge @@ -18,7 +16,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 deposited into L2. */ -contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { +contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { /** * @notice Emitted when an ERC721 bridge to the other network is initiated. * @@ -89,9 +87,6 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { function initialize(address _messenger, address _otherBridge) public initializer { messenger = _messenger; otherBridge = _otherBridge; - - // Initialize upgradable OZ contracts - __Ownable_init(); } /** diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 05b8ddc06ee2c..5f906f73f9599 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -4,14 +4,12 @@ pragma solidity 0.8.15; import { CrossDomainEnabled } from "@eth-optimism/contracts/contracts/libraries/bridge/CrossDomainEnabled.sol"; -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"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; /** * @title L2ERC721Bridge @@ -20,7 +18,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 { +contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { /** * @notice Emitted when an ERC721 bridge to the other network is initiated. * @@ -103,9 +101,6 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable { function initialize(address _messenger, address _otherBridge) public initializer { messenger = _messenger; otherBridge = _otherBridge; - - // Initialize upgradable OZ contracts - __Ownable_init(); } /** diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol index ea5315dc41db3..4364146afc1a8 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. * @@ -52,9 +50,6 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable { function initialize(address _bridge, uint256 _remoteChainId) public initializer { bridge = _bridge; remoteChainId = _remoteChainId; - - // Initialize upgradable OZ contracts - __Ownable_init(); } /** From bbad39a4958dadfffa91fe6a99a8cf58c90fd801 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 11 Aug 2022 15:25:22 -0700 Subject: [PATCH 05/16] nft: N-10 Undocumented implicit approval requirements --- .../contracts-periphery/contracts/L1/L1ERC721Bridge.sol | 8 ++++++-- .../contracts-periphery/contracts/L2/L2ERC721Bridge.sol | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 4385ad647d6e4..1a3368ecd3e13 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -90,7 +90,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @notice Initiates a bridge of an NFT to the caller's account on L2. + * @notice Initiates a bridge of an NFT to the caller'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. * * @param _localToken Address of the ERC721 on this domain. * @param _remoteToken Address of the ERC721 on the remote domain. @@ -123,7 +125,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @notice Initiates a bridge of an NFT to some recipient's account on L2. + * @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. * * @param _localToken Address of the ERC721 on this domain. * @param _remoteToken Address of the ERC721 on the remote domain. diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 5f906f73f9599..e75b84dc0cdde 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -104,7 +104,9 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @notice Initiates a bridge of an NFT to the caller's account on L1. + * @notice Initiates a bridge of an NFT to the caller's account on L1. 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. @@ -137,7 +139,9 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @notice Initiates a bridge of an NFT to some recipient's account on L1. + * @notice Initiates a bridge of an NFT to some recipient's account on L1. 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. From 7832cad9dbd3def666cd4b9d2c7c5849146e65f2 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Thu, 18 Aug 2022 15:31:41 -0700 Subject: [PATCH 06/16] nft: N-08 Easily bypass-able requirement --- .../contracts/L1/L1ERC721Bridge.sol | 12 +++++++++--- .../contracts/L2/L2ERC721Bridge.sol | 12 +++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 1a3368ecd3e13..1b1dd37ff2433 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -92,7 +92,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { /** * @notice Initiates a bridge of an NFT to the caller'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. + * 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. * * @param _localToken Address of the ERC721 on this domain. * @param _remoteToken Address of the ERC721 on the remote domain. @@ -109,8 +111,12 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, 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), "L1ERC721Bridge: account is not externally owned"); _initiateBridgeERC721( diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index e75b84dc0cdde..f30f345a8b272 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -106,7 +106,9 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { /** * @notice Initiates a bridge of an NFT to the caller's account on L1. Note that the current * owner of the token on this chain must approve this contract to operate the NFT before - * it can be bridged. + * 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. * * @param _localToken Address of the ERC721 on this domain. * @param _remoteToken Address of the ERC721 on the remote domain. @@ -123,8 +125,12 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, 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), "L2ERC721Bridge: account is not externally owned"); _initiateBridgeERC721( From d62a38d24adbed84ff19a787203be9885154f798 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Sun, 21 Aug 2022 08:48:57 -0700 Subject: [PATCH 07/16] nft: N-01 Functions fail later than required --- .../contracts/L2/L2ERC721Bridge.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index f30f345a8b272..8188812ab96cd 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -269,11 +269,6 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { "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(); @@ -282,6 +277,11 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { "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, From 61eea8114fdec6e70b1e85fe7ee4162b3da1bc6d Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 15 Aug 2022 12:08:48 -0700 Subject: [PATCH 08/16] nft: L-13 Duplicated code --- .../contracts/L1/L1ERC721Bridge.sol | 145 +------------- .../contracts/L2/L2ERC721Bridge.sol | 163 +--------------- .../universal/op-erc721/ERC721Bridge.sol | 182 ++++++++++++++++++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 2 +- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 2 +- 5 files changed, 192 insertions(+), 302 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 1b1dd37ff2433..9a22b3a124018 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -1,11 +1,11 @@ // 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"; 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"; @@ -16,51 +16,7 @@ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable * make it possible to transfer ERC721 tokens between Optimism and Ethereum. This contract * acts as an escrow for ERC721 tokens deposited into L2. */ -contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { - /** - * @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. @@ -80,89 +36,6 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { 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; - } - - /** - * @notice Initiates a bridge of an NFT to the caller'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. 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. - * - * @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 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), "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. 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 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 * *************************/ @@ -205,17 +78,7 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @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, @@ -225,7 +88,7 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { 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 8188812ab96cd..f1e132dfccee7 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -1,11 +1,11 @@ // 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"; 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"; @@ -18,69 +18,7 @@ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable * 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, Initializable { - /** - * @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 { /** * @custom:semver 0.0.1 * @@ -94,89 +32,6 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { 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; - } - - /** - * @notice Initiates a bridge of an NFT to the caller's account on L1. 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. - * - * @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 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), "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. 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 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. @@ -242,17 +97,7 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { } /** - * @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, @@ -262,7 +107,7 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { 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..064482a569151 --- /dev/null +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -0,0 +1,182 @@ +// 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 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. + * + * @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. + * + * @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 46c6dc1057adb3b48b9fc069200d3819e4aa56c9 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 26 Aug 2022 12:23:52 -0400 Subject: [PATCH 09/16] 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 9a22b3a124018..43b6f31965b97 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -36,6 +36,16 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { 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 f1e132dfccee7..aea089e24dadf 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -32,6 +32,16 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { 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 064482a569151..187158063d141 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 remote chain. Note that @@ -157,6 +153,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 e0b472d95386df1deed98d04f38354cecd52a8db Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 2 Sep 2022 14:20:59 -0700 Subject: [PATCH 10/16] 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 43b6f31965b97..bf9924b825542 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 { 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"; @@ -29,10 +26,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { * @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); } @@ -70,7 +64,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { 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, @@ -115,7 +109,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { 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 aea089e24dadf..1bd3c86173543 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 { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import { L1ERC721Bridge } from "../L1/L1ERC721Bridge.sol"; import { IOptimismMintableERC721 } from "../universal/op-erc721/IOptimismMintableERC721.sol"; @@ -25,10 +22,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { * @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); } @@ -62,7 +56,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { 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 ( @@ -99,7 +93,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { // 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); @@ -149,7 +143,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { // 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 187158063d141..9faeeadb83691 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 remote chain. Note that @@ -164,7 +180,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 8b5bf938771d0b835a12b2a660b014382b95271c Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 19 Aug 2022 12:17:27 -0700 Subject: [PATCH 11/16] nft: M-01 Asymmetric failure behavior of ERC721 bridges --- .../contracts/FakeOptimismMintableERC721.sol | 18 +++++- integration-tests/test/nft-bridge.spec.ts | 60 ++++++++++++++++++- .../contracts/L1/L1ERC721Bridge.sol | 49 +++++++++++---- .../contracts/L2/L2ERC721Bridge.sol | 6 +- .../universal/op-erc721/ERC721Bridge.sol | 8 +++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 48 ++++++++++++--- 6 files changed, 165 insertions(+), 24 deletions(-) diff --git a/integration-tests/contracts/FakeOptimismMintableERC721.sol b/integration-tests/contracts/FakeOptimismMintableERC721.sol index 2108dbbfc373b..5fb4e04def911 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 { @@ -18,5 +20,19 @@ contract FakeOptimismMintableERC721 is ERC721 { } // 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..4885eeea6bf29 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) @@ -303,4 +306,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.mint(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 bf9924b825542..38da591ed5701 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -10,8 +10,8 @@ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable /** * @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 - * acts as an escrow for ERC721 tokens deposited into L2. + * 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 ERC721Bridge, Semver { /** @@ -65,20 +65,43 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { uint256 _tokenId, bytes calldata _extraData ) 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, - "Token ID is not escrowed in the L1 Bridge" - ); + // 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. + if (deposits[_localToken][_remoteToken][_tokenId] == true) { + // 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); - deposits[_localToken][_remoteToken][_tokenId] = false; + // slither-disable-next-line reentrancy-events + emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + } else { + // If the token ID for this L1/L2 NFT pair is not escrowed in the L1 Bridge, 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, + _to, // switched the _to and _from here to bounce back the deposit to the sender + _from, + _tokenId, + _extraData + ); - // 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); + // 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 ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + // slither-disable-next-line reentrancy-events + emit ERC721BridgeFailed(_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 1bd3c86173543..e5e61f2f8b7de 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -11,9 +11,13 @@ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable /** * @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 ERC721Bridge, Semver { /** diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 9faeeadb83691..f810db4e6e180 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -101,6 +101,10 @@ abstract contract ERC721Bridge is Initializable { * 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. @@ -140,6 +144,10 @@ abstract contract ERC721Bridge is Initializable { * @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. diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index e429b18276520..a2ddc1fbd8ffd 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -22,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 @@ -330,20 +330,54 @@ 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, + bobsAddress, // switched the 'from' and 'to' addresses here + 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 () => { From 5977a069fe7a4ed0485ebd779657df688e34e325 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Wed, 7 Sep 2022 17:48:05 -0700 Subject: [PATCH 12/16] nft: N-05 Inconsistent approach to auto-refunding failed token transfers --- .../contracts/L1/L1ERC721Bridge.sol | 61 +++++++++++++------ .../contracts/L2/L2ERC721Bridge.sol | 52 +++++++++++----- .../universal/op-erc721/ERC721Bridge.sol | 8 +++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 13 ++++ .../test/contracts/L2/L2ERC721Bridge.spec.ts | 13 ++++ 5 files changed, 114 insertions(+), 33 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 38da591ed5701..466875d164ce3 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -65,26 +65,14 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { uint256 _tokenId, bytes calldata _extraData ) external onlyOtherBridge { - // 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. - if (deposits[_localToken][_remoteToken][_tokenId] == true) { - // 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); - + try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { // slither-disable-next-line reentrancy-events emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); - } else { - // If the token ID for this L1/L2 NFT pair is not escrowed in the L1 Bridge, 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. + } 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, @@ -107,6 +95,43 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { /** * @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, + "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); + } + + /** + * @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. + */ function _initiateBridgeERC721( address _localToken, address _remoteToken, diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index e5e61f2f8b7de..3ef0800a716a6 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -61,25 +61,13 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { uint256 _tokenId, bytes calldata _extraData ) external onlyOtherBridge { - // 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); + try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { // slither-disable-next-line reentrancy-events emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); - } else { + } 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. @@ -104,6 +92,40 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { } } + /** + * @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 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. + // slither-disable-next-line reentrancy-events + IOptimismMintableERC721(_localToken).mint(_to, _tokenId); + } + /** * @inheritdoc ERC721Bridge */ diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index f810db4e6e180..9bd0e3c84c486 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -95,6 +95,14 @@ abstract contract ERC721Bridge is Initializable { _; } + /** + * @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 diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index a2ddc1fbd8ffd..900a2c6c3c0bf 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -417,4 +417,17 @@ describe('L1ERC721Bridge', () => { }) }) }) + + 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 c448ed9d4243f..b62e4d80c7508 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -214,6 +214,19 @@ describe('L2ERC721Bridge', () => { }) }) + 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', () => { let L2Token: Contract beforeEach(async () => { From 377ceb3f86b0138660de4dfd0280a16e75fd8341 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 22 Aug 2022 13:59:39 -0700 Subject: [PATCH 13/16] nft: L-05 Auto withdrawal transactions can be misleading --- .../contracts/L1/L1ERC721Bridge.sol | 23 +++++++++-- .../contracts/L2/L2ERC721Bridge.sol | 23 +++++++++-- .../universal/op-erc721/ERC721Bridge.sol | 18 +++++++++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 39 ++++++++++++++++++- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 37 +++++++++++++++++- 5 files changed, 129 insertions(+), 11 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 466875d164ce3..5ca764e346ecc 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -66,8 +66,22 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { bytes calldata _extraData ) external onlyOtherBridge { try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { - // slither-disable-next-line reentrancy-events - emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + 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 @@ -77,8 +91,9 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { L2ERC721Bridge.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 ); diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 3ef0800a716a6..a62ad24b9a826 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -62,8 +62,22 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { bytes calldata _extraData ) external onlyOtherBridge { try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) { - // slither-disable-next-line reentrancy-events - emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData); + 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. @@ -77,8 +91,9 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { 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 ); diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 9bd0e3c84c486..25dc87f8001fd 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -74,6 +74,24 @@ abstract contract ERC721Bridge is Initializable { 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. */ diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index 900a2c6c3c0bf..9dbc5e2590a40 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -370,7 +370,7 @@ describe('L1ERC721Bridge', () => { IL2ERC721Bridge.encodeFunctionData('finalizeBridgeERC721', [ DUMMY_L2_ERC721_ADDRESS, RANDOM_L1_ERC721_ADDRESS, - bobsAddress, // switched the 'from' and 'to' addresses here + L1ERC721Bridge.address, // the 'from' address is the l1 bridge to indicate that the l1 recipient never controlled the nft. aliceAddress, tokenId, NON_NULL_BYTES32, @@ -380,7 +380,7 @@ describe('L1ERC721Bridge', () => { 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( @@ -415,6 +415,41 @@ 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) + }) }) }) diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index b62e4d80c7508..8188430611d33 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -156,7 +156,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, @@ -212,6 +212,41 @@ 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 () => { From e25e9b737273c51b80089064edb7321f1a9150e8 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Mon, 22 Aug 2022 14:07:42 -0700 Subject: [PATCH 14/16] nft: M-06 Lack of input validation --- .../contracts/L1/L1ERC721Bridge.sol | 7 ++- .../contracts/L2/L2ERC721Bridge.sol | 4 ++ .../op-erc721/OptimismMintableERC721.sol | 7 +++ .../OptimismMintableERC721Factory.sol | 9 +++ .../test/contracts/L1/L1ERC721Bridge.spec.ts | 61 ++++++++++++++++++- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 52 ++++++++++++++++ .../universal/OptimismMintableERC721.spec.ts | 53 ++++++++++++++-- .../OptimismMintableERC721Factory.spec.ts | 31 +++++++++- 8 files changed, 211 insertions(+), 13 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 5ca764e346ecc..ebd21c9cafb71 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -37,6 +37,8 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { * @param _otherBridge Address of the L2ERC721Bridge. */ 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); } @@ -130,8 +132,7 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { // 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); + IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId); } /** @@ -156,6 +157,8 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { uint32 _minGasLimit, bytes calldata _extraData ) 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, diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index a62ad24b9a826..2c3b0d7fff165 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -37,6 +37,8 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { * @param _otherBridge Address of the L1ERC721Bridge. */ 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); } @@ -153,6 +155,8 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { uint32 _minGasLimit, bytes calldata _extraData ) 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), diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol index de8d1cc7de266..5cc88d18b74db 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; diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol index 4364146afc1a8..7eb755b066de0 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol @@ -48,6 +48,15 @@ contract OptimismMintableERC721Factory is Semver, Initializable { * @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; } diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index 9dbc5e2590a40..c3b5c70c21e82 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -58,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( @@ -66,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') @@ -80,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. @@ -146,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. diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 8188430611d33..1f93551562981 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -19,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 @@ -73,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 @@ -288,6 +315,18 @@ describe('L2ERC721Bridge', () => { await L2Token.connect(signer).mint(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 () => { await expect( L2ERC721Bridge.connect(bob).bridgeERC721( @@ -378,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..766299254e74d 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 @@ -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 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 From 9bedcd397e76cfcb1468d5b4116b436651215cb2 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Sat, 20 Aug 2022 21:33:33 -0700 Subject: [PATCH 15/16] nft: L-08 ERC721 bridge contracts not using safeTransferFrom --- integration-tests/contracts/FakeOptimismMintableERC721.sol | 4 ++-- integration-tests/test/nft-bridge.spec.ts | 7 +++++-- .../contracts-periphery/contracts/L2/L2ERC721Bridge.sol | 3 +-- .../universal/op-erc721/IOptimismMintableERC721.sol | 5 +++-- .../universal/op-erc721/OptimismMintableERC721.sol | 4 ++-- .../test/contracts/L2/L2ERC721Bridge.spec.ts | 2 +- .../contracts/universal/OptimismMintableERC721.spec.ts | 6 +++--- 7 files changed, 17 insertions(+), 14 deletions(-) diff --git a/integration-tests/contracts/FakeOptimismMintableERC721.sol b/integration-tests/contracts/FakeOptimismMintableERC721.sol index 5fb4e04def911..6aad566010e77 100644 --- a/integration-tests/contracts/FakeOptimismMintableERC721.sol +++ b/integration-tests/contracts/FakeOptimismMintableERC721.sol @@ -15,8 +15,8 @@ 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 diff --git a/integration-tests/test/nft-bridge.spec.ts b/integration-tests/test/nft-bridge.spec.ts index 4885eeea6bf29..4a24227fd3d33 100644 --- a/integration-tests/test/nft-bridge.spec.ts +++ b/integration-tests/test/nft-bridge.spec.ts @@ -281,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 @@ -324,7 +327,7 @@ describe('ERC721 Bridge', () => { await L2NativeNFT.deployed() // Alice mints an NFT from the L2 native ERC721 contract - const tx = await L2NativeNFT.mint(aliceAddress, TOKEN_ID) + const tx = await L2NativeNFT.safeMint(aliceAddress, TOKEN_ID) await tx.wait() // Check that Alice owns the L2 NFT diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 2c3b0d7fff165..479790124743b 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -139,8 +139,7 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { // 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); + IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId); } /** diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol index e30e7ba89dc82..a37dbd3d6ac58 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/IOptimismMintableERC721.sol @@ -43,12 +43,13 @@ interface IOptimismMintableERC721 is IERC721Enumerable { 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 5cc88d18b74db..0307a8f580a17 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol @@ -85,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/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 1f93551562981..344e59e103fd9 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -312,7 +312,7 @@ 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 () => { diff --git a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts index 766299254e74d..19ac72d6f5619 100644 --- a/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts +++ b/packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts @@ -53,7 +53,7 @@ describe('OptimismMintableERC721', () => { ) // mint an nft to alice - await OptimismMintableERC721.connect(l2BridgeImpersonator).mint( + await OptimismMintableERC721.connect(l2BridgeImpersonator).safeMint( aliceAddress, TOKEN_ID, { @@ -126,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' ) @@ -145,7 +145,7 @@ describe('OptimismMintableERC721', () => { .true // OptimismMintableERC721 - expect(await OptimismMintableERC721.supportsInterface(0x051e4975)).to.be + expect(await OptimismMintableERC721.supportsInterface(0xe49bc7f8)).to.be .true // ERC721 From d7f639e44049603362df8fdc47d08484304f3c31 Mon Sep 17 00:00:00 2001 From: Sam Goldman Date: Fri, 9 Sep 2022 15:02:34 -0400 Subject: [PATCH 16/16] fix(ctp): bump semver to 1.0.0 --- packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol | 4 ++-- packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol | 4 ++-- .../universal/op-erc721/OptimismMintableERC721Factory.sol | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index ebd21c9cafb71..5480e84ea0723 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -21,12 +21,12 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { 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) { + constructor(address _messenger, address _otherBridge) Semver(1, 0, 0) { initialize(_messenger, _otherBridge); } diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 479790124743b..1af4dad8a2a3f 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol @@ -21,12 +21,12 @@ import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable */ contract L2ERC721Bridge is ERC721Bridge, Semver { /** - * @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) { + constructor(address _messenger, address _otherBridge) Semver(1, 0, 0) { initialize(_messenger, _otherBridge); } diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol index 7eb755b066de0..ddff77b324c6d 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol @@ -34,11 +34,11 @@ contract OptimismMintableERC721Factory is Semver, Initializable { 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); }