feat(ctp): fix erc721 bridge related contracts

[sam-goldman]

Contains all of the fixes for the ERC721 bridge from the OpenZeppelin audit. Bumps the semver version to 1.0.0.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6f94bf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@eth-optimism/contracts-periphery	Minor
@eth-optimism/integration-tests	Patch
@eth-optimism/drippie-mon	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mergify]

Hey @sam-goldman! This PR has merge conflicts. Please fix them before continuing review.

[maurelian]

Not necessarily opposed, but I don't see anything about safeMint in the report?

[sam-goldman]

I thought it was in scope for L-08, where they mention the ERC721 bridges not using safeTransferFrom

[smartcontracts]

My concern with L-13 in general is that the contracts were built with the intent that assets would only be bridgeable from L1 to L1, but L-13 refactors the contracts in a way that makes it seem like assets can be bridged in both directions. I don't think that the warnings are sufficient -- the functions simply shouldn't exist on the L2 side. I therefore have a relatively strong preference for not including L-13 and not including the bridgeERC721 and bridgeERC721To functions in the L2ERC721Bridge contract.

I agree that refactoring to reduce the amount of duplicated code is worthwhile down the line, but I don't think we should do it now.

[smartcontracts]

L-13 also makes the rest of the PR quite difficult to review, unfortunately. Would feel much safer without it, even if that means duplicated code.

[sam-goldman]

The bridgeERC721 and bridgeERC721To functions in the L2ERC721Bridge allow users to bridge L1 native NFTs back to L1 after they've been bridged to L2. We need to keep that functionality or else users' NFTs will be stuck on L2 indefinitely. However, I agree that warnings aren't sufficient to prevent L2 native NFTs from accidentally being bridged to L1. Perhaps we can remove the refactoring that was done in L-13 and name the functions differently?

[smartcontracts]

The bridgeERC721 and bridgeERC721To functions in the L2ERC721Bridge allow users to bridge L1 native NFTs back to L1 after they've been bridged to L2. We need to keep that functionality or else users' NFTs will be stuck on L2 indefinitely. However, I agree that warnings aren't sufficient to prevent L2 native NFTs from accidentally being bridged to L1. Perhaps we can remove the refactoring that was done in L-13 and name the functions differently?

Ohh... I see. Why not just make it possible for L2 native NFTs to be bridged to L1 in that case? That would also put it more in line with the OptimismMintableERC20 which can be used on L1 as well.

[sam-goldman]

@maurelian and I thought it'd be best to keep the scope limited to L1 native NFTs at this point (slack conversation). We can definitely add support for L2 native NFTs later.

[maurelian]

@sam-goldman we've (very) recently had a discussion in which we agreed to move towards smaller PRs which are easier to review. I realize that it was precisely my request to combine these into one larger PR, but I think it would help to get this through if you could break this up into a few smaller ones with changes that are logically grouped together.

[sam-goldman]

@sam-goldman we've (very) recently had a discussion in which we agreed to move towards smaller PRs which are easier to review. I realize that it was precisely my request to combine these into one larger PR, but I think it would help to get this through if you could break this up into a few smaller ones with changes that are logically grouped together.

Done. Closed in favor of the smaller PRs in this repo.