Incorrect `overcount` computation in `_calculateIssuance`

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x33ce87759adbad20a307af9279390af66748881907c774bedacbc84fd6417bbc Severity: high

Description: Description\

The internal function _calculateIssuance which is used to compute the following numbers and returns.

• issuance The issuance in attoCircles
• startPeriod The start of the claimable period.
  • endPeriod The end of the claimable period.

we can see the following line of codes in this function.

// calculate the start of the claimable period
        uint256 startMint = _max(block.timestamp - MAX_CLAIM_DURATION, mintTime.lastMintTime);

        // day of start of mint, dA
        uint256 dA = uint256(day(startMint));

        // day of current block, dB
        uint256 dB = uint256(day(block.timestamp));

        // the difference of days between dB and dA used for the table lookups
        uint256 n = dB - dA;

        // calculate the number of completed hours in day A until `startMint`
        int128 k = Math64x64.fromUInt((startMint - (dA * 1 days + inflationDayZero)) / 1 hours);

        // Calculate the number of incompleted hours remaining in day B from current timestamp
        int128 l = Math64x64.fromUInt(((dB + 1) * 1 days + inflationDayZero - block.timestamp) / 1 hours + 1);

        // calculate the overcounted (demurraged) k (in day A) and l (in day B) hours
        int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), l);

The number of hours completed in dA and incompleted hours in dB are computed and stored in k and l respectively.

based on the k and l value, the overcount value is calculated which will be reduced when returning from this function as issuance amount which will be used when minting.

lets look at this line.

    // calculate the overcounted (demurraged) k (in day A) and l (in day B) hours
    int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), l);

it multiplies the the R table value for n with k and adds to the hours l. This is incorrect.

The actual line of code should look like this. --- Fix

    // calculate the overcounted (demurraged) k (in day A) and l (in day B) hours
    int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), Math64x64.mul(R[n], l));

Impact

Incorrect amount of issuance will be used in the function _mintAndUpdateTotalSupply

    (uint256 issuance, uint256 startPeriod, uint256 endPeriod) = _calculateIssuance(_human);
    if (issuance == 0) {
        // No issuance to claim, simply return without reverting
        return;
    }
    // mint personal Circles to the human
    _mintAndUpdateTotalSupply(_human, toTokenId(_human), issuance, "");

[benjaminbollen]

Thank you for your report on the overcount computation in _calculateIssuance. After review, we've determined this is not an issue.

The current implementation of the overcount terms in the calculateIssuance function is correct and follows the mathematical principles outlined in our specifications. The calculation method, including how the R(n) table values are applied, is intentional and based on thorough mathematical analysis.

For a detailed explanation of the rationale behind these calculations, we encourage reviewing the specification document at https://github.com/hats-finance/Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/blob/rc-v0.3.6-alpha/specifications/TCIP009-demurrage.md#calculating-issuance-in-demurraged-units

We appreciate your careful examination of our mathematical implementations. Your attention to detail in reviewing complex calculations contributes to the robustness of our system. Thank you for your valuable input in this security review.

[0xpinky]

Thank you for your report on the overcount computation in _calculateIssuance. After review, we've determined this is not an issue.

The current implementation of the overcount terms in the calculateIssuance function is correct and follows the mathematical principles outlined in our specifications. The calculation method, including how the R(n) table values are applied, is intentional and based on thorough mathematical analysis.

For a detailed explanation of the rationale behind these calculations, we encourage reviewing the specification document at https://github.com/hats-finance/Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/blob/rc-v0.3.6-alpha/specifications/TCIP009-demurrage.md#calculating-issuance-in-demurraged-units

We appreciate your careful examination of our mathematical implementations. Your attention to detail in reviewing complex calculations contributes to the robustness of our system. Thank you for your valuable input in this security review.

Hi @benjaminbollen are you sure the current implementation correct. ?

we see the following difference with current code and with our proposed fixes.

the current code mints = 23995231968206374978 

our proposed fix mints = 23999999999999999999

difference = 4,768,031,793,625,021

Check this with POC

    function testMintCheck() public {
        // Alice registers in v2
        ITokenV1 tokenAlice = signupInV1(addresses[0]);
        vm.prank(addresses[0]);
        tokenAlice.stop();
        vm.prank(addresses[0]);
        mockHub.registerHuman(address(0), bytes32(0));

        // Bob registers in v2
        ITokenV1 tokenBob = signupInV1(addresses[1]);
        vm.prank(addresses[1]);
        tokenBob.stop();
        vm.prank(addresses[1]);
        mockHub.registerHuman(address(0), bytes32(0));

        // move time
        skipTime(1 days); //24 hours

        (uint256 value, ,) = mockHub.calculateIssuance(addresses[0]);

        console.log("issuance balance", value);

        // Alice can mint in V2
        vm.prank(addresses[0]);
        mockHub.personalMint();
        // Bob can mint in V2
        vm.prank(addresses[1]);
        mockHub.personalMint();

        console.log("bob balance", mockHub.balanceOf(addresses[0],mockHub.toTokenId(addresses[0])));
        console.log("alice balance", mockHub.balanceOf(addresses[1],mockHub.toTokenId(addresses[1])));

        //intentionally reverted to log the balance value.
        assert(1 == 2);
    }```

[0xmahdirostami]

Yes, the math is correct here. Complete hours in dayA should be multiplied by R[n], but incomplete hours in dayB shouldn't.

[0xpinky]

but when we test both case, the current implementation mints less right

[benjaminbollen]

but what is the argument @0xpinky. Why would you propose this fix?

[0xpinky]

Hi @benjaminbollen

let me explain what we found. Lets see the below portion of codes.

 // calculate the number of completed hours in day A until `startMint`
        int128 k = Math64x64.fromUInt((startMint - (dA * 1 days + inflationDayZero)) / 1 hours);

        // Calculate the number of incompleted hours remaining in day B from current timestamp
        int128 l = Math64x64.fromUInt(((dB + 1) * 1 days + inflationDayZero - block.timestamp) / 1 hours + 1);

        // calculate the overcounted (demurraged) k (in day A) and l (in day B) hours
        int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), l);

if we see, the k & l values are in hours. to compute the overcount amount which is number of circles, this line is,

int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), l);

It is mul(R[n], k) + l -- it adds the demurrage value with l hours. which is incorrect.

We applied following fix in the _calculateIssuance.

int128 overcount = Math64x64.add(Math64x64.mul(R[n], k), Math64x64.mul(R[n], l)); this adds the correct amount of circle in k and l hours and calculate the overcount amount.

with this, our POC mints 23999999999999999999 for 24 hours. where as the existing code mints 23995231968206374978. The difference is 4,768,031,793,625,021.

we run few more test cases, this happen when the duration crosses 24 hours and increase further.

you can run our POC with and without proposed fix and can see the difference in circles calculated.