AuctionDemo#claimAuction is permanently DoSed if highest bidder is a contract that does not implement onERC721Received

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

If the winner of an auction is a smart contract that does not implement onERC721Received, or if it is implemented but leads to a revert (which could be done unintentionally or maliciously) then the NFT, as well as the funds of all active bids will be trapped in the contract indefinitely.

Proof of Concept

AuctionDemo#claimAuction uses ERC721's safeTransferFrom to transfer the auctioned NFT from the current owner to the winning bidder. This function checks whether the receiver is a smart contract and if it is, attempts to call onERC721Received.

File: src\AuctionDemo.sol

104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
                     // @audit line below may revert if highestBidder does not implement onERC721Received
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); 
113:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

This scenario would lead to a complete DoS of claimAuction. Since it is the only function that is capable of sending the NFT and the ETH of the bidders from the contract, those funds would all be trapped in the contract.

• there is no way to avoid executing the safeTransferFrom call
• no new bids can be placed to replace highestBidder with a different address that is capable of receiving the NFT
• losing bidders cannot call cancelBid or cancelAllBids to retrieve their ETH because they revert if the auction is completed
• admins have no emergency function to retrieve the NFT or the ETH.

Recommended Mitigation Steps

Use transfer instead of safeTransfer. Although an auction winner that does not implement onERC721Received may not be able to access their NFT, they wouldn't have been able to receive it anyway if safeTransfer was used, and this way losing bidders still get their ETH back.

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
-               IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
+               IERC721(gencore).transferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }

Assessed type

ERC721

[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 primary issue

[c4-judge]

alex-ppg marked issue #739 as primary and marked this issue as a duplicate of 739

[c4-judge]

alex-ppg marked the issue as satisfactory