Non-aging entities have much more possibility to mint due to padding

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/NukeFund/NukeFund.sol#L126 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/NukeFund/NukeFund.sol#L147-L148 https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/NukeFund/NukeFund.sol#L165-L165

Vulnerability details

Vulnerability Details

Age is a key component in trait forge entities. It's calculated using the entropy value of entity. While calculating the age of NFT, it directly multiply value by performance factor. Performance factor is calculated using the last digit of the entropy and the last digit also can be 0. If it's 0, that means it's non-aging entity.

function calculateAge(uint256 tokenId) public view returns (uint256) {
    require(nftContract.ownerOf(tokenId) != address(0), 'Token does not exist');

    uint256 daysOld = (block.timestamp -
      nftContract.getTokenCreationTimestamp(tokenId)) /
      60 /
      60 /
      24;
    uint256 perfomanceFactor = nftContract.getTokenEntropy(tokenId) % 10;

    uint256 age = (daysOld *
      perfomanceFactor *
      MAX_DENOMINATOR *
      ageMultiplier) / 365; // add 5 digits for decimals
    return age;
  }

The probability of getting non-aging NFT is 20% (It's higher than all the other performance factor values). The reason of this is the padding operation of entropy calculation.

Calculation: First situation: Last digit of entropy is 0: There can be 10 possible number for last digit [0,9] which makes 10%

Second situation: If the cutted slot position starts with 0 it will end with 0: In entropy calculation, using slotIndex and numberIndex it cuts 6 digit from random slot and if this number doesn't have 6 digits, it pads the value with 0. (eg: cutted value is 12345, it will be converted to 123450 ), So it makes another 10% possibility and in conclusion it makes 20% of getting non-aging NFT.

Padding code:

uint256 slotValue = entropySlots[slotIndex]; // slice the required [art of the entropy value
    uint256 entropy = (slotValue / (10 ** (72 - position))) % 1000000; // adjust the entropy value based on the number of digits
    uint256 paddedEntropy = entropy * (10 ** (6 - numberOfDigits(entropy)));

    return paddedEntropy; // return the caculated entropy value
  }

According to the documents and sponsor, this possibility should be equal to other possibilities:

https://ibb.co/xCf6fMq (image link from imgbb.com)

Impact

Wrong main functionality

Proof of Concept

You can check the approximation using this test

function setUp() public {
        // deploy contracts

        vm.startPrank(Owner);

        // deploy contracts
        traitForgeNft = new TraitForgeNft();
        traitForgeNft.setWhitelistEndTime(0); // ignore whitelist
        airdrop = new Airdrop();
        nukeFund = new NukeFund(address(traitForgeNft), address(airdrop), payable(dev1), payable(dev2));
        entityForging = new EntityForging(address(traitForgeNft));
        entropyGenerator = new EntropyGenerator(address(traitForgeNft));

        // set traitForge variables
        traitForgeNft.setEntityForgingContract(payable(address(entityForging)));
        traitForgeNft.setEntropyGenerator(address(entropyGenerator));
        traitForgeNft.setNukeFundContract(payable(address(nukeFund)));
        traitForgeNft.setAirdropContract(address(airdrop));

        // set entityForging variables
        entityForging.setNukeFundAddress(payable(address(nukeFund)));

        // ingore ageMultiplier
        nukeFund.setAgeMultplier(1);

        // transfer ownership
        airdrop.transferOwnership(address(traitForgeNft));

        // call batches
        entropyGenerator.writeEntropyBatch1();
        entropyGenerator.writeEntropyBatch2();
        entropyGenerator.writeEntropyBatch3();

        vm.stopPrank();

        vm.deal(Alice, 1000 ether);
        vm.deal(Bob, 1000 ether);

    }
function test_createNonAgingNFTS() public {
        vm.startPrank(Alice);
        uint256 nonAgingNFTS = 0;
        for ( uint256 i = 1; i < 1001; i++) {
            traitForgeNft.mintToken{value: 1 ether}(proof);
        }

        vm.warp(block.timestamp + 365 days);
        for ( uint256 i = 1; i < 1001; i++) {
            if (nukeFund.calculateAge(i) == 0) {
                nonAgingNFTS++;
            }
        }

        console.log("1000 NFT minted, %s of them are non-aging", nonAgingNFTS);
        vm.stopPrank();
    }

Logs:
  1000 NFT minted, 265 of them are non-aging

Tools Used

Foundry and manual review

Recommended Mitigation Steps

There can be many solution to this problem and my suggestion is if entropy is derived from padding adding the last digit of not-padded entropy value to padded version will keep the probability at 10% level

Assessed type

Math

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[DemoreXTess]

I think there is a misunderstanding regarding my submission. It's not duplicate of #224. This issue highlights the potential differences in entropy values, while #224 asserts that performanceFactor cannot be zero. Let me explain again with more details.

Note: Sponsor has also confirmed (in Discord) that this is a valid issue in the game mechanics.

Based on the Traitforge documentation, the entropy value should be a random number within the range of [100,000 to 999,999]. In the current system, entropy values are derived from slots, which involves extracting 6 digits from a random position within a 78-digit number. Due to this extraction process, the resulting 6-digit number may sometimes be smaller if the extracted digits start with zeros.

The system addresses this issue by padding the number with leading zeros until it reaches a 6-digit length. Consequently, obtaining an NFT with an entropy value of 100,000 has a significantly higher probability compared to obtaining one with an entropy value of 123,456. This discrepancy impacts the game mechanics, as entropy is a crucial component for NFTs.

Comparing the probabilities of generating NFTs with entropy values of 100,000 and 123,456:

100,000:

1) The slot can directly yield 100,000. 2) 10,000 -> 100,000 (padding) 3) 1,000 -> 100,000 (padding) 4) 100 -> 100,000 (padding) 5) 10 -> 100,000 (padding) 6) 1 -> 100,000 (padding)

There are 6 ways to generate an entropy value of 100,000.

123,456:

1) The slot can directly yield 123,456.

There is only 1 way to generate an entropy value of 123,456.

Thus, obtaining an NFT with an entropy value of 100,000 is 6 times more likely than obtaining one with 123,456. This is just one example, but many more exist.

Effects on the Game:

In my original submission, I mentioned that non-aging NFTs have a higher probability of being generated compared to aging NFTs. This issue extends further. Consider this example:

Entropy: 123,456 Entropy[0] = PhysicalTrait1 Entropy[1] = PhysicalTrait2 Entropy[2] = PhysicalTrait3 Entropy[3] = PhysicalTrait4 Entropy[4] = Colour1 & ForgePotential Entropy[5] = Colour2 Entropy / 40 = InitialNukeFactor Entropy % 10 = PerformanceFactor Entropy % 3 ? 0 == Role

Forge potential is another key component for entities, and NFTs with a forge potential of 0 have a higher likelihood of being minted than others.

Probability Calculation:

There are two possible ways to obtain a non-forgeable NFT:

Normal method (no padding): 10% Padding method: If the extracted value from the slot has fewer than 5 digits (e.g., 1234), it will be non-forgeable. Calculation: 10% for the first digit, then 10% of that 10% for the second digit = 11% total probability of a non-forgeable NFT ( it should be 10% for fair random distribution ). For non-aging NFTs, this probability is 20%, as previously mentioned ( it should be 10% for fair random distribution ).

I believe this padding algorithm should be reconsidered for non-6-digit slots, as it causes significant differences in probabilities.

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as nullified

[c4-judge]

koolexcrypto marked the issue as not nullified

[koolexcrypto]

This is a valid issue.

To determine the severity, could you please refer where it is mentioned in the docs that the probability should be 1/10 or 10% ?

[DemoreXTess]

@koolexcrypto

The documentation states that the entropy value should be a random number between 100,000 and 999,999. If we focus on the performance factor for NFTs (the last digit of the entropy), there are 10 possible values: 0, 1, 2, 3, 4, 5, 6, 7, 8, and 9. Since 0 is just one of these 10 possible values, it has a probability of 1/10, which equals 10%.

[koolexcrypto]

Thank you for your input.

However, given that the impact is

Wrong main functionality

with no real harm demonstrated. I believe, this can be a QA as I don't see a Med risk here.

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[DemoreXTess]

@koolexcrypto

The game mechanics are significantly impacted by poor randomness. As a result, most NFTs will have a lower performance factor. I provided a proof of concept in my original submission. The performance factor greatly affects various aspects of the game, such as the nuking functionality. Due to the flawed randomness, most users will have non-aging entities, which will lead to their nuking rewards being significantly lower than expected.

[koolexcrypto]

Taking into account the sponsor's input

Valid but not detrimental

stands as is