Generation greater than `maxGeneration` can be minted

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L181-L200

Vulnerability details

Summary

maxGeneration is used to prevent users from minting generations that shouldn't exist. However it is not enforced in the code.

Root Cause

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L181-L200

function mintToken(
    bytes32[] calldata proof
  )
    public
    payable
    whenNotPaused
    nonReentrant
    onlyWhitelisted(proof, keccak256(abi.encodePacked(msg.sender)))
  {
    uint256 mintPrice = calculateMintPrice();
    require(msg.value >= mintPrice, 'Insufficient ETH send for minting.');

    _mintInternal(msg.sender, mintPrice);

    uint256 excessPayment = msg.value - mintPrice;
    if (excessPayment > 0) {
      (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }('');
      require(refundSuccess, 'Refund of excess payment failed.');
    }
  }

Vulnerability details

As we can see there is no check for maxGeneration inside mintToken function. It allows for minting tokens from generations that should not exist. Also there is no such check in _mintInternal function. As a result user can mint NFT with unexpected stats or attributes which might lead to loss of funds as the newly minted NFT can possibly be worthless.

function _mintInternal(address to, uint256 mintPrice) internal {
    if (generationMintCounts[currentGeneration] >= maxTokensPerGen) {
      _incrementGeneration();
    }

    _tokenIds++;
    uint256 newItemId = _tokenIds;
    _mint(to, newItemId);
    uint256 entropyValue = entropyGenerator.getNextEntropy();

    tokenCreationTimestamps[newItemId] = block.timestamp;
    tokenEntropy[newItemId] = entropyValue;
    tokenGenerations[newItemId] = currentGeneration;
    generationMintCounts[currentGeneration]++;
    initialOwners[newItemId] = to;

    if (!airdropContract.airdropStarted()) {
      airdropContract.addUserAmount(to, entropyValue);
    }

    emit Minted(
      msg.sender,
      newItemId,
      currentGeneration,
      entropyValue,
      mintPrice
    );

    _distributeFunds(mintPrice);
  }

Proof of Concept

Add this line to foundry.toml to increase the gas limit.

gas_limit = "18446744073709551615" # u64::MAX

Create new solidity file in test folder an paste this test:

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

import { Test, console } from 'forge-std/Test.sol';
import { EntityForging } from '../contracts/EntityForging/EntityForging.sol';
import { IEntityForging } from '../contracts/EntityForging/IEntityForging.sol';
import { TraitForgeNft } from '../contracts/TraitForgeNft/TraitForgeNft.sol';
import { EntropyGenerator } from '../contracts/EntropyGenerator/EntropyGenerator.sol';
import { Airdrop } from '../contracts/Airdrop/Airdrop.sol';
import { DevFund } from '../contracts/DevFund/DevFund.sol';
import { NukeFund } from '../contracts/NukeFund/NukeFund.sol';
import { EntityTrading } from '../contracts/EntityTrading/EntityTrading.sol';
import { IEntityTrading } from '../contracts/EntityTrading/IEntityTrading.sol';

contract EntityForgingTest is Test {
  EntityForging entityForging;
  TraitForgeNft traitForgeNft;
  EntropyGenerator entropyGenerator;
  Airdrop airdrop;
  DevFund devFund;
  NukeFund nukeFund;
  EntityTrading entityTrading;

  address ownerAddr = makeAddr('ownerAddr');
  address player1 = makeAddr('player1');
  address player2 = makeAddr('player2');

  function setUp() public {
    vm.startPrank(ownerAddr);

    // Deploy TraitForgeNft contract
    traitForgeNft = new TraitForgeNft();

    // Deploy Airdrop contract
    airdrop = new Airdrop();
    traitForgeNft.setAirdropContract(address(airdrop));
    airdrop.transferOwnership(address(traitForgeNft));

    // Deploy entropyGenerator contract
    entropyGenerator = new EntropyGenerator(address(traitForgeNft));
    entropyGenerator.writeEntropyBatch1();
    entropyGenerator.writeEntropyBatch2();
    entropyGenerator.writeEntropyBatch3();
    entropyGenerator.transferOwnership(address(traitForgeNft));
    traitForgeNft.setEntropyGenerator(address(entropyGenerator));

    // Deploy EntityForging contract
    entityForging = new EntityForging(address(traitForgeNft));
    traitForgeNft.setEntityForgingContract(address(entityForging));

    devFund = new DevFund();
    nukeFund = new NukeFund(
      address(traitForgeNft),
      address(airdrop),
      payable(address(devFund)),
      payable(ownerAddr)
    );
    traitForgeNft.setNukeFundContract(payable(address(nukeFund)));

    entityTrading = new EntityTrading(address(traitForgeNft));
    entityTrading.setNukeFundAddress(payable(address(nukeFund)));

    vm.stopPrank();

    vm.deal(player1, 10000 ether);
    vm.deal(player2, 10000 ether);

    // Used to avoid whitelist proof
    vm.warp(86402);
    vm.roll(86402);
  }

  function testMaxGenerationCanBeBypassed() public {
    // Only generation 1 can be minted
    vm.prank(ownerAddr);
    traitForgeNft.setMaxGeneration(1);

    // Mint first generation
    bytes32[] memory proof;
    vm.prank(player1);
    traitForgeNft.mintWithBudget{ value: 2000 ether }(proof);

    console.log(traitForgeNft.balanceOf(address(player1)));

    vm.prank(player2);
    traitForgeNft.mintToken{ value: 1 ether }(proof);

    console.log(traitForgeNft.balanceOf(address(player2)));

    // User 2 minted token from 2nd generation
    assertEq(traitForgeNft.getGeneration(), 2);
  }
}

Impact

Protocol functionality is disrupted as funtion does not work as intended. Users can mint from generations that do not exist. It can lead to loss of funds in a scenario where the protocol does not have prepared attributes for the new generation and user will mint NFT that can't be properly used.

Recommended Mitigation Steps

Enforce the maxGeneration in mintToken and mintWithBudget function.

function mintToken(
    bytes32[] calldata proof
  )
    public
    payable
    whenNotPaused
    nonReentrant
    onlyWhitelisted(proof, keccak256(abi.encodePacked(msg.sender)))
  {
    uint256 mintPrice = calculateMintPrice();
    require(msg.value >= mintPrice, 'Insufficient ETH send for minting.');

    _mintInternal(msg.sender, mintPrice);

+   require(getGeneration() <= maxGeneration, "can't be over max generation");

    uint256 excessPayment = msg.value - mintPrice;
    if (excessPayment > 0) {
      (bool refundSuccess, ) = msg.sender.call{ value: excessPayment }('');
      require(refundSuccess, 'Refund of excess payment failed.');
    }
  }

Assessed type

Invalid Validation

[c4-judge]

koolexcrypto marked the issue as satisfactory