Discrepancy between nfts minted, price of nft when a generation changes & position of _incrementGeneration() inside _mintInternal() & _mintNewEntity()

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L280-L283
https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L227-L232

Vulnerability details

The report covers two bugs:

• User pays the maximum price of the last generation when minting the 1st nft of the next generation.
• Wrong positioning of _incrementGeneration() will lead to the user paying the starting price of 1st nft of generation 1 even when the nft being minted is the 1st nft of a different generation.

Proof of Concept

1st Bug Scenario:

• Taking generation 1 into consideration. Only direct minting can occur. No nft can be minted through forging.

File: TraitForgeNft.sol

  function calculateMintPrice() public view returns (uint256) {
    uint256 currentGenMintCount = generationMintCounts[currentGeneration];
    uint256 priceIncrease = priceIncrement * currentGenMintCount;
    uint256 price = startPrice + priceIncrease;
    return price;
  }

• When the 1st nft is minted, the price to be payed is equal to startPrice (0.005 ether). _tokenIds & generationMintCounts is updated to 1. Meaning that one nft has been minted in the protocol.
• To mint the 2nd nft, the price to be payed is 0.005 + (0.0000245 * 1) = 0.0050245 ether. _tokenIds & generationMintCounts is updated to 2. Meaning that as the first increment in price happens, the 2nd nft for the protocol has been minted.
• Lets move forward to the end of generation 1 when _tokenIds & generationMintCounts has reached 9999.
• The maximum nft that can be minted in a generation is 10000 defined by maxTokensPerGen.
• The price that a user pays to mint the 10,000th nft is 0.005 + (0.0000245 * 9999) = 0.2499755 ether.

File: TraitForgeNft.sol

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;
    ...

• Since generationMintCounts has not reached maxTokensPerGen, the _incrementGeneration() check is skipped. _tokenIds & generationMintCounts is updated to 10000.

Now comes the interesting part.

• The next nft that is to be minted should come in generation 2 because 10000 nfts have already been minted at this point.
• Also the price for the user to pay to mint the 1st nft of generation 2 should be the startPrice plus the priceIncrementByGen that is 0.005 + 0.000005 = 0.005005 ether.
• However, when the user calls mintToken(), the price he has to pay is 0.005 + (0.0000245 * 10000) = 0.25 ether (As mentioned in whitepaper).
• This states that to mint the 1st nft in generation 2, user has to pay the maximum price of generation 1.
• When the call enters _mintInternal(), due to the first check, the generation is incremented from generation 1 to generation 2. _tokenIds is updated to 10001 & generationMintCounts to 1.
• the price the user should pay be 0.005005 ether but instead ends up paying 0.25 ether. User's loss = 0.244995 ether.

2nd Bug Scenario:

• Suppose we are at the end of generation 2. The generationMintCounts is 10000 & no minting has taken place through forging for generation 2. Meaning that nft count for generation 3 is zero.
• If minting happens directly by calling the mintToken(), the same situation is repeated as explained in the scenario above & the user ends up paying the max price of generation 2 to mint the 1st nft of generation 3.

Now if the generationMintCounts is at 9999 & _mintNewEntity() is called internally through forging, a different scenario takes place.

File: TraitForgeNft.sol

  function _mintNewEntity(
    address newOwner,
    uint256 entropy,
    uint256 gen
  ) private returns (uint256) {
    require(
      generationMintCounts[gen] < maxTokensPerGen,
      'Exceeds maxTokensPerGen'
    );

>   _tokenIds++;
    uint256 newTokenId = _tokenIds;
    _mint(newOwner, newTokenId);

    tokenCreationTimestamps[newTokenId] = block.timestamp;
    tokenEntropy[newTokenId] = entropy;
    tokenGenerations[newTokenId] = gen;
>   generationMintCounts[gen]++;
    initialOwners[newTokenId] = newOwner;

    if (
>     generationMintCounts[gen] >= maxTokensPerGen && gen == currentGeneration
    ) {
      _incrementGeneration();
    }
    ...

• Since minting is happening through forging, the mint price is not calculated. _tokenIds becomes 20000 & generationMintCounts becomes 10000.
• Immediately the call enters the if condition & the generation is incremented.
• generationMintCounts for generation 3 is reset to 0.
• Now if a user calls mintToken(), the price calculated will be 0.005 + (0.0000345 * 0) = 0.005 ether which the initial starting price of the 1st nft of generation 1. [priceIncrement for gen 3 would be = 0.0000245 + 0.000005 + 0.000005 = 0.0000345]
• This scenario would lead to a loss to the protocol & benefit for the user.

Tools Used

Manual Review

Recommended Mitigation Steps

Fixing the bugs would require changes in the calculateMintPrice() & _mintInternal(). However the protocol will not be able to claim the the last calculated price as intended by the whitepaper but it would then align with the number of nfts minted & the price the user pays.

File: TraitForgeNft.sol

  function calculateMintPrice() public view returns (uint256) {
+  if (generationMintCounts[currentGeneration] > 0) {
      uint256 currentGenMintCount = generationMintCounts[currentGeneration];
      uint256 priceIncrease = priceIncrement * currentGenMintCount;
      uint256 price = startPrice + priceIncrease;
      return price;
   }
+  else {
+     uint256 price = startPrice + ((currentGeneration - 1) * priceIncrementByGen );
+     return price;
   }
  }

File: TraitForgeNft.sol
 
  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 (generationMintCounts[currentGeneration] >= maxTokensPerGen) {
+     _incrementGeneration();
+   }
    ...

Assessed type

Error

[Shubh0412]

@koolexcrypto

The above report covers two different scenarios.
The first issue is indeed covered in the issue #210 of which this is a duplicate.

The second issue is explained in the 2nd Bug Scenario: part of the report

Wrong positioning of _incrementGeneration() will lead to the user paying the starting price of 1st nft of generation 1 even when the nft being minted is the 1st nft of a different generation.

I clubbed both these issue since they had the same origin point (minting of first nft of a different generation).

A request to consider this report to be 'selected for report' for covering both possibilities of the vulnerability & detailed explanation.

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as selected for report