feat(ctp): fix erc721 bridge related contracts

.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.

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 {
 
@@ -13,10 +15,24 @@ 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
-    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;
+    }
 }

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)
@@ -278,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
@@ -303,4 +309,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.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)
+    }
+  )
 })