AuctionDemo.claimAuction :: Highest bidder can cause permanent DOS and cannot refund to rest of the bidders.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Severity : High (loss of rest bidder funds)
likelihood : medium (the highest bidder should be a contract)

Proof of Concept

When Auction ends, the highest bidder can claim the nft, but being a contractas highest bidder can resfuse to implement onERC721Received fallback, can cause DOS. So the owner of token will not get his ether. And there's no way to refund the bidders if this attack is executed, since you cannot cancel bids after auction ends.

Here's he exact steps to execute this attack:

1. call minter.mintAndAuction() which airdrops the token to recipient and created auction status to true.
2. Now all the bidding takes place and end time of auction is passed.
3. The highest bidder can refuse to accept the ERC721 safeFransfer, so claimAuction will always revert. And the owner of that tokenId it airdropped to can also refust to accept the ether, by beibng a contract (low likelihood).
4. Bidders can't even get refunds by calling cancelBid functions because its past the endTime, and now the ether is stuck.

Tools Used

Manual review & foundry testing

Recommended Mitigation Steps

In AuctionDemo.participateToAuction, use transfer instead of safeTrnafer. Th attacker can afford to lose if doesn't implement the ERC721 receiving standard.

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 duplicate of #1759

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

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