diff --git a/.changeset/khaki-wombats-notice.md b/.changeset/khaki-wombats-notice.md new file mode 100644 index 0000000000000..6f789cfa2f120 --- /dev/null +++ b/.changeset/khaki-wombats-notice.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts-periphery': patch +--- + +Adds input validation to the ERC721Bridge constructor, fixes a typo in the L1ERC721Bridge, and removes the ERC721Refunded event declaration. diff --git a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol index 9bcc9dd250098..74f94d7d2c682 100644 --- a/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/L1/L1ERC721Bridge.sol @@ -10,7 +10,7 @@ import { Semver } from "@eth-optimism/contracts-bedrock/contracts/universal/Semv * @title L1ERC721Bridge * @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 - * 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 { /** diff --git a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol index 10473b32d7083..8aead3daa4b74 100644 --- a/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol +++ b/packages/contracts-periphery/contracts/universal/op-erc721/ERC721Bridge.sol @@ -30,24 +30,6 @@ abstract contract ERC721Bridge { 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. * @@ -125,6 +107,9 @@ abstract contract ERC721Bridge { * @param _otherBridge Address of the ERC721 bridge on the other network. */ 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); otherBridge = _otherBridge; } diff --git a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts index ca46761ccba04..24d49ee6e9e38 100644 --- a/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L1/L1ERC721Bridge.spec.ts @@ -85,6 +85,24 @@ describe('L1ERC721Bridge', () => { describe('constructor', 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( Fake__L1CrossDomainMessenger.address ) diff --git a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts index 5e89afa74807a..2bf492e95f4e1 100644 --- a/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts +++ b/packages/contracts-periphery/test/contracts/L2/L2ERC721Bridge.spec.ts @@ -36,6 +36,7 @@ describe('L2ERC721Bridge', () => { Factory__L1ERC721Bridge = await ethers.getContractFactory('L1ERC721Bridge') }) + let Factory__L2ERC721Bridge: ContractFactory let L2ERC721Bridge: Contract let L2ERC721: Contract let Fake__L2CrossDomainMessenger: FakeContract @@ -48,9 +49,11 @@ describe('L2ERC721Bridge', () => { ) // Deploy the contract under test - L2ERC721Bridge = await ( - await ethers.getContractFactory('L2ERC721Bridge') - ).deploy(Fake__L2CrossDomainMessenger.address, DUMMY_L1BRIDGE_ADDRESS) + Factory__L2ERC721Bridge = await ethers.getContractFactory('L2ERC721Bridge') + L2ERC721Bridge = await Factory__L2ERC721Bridge.deploy( + Fake__L2CrossDomainMessenger.address, + DUMMY_L1BRIDGE_ADDRESS + ) // Deploy an L2 ERC721 L2ERC721 = await ( @@ -66,6 +69,24 @@ describe('L2ERC721Bridge', () => { }) 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 () => { expect(await L2ERC721Bridge.messenger()).equals( Fake__L2CrossDomainMessenger.address