Bozho - Users who backrunning `StakingRewardsV2.sol#notifyRewardAmount()` function with `StakingRewardsV2.sol#stake()` receive same rewards as users staking since `periodFinish`

[sherlock-admin2]

Bozho

Medium

Users who backrunning StakingRewardsV2.sol#notifyRewardAmount() function with StakingRewardsV2.sol#stake() receive same rewards as users staking since periodFinish

Summary

Summary

The StakingRewardsV2.sol#notifyRewardAmount() function manage the reward distribution in the StakingRewardsV2 contract. It updates the reward rates and the reward period whenever new rewards are added. This function ensures that rewards are distributed fairly over the specified duration and handles both the end of a reward period and the continuation of an ongoing period.

Users can exploit the notifyRewardAmount() function by strategically timing their stake() calls, receiving the same rewards as users who staked much earlier. This vulnerability allows for an unfair distribution of rewards, disadvantaging users who have staked for longer periods.

Vulnerability Detail

When rewards are fully streamed i.e. reward duration has ended, the lastTimeRewardApplicable() function starts returning periodFinish, which is initially 1 week + timestamp when notifyRewardAmount() was last called.

This stops reward accumulation per token since lastTimeRewardApplicable() - lastUpdateTime would evaluate to 0 here, causing only the rewardPerTokenAccumulatedCheckpoint last recorded here to be returned.

The issue is that once block.timestamp has passed the lastUpdateTime but before notifyRewardAmount() is called to start the new period, the users staking during this period all are checkpointed to the last recorded checkpoint. This is unfair to users who staked beforehand while other users could just backrun when notifyRewardAmount() is next called since they receive the same initial checkpoint to start earning rewards.

For example:

• User A stakes at t = 100
• Reward end time (duration of a period i.e. reward period ends) at t = 100
• User B stakes (backruns notifyRewardAmount) at t = 200
• notifyRewardAmount is called through StakingRewardsNotifier at t = 200

So basically user B stakes with time difference = 99 (compared to user A) and still receives same rewards as A when the next distribution period starts even though A staked for longer. Note: We assume both A and B stake same amount of tokens for simplicity.

Why I believe this is an issue

This issue is not dependent on the design spec since if the design spec is correct currently, there is an issue and if it is wrong, then the issue identifies the incorrect design spec. Let's take a look at what I mean by this:

If the project intends to allow users to accumulate rewards even when periodFinish is met, then user A and B are not checkpointed correctly.

If the project intends to not allow users to accumulate rewards when periodFinish is met, then it is a problem since both user A and B receive rewards from periodFinish onwards i.e. the last user specific checkpoint. Note that when notifyRewardAmount() is called for the next round, it updates the global reward per token and lastUpdateTime correctly, but it does not consider that the user specific checkpoints still operate on userRewardPerTokenPaid when calculating rewards here.

Depending on the spec, either ways there is an issue in my opinion, which should be corrected accordingly.

Impact

This vulnerability leads to an unfair advantage for users who can time their staking actions just before a new reward period starts. Users who stake at the end of a reward period can receive the same rewards as those who have staked for the entire duration of the period, undermining the intended fairness of the reward distribution system.

Code Snippet

    /// @inheritdoc IStakingRewardsV2
    function notifyRewardAmount(uint256 _reward)
        external
        onlyRewardsNotifier
        updateReward(address(0))
    {
        if (block.timestamp >= periodFinish) {
            rewardRate = _reward / rewardsDuration;
        } else {
            uint256 remaining = periodFinish - block.timestamp;
            uint256 leftover = remaining * rewardRate;
            rewardRate = (_reward + leftover) / rewardsDuration;
        }

        lastUpdateTime = block.timestamp;
        periodFinish = block.timestamp + rewardsDuration;
        emit RewardAdded(_reward);
    }

• https://github.com/Kwenta/token/blob/main/contracts/StakingRewardsV2.sol#L587C1-L603C6

    /// @inheritdoc IStakingRewardsV2
    function stake(uint256 _amount) external whenNotPaused updateReward(msg.sender) {
        if (_amount == 0) revert AmountZero();

        // update state
        userLastStakeTime[msg.sender] = block.timestamp;
        _addTotalSupplyCheckpoint(totalSupply() + _amount);
        _addBalancesCheckpoint(msg.sender, balanceOf(msg.sender) + _amount);

        // emit staking event and index msg.sender
        emit Staked(msg.sender, _amount);

        // transfer token to this contract from the caller
        kwenta.transferFrom(msg.sender, address(this), _amount);
    }

• https://github.com/Kwenta/token/blob/main/contracts/StakingRewardsV2.sol#L216-L229

    /// @notice update reward state for the account and contract
    /// @param _account: address of account which rewards are being updated for
    /// @dev contract state not specific to an account will be updated also
    modifier updateReward(address _account) {
        _updateReward(_account);
        _;
    }

    function _updateReward(address _account) internal {
        rewardPerTokenStored = rewardPerToken();
        lastUpdateTime = lastTimeRewardApplicable();

        if (_account != address(0)) {
            // update amount of rewards a user can claim
            rewards[_account] = earned(_account);

            // update reward per token staked AT this given time
            // (i.e. when this user is interacting with StakingRewards)
            userRewardPerTokenPaid[_account] = rewardPerTokenStored;
        }
    }

• https://github.com/Kwenta/token/blob/main/contracts/StakingRewardsV2.sol#L369-L389

Recommendation

It is most likely the case that reward accumulation is intended to be stopped once periodFinish is passed. Thus, my solution is based on it.

Add a special condition in the user checkpoints update here where if block.timestamp >= periodFinish, we set userRewardPerTokenPaid to rewardPerToken(), which has been updated after notifyRewardAmount() call that started the new period. This would ensure that users do not claim rewards for holding during the period when accumulation is stopped.

If reward accumulation is not intended to be stopped after periodFinish, consider checkpointing the users based on block.timestamp. This is probably not the intended design spec since the code literally implements this check here to stop accumulation.

Root Cause

No response

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

No response

PoC

No response

Mitigation

No response