EntityForging::forge() is too restrictive, resulting in users receiving less rewards than they should

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/EntityForging/EntityForging.sol#L102-L175

Vulnerability details

Impact

The TraitForge protocol allows users to forge two NFTs of the same generation in order to create a new NFT from the next generation by calling the EntityForging::forgeWithListed() function. For each NFT that is used for forging there are two mappings that tract the amount of times this NFT was used for forging mapping(uint256 => uint8) public forgingCounts; and how much time has passed since that NFT was used for forging for the first time mapping(uint256 => uint256) private lastForgeResetTimestamp; . The _resetForgingCountIfNeeded() function resets the mapping(uint256 => uint8) public forgingCounts;, if more than a year has passed since the NFT was used for forging for the first time. The EntityForging::forgeWithListed() function internaly calls the TraitForgeNft::forge() function:

  function forge(address newOwner, uint256 parent1Id, uint256 parent2Id, string memory) external whenNotPaused nonReentrant returns (uint256) {
    require(msg.sender == address(entityForgingContract), 'unauthorized caller');
    uint256 newGeneration = getTokenGeneration(parent1Id) + 1;

    /// Check new generation is not over maxGeneration
    require(newGeneration <= maxGeneration, "can't be over max generation");

    // Calculate the new entity's entropy
    (uint256 forgerEntropy, uint256 mergerEntropy) = getEntropiesForTokens(parent1Id, parent2Id);
    uint256 newEntropy = (forgerEntropy + mergerEntropy) / 2;

    // Mint the new entity
    uint256 newTokenId = _mintNewEntity(newOwner, newEntropy, newGeneration);

    return newTokenId;
  }

As can be seen from the above code snippet, the generation of the NFT a user will receive after forging is the generation of the NFTs supplied as params to the EntityForging::forgeWithListed() function (which have to be equal) + 1. Thus if both the parents are of generation 1, the forged NFT can only be of generation 2. Additionally there is a check that the new generation can't exceed the maxGeneration which is set to 10

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

An NFT can have two roles a forger and a merger, a forger can list his NFT via the listForForging() function and receive fees when someone merges with his NFT. Each NFT has a forgingPotential which is calculated in the following way:

uint8 forgePotential = uint8((entropy / 10) % 10);

The merger has to pay a fee to the forger in order to forge with his NFT and receive a new NFT. The ability of an NFT to be used for forging, is an important factor in determining the price of the NFT itself, as well as how much a user can profit by listing his NFT for forging, or merging with his NFT in order to receive a new NFT. There are several highly likely scenarios where users will receive much less rewards than they are supposed to.

• The most obviously one is that the two NFTs used for merging have to be of the same generation, there are supposed to be 10_000 NFTs in each generation, and the NFT that the merger will receive is of the next generation. Consider the following scenario. The game becomes so popular that when minting starts, users mint 20_000 NFTs, essentially minting generation 1 and 2. Now all the users that have an NFT of generation 1 can't use it for forging, due to the following check in the _mintNewEntity() function:

  require(generationMintCounts[gen] < maxTokensPerGen, 'Exceeds maxTokensPerGen');

  There will already be 10_000 NFTs in the generationMintCounts[gen] mapping for generation 2, and the forge function will revert. This severely restricts the profitability of gen 1 NFT owners. Given that forging is one of the main ways to profit from the game.

• As previously mentioned there are two mapping that track how many times an NFT was used for forging mapping(uint256 => uint8) public forgingCounts; and when mapping(uint256 => uint256) private lastForgeResetTimestamp; . The _resetForgingCountIfNeeded() function resets the mapping(uint256 => uint8) public forgingCounts;. The forgingPotential of each NFT determines how many times an NFT can be used for forging. The _resetForgingCountIfNeeded() function is supposed to reset the forgingPotential after a year has passed. Consider the following scenario. A user has an NFT of generation 3 with forgingPotential of 3, and he uses it three times to mint NFTs of generation 4. It is highly likely that after a year the current generation of NFTs will be much higher than 4, otherwise most likely nobody will be playing the game. For reasons described previously NFTs of generation 3 can't forge NFTs of generation 5 or higher, and if there are already 10_000 NFTs in generation 4, the forging will revert. This highly restricts the profitability of a given NFT. Plus if the protocol claims that the forge count of NFTs will be reset after one year, those NFTs will be sold for a bigger price, as the potential buyers will think they can use them to forge new NFTs after a certain period of time. This is effectively stealing money from the NFT buyers.
• When there are 10_000 NFTs in a generation, when the next NFT is minted or forged, the _incrementGeneration() function will increase the current generation. However there is no check whether that generation is bigger than the maxGeneration. And in the TraitForgeNft::forge() function we have the following check:

  require(newGeneration <= maxGeneration, "can't be over max generation")

  Consider the following scenario, users mint most of generation 10 NFTs. Users that try to forge with NFTs from generation 10, won't be able to due to the above mentioned check. However users can still mint new NFTs, and if all of the NFTs in generation 10 are minted, the generation will be increased, and users will be able to mint NFTs from generation 11, however they still won't be able to forge with NFTs from generation 10. If all of the NFTs from generation 11 are minted, the NFTs from generation 10 will never be able to forge, thus severely decreasing the profitability of each user that owns an NFT of generation 10.

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Consider making the forging process less restrictive, for example modify the EntityForging::forgeWithListed() function to allow users to mint NFTs for each next generation, not only for the parent's next generation. Don't allow the _incrementGeneration() function to increase the currentGeneration to a number greater than the maxGeneration.

Assessed type

Context

[c4-judge]

koolexcrypto marked the issue as satisfactory

[AtanasDimulski]

Hey @koolexcrypto thanks for the swift judging! This issue has nothing to do with the vulnerability described in #211, this issue describes how users will actually forge fewer NFTs than they are supposed to, not more. This issue clearly describes why forging won't work as expected which breaks core protocol functionality and results in users forging fewer NFTs than they are supposed to, which is a direct loss of funds. This issue describes two broken core protocol invariants. Please have another look. Thank you for your time!

[koolexcrypto]

Could you please summarize the issue in one or two paragraphs at max?

Please include root cause, impact and the two broken core protocol invariants that are never clarified in your comment nor in the issue or even README page of the contest.

[AtanasDimulski]

Hey @koolexcrypto, The first invariant is that each NFT based on entropy has a forge potential - meaning the times it can be used for merging, although this is not specified as an invariant in the README, it is pretty logical that if an NFT has a forge potential of 3, it should be able to forge 3 times. The different scenarios of why this won't work as expected are described in the first part of the report. The main root cause is that NFTs that merge have to be of the same generation and can mint an NFT only of the next generation which should be capped at 10_000. The second functionality that is broken is that each year that passes the forging of the NFTs should be reset, so if an NFT with a forging potential of 3, already merged 3 times, and a year has passed since the last merging the NFT owner is supposed to be able to merge again. If the NFT is of generation 1 but after a year the current generation is 6, the NFT of generation 1 won't be able to forge. Again this is not a specified invariant in the README, however, it is a pretty logical one, why else implement that functionality? I believe the report is pretty clear and provides different highly possible scenarios. Hopefully, this comment cleared up what is meant by invariants in the report. Thank you for your time!

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[koolexcrypto]

Thank you, just an advice . Please split the texts into paragraphs. Otherwise, it is not readable.

[koolexcrypto]

It is clear now that this is not a dup of #211 . However, I still don't see what is the issue here. having an NFT from generation 1 and that couldn't be forged with another token due to the diff in generation is clearly a game design.

it is pretty logical that if an NFT has a forge potential of 3, it should be able to forge 3 times

Inaccurate, 3 forge potential are 3 forge potential , it doesn't mean you can forge. This depends on many factors including the generation and the exhausted forge count ...etc. It is not an absolute and can not hold always, that's why naming it as invariant is incorrect.

The second functionality that is broken is that each year that passes the forging of the NFTs should be reset, so if an NFT with a forging potential of 3, already merged 3 times, and a year has passed since the last merging the NFT owner is supposed to be able to merge again. If the NFT is of generation 1 but after a year the current generation is 6, the NFT of generation 1 won't be able to forge

I understand your concern here. However it is a part of the game design.

Given this, the issue can still be a QA.

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto changed the severity to G (Gas Optimization)

[c4-judge]

koolexcrypto marked the issue as grade-c

[AtanasDimulski]

Hey @koolexcrypto, thanks for the feedback. With all the more than questionable submissions that you have accepted as valid meds and highs, stating that this is not a valid med is not serious. You accepted an issue where the sponsor clearly mentioned a variable won't be used in the code, and the argument you presented was that it is to keep everything fair, how exactly is this fair - the auditors who asked what the variable will be used for got penalized. Secondly, you made up arguments in issue #222 such as

From README

Core Mechanics
Entities: NFTs with unique traits and parameters impacting the game.

Same generation and same entropy goes against this Mechanic. Therefore, I still see it an issue.

Which doesn't state clearly that NFTs can't be with the same entropy, nevertheless you accepted is a valid med. Now in this issue where more than logical functionality of the protocol is broken, you are claiming that this is part of the game design. Inability to fully utilize the potential of your NFT is a game design? Can you please provide me a single sentence where that is stated, and that forging 3 times is only a possibility - it is an absolute, why else would you have forging possibility? For the second part of your argument, I don't know how you came up with that, - a functionality that doesn't work is part of the game design, this is not serious. I understand that judging NFT projects is a complex task and judging standards can vary from protocol to protocol but at least try to keep the same standards for the same contest. Thanks for your time.