Unnecessary Hourly Check in _calculateIssuance Function Prevents Users from Receiving Rewards

[hats-bug-reporter]

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

Description:

Description

The _calculateIssuance function in the contract includes a check that prevents users from claiming rewards if less than 1 hour has passed since their last mint time:

Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/src/circles/Circles.sol

Lines 94 to 98 in 507e185

	if (uint256(mintTime.lastMintTime) + 1 hours > block.timestamp) {
	// Mint time is set to indefinite future for stopped mints in v2
	// and only complete hours get minted, so shortcut the calculation
	return (0, 0, 0);
	}

this check just prevents users from getting rewards.

For example, if a user mints after 1 hour and 50 minutes, they should be able to mint after waiting an additional 11 minutes (to complete the next full hour "50m + 11m = 61m"). However, the current check returns zero and prevents this from happening.

POC

function testIssuanceOnExactHour() public {
    // start time sets time to zero time + 1 second
    skipTime(5 minutes);
    vm.prank(addresses[0]);
    circles.claimIssuance();

    vm.prank(addresses[0]);
    _skipAndMint(1 hours + 50 minutes, addresses[0]);
    _skipAndMint(11 minutes, addresses[0]);
}

Logs with current implementation:

[PASS] testIssuanceOnExactHour() (gas: 114463)
Logs:
  hoursCount 1 days: 0
  issuance 1000000000000000000
  hoursCount 0 days: 0
  issuance 0

Logs after mitigation:

[PASS] testIssuanceOnExactHour() (gas: 149317)
Logs:
  hoursCount 1 days: 0
  issuance 1000000000000000000
  hoursCount 1 days: 0
  issuance 1000000000000000000

Impact

This issue unnecessarily prevents users from receiving rewards, delaying their ability to mint even when the required time has passed.

Mitigation

Remove the check that prevents minting if less than 1 hour has passed:

-        if (uint256(mintTime.lastMintTime) + 1 hours >= block.timestamp) {
-            return (0, 0, 0);
-        }

there is no issue on deleting this check, because _claimIssuance will return 0, if user doesn't gain any token.

Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf/src/circles/Circles.sol

Lines 135 to 140 in 507e185

	function _claimIssuance(address _human) internal {
	(uint256 issuance, uint256 startPeriod, uint256 endPeriod) = _calculateIssuance(_human);
	if (issuance == 0) {
	// No issuance to claim, simply return without reverting
	return;
	}

[benjaminbollen]

this check also was meant to block "stopped tokens" (with lastMintTime set to INDEFINITE_FUTURE) so I need to think for a moment, but I see your point

[benjaminbollen]

So I think the better fix might be trying to round the lastMintTime down to the last complete hour; and then evaluate the + 1 hour. But that is my problem later this week, patching it.

For the record this was originally put in place as one of the measures to "slow down the updates to the graph" (so that graph algorithms have at least a few seconds to search the solution space), but lost that value, when

• we've removed other such measures to delay graph updates (eg, untrust was initially also delayed in its effect)
• and later we decided to round mints to complete hours

so now indeed, it has perhaps only a poorer UX as a result. While this was intended design, it is design that is worth improving