sherlock-audit / 2024-06-makerdao-endgame-judging

1 stars 1 forks source link

bareli - stakingRewards reward rate can be dragged out and diluted #56

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

bareli

Medium

stakingRewards reward rate can be dragged out and diluted

Summary

stakingRewards reward rate can be dragged out and diluted

Vulnerability Detail

function notifyRewardAmount(uint256 reward) external override onlyRewardsDistribution updateReward(address(0)) { if (block.timestamp >= periodFinish) { rewardRate = reward / rewardsDuration; } else { uint256 remaining = periodFinish - block.timestamp; uint256 leftover = remaining * rewardRate; rewardRate = (reward + leftover) / rewardsDuration; }

Impact

The StakingRewards.notifyRewardAmount function receives a reward amount and extends the current reward end time to now + rewardsDuration. It rebases the currently remaining rewards + the new rewards (reward + leftover) over this new rewardsDuration period. This can lead to a dilution of the reward rate and rewards being dragged out forever by malicious new reward deposits.

Imagine the current rewardRate is 1000 rewards / rewardsDuration.

20% of the rewardsDuration passed, i.e., now = lastUpdateTime + 20% * rewardsDuration.

A malicious actor notifies the contract with a reward of 0: notifyRewardAmount(0).

Then the new rewardRate = (reward + leftover) / rewardsDuration = (0 + 800) / rewardsDuration = 800 / rewardsDuration.

The rewardRate just dropped by 20%. This can be repeated infinitely. After another 20% of reward time passed, they trigger notifyRewardAmount(0) to reduce it by another 20% again: rewardRate = (0 + 640) / rewardsDuration = 640 / rewardsDuration.

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol#L144C4-L163C6

Tool used

Manual Review

Recommendation

The rewardRate should never decrease by a notifyRewardAmount call. Consider not extending the reward payouts by rewardsDuration on every call. periodFinish probably shouldn't change at all, the rewardRate should just increase by rewardRate += reward / (periodFinish - block.timestamp).

Alternatively, consider keeping the rewardRate constant but extend periodFinish time by += reward / rewardRate.

telome commented 1 month ago

Known issue. See Section 7.2 of https://github.com/makerdao/endgame-toolkit/blob/5dc625fd6a07c7c24a97a45553c2287f38807e44/audits/ChainSecurity_Maker_EndGame_Toolkit_audit_v3.pdf Note that passing 0 to notifyRewardAmount is not actually possible as there will always be some amount of vested reward in each call. In practice, in the steady state, when the dssVest has been supplying a constant stream of reward for a long time, the rewardRate in StakingRewards will converge to the same constant rate as in dssVest.