Commit d3fe9b6d authored by sam-goldman's avatar sam-goldman Committed by GitHub

fix(ctp): post-audit cleanup for the erc721 bridge (#3549)

Co-authored-by: default avatarmergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
parent 590a4084
---
'@eth-optimism/contracts-periphery': patch
---
Adds input validation to the ERC721Bridge constructor, fixes a typo in the L1ERC721Bridge, and removes the ERC721Refunded event declaration.
...@@ -10,7 +10,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv ...@@ -10,7 +10,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv
* @title L1ERC721Bridge * @title L1ERC721Bridge
* @notice The L1 ERC721 bridge is a contract which works together with the L2 ERC721 bridge to * @notice The L1 ERC721 bridge is a contract which works together with the L2 ERC721 bridge to
* make it possible to transfer ERC721 tokens from Ethereum to Optimism. This contract * make it possible to transfer ERC721 tokens from Ethereum to Optimism. This contract
* acts as an escrow for ERC721 tokens deposted into L2. * acts as an escrow for ERC721 tokens deposited into L2.
*/ */
contract L1ERC721Bridge is ERC721Bridge, Semver { contract L1ERC721Bridge is ERC721Bridge, Semver {
/** /**
......
...@@ -30,24 +30,6 @@ abstract contract ERC721Bridge { ...@@ -30,24 +30,6 @@ abstract contract ERC721Bridge {
bytes extraData bytes extraData
); );
/**
* @notice Emitted when an NFT is refunded to the owner after an ERC721 bridge from the other
* chain fails.
*
* @param localToken Address of the token on this domain.
* @param remoteToken Address of the token on the remote domain.
* @param to Address to receive the refunded token.
* @param tokenId ID of the specific token being refunded.
* @param extraData Extra data for use on the client-side.
*/
event ERC721Refunded(
address indexed localToken,
address indexed remoteToken,
address indexed to,
uint256 tokenId,
bytes extraData
);
/** /**
* @notice Emitted when an ERC721 bridge from the other network is finalized. * @notice Emitted when an ERC721 bridge from the other network is finalized.
* *
...@@ -125,6 +107,9 @@ abstract contract ERC721Bridge { ...@@ -125,6 +107,9 @@ abstract contract ERC721Bridge {
* @param _otherBridge Address of the ERC721 bridge on the other network. * @param _otherBridge Address of the ERC721 bridge on the other network.
*/ */
constructor(address _messenger, address _otherBridge) { constructor(address _messenger, address _otherBridge) {
require(_messenger != address(0), "ERC721Bridge: messenger cannot be address(0)");
require(_otherBridge != address(0), "ERC721Bridge: other bridge cannot be address(0)");
messenger = CrossDomainMessenger(_messenger); messenger = CrossDomainMessenger(_messenger);
otherBridge = _otherBridge; otherBridge = _otherBridge;
} }
......
...@@ -85,6 +85,24 @@ describe('L1ERC721Bridge', () => { ...@@ -85,6 +85,24 @@ describe('L1ERC721Bridge', () => {
describe('constructor', async () => { describe('constructor', async () => {
it('initializes correctly', async () => { it('initializes correctly', async () => {
it('reverts if cross domain messenger is address(0)', async () => {
await expect(
Factory__L1ERC721Bridge.deploy(
constants.AddressZero,
DUMMY_L2_BRIDGE_ADDRESS
)
).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)')
})
it('reverts if other bridge is address(0)', async () => {
await expect(
Factory__L1ERC721Bridge.deploy(
Fake__L1CrossDomainMessenger.address,
constants.AddressZero
)
).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)')
})
expect(await L1ERC721Bridge.messenger()).equals( expect(await L1ERC721Bridge.messenger()).equals(
Fake__L1CrossDomainMessenger.address Fake__L1CrossDomainMessenger.address
) )
......
...@@ -36,6 +36,7 @@ describe('L2ERC721Bridge', () => { ...@@ -36,6 +36,7 @@ describe('L2ERC721Bridge', () => {
Factory__L1ERC721Bridge = await ethers.getContractFactory('L1ERC721Bridge') Factory__L1ERC721Bridge = await ethers.getContractFactory('L1ERC721Bridge')
}) })
let Factory__L2ERC721Bridge: ContractFactory
let L2ERC721Bridge: Contract let L2ERC721Bridge: Contract
let L2ERC721: Contract let L2ERC721: Contract
let Fake__L2CrossDomainMessenger: FakeContract let Fake__L2CrossDomainMessenger: FakeContract
...@@ -48,9 +49,11 @@ describe('L2ERC721Bridge', () => { ...@@ -48,9 +49,11 @@ describe('L2ERC721Bridge', () => {
) )
// Deploy the contract under test // Deploy the contract under test
L2ERC721Bridge = await ( Factory__L2ERC721Bridge = await ethers.getContractFactory('L2ERC721Bridge')
await ethers.getContractFactory('L2ERC721Bridge') L2ERC721Bridge = await Factory__L2ERC721Bridge.deploy(
).deploy(Fake__L2CrossDomainMessenger.address, DUMMY_L1BRIDGE_ADDRESS) Fake__L2CrossDomainMessenger.address,
DUMMY_L1BRIDGE_ADDRESS
)
// Deploy an L2 ERC721 // Deploy an L2 ERC721
L2ERC721 = await ( L2ERC721 = await (
...@@ -66,6 +69,24 @@ describe('L2ERC721Bridge', () => { ...@@ -66,6 +69,24 @@ describe('L2ERC721Bridge', () => {
}) })
describe('constructor', async () => { describe('constructor', async () => {
it('reverts if cross domain messenger is address(0)', async () => {
await expect(
Factory__L2ERC721Bridge.deploy(
constants.AddressZero,
DUMMY_L1BRIDGE_ADDRESS
)
).to.be.revertedWith('ERC721Bridge: messenger cannot be address(0)')
})
it('reverts if other bridge is address(0)', async () => {
await expect(
Factory__L2ERC721Bridge.deploy(
Fake__L2CrossDomainMessenger.address,
constants.AddressZero
)
).to.be.revertedWith('ERC721Bridge: other bridge cannot be address(0)')
})
it('initializes correctly', async () => { it('initializes correctly', async () => {
expect(await L2ERC721Bridge.messenger()).equals( expect(await L2ERC721Bridge.messenger()).equals(
Fake__L2CrossDomainMessenger.address Fake__L2CrossDomainMessenger.address
......
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