AuctionDemo opens itself several DoS attack vectors

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The auctionDemo.auctionInfoData map holds important info on auctions, and hold this info per tokenId. Needless to say, for many auctions that may become popular and/or long running, the auctionInfoStru[] array for a given _tokenid can get very large.

mapping (uint256 => auctionInfoStru[]) public auctionInfoData;

There are several methods in this contract involve core logic that loops through the whole auctionInfoStru[] array for a tokenId. If this array is sufficiently large enough, then the transaction that called any of the methods that directly invoke or indirectly rely on this looping logic can easily run out of gas and thus fail. In order to retry again and have the transaction success, it might require a huge amount of gas which can be very expensive.

The following function directly invoke or indirectly rely on looping through auctionInfoData[_tokenid], where _tokenid is any given token being auctioned, which could very easily have potentially large length, enough so for a tx to run out of gas:

• participateToAuction
• returnHighestBid
• returnHighestBidder
• claimAuction
• cancelAllBids

This is essentially all the major functions of the NextGen auction functionality.

Some more details on the issues:

• returnHighestBid is widely used; any function that calls this is subject to potential tx failure if the loop through bids is large enough, which could happen quite often
• if an Admin calls claimAuction to reward winners and there was a sufficiently large enough amount of bidders, the tx to refund everyone and reward the winner will either fail or require a HUGE amount of gas to assure the tx finishes to completion; if Winner calls, they face the same issue rewarding themselves rightfully.

Tools Used

Solidity

Recommended Mitigation Steps

Pull model over push model -> Require Winners seeking their rightful prize and losers seeking refund to manually refund themselves; this can easily be implemented with a map lookup instead of large for loops.

Assessed type

DoS

[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

[alex-ppg]

This finding has been detected by the bot in H-01 and the bot has adequately labeled it as "High" in severity. It is indeed an issue that the Sponsor needs to consider, however, the bot issue alone is sufficient in highlighting what the problem with this particular array is.

[c4-judge]

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

[alex-ppg]

I will summarize all Wardens' concerns by specifying that the Sponsor must remediate this exhibit in their code. Specifically, they should either:

• Implement a mechanism whereby the last highest bid is refunded whenever a new one is placed, rendering the array unnecessary
• Implement an on-chain pagination mechanism that will attempt to refund users but be able to be stopped and resumed later by consecutive invocations.

Presently, it is trivial to force an auction into being inoperable which can lead to significant loss of funds for bidders.

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

this has been reported on the bot racer report [H-01] Permanent DoS due to non-shrinking array usage in an unbounded loop

[c4-judge]

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