The Function`_calculateRewardPerEpoch()`Calculates Accumulated Reward Incorrectly on Each Epoch Boundary

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0x1cee266e239af0e47816fbac839bd5cc46bd39487efb6c690c34e30328724cba 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;
    }

[BohdanHrytsak]

Explanation of Reward Distribution Logic

Thank you for your submission and participation in the competition.

Explanation of Reward Logic

The key point is understanding how the mVeNFT voting and reward system works across epochs. Here's a breakdown of the logic:

1. Deposits and Voting Power Calculation:

   • When users deposit during an epoch, their deposits contribute to the total mVeNFT voting power for that epoch.
   • This total voting power is determined at the end of the epoch.

2. Reward Distribution Timing:

   • Rewards are based on the mVeNFT voting power accumulated by the end of the previous epoch.
   • Therefore, rewards for the next epoch are calculated and distributed at the beginning of the new epoch, based on the voting power from the previous epoch.

The point is that users who deposit in an epoch add up to the total mVeNFT vote at the end of the epoch, for which they will receive a reward based on this vote at the beginning of the next epoch (after the epoch change) And the reward for the new epoch will be distributed among those who had a balance in the previous one.

Therefore, the logic is correct, since the reward for the previous era will be credited only in the current one.

In the case of the option that you need to take the reward for the current one, it will turn out that the user will deposit, his balance will not participate in receiving the reward, but he will receive the reward - which, as we can see, is logically incorrect.