0x73696d616f - Rewards incorrectly use new stake amount when calculating rewards from previous periods

[sherlock-admin]

0x73696d616f

high

Rewards incorrectly use new stake amount when calculating rewards from previous periods

Summary

When a new amount is staked in the MainRewarder, it calculates the rewards of the previous period taking into account the new stake. The correct behaviour would be to only account the new stake in the period after which the stake is done, not also before. This error would allow anyone to steal all the rewards accrued over the past duration. Notice the difference to the original implementation.

Vulnerability Detail

The MainRewarder lets the StakeTracker contract call stake() and withdraw().

The StakeTracker is going to be either a DestinationVault or LMPVault, which stake inside the _afterTokenTransfer() function.

When this function is called (in _transfer()), the balances of the accounts and the totalSupply() have already been updated, such that the function call to stake() of the Rewarder, will update the previous rewards in _updateReward() with additional amounts, leading to extra rewards.

Impact

This bug can be abused to steal all rewards, consider the following attack:

• n blocks have passed, equaling r rewards
• attacker deposits into vault, which stakes and accrues rewards from the previous n blocks with the new deposited amount a
• attacker then transfers the shares of the vault to another address, which withdraws from the original account and stakes again on the new account. The new account will receive the same new rewards as if it had staked in the previous n blocks.
• repeat transfer to another address until all the rewards have been drained.

Code Snippet

Wrote the following POC in RewardVault.t.sol:

function test_POC_stakeAttributesRewards_fromPreviousDurations() public {
    vm.roll(block.number + 100);

    // stakes now, but gets rewards from previous duration
    // would have the same problem if a stake was triggered by a vault deposit
    // as the balance would be updated before the stake function call
    // which would result in rewards being earned via _updateReward() in stake().
    vm.prank(address(stakeTracker));
    mainRewardVault.stake(RANDOM, amount);

    uint256 earned = mainRewardVault.earned(RANDOM);
    assertEq(earned, amount);

    uint256 rewardBalanceBefore = mainReward.balanceOf(RANDOM);
    uint256 extraReward1BalanceBefore = extraReward1.balanceOf(RANDOM);
    uint256 extraReward2BalanceBefore = extraReward2.balanceOf(RANDOM);

    vm.prank(RANDOM);
    mainRewardVault.getReward();

    uint256 rewardBalanceAfter = mainReward.balanceOf(RANDOM);
    uint256 extraReward1BalanceAfter = extraReward1.balanceOf(RANDOM);
    uint256 extraReward2BalanceAfter = extraReward2.balanceOf(RANDOM);

    assertEq(rewardBalanceAfter - rewardBalanceBefore, amount);
    assertEq(extraReward1BalanceAfter - extraReward1BalanceBefore, amount);
    assertEq(extraReward2BalanceAfter - extraReward2BalanceBefore, amount);
}

Notice that the stake is performed after the 100 blocks have passed, but the address RANDOM still gets all the rewards as if it had been there from the beginning. Thus, it enables the previously mentioned attack of moving tokens to new addresses and claiming extra rewards.

Tool used

Vscode
Foundry
Manual Review

Recommendation

_updateRewards() should calculate the rewards from the previous period with the totalSupply() and balanceOf(account) before the new staked amount. It does it correctly for the account that is losing the tokens, but not for the one receiving them. Consider adding an updateRewards(address account) external function and call it with the to address in _beforeTokenTransfer(). Then, stake() can skip the _updateReward() call.

Duplicate of #603