hats-finance / Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf

Circles Protocol contracts
https://aboutcircles.com
GNU Affero General Public License v3.0
0 stars 0 forks source link

Incorrect Calculation of Remaining Hours in Day B in `_calculateIssuance` Function #39

Open hats-bug-reporter[bot] opened 2 weeks ago

hats-bug-reporter[bot] commented 2 weeks ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x27f21c8e50f4ba7fcba30aeb1c79379e795d8b487a744f8fd21f2a7e6f220413 Severity: low

Description:

Vulnerability Detail

The _calculateIssuance function uses a formula to compute the number of incomplete hours remaining in day B from the current timestamp:

https://github.com/hats-finance/Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/blob/507e18587b8a0b64a4bb21db01ecf876dc607e47/src/circles/Circles.sol#L116

However, the current implementation assumes that an extra hour is always added, which is not the case when:

((dB + 1) * 1 days + inflationDayZero - block.timestamp) % 1 hours == 0

In this scenario, the function will not calculate the remaining hours correctly, leading to inaccurate results.

Impact

The contract may not work as intended, causing errors in the issuance calculations, which can lead to inaccurate rewards or token minting.

Mitigation

Update the logic to handle cases where the remainder is zero:

/
int128 l;
uint256 middle = (dB + 1) * 1 days + inflationDayZero - block.timestamp;
if ((middle % 1 hours) == 0) {
    l = Math64x64.fromUInt(middle / 1 hours);
} else {
    l = Math64x64.fromUInt(middle / 1 hours + 1);
}
benjaminbollen commented 2 weeks ago

Thank you for your report on the calculation of remaining hours in the _calculateIssuance function. We've reviewed your submission and agree that this is a valid low-severity issue.

You've correctly identified that when the current time is exactly on the hour, our calculation incorrectly subtracts an extra hour, potentially resulting in a lost hour's worth of minting.

We appreciate your attention to detail in identifying this edge case. Your contribution helps improve the correctness of the system. Thank you for your valuable input.

aktech297 commented 1 week ago

potentially resulting in a lost hour's worth of minting. is it low ? why not medium ? over a period of time, it is going to increase by counting one by one.

0xmahdirostami commented 1 week ago

@aktech297 because the chance of ((dB + 1) * 1 days + inflationDayZero - block.timestamp) % 1 hours == 0 is low.

benjaminbollen commented 4 days ago

Im now coming around to try to write my own tests for this issue;

benjaminbollen commented 4 days ago

@0xmahdirostami for my initial unit tests to cover this issue I am actually (so far) not recovering this as an issue. If you have any tests on your side that highlight this as an issue, it would be good to share.