Auction can be DoS-ed with too many bids

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-size/blob/fec5a6148e9c08518465a04c08f1f3e9908eb5a8/src/SizeSealed.sol#L157

Vulnerability details

Proof of Concept

The bid functionality pushes every new bid to the a.bids array. On finalize this array is looped over. Now, if there are too many bids in it, the for loop gas consumption will go over the block gas limit, which is 30M gas units currently.

The code has a check to prevent this, by requiring that there is no more than 1000 bids, but a DoS can happen before 1000 bids. I calculated this the following way: Ran this test that does 2 bids

function testgas() public {
        (uint256 sellerQuoteBeforeFinalize, uint256 sellerBaseBeforeFinalize) = seller.balances();
        uint256 aid = seller.createAuction(
            baseToSell, reserveQuotePerBase, minimumBidQuote, startTime, endTime, unlockTime, unlockEnd, cliffPercent
        );
        bidder1.setAuctionId(aid);
        bidder2.setAuctionId(aid);
        (uint256 b1QuoteBeforeFinalize,) = bidder1.balances();
        (uint256 b2QuoteBeforeFinalize,) = bidder2.balances();
        bidder1.bidOnAuctionWithSalt(9 ether, 4.5e6, "hello");
        bidder2.bidOnAuctionWithSalt(2 ether, 2e6, "hello2");
        uint256[] memory bidIndices = new uint[](2);
        bidIndices[0] = 1;
        bidIndices[1] = 0;
    }

Cost is 850927 gas

Then ran the same test, but with calling finalize at the end

function testAuctionFinalizePartial() public {
        (uint256 sellerQuoteBeforeFinalize, uint256 sellerBaseBeforeFinalize) = seller.balances();
        uint256 aid = seller.createAuction(
            baseToSell, reserveQuotePerBase, minimumBidQuote, startTime, endTime, unlockTime, unlockEnd, cliffPercent
        );
        bidder1.setAuctionId(aid);
        bidder2.setAuctionId(aid);
        (uint256 b1QuoteBeforeFinalize,) = bidder1.balances();
        (uint256 b2QuoteBeforeFinalize,) = bidder2.balances();
        bidder1.bidOnAuctionWithSalt(9 ether, 4.5e6, "hello");
        bidder2.bidOnAuctionWithSalt(2 ether, 2e6, "hello2");
        uint256[] memory bidIndices = new uint[](2);
        bidIndices[0] = 1;
        bidIndices[1] = 0;
        vm.warp(endTime + 1);

        seller.finalize(bidIndices, 9 ether, 4.5e6);
    }

Cost is 938606 gas, so ~90k more, or you could say finalize with 2 bids costs 90k gas. Now, if we divide block gas limit by the gas cost of finalize with 2 bids we will do 30M / 90k = 333. This means we can have 2 bids 333 times, or ~666 bids in total before we hit the block gas limit on finalize which is way before the 1000 limit check.

Impact

The impact is any bad actor can DoS any auction by bidding a lot of times. Or if there are too many bids (no bad actor) it is possible to get into a DoS state also.

Recommendation

Change

if (bidIndex >= 1000) {
            revert InvalidState();
        }

to

if (bidIndex >= 500) {
            revert InvalidState();
 }

[trust1995]

Good effort, but the report assumes the difference in gas between no finalize and finalize is divided linearly for every bid, which is definitely far from the case. Additional bids only cost a (small) subset of finalize to be executed additional times.

[0xean]

yea, the gas assumptions here are wrong.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof