search

code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

Use safeTransfer to send ERC721 tokens #711

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L192

Vulnerability details

Impact

Winner of the auction can lose his NFT

Proof of Concept

When you settle and auction you transfer the NFT using transferFrom

        token.transferFrom(address(this), _auction.highestBidder, _auction.tokenId);

Maybe this is just an intended behaviour and is the responsability of the bidder but maybe you could use a withdraw pattern

Recommended Mitigation Steps

A solution could be to use a withdraw pattern and check if the receiver cannot handle nft after the send fails allow him to claim in another functions. I believe that this change would be consistent and similar to the funciton _handleOutgoingTransfer.

GalloDaSballo commented 2 years ago

Dup of https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/356

GalloDaSballo commented 2 years ago

This report is the only one that spoke about using Pull Payments when dealing with griefers, because the finding is one line I will still keep as invalid.

However am writing this because I have considered making this the only valid ERC721.safeTransfer report as it at least explained what the actual refactoring needs to look like (using safeTransfer is a grief / DOS vector)

I recommend the warden to expand next time, I will notice