nft: L-08, M-06, L-05, N-05, M-01 (#3495)

* nft: M-01 Asymmetric failure behavior of ERC721 bridges

* nft: N-05 Inconsistent approach to auto-refunding failed token transfers

* nft: L-05 Auto withdrawal transactions can be misleading

* nft: M-06 Lack of input validation

* nft: L-08 ERC721 bridge contracts not using safeTransferFrom

* refactor(ctp): fix  `FakeOptimismMintableERC721` inherited contract

* feat(ctp)!: update nft bridge post-audit

* update `CrossDomainEnabled` import paths in nft bridges

* bump `OptimismMintableERC721Factory` semver

* Update packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
Co-authored-by: Maurelian <maurelian@protonmail.ch>

* added `localToken` check in the `completeOutBoundTransfer` function in NFT bridges

* nit: fix error message in `L1ERC721Bridge`
Co-authored-by: Maurelian <maurelian@protonmail.ch>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

.changeset/clever-spoons-kneel.md

+---
+'@eth-optimism/contracts-periphery': major
+'@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.

integration-tests/contracts/FakeOptimismMintableERC721.sol

@@ -2,21 +2,29 @@
 pragma solidity ^0.8.9;
 
 import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
+import { OptimismMintableERC721 } from "@eth-optimism/contracts-periphery/contracts/universal/op-erc721/OptimismMintableERC721.sol";
+import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 
-contract FakeOptimismMintableERC721 is ERC721 {
+contract FakeOptimismMintableERC721 is OptimismMintableERC721 {
 
-    address public immutable remoteToken;
-    address public immutable bridge;
+    constructor(
+        address _bridge,
+        address _remoteToken,
+        uint256 _remoteChainId
+    ) OptimismMintableERC721(
+        _bridge,
+        _remoteChainId,
+        _remoteToken,
+        "FakeERC721",
+        "FAKE"
+    ) {}
 
-    constructor(address _remoteToken, address _bridge) ERC721("FakeERC721", "FAKE") {
-        remoteToken = _remoteToken;
-        bridge = _bridge;
-    }
-
-    function mint(address to, uint256 tokenId) public {
-        _mint(to, tokenId);
+    function safeMint(address to, uint256 tokenId) external override {
+        _safeMint(to, tokenId);
     }
 
     // Burn will be called by the L2 Bridge to burn the NFT we are bridging to L1
-    function burn(address, uint256) external {}
+    function burn(address, uint256 tokenId) external override {
+        _burn(tokenId);
+    }
 }

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)
@@ -274,11 +277,18 @@ describe('ERC721 Bridge', () => {
           'FakeOptimismMintableERC721',
           bobWalletL2
         )
-      ).deploy(L1ERC721.address, L2ERC721Bridge.address)
+      ).deploy(
+        L2ERC721Bridge.address,
+        L1ERC721.address,
+        await getChainId(env.l1Wallet.provider)
+      )
       await FakeOptimismMintableERC721.deployed()
 
       // Use the fake contract to mint Alice an NFT with the same token ID
-      const tx = await FakeOptimismMintableERC721.mint(aliceAddress, TOKEN_ID)
+      const tx = await FakeOptimismMintableERC721.safeMint(
+        aliceAddress,
+        TOKEN_ID
+      )
       await tx.wait()
 
       // Check that Alice owns the NFT from the fake ERC721 contract
@@ -303,4 +313,61 @@ 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(
+        L2ERC721Bridge.address,
+        DUMMY_L1ERC721_ADDRESS,
+        await getChainId(env.l1Wallet.provider)
+      )
+      await L2NativeNFT.deployed()
+
+      // Alice mints an NFT from the L2 native ERC721 contract
+      const tx = await L2NativeNFT.safeMint(aliceAddress, TOKEN_ID)
+      await tx.wait()
+
+      // Check that Alice owns the L2 NFT
+      expect(await L2NativeNFT.ownerOf(TOKEN_ID)).to.equal(aliceAddress)
+
+      // Alice bridges her L2 native NFT to L1, which burns the L2 NFT.
+      const withdrawalTx = await L2ERC721Bridge.connect(
+        aliceWalletL2
+      ).bridgeERC721(
+        L2NativeNFT.address,
+        DUMMY_L1ERC721_ADDRESS,
+        TOKEN_ID,
+        FINALIZATION_GAS,
+        NON_NULL_BYTES
+      )
+      await withdrawalTx.wait()
+
+      // Check that the token was burnt on L2 (pre-refund).
+      await expect(L2NativeNFT.ownerOf(TOKEN_ID)).to.be.revertedWith(
+        'ERC721: owner query for nonexistent token'
+      )
+
+      // Relay the cross-domain transaction to L1, which initiates an L1 -> L2 message to refund
+      // Alice her L2 NFT.
+      await env.relayXDomainMessages(withdrawalTx)
+
+      // Wait for the L1 -> L2 message to finalize on L2
+      const txPair = await env.waitForXDomainTransaction(withdrawalTx)
+      await env.messenger.waitForMessageReceipt(txPair.remoteTx as MessageLike)
+
+      // Check that the L2 NFT has been refunded to Alice.
+      expect(await L2NativeNFT.ownerOf(TOKEN_ID)).to.equal(aliceAddress)
+    }
+  )
 })

packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol

 // 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";
+} from "@eth-optimism/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";
@@ -13,10 +14,10 @@ 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 Semver, CrossDomainEnabled, Initializable {
+contract L1ERC721Bridge is ERC721Bridge, Semver {
     /**
      * @notice Emitted when an ERC721 bridge to the other network is initiated.
      *
@@ -55,12 +56,30 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
         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;
 
-    // Maps L1 token to L2 token to token ID to a boolean indicating if the token is deposited
     /**
      * @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.
@@ -68,13 +87,13 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
     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)
+        Semver(1, 0, 0)
         CrossDomainEnabled(address(0))
     {
         initialize(_messenger, _otherBridge);
@@ -85,6 +104,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
      * @param _otherBridge Address of the ERC721 bridge on the other network.
      */
     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)");
+
         messenger = _messenger;
         otherBridge = _otherBridge;
     }
@@ -152,6 +174,8 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
         uint32 _minGasLimit,
         bytes calldata _extraData
     ) external {
+        require(_to != address(0), "ERC721Bridge: nft recipient cannot be address(0)");
+
         _initiateBridgeERC721(
             _localToken,
             _remoteToken,
@@ -188,20 +212,82 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
         uint256 _tokenId,
         bytes calldata _extraData
     ) external onlyFromCrossDomainAccount(otherBridge) {
-        // Checks that the L1/L2 token pair has a token ID that is escrowed in the L1 Bridge
+        try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) {
+            if (_from == otherBridge) {
+                // The _from address is the address of the remote bridge if a transfer fails to be
+                // finalized on the remote chain.
+                // slither-disable-next-line reentrancy-events
+                emit ERC721Refunded(_localToken, _remoteToken, _to, _tokenId, _extraData);
+            } else {
+                // slither-disable-next-line reentrancy-events
+                emit ERC721BridgeFinalized(
+                    _localToken,
+                    _remoteToken,
+                    _from,
+                    _to,
+                    _tokenId,
+                    _extraData
+                );
+            }
+        } catch {
+            // If the token ID for this L1/L2 NFT pair is not escrowed in the L1 Bridge or if
+            // another error occurred during finalization, we initiate a cross-domain message to
+            // send the NFT back to its original owner on L2. This can happen if an L2 native NFT is
+            // bridged to L1, or if a user mistakenly entered an incorrect L1 ERC721 address.
+            bytes memory message = abi.encodeWithSelector(
+                L2ERC721Bridge.finalizeBridgeERC721.selector,
+                _remoteToken,
+                _localToken,
+                address(this), // Set the new _from address to be this contract since the NFT was
+                // never transferred to the recipient on this chain.
+                _from, // Refund the NFT to the original owner on the remote chain.
+                _tokenId,
+                _extraData
+            );
+
+            // Send the message to the L2 bridge.
+            // slither-disable-next-line reentrancy-events
+            sendCrossDomainMessage(otherBridge, 600_000, message);
+
+            // slither-disable-next-line reentrancy-events
+            emit ERC721BridgeFailed(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
+        }
+    }
+
+    /**
+     * @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 {
+        // Checks that the L1/L2 NFT pair has a token ID that is escrowed in the L1 Bridge. Without
+        // this check, an attacker could steal a legitimate L1 NFT by supplying an arbitrary L2 NFT
+        // that maps to the L1 NFT.
         require(
             deposits[_localToken][_remoteToken][_tokenId] == true,
-            "Token ID is not escrowed in the L1 Bridge"
+            "L1ERC721Bridge: Token ID is not escrowed in the L1 Bridge"
         );
+        require(_localToken != address(this), "L1ERC721Bridge: local token cannot be self");
 
+        // Mark that the token ID for this L1/L2 token pair is no longer escrowed in the L1
+        // Bridge.
         deposits[_localToken][_remoteToken][_tokenId] = false;
 
-        // When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the withdrawer
-        // slither-disable-next-line reentrancy-events
-        IERC721(_localToken).transferFrom(address(this), _to, _tokenId);
-
-        // slither-disable-next-line reentrancy-events
-        emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
+        // When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the
+        // withdrawer.
+        IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId);
     }
 
     /**
@@ -226,6 +312,8 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
         uint32 _minGasLimit,
         bytes calldata _extraData
     ) internal {
+        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,

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

+// SPDX-License-Identifier: MIT
+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";
+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 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 Ensures that the caller is this contract.
+     */
+    modifier onlySelf() {
+        require(msg.sender == address(this), "ERC721Bridge: function can only be called by self");
+        _;
+    }
+}

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.

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

@@ -50,6 +50,13 @@ contract OptimismMintableERC721 is ERC721Enumerable, IOptimismMintableERC721 {
         string memory _name,
         string memory _symbol
     ) ERC721(_name, _symbol) {
+        require(_bridge != address(0), "OptimismMintableERC721: bridge cannot be address(0)");
+        require(_remoteChainId != 0, "OptimismMintableERC721: remote chain id cannot be zero");
+        require(
+            _remoteToken != address(0),
+            "OptimismMintableERC721: remote token cannot be address(0)"
+        );
+
         remoteChainId = _remoteChainId;
         remoteToken = _remoteToken;
         bridge = _bridge;
@@ -78,8 +85,8 @@ contract OptimismMintableERC721 is ERC721Enumerable, IOptimismMintableERC721 {
     /**
      * @inheritdoc IOptimismMintableERC721
      */
-    function mint(address _to, uint256 _tokenId) external virtual onlyBridge {
-        _mint(_to, _tokenId);
+    function safeMint(address _to, uint256 _tokenId) external virtual onlyBridge {
+        _safeMint(_to, _tokenId);
 
         emit Mint(_to, _tokenId);
     }

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

@@ -36,11 +36,11 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable {
     mapping(address => bool) public isStandardOptimismMintableERC721;
 
     /**
-     * @custom:semver 0.0.1
+     * @custom:semver 1.0.0
      *
      * @param _bridge Address of the ERC721 bridge on this network.
      */
-    constructor(address _bridge, uint256 _remoteChainId) Semver(0, 0, 1) {
+    constructor(address _bridge, uint256 _remoteChainId) Semver(1, 0, 0) {
         initialize(_bridge, _remoteChainId);
     }
 
@@ -50,6 +50,15 @@ contract OptimismMintableERC721Factory is Semver, OwnableUpgradeable {
      * @param _bridge Address of the ERC721 bridge on this network.
      */
     function initialize(address _bridge, uint256 _remoteChainId) public initializer {
+        require(
+            _bridge != address(0),
+            "OptimismMintableERC721Factory: bridge cannot be address(0)"
+        );
+        require(
+            _remoteChainId != 0,
+            "OptimismMintableERC721Factory: remote chain id cannot be zero"
+        );
+
         bridge = _bridge;
         remoteChainId = _remoteChainId;
 

packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts

@@ -23,7 +23,7 @@ const DUMMY_L2_BRIDGE_ADDRESS = ethers.utils.getAddress(
   '0x' + 'acdc'.repeat(10)
 )
 
-const FINALIZATION_GAS = 1_200_000
+const FINALIZATION_GAS = 600_000
 
 describe('L1ERC721Bridge', () => {
   // init signers
@@ -59,6 +59,7 @@ describe('L1ERC721Bridge', () => {
   let L1ERC721: MockContract<Contract>
   let L1ERC721Bridge: Contract
   let Fake__L1CrossDomainMessenger: FakeContract
+  let Factory__L1ERC721Bridge: ContractFactory
   beforeEach(async () => {
     // Get a new mock L1 messenger
     Fake__L1CrossDomainMessenger = await smock.fake<Contract>(
@@ -67,9 +68,11 @@ describe('L1ERC721Bridge', () => {
     )
 
     // Deploy the contract under test
-    L1ERC721Bridge = await (
-      await ethers.getContractFactory('L1ERC721Bridge')
-    ).deploy(Fake__L1CrossDomainMessenger.address, DUMMY_L2_BRIDGE_ADDRESS)
+    Factory__L1ERC721Bridge = await ethers.getContractFactory('L1ERC721Bridge')
+    L1ERC721Bridge = await Factory__L1ERC721Bridge.deploy(
+      Fake__L1CrossDomainMessenger.address,
+      DUMMY_L2_BRIDGE_ADDRESS
+    )
 
     L1ERC721 = await Factory__L1ERC721.deploy('L1ERC721', 'ERC')
 
@@ -81,11 +84,50 @@ describe('L1ERC721Bridge', () => {
     })
   })
 
+  describe('initialize', async () => {
+    it('reverts if messenger is address(0)', async () => {
+      await expect(
+        Factory__L1ERC721Bridge.deploy(
+          constants.AddressZero,
+          DUMMY_L2_BRIDGE_ADDRESS
+        )
+      ).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)')
+    })
+
+    it('reverts if otherBridge is address(0)', async () => {
+      await expect(
+        Factory__L1ERC721Bridge.deploy(
+          Fake__L1CrossDomainMessenger.address,
+          constants.AddressZero
+        )
+      ).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)')
+    })
+
+    it('initializes correctly', async () => {
+      expect(await L1ERC721Bridge.messenger()).equals(
+        Fake__L1CrossDomainMessenger.address
+      )
+      expect(await L1ERC721Bridge.otherBridge()).equals(DUMMY_L2_BRIDGE_ADDRESS)
+    })
+  })
+
   describe('ERC721 deposits', () => {
     beforeEach(async () => {
       await L1ERC721.connect(alice).approve(L1ERC721Bridge.address, tokenId)
     })
 
+    it('bridgeERC721() reverts if remote token is address(0)', async () => {
+      await expect(
+        L1ERC721Bridge.connect(alice).bridgeERC721(
+          L1ERC721.address,
+          constants.AddressZero,
+          tokenId,
+          FINALIZATION_GAS,
+          NON_NULL_BYTES32
+        )
+      ).to.be.revertedWith('ERC721Bridge: remote token cannot be address(0)')
+    })
+
     it('bridgeERC721() escrows the deposit and sends the correct deposit message', async () => {
       // alice calls deposit on the bridge and the L1 bridge calls transferFrom on the token.
       // emits an ERC721BridgeInitiated event with the correct arguments.
@@ -147,6 +189,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.
@@ -331,23 +386,57 @@ describe('L1ERC721Bridge', () => {
         )
       })
 
-      it('should revert if the l1/l2 token pair has a token ID that has not been escrowed in the l1 bridge', async () => {
+      it('should refund an L2 NFT that fails to be finalized on l1', async () => {
+        const RANDOM_L1_ERC721_ADDRESS = ethers.utils.getAddress(
+          '0x' + 'cdbc'.repeat(10)
+        )
+        // alice sends bob an nft that has an incorrect l1 erc721 address
         await expect(
           L1ERC721Bridge.finalizeBridgeERC721(
-            L1ERC721.address,
-            DUMMY_L2_BRIDGE_ADDRESS, // incorrect l2 token address
-            constants.AddressZero,
-            constants.AddressZero,
+            RANDOM_L1_ERC721_ADDRESS, // incorrect address for the l1 erc721
+            DUMMY_L2_ERC721_ADDRESS,
+            aliceAddress,
+            bobsAddress,
             tokenId,
             NON_NULL_BYTES32,
             {
               from: Fake__L1CrossDomainMessenger.address,
             }
           )
-        ).to.be.revertedWith('Token ID is not escrowed in the L1 Bridge')
+        )
+          .to.emit(L1ERC721Bridge, 'ERC721BridgeFailed')
+          .withArgs(
+            RANDOM_L1_ERC721_ADDRESS,
+            DUMMY_L2_ERC721_ADDRESS,
+            aliceAddress,
+            bobsAddress,
+            tokenId,
+            NON_NULL_BYTES32
+          )
+
+        // Get the second call from `Fake__L1CrossDomainMessenger` because the first call is `finalizeBridgeERC721`.
+        const depositCallToMessenger =
+          Fake__L1CrossDomainMessenger.sendMessage.getCall(1)
+
+        // Check the correct cross-chain call was sent:
+        // Message should be sent to the L2 bridge
+        expect(depositCallToMessenger.args[0]).to.equal(DUMMY_L2_BRIDGE_ADDRESS)
+        // Message data should be a call telling the L2DepositedERC721 to finalize the deposit
+        expect(depositCallToMessenger.args[1]).to.equal(
+          IL2ERC721Bridge.encodeFunctionData('finalizeBridgeERC721', [
+            DUMMY_L2_ERC721_ADDRESS,
+            RANDOM_L1_ERC721_ADDRESS,
+            L1ERC721Bridge.address, // the 'from' address is the l1 bridge to indicate that the l1 recipient never controlled the nft.
+            aliceAddress,
+            tokenId,
+            NON_NULL_BYTES32,
+          ])
+        )
+        // Gas limit is 0
+        expect(depositCallToMessenger.args[2]).to.equal(FINALIZATION_GAS)
       })
 
-      it('should credit funds to the withdrawer and not use too much gas', async () => {
+      it('should credit funds to the withdrawer to finalize withdrawal', async () => {
         // finalizing the withdrawal emits an ERC721BridgeFinalized event with the correct arguments.
         await expect(
           L1ERC721Bridge.finalizeBridgeERC721(
@@ -382,6 +471,54 @@ describe('L1ERC721Bridge', () => {
           )
         ).to.equal(false)
       })
+
+      it('should credit funds to the withdrawer to finalize refund', async () => {
+        // finalizing a refund emits an ERC721Refunded event with the correct arguments.
+        await expect(
+          L1ERC721Bridge.finalizeBridgeERC721(
+            L1ERC721.address,
+            DUMMY_L2_ERC721_ADDRESS,
+            DUMMY_L2_BRIDGE_ADDRESS, // _from is the l2 bridge for refunds
+            NON_ZERO_ADDRESS,
+            tokenId,
+            NON_NULL_BYTES32,
+            { from: Fake__L1CrossDomainMessenger.address }
+          )
+        )
+          .to.emit(L1ERC721Bridge, 'ERC721Refunded')
+          .withArgs(
+            L1ERC721.address,
+            DUMMY_L2_ERC721_ADDRESS,
+            NON_ZERO_ADDRESS,
+            tokenId,
+            NON_NULL_BYTES32
+          )
+
+        // NFT is transferred to new owner
+        expect(await L1ERC721.ownerOf(tokenId)).to.equal(NON_ZERO_ADDRESS)
+
+        // deposits state variable is updated
+        expect(
+          await L1ERC721Bridge.deposits(
+            L1ERC721.address,
+            DUMMY_L2_ERC721_ADDRESS,
+            tokenId
+          )
+        ).to.equal(false)
+      })
+    })
+  })
+
+  describe('completeOutboundTransfer', async () => {
+    it('reverts if caller is not L1 bridge', async () => {
+      await expect(
+        L1ERC721Bridge.completeOutboundTransfer(
+          L1ERC721.address,
+          DUMMY_L2_ERC721_ADDRESS,
+          bobsAddress,
+          tokenId
+        )
+      ).to.be.revertedWith('ERC721Bridge: function can only be called by self')
     })
   })
 })

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

@@ -20,6 +20,8 @@ const ERR_INVALID_WITHDRAWAL: string =
   'Withdrawal is not being initiated by NFT owner'
 const TOKEN_ID: number = 10
 
+const FINALIZATION_GAS = 600_000
+
 describe('L2ERC721Bridge', () => {
   let alice: Signer
   let aliceAddress: string
@@ -74,6 +76,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
@@ -157,7 +184,7 @@ describe('L2ERC721Bridge', () => {
           [
             DUMMY_L1ERC721_ADDRESS,
             NonCompliantERC721.address,
-            bobsAddress,
+            L2ERC721Bridge.address, // the 'from' address is the l2 bridge to indicate that the l2 recipient never controlled the nft.
             aliceAddress,
             TOKEN_ID,
             NON_NULL_BYTES32,
@@ -213,6 +240,54 @@ describe('L2ERC721Bridge', () => {
       const tokenIdOwner = await L2ERC721.ownerOf(TOKEN_ID)
       tokenIdOwner.should.equal(bobsAddress)
     })
+
+    it('should credit funds to the depositer to finalize refund', async () => {
+      Fake__L2CrossDomainMessenger.xDomainMessageSender.returns(
+        DUMMY_L1BRIDGE_ADDRESS
+      )
+
+      // Assert that nobody owns the L2 token initially
+      await expect(L2ERC721.ownerOf(TOKEN_ID)).to.be.revertedWith(
+        'ERC721: owner query for nonexistent token'
+      )
+
+      // finalizing a refund emits an ERC721Refunded event with the correct arguments.
+      await expect(
+        L2ERC721Bridge.connect(l2MessengerImpersonator).finalizeBridgeERC721(
+          L2ERC721.address,
+          DUMMY_L1ERC721_ADDRESS,
+          DUMMY_L1BRIDGE_ADDRESS, // _from is the l1 bridge for refunds
+          aliceAddress,
+          TOKEN_ID,
+          NON_NULL_BYTES32,
+          { from: Fake__L2CrossDomainMessenger.address }
+        )
+      )
+        .to.emit(L2ERC721Bridge, 'ERC721Refunded')
+        .withArgs(
+          L2ERC721.address,
+          DUMMY_L1ERC721_ADDRESS,
+          aliceAddress,
+          TOKEN_ID,
+          NON_NULL_BYTES32
+        )
+
+      // NFT is transferred to new owner
+      expect(await L2ERC721.ownerOf(TOKEN_ID)).to.equal(aliceAddress)
+    })
+  })
+
+  describe('completeOutboundTransfer', async () => {
+    it('reverts if caller is not L2 bridge', async () => {
+      await expect(
+        L2ERC721Bridge.completeOutboundTransfer(
+          L2ERC721.address,
+          DUMMY_L1ERC721_ADDRESS,
+          bobsAddress,
+          TOKEN_ID
+        )
+      ).to.be.revertedWith('ERC721Bridge: function can only be called by self')
+    })
   })
 
   describe('withdrawals', () => {
@@ -238,7 +313,19 @@ describe('L2ERC721Bridge', () => {
       ])
 
       const signer = await ethers.getSigner(L2ERC721Bridge.address)
-      await L2Token.connect(signer).mint(aliceAddress, TOKEN_ID)
+      await L2Token.connect(signer).safeMint(aliceAddress, TOKEN_ID)
+    })
+
+    it('bridgeERC721() reverts if remote token is address(0)', async () => {
+      await expect(
+        L2ERC721Bridge.connect(alice).bridgeERC721(
+          L2Token.address,
+          constants.AddressZero,
+          TOKEN_ID,
+          FINALIZATION_GAS,
+          NON_NULL_BYTES32
+        )
+      ).to.be.revertedWith('ERC721Bridge: remote token cannot be address(0)')
     })
 
     it('bridgeERC721() reverts when called by non-owner of nft', async () => {
@@ -331,6 +418,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(

packages/contracts-periphery/test/contracts/universal/OptimismMintableERC721.spec.ts

@@ -20,6 +20,7 @@ describe('OptimismMintableERC721', () => {
   let baseUri: string
   const remoteChainId = 100
 
+  let Factory__OptimismMintableERC721
   before(async () => {
     ;[l2BridgeImpersonator, alice] = await ethers.getSigners()
     l2BridgeImpersonatorAddress = await l2BridgeImpersonator.getAddress()
@@ -33,15 +34,15 @@ describe('OptimismMintableERC721', () => {
       '/tokenURI?uint256='
     )
 
-    OptimismMintableERC721 = await (
-      await ethers.getContractFactory('OptimismMintableERC721')
-    ).deploy(
+    Factory__OptimismMintableERC721 = await ethers.getContractFactory(
+      'OptimismMintableERC721'
+    )
+    OptimismMintableERC721 = await Factory__OptimismMintableERC721.deploy(
       l2BridgeImpersonatorAddress,
       remoteChainId,
       DUMMY_L1ERC721_ADDRESS,
       'L2ERC721',
-      'ERC',
-      { gasLimit: 4_000_000 } // Necessary to avoid an out-of-gas error
+      'ERC'
     )
 
     // Get a new fake L2 bridge
@@ -52,7 +53,7 @@ describe('OptimismMintableERC721', () => {
     )
 
     // mint an nft to alice
-    await OptimismMintableERC721.connect(l2BridgeImpersonator).mint(
+    await OptimismMintableERC721.connect(l2BridgeImpersonator).safeMint(
       aliceAddress,
       TOKEN_ID,
       {
@@ -62,6 +63,48 @@ describe('OptimismMintableERC721', () => {
   })
 
   describe('constructor', () => {
+    it('should revert if bridge is address(0)', async () => {
+      await expect(
+        Factory__OptimismMintableERC721.deploy(
+          ethers.constants.AddressZero,
+          remoteChainId,
+          DUMMY_L1ERC721_ADDRESS,
+          'L2ERC721',
+          'ERC'
+        )
+      ).to.be.revertedWith(
+        'OptimismMintableERC721: bridge cannot be address(0)'
+      )
+    })
+
+    it('should revert if remote chain id is address(0)', async () => {
+      await expect(
+        Factory__OptimismMintableERC721.deploy(
+          l2BridgeImpersonatorAddress,
+          0,
+          DUMMY_L1ERC721_ADDRESS,
+          'L2ERC721',
+          'ERC'
+        )
+      ).to.be.revertedWith(
+        'OptimismMintableERC721: remote chain id cannot be zero'
+      )
+    })
+
+    it('should revert if remote token is address(0)', async () => {
+      await expect(
+        Factory__OptimismMintableERC721.deploy(
+          l2BridgeImpersonatorAddress,
+          remoteChainId,
+          ethers.constants.AddressZero,
+          'L2ERC721',
+          'ERC'
+        )
+      ).to.be.revertedWith(
+        'OptimismMintableERC721: remote token cannot be address(0)'
+      )
+    })
+
     it('should be able to create a standard ERC721 contract with the correct parameters', async () => {
       expect(await OptimismMintableERC721.bridge()).to.equal(
         l2BridgeImpersonatorAddress
@@ -83,7 +126,7 @@ describe('OptimismMintableERC721', () => {
   describe('mint and burn', () => {
     it('should not allow anyone but the L2 bridge to mint and burn', async () => {
       await expect(
-        OptimismMintableERC721.connect(alice).mint(aliceAddress, 100)
+        OptimismMintableERC721.connect(alice).safeMint(aliceAddress, 100)
       ).to.be.revertedWith(
         'OptimismMintableERC721: only bridge can call this function'
       )
@@ -102,7 +145,7 @@ describe('OptimismMintableERC721', () => {
         .true
 
       // OptimismMintableERC721
-      expect(await OptimismMintableERC721.supportsInterface(0x051e4975)).to.be
+      expect(await OptimismMintableERC721.supportsInterface(0xe49bc7f8)).to.be
         .true
 
       // ERC721

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