howlbot-integration[bot] commented 2 months ago

Lines of code

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

Vulnerability details

Impact

The mintWithBudget function allows players to mint multiple entities in one transaction. The function continues minting new entities as long as the remaining budget is sufficient for the next entity to mint. However, the function contains an incorrect check that renders it unusable after the first generation of entities is minted:

function mintWithBudget(
    bytes32[] calldata proof
  )
    public
    payable
    whenNotPaused
    nonReentrant
    onlyWhitelisted(proof, keccak256(abi.encodePacked(msg.sender)))
  {
    uint256 mintPrice = calculateMintPrice();
    uint256 amountMinted = 0;
    uint256 budgetLeft = msg.value;

    while (budgetLeft >= mintPrice && _tokenIds < maxTokensPerGen) { // @audit wrong check
      _mintInternal(msg.sender, mintPrice);
      amountMinted++;
      budgetLeft -= mintPrice;
      mintPrice = calculateMintPrice();
    }
    if (budgetLeft > 0) {
      (bool refundSuccess, ) = msg.sender.call{ value: budgetLeft }('');
      require(refundSuccess, 'Refund failed.');
    }
}

The check _tokenIds < maxTokensPerGen is incorrect. This check prevents the function from being used after the first generation's entities are minted since _tokenIds is incremented after each mint.

Proof Of Concept

Copy and Paste the following test in test/TraitForgeNft.test.s:

it('mintWithBudget will be useless after first generation', async () => {
  await nft.setMaxTokensPerGen(3); // We added this function in the smart contract for simplicity
  await entropyGenerator.transferOwnership(await nft.getAddress())

  // mint token 1 to user 1 => gen(token1) = 1
  await nft.connect(user1).mintToken(merkleInfo.whitelist[1].proof, {
    value: ethers.parseEther('1'),
   });

  // mint token 2 to user 1 => gen(token2) = 1
  await nft.connect(user1).mintToken(merkleInfo.whitelist[1].proof, {
    value: ethers.parseEther('1'),
  });

  // mint token 3 to user 2 => gen(token3) = 1 
  await nft.connect(user2).mintToken(merkleInfo.whitelist[2].proof, {
    value: ethers.parseEther('1'),
   });

  // at this point, generation1's entities are minted (3)

  const totalSupplyBefore = await nft.totalSupply()
  console.log("Total supply before: ", totalSupplyBefore)

  await nft.connect(user1).mintWithBudget(merkleInfo.whitelist[1].proof, {
    value: ethers.parseEther('10'),
  })
  const totalSupplyAfter = await nft.totalSupply()

  // No token was minted
  assert.equal(totalSupplyAfter, totalSupplyBefore)
})

yarn test --grep "mintWithBudget will be useless after first generation"

NOTE1: entropyGenerator is needed in the test to transfer the ownership of the entropyGenerator to nft so the _incrementGeneration will not revert, and entropyGenerator is not set globally form the base describe block, so make sure to set it as global variable:

describe('TraitForgeNFT', () => {
  let entityForging: EntityForging;
  let nft: TraitForgeNft;
+  let entropyGenerator: EntropyGenerator;
  let owner: HardhatEthersSigner;

  //...
-  const entropyGenerator = (await EntropyGenerator.deploy(await nft.getAddress())) as EntropyGenerator;
+   entropyGenerator = (await EntropyGenerator.deploy(await nft.getAddress())) as EntropyGenerator;
  //...
}

Note2: For simplicity purposes, we added a new function in TraitForgeNft to set the MaxTokensPerGen manually (so we don't need to mint 10,000 tokens):

function setMaxTokensPerGen(uint _max) external onlyOwner {
    maxTokensPerGen = _max;
}

Tools Used

Manual Review

Recommended Mitigation Steps

The incorrect check is supposed to prevent players from minting more than the allowed number of tokens per generation in one batch. Update the function as follows:

 function mintWithBudget(
    bytes32[] calldata proof
  )
    public
    payable
    whenNotPaused
    nonReentrant
    onlyWhitelisted(proof, keccak256(abi.encodePacked(msg.sender)))
  {
    uint256 mintPrice = calculateMintPrice();
    uint256 amountMinted = 0;
    uint256 budgetLeft = msg.value;

    while (budgetLeft >= mintPrice &&
-       _tokenIds < maxTokensPerGen
+       amountMinted < maxTokensPerGen
    ){
      _mintInternal(msg.sender, mintPrice);
      amountMinted++;
      budgetLeft -= mintPrice;
      mintPrice = calculateMintPrice();
    }
    if (budgetLeft > 0) {
      (bool refundSuccess, ) = msg.sender.call{ value: budgetLeft }('');
      require(refundSuccess, 'Refund failed.');
    }
  }

Assessed type

Invalid Validation

c4-judge commented 2 months ago

koolexcrypto changed the severity to 3 (High Risk)