DoS via external call in AuctionDemo.claimAuction() permanently locks all bids on an auction in the contract

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

All bids on the affected auction will be locked in the contract; the NFT will not be transferred to the auction winner.

Proof of Concept

Notice the claimAuction() function and inline comment below:

    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);
                (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}(""); //No gas limit, DoS can occur
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

Notice that claimAuction() uses call() to transfer ETH to the bidders, and there is no gas limit on the call. As a result, if bidder's address is an attacker-controlled contract, it can execute arbitrary computation in the fallback function such that claimAuction() is guaranteed to out-of-gas revert. The cost to the attacker can be quite low, as they can bid first on the auction for a small/negligible amount.

The only way for bidders to be refunded after the auction is over is for claimAuction() to be called, so the affected auction's bids will be stuck in the contract forever. Furthermore, the NFT will not be transferred to the winning bidder.

Recommended Mitigation

Add a gas limit to the transfer calls. Alternatively, refactor the contract so that claimAuction() only transfers the NFT, and bidders must call a separate function to be refunded after the auction ends.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #1632

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[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 #1782

[c4-judge]

alex-ppg marked the issue as satisfactory