Usage of Safe wallet can lead to permanent DoS of an auction, losing all funds of participants

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L112

Vulnerability details

Background

The Sponsor stated in the README.md of this contest:

Consider if any functionality does not work with a SAFE (formerly Gnosis) wallet.

However, any function that ends up using .safeTransferFrom to send an ERC721 token will not always work with Safe wallets because not all Safe wallets will have a fallbackManager implemented due to various (security) reasons. The fallbackManager ensures that a Safe wallet is able to receive ERC721 tokens by implementing onERC721Received.

Since in this project, safeTransferFrom is used to transfer the ERC721, this transfer will revert with the following error code:

file: smart-contracts/ERC721.sol
require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner or approved");

What for impact does this have?
Consider the following:

• Alice put's her 1/1 NFT on auction.
• BobDAO bids on the 1/1 NFT with the intention to win. They use their Safe multisig wallet.
• BobDAO ends up winning the auction out of 100 participants.
• The admin calls AuctionDemo.claimAuction to distribute the NFT.
• However, this call will revert because the Safe does not implement the ERC721TokenReceiver.
• This means that, all 100 other participants, will not be able to be refunded their money. The participants can not call cancelBid because the auction has ended.

Tools used

Manual review.

Recommended mitigation

For the exact scenario described above, split the refund mechanism into a different function that is not at mercy of the ERC721.safeTransferFrom() function succeeding, which people can call after the auction has ended.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1759

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)