ctp: Remove ERC721Refunded events

.changeset/five-laws-drop.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-periphery': patch
+---
+
+Remove ERC721Refunded events

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

@@ -56,33 +56,22 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
         bytes calldata _extraData
     ) external onlyOtherBridge {
         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
-                );
-            }
+            // 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.
+            // In either case, we stop the process here and construct a withdrawal in which we
+            // flip the to and from addresses. This ensures that event-based accounting
+            // will indicate net-zero transfer to the recipient. The ERC721BridgeFailed event
+            // emitted below can also be used to identify this occurence.
             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.
+                _to,
                 _from, // Refund the NFT to the original owner on the remote chain.
                 _tokenId,
                 _extraData
@@ -115,14 +104,13 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
         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(_localToken != address(this), "L1ERC721Bridge: local token cannot be self");
+
+        // Checks that the L1/L2 NFT pair has a token ID that is escrowed in the L1 Bridge.
         require(
             deposits[_localToken][_remoteToken][_tokenId] == true,
             "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.

packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol

@@ -52,43 +52,30 @@ contract L2ERC721Bridge is ERC721Bridge, Semver {
         bytes calldata _extraData
     ) external onlyOtherBridge {
         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
-                );
-            }
+            // 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.
             // 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.
             // There is no way to prevent malicious token contracts altogether, but this does limit
             // user error and mitigate some forms of malicious contract behavior.
+            /// In either case, we stop the process here and construct a withdrawal in which we
+            // flip the to and from addresses. This ensures that event-based accounting
+            // will indicate net-zero transfer to the recipient. The ERC721BridgeFailed event
+            // emitted below can also be used to identify this occurence.
             bytes memory message = abi.encodeWithSelector(
                 L1ERC721Bridge.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.
+                _to,
                 _from, // Refund the NFT to the original owner on the remote chain.
                 _tokenId,
                 _extraData
             );
 
-            // Send message up to L1 bridge
+            // Send the message to the L1 bridge
             // slither-disable-next-line reentrancy-events
             messenger.sendMessage(otherBridge, message, 0);
 
@@ -115,16 +102,18 @@ contract L2ERC721Bridge is ERC721Bridge, Semver {
         address _to,
         uint256 _tokenId
     ) external onlySelf {
+        require(_localToken != address(this), "L2ERC721Bridge: local token cannot be self");
+
         require(
-            // slither-disable-next-line reentrancy-events
+            // Note that supportsInterface makes a callback to the _localToken address
+            // which is user provided.
             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"
         );
-        require(_localToken != address(this), "L2ERC721Bridge: local token cannot be self");
 
         // When a deposit is finalized, we give the NFT with the same tokenId to the account
         // on L2. Note that safeMint makes a callback to the _to address which is user provided.

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

@@ -407,7 +407,7 @@ describe('L1ERC721Bridge', () => {
           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.
+            bobsAddress,
             aliceAddress,
             tokenId,
             NON_NULL_BYTES32,
@@ -452,41 +452,6 @@ 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)
-      })
     })
   })
 

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

@@ -155,7 +155,7 @@ describe('L2ERC721Bridge', () => {
           [
             DUMMY_L1ERC721_ADDRESS,
             NonCompliantERC721.address,
-            L2ERC721Bridge.address, // the 'from' address is the l2 bridge to indicate that the l2 recipient never controlled the nft.
+            bobsAddress,
             aliceAddress,
             TOKEN_ID,
             NON_NULL_BYTES32,
@@ -211,41 +211,6 @@ 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 () => {