Commit 9bb9fb28 authored by sam-goldman's avatar sam-goldman Committed by GitHub

nft: N-08 Easily bypass-able requirement (#3493)

parent fb27103f
...@@ -90,9 +90,11 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { ...@@ -90,9 +90,11 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
} }
/** /**
* @notice Initiates a bridge of an NFT to the caller's account on L2. Note that the current * @notice Initiates a bridge of an NFT to the caller's account on L2. Note that this function
* owner of the token on this chain must approve this contract to operate the NFT before * can only be called by EOAs. Smart contract wallets should use the `bridgeERC721To`
* it can be bridged. * function after ensuring that the recipient address on the remote chain exists. Also
* note that the current owner of the token on this chain must approve this contract to
* operate the NFT before it can be bridged.
* *
* @param _localToken Address of the ERC721 on this domain. * @param _localToken Address of the ERC721 on this domain.
* @param _remoteToken Address of the ERC721 on the remote domain. * @param _remoteToken Address of the ERC721 on the remote domain.
...@@ -109,8 +111,12 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable { ...@@ -109,8 +111,12 @@ contract L1ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
uint32 _minGasLimit, uint32 _minGasLimit,
bytes calldata _extraData bytes calldata _extraData
) external { ) external {
// Modifier requiring sender to be EOA. This check could be bypassed by a malicious // Modifier requiring sender to be EOA. This prevents against a user error that would occur
// contract via initcode, but it takes care of the user error we want to avoid. // 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"); require(!Address.isContract(msg.sender), "L1ERC721Bridge: account is not externally owned");
_initiateBridgeERC721( _initiateBridgeERC721(
......
...@@ -104,9 +104,11 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { ...@@ -104,9 +104,11 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
} }
/** /**
* @notice Initiates a bridge of an NFT to the caller's account on L1. Note that the current * @notice Initiates a bridge of an NFT to the caller's account on L1. Note that this function
* owner of the token on this chain must approve this contract to operate the NFT before * can only be called by EOAs. Smart contract wallets should use the `bridgeERC721To`
* it can be bridged. * function after ensuring that the recipient address on the remote chain exists. Also
* note that the current owner of the token on this chain must approve this contract to
* operate the NFT before it can be bridged.
* *
* @param _localToken Address of the ERC721 on this domain. * @param _localToken Address of the ERC721 on this domain.
* @param _remoteToken Address of the ERC721 on the remote domain. * @param _remoteToken Address of the ERC721 on the remote domain.
...@@ -123,8 +125,12 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable { ...@@ -123,8 +125,12 @@ contract L2ERC721Bridge is Semver, CrossDomainEnabled, Initializable {
uint32 _minGasLimit, uint32 _minGasLimit,
bytes calldata _extraData bytes calldata _extraData
) external { ) external {
// Modifier requiring sender to be EOA. This check could be bypassed by a malicious // Modifier requiring sender to be EOA. This prevents against a user error that would occur
// contract via initcode, but it takes care of the user error we want to avoid. // 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"); require(!Address.isContract(msg.sender), "L2ERC721Bridge: account is not externally owned");
_initiateBridgeERC721( _initiateBridgeERC721(
......
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