Skip to content

nft: N-08 Easily bypass-able requirement #3455

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

Closed
sam-goldman wants to merge 2 commits into from
Closed

nft: N-08 Easily bypass-able requirement #3455

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
f6916d5
nft: N-08 Easily bypass-able requirement
sam-goldman Aug 18, 2022
cce06de
Merge branch 'develop' into ctp/n-08
sam-goldman Sep 15, 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
12 changes: 9 additions & 3 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 @@ -95,7 +95,9 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable {
}

/**
* @notice Initiates a bridge of an NFT to the caller's account on L2.
* @notice Initiates a bridge of an NFT to the caller's account on L2. Note that this function
* can only be called by EOAs. Smart contract wallets should use the `bridgeERC721To`
* function after ensuring that the recipient address on the remote chain exists.
*
* @param _localToken Address of the ERC721 on this domain.
* @param _remoteToken Address of the ERC721 on the remote domain.
Expand All @@ -112,8 +114,12 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable {
uint32 _minGasLimit,
bytes calldata _extraData
) external {
// Modifier requiring sender to be EOA. This check could be bypassed by a malicious
// contract via initcode, but it takes care of the user error we want to avoid.
// Modifier requiring sender to be EOA. This prevents against a user error that would occur
// if the sender is a smart contract wallet that has a different address on the remote chain
// (or doesn't have an address on the remote chain at all). The user would fail to receive
// the NFT if they use this function because it sends the NFT to the same address as the
// caller. This check could be bypassed by a malicious contract via initcode, but it takes
// care of the user error we want to avoid.
require(!Address.isContract(msg.sender), "L1ERC721Bridge: account is not externally owned");

_initiateBridgeERC721(
Expand Down
12 changes: 9 additions & 3 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 @@ -109,7 +109,9 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable {
}

/**
* @notice Initiates a bridge of an NFT to the caller's account on L1.
* @notice Initiates a bridge of an NFT to the caller's account on L1. Note that this function
* can only be called by EOAs. Smart contract wallets should use the `bridgeERC721To`
* function after ensuring that the recipient address on the remote chain exists.
*
* @param _localToken Address of the ERC721 on this domain.
* @param _remoteToken Address of the ERC721 on the remote domain.
Expand All @@ -126,8 +128,12 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, OwnableUpgradeable {
uint32 _minGasLimit,
bytes calldata _extraData
) external {
// Modifier requiring sender to be EOA. This check could be bypassed by a malicious
// contract via initcode, but it takes care of the user error we want to avoid.
// Modifier requiring sender to be EOA. This prevents against a user error that would occur
// if the sender is a smart contract wallet that has a different address on the remote chain
// (or doesn't have an address on the remote chain at all). The user would fail to receive
// the NFT if they use this function because it sends the NFT to the same address as the
// caller. This check could be bypassed by a malicious contract via initcode, but it takes
// care of the user error we want to avoid.
require(!Address.isContract(msg.sender), "L2ERC721Bridge: account is not externally owned");

_initiateBridgeERC721(
Expand Down