Due to forging, it is possible a generation will not contain a 'Golden God' token

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L293 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L328 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L288 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/EntropyGenerator/EntropyGenerator.sol#L101-L120 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/TraitForgeNft/TraitForgeNft.sol#L176

Vulnerability details

Bug Description

As confirmed by the docs and the protocol sponsor, there should be 1 'Golden God' entropy/token per generation. However this is unlikely to be the case due to forging.

After the generation has incremented, forges to the current generation and 'standard' mints both contribute towards the total tokens minted in the current generation. This is because TraitForgeNft::_mintInternal() and TraitForgeNft::_mintNewEntity() both increment the generationMintCounts mapping variable.

However, TraitForgeNft::_mintInternal() fetches the entropy from the newly minted token via the EntropyGenerator::getNextEntropy() call and TraitForgeNft::_mintNewEntity() recieves the entropy of the newly minted token as calculated in the 'TraitForgeNft::forge()' function.

The important difference between the entropy retrieval between the above two types of minting is _mintInternal() increments the EntropyGenerator::currentSlotIndex and EntropyGenerator::currentNumberIndex state variables whilst _mintNewEntity() does not. The result is the 'forges' contribute to exceeding the generationMintCounts, whilst not incrementing the entropy indicies, so it is possible a whole generation gets minted without reaching the 'Golden God' entropy index.

Imagine the following scenario which will also be demonstrated below with a POC:

• Generation 1 is minted out with 'standard' mints
• The next mint increments the TraitForgeNft::currentGeneration() to 2
• The call to entropyGenerator.initializeAlphaIndices() initializes the indicies such that the 'Golden God' will be minted at slot 769, index 3.
• 30 tokens from gen 1 are now forged to gen 2 (importantly this occurs after the generation has incremented to 2). This results in 15 tokens minted in gen 2 via the TraitForgeNft::_mintNewEntity() method.
• After a total of 986 gen 1 tokens are minted via TraitForgeNft::_mintInternal(), the generation increments to 3.
• There is no 'Golden God' in generation 2.

Impact

• It is possible that generations contains no 'Golden God'
• When this occurs, it is likely that the 'Golden God' token will occur earlier than expected in the following generation (earlier than the first 66% of mints).

Note this has the same impact as another issue I submitted, however both issues have distinct root causes.

Proof of Concept

The test test_POC_NoGoldenGodEntropyDueToForging() does the following:

• User1 mints 5000x gen 1 tokens
• User2 mints 5000 gen 1 tokens and 1x gen 2 tokens
• User2 lists 40x of their gen 1 forgers for forging
• User1 matches the above 40x forgers with mergers, this mints 40 tokens in gen 2
• User1 mints out the remainder of gen 2
• No 'Golden God' exists in gen 2

Please note this is a foundry test and the repo must be setup as a foundry project before running.

Before running the tests, make the below changes to the EntropyGenerator contract (to allow generation increments):

  modifier onlyAllowedCaller() {
-   require(msg.sender == allowedCaller, 'Caller is not allowed');
+   require(msg.sender == allowedCaller || msg.sender == owner(), 'Caller is not allowed');
    _;
  }

- function initializeAlphaIndices() public whenNotPaused onlyOwner {
+ function initializeAlphaIndices() public whenNotPaused onlyAllowedCaller {
    uint256 hashValue = uint256(
      keccak256(abi.encodePacked(blockhash(block.number - 1), block.timestamp))
    );

    uint256 slotIndexSelection = (hashValue % 258) + 512;
    uint256 numberIndexSelection = hashValue % 13;

    slotIndexSelectionPoint = slotIndexSelection;
    numberIndexSelectionPoint = numberIndexSelection;
  }

Paste the below code into a new file in the test folder.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "../../lib/forge-std/src/Test.sol";

import { EntropyGenerator} from '../contracts/EntropyGenerator/EntropyGenerator.sol';
import { Airdrop } from '../contracts/Airdrop/Airdrop.sol';
import { TraitForgeNft } from '../contracts/TraitForgeNft/TraitForgeNft.sol';
import { EntityForging } from '../contracts/EntityForging/EntityForging.sol';
import { DevFund } from '../contracts/DevFund/DevFund.sol';
import { EntityTrading } from '../contracts/EntityTrading/EntityTrading.sol';
import { NukeFund } from '../contracts/NukeFund/NukeFund.sol';

contract TraitForgeNftTest is Test {
    address public owner = makeAddr("owner");
    address public user1 = makeAddr("user1");
    address public user2 = makeAddr("user2");

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

    function setUp() public {
        vm.roll(42);

        vm.startPrank(owner);

        traitForgeNft = new TraitForgeNft();
        airdrop = new Airdrop();
        entropyGenerator = new EntropyGenerator(address(traitForgeNft));
        entityForging = new EntityForging(address(traitForgeNft));

        // Set contracts on the NFT contract
        traitForgeNft.setEntityForgingContract(address(entityForging));
        traitForgeNft.setEntropyGenerator(address(entropyGenerator));
        traitForgeNft.setAirdropContract(address(airdrop));

        // Transfer ownership of the airdrop contract to the NFT contract
        airdrop.transferOwnership(address(traitForgeNft));

        // Write the entropy in 3 batches
        entropyGenerator.writeEntropyBatch1();
        entropyGenerator.writeEntropyBatch2();
        entropyGenerator.writeEntropyBatch3();

        //devFund = new DevFund();

        vm.stopPrank();
    }

    function test_POC_NoGoldenGodEntropyDueToForging() public { 

        vm.warp(block.timestamp + 24 hours + 1); // Fast forward 24 hours to bypass the mint whitelist
        bytes32[] memory _whiteListProof = new bytes32[](1); // dummy calldata, not required because the call is after the whitelist timestamp

        // user 1 mints 5000x gen 1 tokens
        vm.deal(user1, 5000 ether);
        vm.startPrank(user1);
        for (uint256 i = 0; i < 5000; i++) {
            traitForgeNft.mintToken{value: 1 ether}(_whiteListProof);
        }

        // user 2 mints 5000x gen 1 tokens and 1x gen 2 token
        vm.deal(user2, 5000 ether);
        vm.stopPrank();
        vm.startPrank(user2);
        for (uint256 i = 0; i < 5000; i++) {
            traitForgeNft.mintToken{value: 1 ether}(_whiteListProof);
        }

        // set seed for gen 2 alpha indicies
        vm.roll(1044); // note this is not highly constrained due to the potential of thousands of forges

        // mint a token triggering a generation increment
        traitForgeNft.mintToken{value: 1 ether}(_whiteListProof);

        // Check alpha indicies in gen 2
        assertEq(traitForgeNft.currentGeneration(), 2);

        // The alpha entropy is at slot 768, number 0. So to demonstrate this issue we need to forge > 10 + 2*13 ~ 36 tokens in gen 2.

        // User 2 lists 40 tokens for forging
        uint256 numListed = 0;
        uint256 tokenId = 5000;
        uint256 forgerFee = 0.01 ether;
        uint256[] memory listedTokenIds = new uint256[](40);

        while (numListed < 40) {
            if (traitForgeNft.isForger(tokenId) && (uint8((traitForgeNft.tokenEntropy(tokenId) / 10) % 10)) > 0) {
                entityForging.listForForging(tokenId, forgerFee);

                listedTokenIds[numListed] = tokenId;
                numListed++;
            }
            tokenId++;
        }

        // User 1 matches the 40 listed tokens
        vm.stopPrank();
        vm.startPrank(user1);

        uint256 numMatched = 0;
        tokenId = 0;

        while (numMatched < 40) {
            if (!traitForgeNft.isForger(tokenId) && (uint8((traitForgeNft.tokenEntropy(tokenId) / 10) % 10)) > 0) {
                entityForging.forgeWithListed{value: forgerFee}(listedTokenIds[numMatched], tokenId);

                numMatched++;
            }
            tokenId++;
        }

        assertEq(traitForgeNft.generationMintCounts(2), 41); // 41 tokens minted in gen 2

        // User 1 mints out the remainder of gen 2
        for (uint256 j = 0; j < 10000 - 41; j++) {
            traitForgeNft.mintToken{value: 1 ether}(_whiteListProof);
        }

        assertEq(traitForgeNft.generationMintCounts(2), 10000); // Gen 2 minted out

        // None of the 10,000 tokens in gen 2 are a 'Golden God'
        for (uint256 z = 10000; z < 2*10000; z++) { // in this case, we minted the tokens in order, so the tokenIds 10000 -> 19999 are all gen 2
            assert(traitForgeNft.tokenEntropy(z) != 999999);
        }
    }

}

Tools Used

Manual Review

Recommended Mitigation Steps

One solution is to increment the EntropyGenerator::currentSlotIndex and EntropyGeneratior::currentNumberIndex parameters when a token is minted due to forging. In addition, if the forged token is on the current generations's 'alpha indicies' (slotIndexSelectionPoint, numberIndexSelectionPoint) set the entropy for this token to 999,999.

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as duplicate of #656

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)