_performERC721Transfer didnt check the return value

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/TokenTransferrer.sol#L220

Vulnerability details

IMPACT

_performERC721Transfer() didnt check if the return value is true or false, therefore this can be abused by supplying minimetoken like BADGER token, that will return false instead of revert, when trying to transfer more than the balance that the from has. https://etherscan.io/address/0x3472A5A71965499acd81997a54BBA8D852C6E53d#code and since the erc20transferFrom signature is the same as erc721transfFrom signature, we can supply BADGER token through the _performERC721Transfer, by inflating the identifier value, since the _transferERC721() only checking if the amount should be 1, instead of checking the identifier therefore we can bypass this and since the _performERC721Transfer didnt revert when doing the malicious transfer, this call will still be success

[0age]

this is incorrect, we check for falsey ERC20 return values and revert on them

[MrToph]

some additional info because they talked past each other: the linked _performERC721Transfer does indeed not check for a return value but that's because ERC721.transferFrom does not return a boolean - it returns void. So checking return values is not necessary for ERC721 but it is for ERC20. The _performERC20Transfer does indeed check the return value.

[0xleastwood]

As highlighted by the sponsor and @MrToph, ERC721 tokens do not return any value. As such, this is invalid.

[rfart]

The issue that was raised on this report is highlighting the way _preformERC721Transfer didint have a returns value check, therefore can be exploited by setting the NFT contract as an ERC20 by changing the itemType, and set the BADGER token as an ERC721 by changing the itemType, since both the transferFrom ABI that ERC20, and ERC721 has is the same, therefore, the NFT will still be transferred, but the BADGER token is not sent to the seller.

[HardlyDifficult]

Thanks for that clarification. That makes sense. The report does describe a potential bug but does not explore the impact or how it may be abused to take advantage of this possibility.

Lowering risk and merging with the warden's QA report #194

[rfart]

Hi there, can you give me a reason on why this report is considered a QA instead of a medium? since my explanation here https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/190#issuecomment-1192533829 is explaining on how to attack this with BADGER token or any Minime Token.

I think the report #194 is raising a different issue and didn't have any similarity with this report.

Thank you

[HardlyDifficult]

To keep the competition fair, we cannot consider additional information submitted after the contest ended. We can however use your feedback to help uncover something we overlooked or misunderstood from the original report. Making this a QA submission because the original submission did not sufficiently explore the potential impact.

The relation to #194 is just to group all your QA submissions into a single report.