TokenTransferrer is using unsafe transferFrom for ERC721 instead of safeTransferFrom. Necessary checks will be skipped!

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L220-L327
https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/test/index.js#L16275-L16323

Vulnerability details

Impact

TokenTransferrer is using unsafe transferFrom for ERC721 instead of safeTransferFrom. Necessary checks will be skipped!

Necessary checks include

• Check whether target is an EOA.
• Check whether target is a contract that implement ERC721Receiver.
• Check whether target contract is accepting transfer with particular _data bytes.

If these checks fail, transaction will be reverted. This is done to prevent the loss of NFT because of transferring into a contract that cannot receive ERC721.

But TokenTransferrer is using unsafe transferFrom which doesn't perform these check, so NFT may be transferred into a contract that cannot receive ERC721.

Moreover, with special DataRestricted NFT, a NFT that can only be transferred if and only if valid _data bytes is specified. Seaport can't handle this kind of NFT at all cost because _data bytes is only available on safeTransferFrom. This kind of NFT is likely block unsafe transferFrom function from being used.

Data restricted NFT has use case in GameFi where every transfer must be approved by the offchain backend.

You can find implementation of Data restricted NFT below

• DataRestrictedNft721: https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/contracts/attack/DataRestrictedNFT721.sol
• DataRestrictedNft1155: https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/contracts/attack/DataRestrictedNFT1155.sol

More detail: https://anallergytoanalogy.medium.com/jumping-into-solidity-the-erc721-standard-part-4-ad21e3a5d9c

Proof of Concept

Assembly code in _performERC721Transfer in TokenTransferrer clearly state that Seaport use unsafe transferFrom as shown below

            // Write calldata to memory starting with function selector.
            mstore(ERC721_transferFrom_sig_ptr, ERC721_transferFrom_signature)
            mstore(ERC721_transferFrom_from_ptr, from)
            mstore(ERC721_transferFrom_to_ptr, to)
            mstore(ERC721_transferFrom_id_ptr, identifier)

Furthermore, my new testcases show a revert message which indicate Seaport is calling unsafe transferFrom function which is blocked in DataRestrictedNft721 implementation

https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/test/index.js#L16275-L16323

    it("Seaport use unsafe transferFrom instead of safeTransferFrom", async () => {
      // Buyer mints nft
      const nftId = await mint721(buyer);

      // Buyer approves marketplace contract to transfer NFT
      await set721ApprovalForAll(buyer, marketplaceContract.address, true);

      // Seller mints ERC20
      const tokenAmount = minRandom(100);
      await mintAndApproveERC20(
        seller,
        marketplaceContract.address,
        tokenAmount
      );

      // Buyer approves marketplace contract to transfer ERC20 tokens too
      await expect(
        testERC20
          .connect(buyer)
          .approve(marketplaceContract.address, tokenAmount)
      )
        .to.emit(testERC20, "Approval")
        .withArgs(buyer.address, marketplaceContract.address, tokenAmount);

      const offer = [
        getTestItem20(tokenAmount.sub(100), tokenAmount.sub(100)),
      ];

      const consideration = [
        getTestItem721(nftId, 1, 1, seller.address),
        getTestItem20(50, 50, zone.address),
        getTestItem20(50, 50, owner.address),
      ];

      const { order, orderHash } = await createOrder(
        seller,
        zone,
        offer,
        consideration,
        0 // FULL_OPEN
      );

      await expect(
        marketplaceContract
          .connect(buyer)
          .fulfillOrder(order, toKey(false))
      // ).to.be.revertedWith("Forbidden");
      ).to.be.revertedWith("Use safeTransfer instead");
    })

The DataRestrictedNft721 implementation: https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/contracts/attack/DataRestrictedNFT721.sol is blocking unsafe transferFrom using

  function _transfer(address from, address, uint256) internal virtual override {
    require(from == address(0), "Use safeTransfer instead");
  }

  function _safeTransfer(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
  ) internal virtual override {
    require(keccak256(data) == keccak256(hex"1234"), "Forbidden");
    super._transfer(from, to, tokenId);
    require(checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer");
  }

This testcase is passed, which is a proof that Seaport use unsafe transferFrom instead of safeTransferFrom

Tools Used

Fork codebase and create special NFT and then create more testcase to check for reverted message.

More testcases created at https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/test/index.js#L16275-L16323

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom.

[0age]

design decision not to use safeTransferFrom, adds appreciable overhead, introduces new griefing vectors, and can be implemented out-of-band by a wrapper if desired.

[HardlyDifficult]

Using transferFrom instead of safeTransferFrom in order to avoid DoS or reentrancy concerns (and save gas) is a common approach. The concern raised is potentially valid however. Merging with the warden's QA report #188