Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Latest commit

 

History

History
112 lines (85 loc) · 4.23 KB

082.md

File metadata and controls

112 lines (85 loc) · 4.23 KB

0xdeadbeef

high

NFTs will be frozen if receiver did not implement onERC721Received

Summary

The bridge allows sending ERC721 tokens to non-EOA addresses. If these contracts do not implement onERC721Received the ERC721 token will be locked in the bridge.

There is no warning of this behavior in docs or code.

Vulnerability Detail

The bridge implements a bridgeERC721To function that allows a caller to send NFT to an address on L1/L2. The NFT is sent from the to the corresponding layer and is either minted or transferred to the _to address.

If the _to address is a contract without an onERC721Received implementation - the NFT will be locked in the bridge.

On L1 L1ERC721Bridge:

    function finalizeBridgeERC721(
------
        address _to,
        uint256 _tokenId,
        bytes calldata _extraData
    ) external onlyOtherBridge {
------
        IERC721(_localToken).safeTransferFrom(address(this), _to, _tokenId);
------
    }

On L2 L2ERC721Bridge:

    function finalizeBridgeERC721(
------
        address _to,
        uint256 _tokenId,
        bytes calldata _extraData
    ) external onlyOtherBridge {
------
        IOptimismMintableERC721(_localToken).safeMint(_to, _tokenId);
------
    }

Both IOptimismMintableERC721 safeMint and safeTransferFrom use OpenZeppelin _checkOnERC721Received and revert if the target is a contract without the onERC721Received implementation.

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert("ERC721: transfer to non ERC721Receiver implementer");
                } else {
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

OpenZeppelin has implemented both _safeMint and safeTransferFrom in order to prevent user errors that transfer to contracts that cannot handle ERC721. In our case reverting will not prevent the user error, instead it will lock the NFT up.

Therefore, it is safer and more reasonable to "force" transferring/minting of the NFT

Impact

Users NFT will be locked permanently in the bridge.

There could be multiple use-cases where NFT would be sent to a contract without onERC721Received implementation.

An example use-case:

  1. On L1 Alice has a smart wallet/multisig/vault that holds the NFT and does not implement onERC721Received. Alice sends the NFT from her smart wallet/multisig/vault to her EOA address on L2 through bridgeERC721To to interact with L2 protocols.
  2. Alice EOA on L2 decides to send back her NFT to her smart wallet/multisig/vault through bridgeERC721To
  3. The NFT will be locked on the L1 bridge because the bridge cannot send transfer to her smart wallet/multisig/vault

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L1ERC721Bridge.sol#L68 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2ERC721Bridge.sol#L70

Tool used

Manual Review

Recommendation

While it is complex to validate the recipient of the NFT on the other layer there are some possible changes that can help.

Instead of calling safeMint or safeTransferFrom, the bridge can call the mint and transferFrom to "force" the transferring of the token. After the call, attempt to call the onERC721Received of the recipient a using the following example code:

if (Address.isContract(_to)) {
    try IERC721Receiver(_to).onERC721Received(address(this), _from, _tokenId, "") returns (bytes4 retval) {
    } catch (bytes memory reason) {}

Another possible solution is to add _to to the ERC721 approvals and require the caller to "pull" the NFT.