Incorrect mint price is applied if there is a generation increment

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L190
https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L280-L294
https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L350-L352

Vulnerability details

Impact

This is an edge case that happens every time there is a generation increment when minting a token. As we can see in the mintToken() function in TraitForgeNft.sol there is a mint price calculation after which the calculated mintPrice is passed to the _mintInternal() function.
In the _mintInternal() function we first check if there is a need to increment generation, before minting the token and setting all the storage variables (e.g., tokenGenerations[newItemId] = currentGeneration;).
The problem arises when it is time to increment generation (_incrementGeneration()) where the current generation is incremented by one, generation mint count is set to 0 and price increment is also recalculated, but there is no update to mintPrice.

This means two things:

1. The protocol incurs a loss as the user does not pay the correct price for the intended generation. Currently, this loss is 0.05 ETH, which is the price difference between two generations. Although this may not seem significant initially, if the protocol adjusts prices in the future, the loss could be greater. Additionally, since this occurs with every generation change, the loss will multiply by the number of generations.
2. The user is able to acquire a more valuable NFT for the price of a less valuable one, giving them an immediate advantage and the opportunity to sell it at a higher price.

Proof of Concept

Imagine following scenario:

1. We have currently minted 10,000 NFTs of Gen1
2. User A is calling mintToken to mint another token
3. The mint price is calculated by using properties from the Gen1 since the currentGeneration is still 1
4. We check msg.value and it is equal to the calculated mint price. All good so far.
5. In the _mintInternal() function, we see that generationMintCounts[currentGeneration] >= maxTokensPerGen is satisfied since 10_000 >= 10_000, so we go to _incrementGeneration()
6. In the _incrementGeneration() we do currentGeneration++ setting it to 2. We reset the generationMintCount[currentGeneration] to 0, and we add priceIncrementByGen to the previous priceIncrement. The function is done.
7. The function _mintInternal() continues its execution where we actually mint the token and set following storage variables

    tokenGenerations[newItemId] = currentGeneration; // currentGeneration = 2
    generationMintCounts[currentGeneration]++;
    initialOwners[newItemId] = to;

8. We incremented generation and minted a new token, however after doing that, we forgot to recheck if the msg.value >= newMintPrice. This is necessary because we also increased priceIncrement which is part of the mint price calculation.
9. User got Gen2 NFT but paid for GEN1.

Tools Used

Manual Review, VS Code

Recommended Mitigation Steps

Considering the calculateMintPrice() function, the first NFT of the new generation has to be minted (so that generationMintCounts[currentGeneration] is not 0, which will cause priceIncrement to be 0 as well).
Therefore, after minting a new token and setting all the variables in _mintInternal(), recalculate the price again and check if the msg.value will be enough for the new mint price. It would look like this:

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

    uint256 newMintPrice = calculateMintPrice();
    require(msg.value == newMintPrice, 'Insufficient ETH for the new generation');

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as duplicate of #564