Imprecise token age calculation results in an incorrect nuke factor, causing users to claim the wrong amount

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-traitforge/blob/main/contracts/NukeFund/NukeFund.sol#L118-L133

Vulnerability details

Impact

The function calculateAge() computes the age of a token ID based on the current and creation time difference. This token age is then used to determine the nuke factor to calculate the possible claim amount.

The problem here is that when calculating the age of a token, it first divides the time difference to 1 day in seconds, and then multiplies the result to the other variables.

  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;
  }

With having this pattern, users will face significant losses. Especially, in the case of small differences (less than 1 day), the daysOld variable becomes 0, making the age zero respectively.

Proof of Concept

The function below illustrates the significant discrepancies between the results:

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

import "forge-std/Test.sol";

contract DifferenceTest is Test {

    uint256 public constant MAX_DENOMINATOR = 100000;
    error InvalidTokenAmount();

    function setUp() public {

    }

    function calculateAge_precision(
        uint256 creationTimestamp,
        uint256 startTime,
        uint256 ageMultiplier
    )  public view returns (uint256) {

        uint256 perfomanceFactor = 150000;

        uint256 age = ((startTime - creationTimestamp) *
        perfomanceFactor *
        MAX_DENOMINATOR *
        ageMultiplier) / (60 * 60 * 24 * 365);
        return age;
    }

    function calculateAge_actual(
        uint256 creationTimestamp,
        uint256 startTime,
        uint256 ageMultiplier
    )  public view returns (uint256) {

        uint256 perfomanceFactor = 150000;

        uint256 daysOld = (startTime -
        creationTimestamp) /
        60 /
        60 /
        24;

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

    function test_diffAges(
        uint256 creationTimestamp,
        uint256 startTime,
        uint256 ageMultiplier
    )  public {

        vm.assume(startTime > creationTimestamp);
        vm.assume(ageMultiplier < MAX_DENOMINATOR);
        uint actualAge = calculateAge_actual(creationTimestamp, startTime, ageMultiplier);
        uint accurateAge = calculateAge_precision(creationTimestamp, startTime, ageMultiplier);
        console.log("The actual age is:   ", actualAge);
        console.log("The accurate age is: ", accurateAge);
        assertFalse(actualAge == accurateAge);
    }
}

For these results we will have:

(creationTimestamp = 1722511200,

startTime = 1722799000,

ageMultiplier = 150000,

perfomanceFactor = 15620):

[PASS] test_diffAges() (gas: 8201)
Logs:
  The actual age is:    1925753424657
  The accurate age is:  2138240106544

Traces:
  [8201] DifferenceTest::test_diffAges()
    ├─ [0] console::log("The actual age is:   ", 1925753424657 [1.925e12]) [staticcall]
    │   └─ ← [Stop]
    ├─ [0] console::log("The accurate age is: ", 2138240106544 [2.138e12]) [staticcall]
    │   └─ ← [Stop]
    ├─ [0] VM::assertFalse(false) [staticcall]
    │   └─ ← [Return]
    └─ ← [Stop]

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 520.80µs (150.20µs CPU time)

The calculated age is: 1925753424657 The accurate age is : 2138240106544

This will lead to ~ 11% of error which is a significant error here. This huge difference will also increase the nuke factor (even more) and affect the results.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider multiplying the numerator variables first and then divide by the denominator:

  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 *
+    uint256 age = (perfomanceFactor *
      MAX_DENOMINATOR *
+      (block.timestamp - nftContract.getTokenCreationTimestamp(tokenId)) *
      ageMultiplier) / (60 * 60 * 24 * 365); // add 5 digits for decimals
    return age;
  }

Assessed type

Math

[TForge1]

age multiplier is marked as an invalid value, and should not have been used as I marked several times in the public channel (I forgot to remove from codebase which is my fault). so the default for age multiplier should be 0.

the difference of nuke factor % between each day is 0.006% which is not much 2.5% / 365. although this is still a valid finding as it could be more accurate, I personally would mark as low risk.

[koolexcrypto]

Thank you for the input.

The issue as demonstrated is valid with a Med risk. For fairness, since it is a part of the actual code, I will keep it as it is (Med). If there is something that I may have missed here, that may change the risk level, please let me know. will consider inputs from all parties in PJQA as usual.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[AtanasDimulski]

Hey @koolexcrypto thanks for the swift judging! I believe this issue is invalid, the sponsor was asked several times about the ageMultiplier value in the public chat as this value can't be found anywhere in the tests or deployments scripts, and he was adamant that this value won't be used. From the code implementation, it is clear that the age multiplier should increase only once whole days are passed not seconds or minutes. I believe this issue is invalid, or QA at best. Given the fact that the sponsor has mentioned that ageMultiplier won't be used multiple times in the public channel it will be extremely unfair to other wardens to validate this issue. Thanks for your time!

[Kalogerone]

I agree with the comment above and want to add some additional info as to why this issue seems like an invalid, at best QA. Quoting from the issue report:

With having this pattern, users will face significant losses. Especially, in the case of small differences (less than 1 day), the daysOld variable becomes 0, making the age zero respectively.

The daysOld variable is how many days old an NFT is. I don't understand why it shouldn't be 0 if an NFT is minted less than 24 hours ago. It's not an hoursOld variable. What matters to the protocol is how many full days old an NFT is. There is no precision loss here and the variable works as intended.

Also the issue above in the PoC is setting the ageMultiplier variable to something different that 1 to make the percentage of the error seem big (ageMultiplier = 150000). With the correct ageMultiplier = 1, the calculation of the daysOld variable is at most 23 hours and 59 minutes behind. Which is not really behind because, again, the variable is supposed to calculate the full days of the NFT's existence. The sponsors have mentioned multiple times that the ageMultiplier should be set to 1 for testing. @TForge1's first comment on this issue mistakenly mentions:

so the default for age multiplier should be 0.

This is 100% a mistake on his part as he has said so many times that it should be set to 1 for testing or completely deleted from the code. I believe this is the reason he commented with the value 0 instead of 1 in his rush.

[udsene]

Hey @koolexcrypto,

Can you look into the #317 which is a duplicate of this issue. Even after considering the ageMultiplier to be 1 still there is a considerable rounding error which could affect both the claim amount of the nuke transaction and also could lead to erroneous evaluation of the finalNukeFactor > nukeFactorMaxParam conditional check in the NukeFund.nuke function. This is because the above error of 1 day is multiplied by both the perfomanceFactor and defaultNukeFactorIncrease when the finalNukeFactor is calculated as explained in #317.

Thank You

[koolexcrypto]

Thank you everyone for your input.

After further review, I will keep it as it is (Med), since it is a part of the actual code which I believe it should take priority over the public chat. I understand that this might not be favourable to some of you, but I have to do this, to keep the game fair.

Might change the selected report though.

[koolexcrypto]

The selected report stands as is