diff --git a/.changeset/five-laws-drop.md b/.changeset/five-laws-drop.md new file mode 100644 index 0000000000000..7c985f2b7ad85 --- /dev/null +++ b/.changeset/five-laws-drop.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts-periphery': patch +--- + +Remove ERC721Refunded events diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 444926bc756cc..9bcc9dd250098 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/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. diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol index 30a17660835cc..18ae5ed667d48 100644 --- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol +++ b/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,8 +102,11 @@ 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" ); @@ -124,7 +114,6 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { _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. diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index 6c05d18eed302..ca46761ccba04 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/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) - }) }) }) diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index f46fb134b5380..5e89afa74807a 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/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 () => {