search

code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

TokenTransferrer: to is unchecked in _performERC721Transfer(), which can cause user's NFT to be frozen #19

Closed code423n4 closed 2 years ago

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

The address to will receive the NFT when _performERC721Transfer() is called. However, if to is a contract address that does not support ERC721, the NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom

0age commented 2 years ago

safeTransferFrom introduces additional overhead and potential for denial-of-service and/or reentrancy; we made the conscious decision to use transferFrom instead.

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