Skip to content

N
nebula
Commit 02c457a5 authored by smartcontracts's avatar smartcontracts Committed by GitHub
Browse files
Options

feat(ctp): no refunds for NFTs (#3582) 

Removes refunds for failed NFT deposits.
Co-authored-by: default avatarmergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
parent 628affc7
Show whitespace changes
Inline Side-by-side
Showing with 15 additions and 322 deletions
.changeset/calm-ladybugs-occur.md 0 → 100644
View file @ 02c457a5
---
'@eth-optimism/integration-tests': patch
'@eth-optimism/contracts-periphery': patch
---
Removes NFT refund logic if withdrawals fail.
integration-tests/test/nft-bridge.spec.ts
View file @ 02c457a5
...@@ -3,7 +3,6 @@ import { Contract, ContractFactory, utils, Wallet } from 'ethers' ...@@ -3,7 +3,6 @@ import { Contract, ContractFactory, utils, Wallet } from 'ethers'
import { ethers } from 'hardhat' import { ethers } from 'hardhat'
import { getChainId } from '@eth-optimism/core-utils' import { getChainId } from '@eth-optimism/core-utils'
import { predeploys } from '@eth-optimism/contracts' 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__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__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' import Artifact__L2ERC721Bridge from '@eth-optimism/contracts-periphery/artifacts/contracts/L2/L2ERC721Bridge.sol/L2ERC721Bridge.json'
...@@ -18,9 +17,6 @@ import { withdrawalTest } from './shared/utils' ...@@ -18,9 +17,6 @@ import { withdrawalTest } from './shared/utils'
const TOKEN_ID: number = 1 const TOKEN_ID: number = 1
const FINALIZATION_GAS: number = 600_000 const FINALIZATION_GAS: number = 600_000
const NON_NULL_BYTES: string = '0x1111' const NON_NULL_BYTES: string = '0x1111'
const DUMMY_L1ERC721_ADDRESS: string = ethers.utils.getAddress(
'0x' + 'acdc'.repeat(10)
)
describe('ERC721 Bridge', () => { describe('ERC721 Bridge', () => {
let env: OptimismEnv let env: OptimismEnv
...@@ -313,61 +309,4 @@ describe('ERC721 Bridge', () => { ...@@ -313,61 +309,4 @@ describe('ERC721 Bridge', () => {
expect(await L1ERC721.ownerOf(TOKEN_ID)).to.equal(L1ERC721Bridge.address) 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
View file @ 02c457a5
...@@ -55,55 +55,6 @@ contract L1ERC721Bridge is ERC721Bridge, Semver { ...@@ -55,55 +55,6 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
uint256 _tokenId, uint256 _tokenId,
bytes calldata _extraData bytes calldata _extraData
) external onlyOtherBridge { ) 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"); 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. // 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 { ...@@ -119,6 +70,9 @@ contract L1ERC721Bridge is ERC721Bridge, Semver {
// When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the // When a withdrawal is finalized on L1, the L1 Bridge transfers the NFT to the
// withdrawer. // withdrawer.
IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId); IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId);
// slither-disable-next-line reentrancy-events
emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
} }
/** /**
......
packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
View file @ 02c457a5
...@@ -51,65 +51,15 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { ...@@ -51,65 +51,15 @@ contract L2ERC721Bridge is ERC721Bridge, Semver {
uint256 _tokenId, uint256 _tokenId,
bytes calldata _extraData bytes calldata _extraData
) external onlyOtherBridge { ) 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"); 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( require(
// Note that supportsInterface makes a callback to the _localToken address
// which is user provided.
ERC165Checker.supportsInterface(_localToken, type(IOptimismMintableERC721).interfaceId), ERC165Checker.supportsInterface(_localToken, type(IOptimismMintableERC721).interfaceId),
"L2ERC721Bridge: local token interface is not compliant" "L2ERC721Bridge: local token interface is not compliant"
); );
require( require(
_remoteToken == IOptimismMintableERC721(_localToken).remoteToken(), _remoteToken == IOptimismMintableERC721(_localToken).remoteToken(),
"L2ERC721Bridge: wrong remote token for Optimism Mintable ERC721 local token" "L2ERC721Bridge: wrong remote token for Optimism Mintable ERC721 local token"
...@@ -118,6 +68,9 @@ contract L2ERC721Bridge is ERC721Bridge, Semver { ...@@ -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 // 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. // on L2. Note that safeMint makes a callback to the _to address which is user provided.
IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId); IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId);
// slither-disable-next-line reentrancy-events
emit ERC721BridgeFinalized(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
} }
/** /**
......
packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol
View file @ 02c457a5
...@@ -49,25 +49,6 @@ abstract contract ERC721Bridge { ...@@ -49,25 +49,6 @@ abstract contract ERC721Bridge {
bytes extraData 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. * @notice Messenger contract on this domain.
*/ */
...@@ -94,14 +75,6 @@ abstract contract ERC721Bridge { ...@@ -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 _messenger Address of the CrossDomainMessenger on this network.
* @param _otherBridge Address of the ERC721 bridge on the other network. * @param _otherBridge Address of the ERC721 bridge on the other network.
......
packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts
View file @ 02c457a5
...@@ -385,56 +385,6 @@ describe('L1ERC721Bridge', () => { ...@@ -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 () => { it('should credit funds to the withdrawer to finalize withdrawal', async () => {
// finalizing the withdrawal emits an ERC721BridgeFinalized event with the correct arguments. // finalizing the withdrawal emits an ERC721BridgeFinalized event with the correct arguments.
await expect( await expect(
...@@ -472,17 +422,4 @@ describe('L1ERC721Bridge', () => { ...@@ -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')
})
})
}) })
packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts
View file @ 02c457a5
...@@ -130,62 +130,6 @@ describe('L2ERC721Bridge', () => { ...@@ -130,62 +130,6 @@ describe('L2ERC721Bridge', () => {
).to.be.revertedWith(ERR_INVALID_X_DOMAIN_MESSAGE) ).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 () => { it('should credit funds to the depositor', async () => {
Fake__L2CrossDomainMessenger.xDomainMessageSender.returns( Fake__L2CrossDomainMessenger.xDomainMessageSender.returns(
DUMMY_L1BRIDGE_ADDRESS DUMMY_L1BRIDGE_ADDRESS
...@@ -234,19 +178,6 @@ describe('L2ERC721Bridge', () => { ...@@ -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', () => { describe('withdrawals', () => {
let L2Token: Contract let L2Token: Contract
beforeEach(async () => { beforeEach(async () => {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment