claimAuction will be permanently DoS'd if there are too many participants due to gas limit

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

If there are too many participants, claimAuction() cannot be called due to gas limit, which results in permanent denial of service. Non-winners will not be able to get their ETH back. The winner cannot get his NFT and the NFT owner cannot get his money.

Proof of Concept

This issue builds on top of [H-01] in the automated report. In the report, it mentions

Permanent DoS due to non-shrinking array usage in an unbounded loop

Although theoretically true, the reported issue is extremely difficult to achieve because of how big uint256 is.

In this issue however, it only takes about 4000 genuine participants in an auction to permanently DoS the claimAuction() function.

Let's look at how the auction works.

Whenever a participant has a bid higher than the previous highest bid, their participation will be sored in the auctionInfoData mapping.

    function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

The participant can choose to cancel their bid and reclaim their ETH through cancelBid() and cancelAllBids(), provided that the auction has not ended. Otherwise, they can only reclaim their ETH when claimAuction() is called by either the winner or the admin.

The issue lies in the gas cost for refunding all the bids. If there are too many participants to refund, the claimAuction() function will reach ETH gas limit, which is 30 million, and will result in a permanent DoS.

Take a look at the simple contract below, and run it on Remix. Call deposit with some ETH to deposit ETH into the contract.

pragma solidity ^0.8.0;

contract Paying {

    function deposit() public payable {}

    function emergencyWithdraw(address owner) public {
        (bool success, ) = owner.call{value: 1 ether}("");
    }

    function emergencyWithdraw2(address owner) public {
        (bool success, ) = owner.call{value: 1 ether}("");
        (bool success2, ) = owner.call{value: 1 ether}("");
    }
  
    function LoopemergencyWithdraw(address owner, uint len) public {
        for(uint256 i; i <= len; i ++){
        (bool success, ) = owner.call{value: 0.0001 ether}("");
        }
    }
}

1. emergencyWithdraw costs 28946 gas (low level call is used once)
2. emergencyWithdraw2 costs 36055 gas (low level call is used twice)
3. If the loop runs 4200 times, LoopemergencyWithdraw costs 30,513,134 gas, which is higher than the gas limit.

The number of runnable loop in the actual protocol will be a lot less than 4200 because of all the additional checks involved in claimAuction(), but this shows that if there is ~4000 participants in an auction that didn't cancel their bid, the claimAuction() function will be permanently DoS'd. 4000 genuine participants is not that impossible to achieve, unlike reaching uint256 value.

Users cannot get back their ETH and the winner cannot get his NFT. Worst still, it looks like the ETH will be stuck in the contract since there is no function to withdraw ETH from the contract.

Tools Used

Remix

Recommended Mitigation Steps

1. Limit the amount of participants per auction.
2. Have another function to allow the non-winners to claim their own bid amount even after the auction end time.
3. Have an emergencyWithdraw function, usable only by the admin to circumvent such errors.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #1952

[c4-judge]

alex-ppg marked the issue as duplicate of #2038

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Out of scope

[cryptostaker2]

Hi @alex-ppg,

This is not a duplicate of H-01 , which states that attacker can increase the array length by himself (mentioned at the start of the issue).

If the attacker increases the array length to DoS auction, bidders can simply not participate in the auction.

This issue is about logic failure instead of attack griefing due to genuine auction behaviour. If many people participate in the auction (given the condition that auction time is long, auction prices are low, people are interested in getting the NFT, and they do not reclaim their funds back before the auction ends), the claim function right at the very end will never be able to run because function is out of gas.

Most of the duplicated issues talk about attacker growing the array size, which is a duplicate of H-01. Some issues that are like mine are those that are not talking about attackers, like #1851, #435, #639, #1200

Thank you for your time!

[alex-ppg]

Hey @cryptostaker2, thanks for bringing this to my attention. Regardless of the rationalization of the severity or the way that the array size increases, the root cause of all duplicated vulnerabilities is that the array increases its size in an unbounded way, resulting in a DoS attack.

The bot report that caused this and relevant submissions to be OOS denotes the root cause and marks it as "high-severity", meaning that it has been aptly given the attention it needs. If the Sponsor corrects the issue described in the relevant bot report, this attack / natural DoS will no longer be possible. As the root cause has been identified by the bot report, I am inclined to mark any vulnerability arising from it (regardless of how it results in a high size) to be OOS.