StakingRewards reward rate can be dragged out and diluted

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L161

Vulnerability details

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.

function withdraw(IERC20 _token, address _to) external override {
    require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
    // @audit uses `msg.sender`'s share but sets `claimed` for _to! can claim for many `_to`s
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
    claimed[_token][_to] = true;
    emit ExitShelter(_token, msg.sender, _to, amount);
    _token.safeTransfer(_to, amount);
}

This can lead to a dilution of the reward rate and rewards being dragged out forever by malicious new reward deposits.

POC

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.

Recommended Mitigation Steps

Imo, 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.

[r2moon]

notifyRewardAmount check msg.sender's permission.

[GalloDaSballo]

The warden is pointing out an admin privilege that would allow the admin to dilute current rewards.

While the sponsor claims this won't happen I can only judge based on the code that is available to me.
And at this point there seems to be no code for the rewardsDistribution contract that would be calling notifyRewardAmount

Given this, I believe the finding to be valid as the POC works out to demonstrate how a malicious owner could dilute the rewardRate.

This would cause loss of yield for all depositors, which makes the finding of Medium Severity