Closed code423n4 closed 2 years ago
adds substantial overhead and introduces potential reentrancy / DOS / griefing vectors; fulfillers can use a wrapper contract that asserts these checks if desired.
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
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 notsafeTransferFrom
. 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
.