ERC-721 transfers do not use safeTransferFrom

[code423n4]

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/TokenTransferrerConstants.sol#L85
https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/TokenTransferrer.sol#L239
https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/TokenTransferrer.sol#L211

Vulnerability details

Impact

The cases where ERC-721 tokens are transferred are always using transferFrom and not safeTransferFrom. This means users need to be aware their tokens may get locked up in an unprepared contract recipient.

This may be intentional to reduce the potential for reentrancy, but it may strike users unprepared.

If it is by design, then it means users must be educated about this fact and the frontend would need to verify target addresses prior to submitting any transactions and hope that other frontends/integrations do not exists.

Should a ERC-721 compatible token be transferred to an unprepared contract, it would end up being locked up there. Moreover, if a contract explicitly wanted to reject ERC-721 safeTransfers, Seaport currently ignores it, effectively being non-compliant with the ERC-721 standard. Since Seaport is designed to be as generic as possible, this makes this issue a severity of Medium.

Proof of Concept

Context: TokenTransferrerConstants.sol#85, TokenTransferrer.sol#239, TokenTransferrer.sol#211

Tools Used

Manual review

Recommended Mitigation Steps

Consider using safeTransferFrom.

[0age]

adds substantial overhead and introduces potential reentrancy / DOS / griefing vectors; fulfillers can use a wrapper contract that asserts these checks 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 #203