Incorrect Reward Calculation Due to Binary Search Related Problem inside the `SingelTokenVirtualRewarderUpgradeable`

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0x251a5fcf58d7d621b1837aa5b5e9f163253e9b180d969bc516b712d0db9da98a Severity: high

Description: Description\ The total accumulated reward since the last claim is calculated based on the rewardsPerEpoch variable at the end of each epoch. However, due to a math-related timing issue, the rewards per epoch used for calculating rewards is based on the first second of the next epoch instead of the end of the current epoch. This causes several problems such as incorrect reward allocation which is described bellow:

1. When the strategy deposits at the boundary of epochs, its deposit is counted in the current epoch's rewardsPerEpoch via notifyRewardAmount().
2. When calculating the reward, however, the next epoch's start is considered.
3. This means users who deposit at the start of a new epoch dilute the reward rate for the previous epoch but do not receive rewards for that epoch.
4. As a result, some rewards remain unclaimed and locked within the contract because they do not roll over to future epochs.

The math below shows that epoch_ + _WEEK is indeed the first second of the next epoch and not the last of the prior epoch:

function epochStart(uint256 timestamp) internal pure returns (uint256) {
    return timestamp - (timestamp % _WEEK);
}

Outputs:

Expression	Type	Hex	Decimal
epochStart(123)	uint	0x0	0
epochStart(100000000)	uint	0x5f2b480	99792000
WEEK	uint	0x93a80	604800
epochStart(WEEK)	uint	0x93a80	604800
epochStart(1 + WEEK)	uint	0x93a80	604800
epochStart(0 + WEEK)	uint	0x93a80	604800
epochStart(WEEK - 1)	uint	0x0	0

As it is clear from the abovementioned table, the epoch_ + _WEEK shows the start of the next epoch.

Attachments

2. Revised Code File (Optional)\ Consider modifying the rewardsPerEpoch boundaries inside the _calculateRewardPerEpoch() function:

    function _calculateRewardPerEpoch(uint256 tokenId_, uint256 epoch_) internal view returns (uint256) {
        uint256 balance = VirtualRewarderCheckpoints.getAmount(
            tokensInfo[tokenId_].balanceCheckpoints,
            tokensInfo[tokenId_].checkpointLastIndex,
            epoch_
        );

        uint256 supply = VirtualRewarderCheckpoints.getAmount(totalSupplyCheckpoints, totalSupplyCheckpointLastIndex, epoch_);

        if (supply == 0) {
            return 0;
        }

-        return (balance * rewardsPerEpoch[epoch_ + _WEEK]) / supply;
+       return (balance * rewardsPerEpoch[epoch_ + _WEEK - 1]) / supply;
    }

[MatinR1]

I made a mistake in writing the title of the issue and redeployed it (#49) Consider ignoring this report. Sorry for this inconvenience.

[0xmahdirostami]

No problem, we will review issue #49, thank you for commenting