nft: L-13 and M-08 (#3491)

* nft: L-13 Duplicated code

* nft: M-08 Upgradeability inconsistencies

* fix(ctp): change `CrossDomainEnabled` to `CrossDomainMessenger`

* fix(ctp): make `otherBridge` and `messenger` in nft bridge contracts immutable

* docs(ctp): update `ERC721Bridge` bridging function docs

* re-order `OptimismMintableERC721Factory` event args

* remove unnecessary `Initializable` import in nft bridges

* changed legacy `CrossDomainEnabled` call to `CrossDomainMessenger
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>

packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol

@@ -2,16 +2,34 @@
 pragma solidity 0.8.15;
 
 import {
-    CrossDomainEnabled
-} from "@eth-optimism/contracts/libraries/bridge/CrossDomainEnabled.sol";
-import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+    CrossDomainMessenger
+} from "@eth-optimism/contracts-bedrock/contracts/universal/CrossDomainMessenger.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 {
+abstract contract ERC721Bridge {
+    /**
+     * @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 NFT is refunded to the owner after an ERC721 bridge from the other
      *         chain fails.
@@ -30,6 +48,70 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled {
         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 Messenger contract on this domain.
+     */
+    CrossDomainMessenger public immutable messenger;
+
+    /**
+     * @notice Address of the bridge on the other network.
+     */
+    address public immutable otherBridge;
+
+    /**
+     * @notice Reserve extra slots (to a total of 50) in the storage layout for future upgrades.
+     */
+    uint256[49] private __gap;
+
+    /**
+     * @notice Ensures that the caller is a cross-chain message from the other bridge.
+     */
+    modifier onlyOtherBridge() {
+        require(
+            msg.sender == address(messenger) && messenger.xDomainMessageSender() == otherBridge,
+            "ERC721Bridge: function can only be called from the other bridge"
+        );
+        _;
+    }
+
     /**
      * @notice Ensures that the caller is this contract.
      */
@@ -37,4 +119,120 @@ abstract contract ERC721Bridge is Initializable, CrossDomainEnabled {
         require(msg.sender == address(this), "ERC721Bridge: function can only be called by self");
         _;
     }
+
+    /**
+     * @param _messenger   Address of the CrossDomainMessenger on this network.
+     * @param _otherBridge Address of the ERC721 bridge on the other network.
+     */
+    constructor(address _messenger, address _otherBridge) {
+        messenger = CrossDomainMessenger(_messenger);
+        otherBridge = _otherBridge;
+    }
+
+    /**
+     * @notice Initiates a bridge of an NFT to the caller's account on the other chain. Note that
+     *         this function can only be called by EOAs. Smart contract wallets should use the
+     *         `bridgeERC721To` function after ensuring that the recipient address on the remote
+     *         chain exists. Also note that the current owner of the token on this chain must
+     *         approve this contract to operate the NFT before it can be bridged.
+     *         **WARNING**: Do not bridge an ERC721 that was originally deployed on Optimism. This
+     *         bridge only supports ERC721s originally deployed on Ethereum. Users will need to
+     *         wait for the one-week challenge period to elapse before their Optimism-native NFT
+     *         can be refunded on L2.
+     *
+     * @param _localToken  Address of the ERC721 on this domain.
+     * @param _remoteToken Address of the ERC721 on the remote domain.
+     * @param _tokenId     Token ID to bridge.
+     * @param _minGasLimit Minimum gas limit for the bridge message on the other domain.
+     * @param _extraData   Optional data to forward to the other chain. Data supplied here will not
+     *                     be used to execute any code on the other chain and is only emitted as
+     *                     extra data for the convenience of off-chain tooling.
+     */
+    function bridgeERC721(
+        address _localToken,
+        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 the other chain. Note
+     *         that the current owner of the token on this chain must approve this contract to
+     *         operate the NFT before it can be bridged.
+     *         **WARNING**: Do not bridge an ERC721 that was originally deployed on Optimism. This
+     *         bridge only supports ERC721s originally deployed on Ethereum. Users will need to
+     *         wait for the one-week challenge period to elapse before their Optimism-native NFT
+     *         can be refunded on L2.
+     *
+     * @param _localToken  Address of the ERC721 on this domain.
+     * @param _remoteToken Address of the ERC721 on the remote domain.
+     * @param _to          Address to receive the token on the other domain.
+     * @param _tokenId     Token ID to bridge.
+     * @param _minGasLimit Minimum gas limit for the bridge message on the other domain.
+     * @param _extraData   Optional data to forward to the other chain. Data supplied here will not
+     *                     be used to execute any code on the other chain and is only emitted as
+     *                     extra data for the convenience of off-chain tooling.
+     */
+    function bridgeERC721To(
+        address _localToken,
+        address _remoteToken,
+        address _to,
+        uint256 _tokenId,
+        uint32 _minGasLimit,
+        bytes calldata _extraData
+    ) external {
+        require(_to != address(0), "ERC721Bridge: nft recipient cannot be address(0)");
+
+        _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;
 }

packages/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721Factory.sol

@@ -15,10 +15,10 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable {
     /**
      * @notice Emitted whenever a new OptimismMintableERC721 contract is created.
      *
-     * @param remoteToken Address of the token on the remote domain.
      * @param localToken  Address of the token on the this domain.
+     * @param remoteToken Address of the token on the remote domain.
      */
-    event OptimismMintableERC721Created(address indexed remoteToken, address indexed localToken);
+    event OptimismMintableERC721Created(address indexed localToken, address indexed remoteToken);
 
     /**
      * @notice Address of the ERC721 bridge on this network.
@@ -97,6 +97,6 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable {
         );
 
         isStandardOptimismMintableERC721[address(localToken)] = true;
-        emit OptimismMintableERC721Created(_remoteToken, address(localToken));
+        emit OptimismMintableERC721Created(address(localToken), _remoteToken);
     }
 }

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)
 )
@@ -84,25 +83,7 @@ describe('L1ERC721Bridge', () => {
     })
   })
 
-  describe('initialize', async () => {
-    it('reverts if messenger is address(0)', async () => {
-      await expect(
-        Factory__L1ERC721Bridge.deploy(
-          constants.AddressZero,
-          DUMMY_L2_BRIDGE_ADDRESS
-        )
-      ).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)')
-    })
-
-    it('reverts if otherBridge is address(0)', async () => {
-      await expect(
-        Factory__L1ERC721Bridge.deploy(
-          Fake__L1CrossDomainMessenger.address,
-          constants.AddressZero
-        )
-      ).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)')
-    })
-
+  describe('constructor', async () => {
     it('initializes correctly', async () => {
       expect(await L1ERC721Bridge.messenger()).equals(
         Fake__L1CrossDomainMessenger.address
@@ -277,7 +258,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', () => {
@@ -346,7 +327,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 () => {
@@ -362,7 +343,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', () => {

packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts

@@ -8,10 +8,8 @@ import { toRpcHexString } from '@eth-optimism/core-utils'
 import { NON_NULL_BYTES32, NON_ZERO_ADDRESS } from '../../helpers'
 import { expect } from '../../setup'
 
-const ERR_ALREADY_INITIALIZED = 'Initializable: contract is already initialized'
-const ERR_INVALID_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 =
@@ -67,34 +65,7 @@ describe('L2ERC721Bridge', () => {
     )
   })
 
-  describe('initialize', () => {
-    it('Should only be callable once', async () => {
-      await expect(
-        L2ERC721Bridge.initialize(
-          Fake__L2CrossDomainMessenger.address,
-          DUMMY_L1BRIDGE_ADDRESS
-        )
-      ).to.be.revertedWith(ERR_ALREADY_INITIALIZED)
-    })
-
-    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)')
-    })
-
+  describe('constructor', async () => {
     it('initializes correctly', async () => {
       expect(await L2ERC721Bridge.messenger()).equals(
         Fake__L2CrossDomainMessenger.address
@@ -115,7 +86,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 () => {
@@ -135,7 +106,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 () => {
@@ -349,7 +320,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 () => {