From 02c457a5195b10b24bbfd78384dba2d3944e2c87 Mon Sep 17 00:00:00 2001
From: smartcontracts <kelvin@optimism.io>
Date: Tue, 27 Sep 2022 17:58:30 -0400
Subject: [PATCH] feat(ctp): no refunds for NFTs (#3582)
Removes refunds for failed NFT deposits.
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
---
.changeset/calm-ladybugs-occur.md | 6 ++
integration-tests/test/nft-bridge.spec.ts | 61 ----------------
.../contracts/L1/L1ERC721Bridge.sol | 52 +-------------
.../contracts/L2/L2ERC721Bridge.sol | 59 ++--------------
.../universal/op-erc721/ERC721Bridge.sol | 27 --------
.../test/contracts/L1/L1ERC721Bridge.spec.ts | 63 -----------------
.../test/contracts/L2/L2ERC721Bridge.spec.ts | 69 -------------------
7 files changed, 15 insertions(+), 322 deletions(-)
create mode 100644 .changeset/calm-ladybugs-occur.md
diff --git a/.changeset/calm-ladybugs-occur.md b/.changeset/calm-ladybugs-occur.md
new file mode 100644
index 000000000..00659e1ef
--- /dev/null
+++ b/.changeset/calm-ladybugs-occur.md
@@ -0,0 +1,6 @@
+---
+'@eth-optimism/integration-tests': patch
+'@eth-optimism/contracts-periphery': patch
+---
+
+Removes NFT refund logic if withdrawals fail.
diff --git a/integration-tests/test/nft-bridge.spec.ts b/integration-tests/test/nft-bridge.spec.ts
index b6632a14d..0640d97d8 100644
--- a/integration-tests/test/nft-bridge.spec.ts
+++ b/integration-tests/test/nft-bridge.spec.ts
@@ -3,7 +3,6 @@ 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'
@@ -18,9 +17,6 @@ import { withdrawalTest } from './shared/utils'
const TOKEN_ID: number = 1
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
@@ -313,61 +309,4 @@ 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)
- }
- )
})
diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol
index 74f94d7d2..5e8fa6bc8 100644
--- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol
+++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol
@@ -55,55 +55,6 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
uint256 _tokenId,
bytes calldata _extraData
) external onlyOtherBridge {
- try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) {
- // 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,
- _to,
- _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
- messenger.sendMessage(otherBridge, message, 600_000);
-
- // 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 {
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.
@@ -119,6 +70,9 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
// When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the
// withdrawer.
IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId);
+
+ // slither-disable-next-line reentrancy-events
+ emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
}
/**
diff --git a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
index 18ae5ed66..7ff753d71 100644
--- a/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
+++ b/packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
@@ -51,65 +51,15 @@ contract L2ERC721Bridge is ERC721Bridge, Semver {
uint256 _tokenId,
bytes calldata _extraData
) external onlyOtherBridge {
- try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _tokenId) {
- // 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.
- // 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,
- _to,
- _from, // Refund the NFT to the original owner on the remote chain.
- _tokenId,
- _extraData
- );
-
- // Send the message to the L1 bridge
- // slither-disable-next-line reentrancy-events
- messenger.sendMessage(otherBridge, message, 0);
-
- // 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 {
require(_localToken != address(this), "L2ERC721Bridge: local token cannot be self");
+ // Note that supportsInterface makes a callback to the _localToken address which is user
+ // provided.
require(
- // 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"
@@ -118,6 +68,9 @@ contract L2ERC721Bridge is ERC721Bridge, Semver {
// 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.
IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId);
+
+ // slither-disable-next-line reentrancy-events
+ emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
}
/**
diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol
index 8aead3daa..df892cf61 100644
--- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol
+++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol
@@ -49,25 +49,6 @@ abstract contract ERC721Bridge {
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.
*/
@@ -94,14 +75,6 @@ abstract contract ERC721Bridge {
_;
}
- /**
- * @notice Ensures that the caller is this contract.
- */
- modifier onlySelf() {
- 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.
diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts
index 24d49ee6e..8246eba2d 100644
--- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts
+++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts
@@ -385,56 +385,6 @@ describe('L1ERC721Bridge', () => {
)
})
- 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(
- 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.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,
- bobsAddress,
- aliceAddress,
- tokenId,
- NON_NULL_BYTES32,
- ])
- )
- // Gas limit is 0
- expect(depositCallToMessenger.args[2]).to.equal(FINALIZATION_GAS)
- })
-
it('should credit funds to the withdrawer to finalize withdrawal', async () => {
// finalizing the withdrawal emits an ERC721BridgeFinalized event with the correct arguments.
await expect(
@@ -472,17 +422,4 @@ describe('L1ERC721Bridge', () => {
})
})
})
-
- 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')
- })
- })
})
diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts
index 2bf492e95..aeb1f4c63 100644
--- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts
+++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts
@@ -130,62 +130,6 @@ describe('L2ERC721Bridge', () => {
).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE)
})
- it('should initialize a withdrawal if the L2 token is not compliant', async () => {
- // Deploy a non compliant ERC721
- const NonCompliantERC721 = await (
- await ethers.getContractFactory(
- '@openzeppelin/contracts/token/ERC721/ERC721.sol:ERC721'
- )
- ).deploy('L2Token', 'L2T')
-
- Fake__L2CrossDomainMessenger.xDomainMessageSender.returns(
- DUMMY_L1BRIDGE_ADDRESS
- )
-
- // A failed attempt to finalize the deposit causes an ERC721BridgeFailed event to be emitted.
- await expect(
- L2ERC721Bridge.connect(l2MessengerImpersonator).finalizeBridgeERC721(
- NonCompliantERC721.address,
- DUMMY_L1ERC721_ADDRESS,
- aliceAddress,
- bobsAddress,
- TOKEN_ID,
- NON_NULL_BYTES32,
- {
- from: Fake__L2CrossDomainMessenger.address,
- }
- )
- )
- .to.emit(L2ERC721Bridge, 'ERC721BridgeFailed')
- .withArgs(
- NonCompliantERC721.address,
- DUMMY_L1ERC721_ADDRESS,
- aliceAddress,
- bobsAddress,
- TOKEN_ID,
- NON_NULL_BYTES32
- )
-
- const withdrawalCallToMessenger =
- Fake__L2CrossDomainMessenger.sendMessage.getCall(0)
-
- expect(withdrawalCallToMessenger.args[0]).to.equal(DUMMY_L1BRIDGE_ADDRESS)
- expect(withdrawalCallToMessenger.args[1]).to.equal(
- Factory__L1ERC721Bridge.interface.encodeFunctionData(
- 'finalizeBridgeERC721',
- [
- DUMMY_L1ERC721_ADDRESS,
- NonCompliantERC721.address,
- bobsAddress,
- aliceAddress,
- TOKEN_ID,
- NON_NULL_BYTES32,
- ]
- )
- )
- expect(withdrawalCallToMessenger.args[2]).to.equal(0)
- })
-
it('should credit funds to the depositor', async () => {
Fake__L2CrossDomainMessenger.xDomainMessageSender.returns(
DUMMY_L1BRIDGE_ADDRESS
@@ -234,19 +178,6 @@ describe('L2ERC721Bridge', () => {
})
})
- 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', () => {
let L2Token: Contract
beforeEach(async () => {
--
2.23.0