code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L239-L253

Vulnerability details

Impact

By not using safeTransferFrom() on NFTs it's possible for the NFT being bought or tipped, to be sent to a contract that is unable to handle it and therefore the NFT may be locked forever.

This has been found to be of Medium risk in multiple contests and falls into the sponsor's description of the Medium tier: might lead to a small subset of funds being at risk

Note that this report is only about the optimized version of the code. I've split off a similar issue in the reference code to my QA report, since issues with the reference code are considered to be of low risk

Proof of Concept

The EIP-721 standard says the following about transferFrom():

    /// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
    ///  TO CONFIRM THAT `_to` IS CAPABLE OF RECEIVING NFTS OR ELSE
    ///  THEY MAY BE PERMANENTLY LOST
    /// @dev Throws unless `msg.sender` is the current owner, an authorized
    ///  operator, or the approved address for this NFT. Throws if `_from` is
    ///  not the current owner. Throws if `_to` is the zero address. Throws if
    ///  `_tokenId` is not a valid NFT.
    /// @param _from The current owner of the NFT
    /// @param _to The new owner
    /// @param _tokenId The NFT to transfer
    function transferFrom(address _from, address _to, uint256 _tokenId) external payable;

https://github.com/ethereum/EIPs/blob/78e2c297611f5e92b6a5112819ab71f74041ff25/EIPS/eip-721.md?plain=1#L103-L113

The sponsors code has no such capability checks, and uses transferFrom() rather than safeTransferFrom() in the optimized code:

File: contracts/lib/TokenTransferrer.sol   #1

239               mstore(ERC721_transferFrom_sig_ptr, ERC721_transferFrom_signature)
240               mstore(ERC721_transferFrom_from_ptr, from)
241               mstore(ERC721_transferFrom_to_ptr, to)
242               mstore(ERC721_transferFrom_id_ptr, identifier)
243   
244               // Perform the call, ignoring return data.
245               let success := call(
246                   gas(),
247                   token,
248                   0,
249                   ERC721_transferFrom_sig_ptr,
250                   ERC721_transferFrom_length,
251                   0,
252                   0
253               )

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L239-L253

File: contracts/lib/TokenTransferrerConstants.sol   #2

85   uint256 constant ERC721_transferFrom_signature = ERC20_transferFrom_signature;

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrerConstants.sol#L85

File: contracts/lib/TokenTransferrerConstants.sol   #3

48   // abi.encodeWithSignature("transferFrom(address,address,uint256)")
49   uint256 constant ERC20_transferFrom_signature = (
50       0x23b872dd00000000000000000000000000000000000000000000000000000000
51   );

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrerConstants.sol#L48-L51

As with one of the previous contests linked to above, there's a mixture of both safe and non-safe functions in the code, because the safe method is used for ERC1155 tokens:

File: contracts/lib/TokenTransferrer.sol   #5

365               mstore(
366                   ERC1155_safeTransferFrom_sig_ptr,
367                   ERC1155_safeTransferFrom_signature
368               )
369               mstore(ERC1155_safeTransferFrom_from_ptr, from)
370               mstore(ERC1155_safeTransferFrom_to_ptr, to)
371               mstore(ERC1155_safeTransferFrom_id_ptr, identifier)
372               mstore(ERC1155_safeTransferFrom_amount_ptr, amount)
373               mstore(
374                   ERC1155_safeTransferFrom_data_offset_ptr,
375                   ERC1155_safeTransferFrom_data_length_offset
376               )
377               mstore(ERC1155_safeTransferFrom_data_length_ptr, 0)
378   
379               let success := call(
380                   gas(),
381                   token,
382                   0,
383                   ERC1155_safeTransferFrom_sig_ptr,
384                   ERC1155_safeTransferFrom_length,
385                   0,
386                   0
387               )

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L365-L387

File: contracts/lib/TokenTransferrerConstants.sol   #6

58   // abi.encodeWithSignature(
59   //     "safeTransferFrom(address,address,uint256,uint256,bytes)"
60   // )
61   uint256 constant ERC1155_safeTransferFrom_signature = (
62       0xf242432a00000000000000000000000000000000000000000000000000000000
63   );

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrerConstants.sol#L58-L63

Surprisingly, the Trail of Bits audit flagged the NFT-related code as having a Medium-severity issue due to onERC721Received() allowing reentrant changes to the NFT on transfer, even though neither of the versions of the code that was audited actually calls the safe version of the function, which is the only way the onERC721Received() call would be triggered.

As one would expect based on the EIP, neither OpenZeppelin nor solmate execute the onERC721Received() callback for the non-safe versions

Tools Used

Code inspection

Recommended Mitigation Steps

Use safeTransferFrom() rather than transferFrom()

0age commented 2 years ago

addressed in other comments

HardlyDifficult commented 2 years ago

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. Grouping with the warden's QA report #145

IllIllI000 commented 1 year ago

@HardlyDifficult the readme doesn't mention that it's in order to save gas/avoid reentrancy, which usually is required for things like this to be downgraded

HardlyDifficult commented 1 year ago

For some protocols I would support a higher severity for this issue. But an NFT marketplace is different IMO. The user is attempting to purchase an NFT - if they send it to an unsupported wallet that was an explicit decision they made as the intent was clearly to acquire that asset.