The _settleAuction() function uses transferFrom to transfer the new token to the highest bidder but the transferFrom function doesn't check if the recipient aka the highest bidder is capable of receiving ERC721, in case the highest bidder is a custom contract that doesn't accept ERC721 transfer the token may be lost forever, and because those ERC721 tokens are later used for governance voting it result that some voting power is not used because of those lost tokens.
The _settleAuction() function uses transferFrom to transfer the new token to the highest bidder :
// Transfer the token to the highest bidder
token.transferFrom(address(this), _auction.highestBidder, _auction.tokenId);
Which is not recommended even if it uses less gas, and OpenZeppelin’s documentation also discourages the use of transferFrom(), and recommend using safeTransferFrom() whenever possible.
Tools Used
Visual Audit
Recommended Mitigation Steps
Call the safeTransferFrom method instead of transferFrom for NFT tokens transfers in the _settleAuction() function inside the Auction contract. Note that the Auction contract should inherit the ERC721TokenReceiver contract as a consequence.
GalloDaSballo
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L192
Vulnerability details
Impact
The
_settleAuction()function usestransferFromto transfer the new token to the highest bidder but thetransferFromfunction doesn't check if the recipient aka the highest bidder is capable of receiving ERC721, in case the highest bidder is a custom contract that doesn't accept ERC721 transfer the token may be lost forever, and because those ERC721 tokens are later used for governance voting it result that some voting power is not used because of those lost tokens.Proof of Concept
Instance occurs here : Auction.sol#L192
The
_settleAuction()function usestransferFromto transfer the new token to the highest bidder :Which is not recommended even if it uses less gas, and OpenZeppelin’s documentation also discourages the use of transferFrom(), and recommend using safeTransferFrom() whenever possible.
Tools Used
Visual Audit
Recommended Mitigation Steps
Call the
safeTransferFrommethod instead oftransferFromfor NFT tokens transfers in the_settleAuction()function inside the Auction contract. Note that theAuctioncontract should inherit the ERC721TokenReceiver contract as a consequence.