howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L116

Vulnerability details

Impact

In the current implementation of TraitForgeNft contract, there is a function called setMaxGeneration() that allows the owner to set the maximum generation. It checks whether the new max generation is greater or equal to the current generation. The problem is that the new max generation can be bypassed by forging if it's set to the current generation.

Proof of Concept

Take a look at the setMaxGeneration():

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L114-120

 function setMaxGeneration(uint maxGeneration_) external onlyOwner {
    require(
      maxGeneration_ >= currentGeneration,
      "can't below than current generation"
    );
    maxGeneration = maxGeneration_;
  }

The crucial part here is that the function checks whether the new max generation is greater or equal to the current generation. However, if there are tokens that were already created by forging - they would simply be in a disallowed generation:

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/TraitForgeNft/TraitForgeNft.sol#L166

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

forge() correctly checks for the new max generation but the problem is that there can already be forged tokens in the new generation from the current generation. Even if there are not, users can just front-run setting setMaxGeneration() and create tokenIds in a new generation. This would put protocol in an unexpected state where there are existing entities that are effectively above the maximum allowed generation.

Tools Used

Manual review.

Recommended Mitigation Steps

Reconsider the logic regarding forging and when it's allowed. Or the simplest solution is just to remove setMaxGeneration() so the maximum generation would be hardcoded.

Assessed type

Other

c4-judge commented 2 months ago

koolexcrypto changed the severity to QA (Quality Assurance)