Skip to content

feat(ctp): no refunds for NFTs #3582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 2 commits into from
Sep 27, 2022
Merged

feat(ctp): no refunds for NFTs #3582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76e8065
feat(ctp): no refunds for NFTs
smartcontracts Sep 27, 2022
ad061c7
Merge branch 'develop' into sc/ctp-no-refunds
mergify[bot] Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/calm-ladybugs-occur.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@eth-optimism/integration-tests': patch
'@eth-optimism/contracts-periphery': patch
---

Removes NFT refund logic if withdrawals fail.
61 changes: 0 additions & 61 deletions integration-tests/test/nft-bridge.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
)
})
52 changes: 3 additions & 49 deletions packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
}

/**
Expand Down
59 changes: 6 additions & 53 deletions packages/contracts-periphery/contracts/L2/L2ERC721Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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);
}

/**
Expand Down
27 changes: 0 additions & 27 deletions packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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.
Expand Down
63 changes: 0 additions & 63 deletions packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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')
})
})
})
Loading