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

1 stars 1 forks source link

Matin - StakingRewards `rewardPerTokenStored` can be inflated and rewards can be stolen #50

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

Matin

Medium

StakingRewards rewardPerTokenStored can be inflated and rewards can be stolen

Summary

Inflating the rewardPerTokenStored is applicable with staking 1 wei using the first staker and this will lead to drainage of rewards.

Vulnerability Detail

As there isn't a minimum deposit amount criteria, any amount can be staked inside the StakingRewards contract. Considering this fact, when a user calls stake() with 1 wei, it updates the _totalSupply as 1 wei and the corresponding rewards through updateReward modifier. This modifier calls the function rewardPerToken():

    function rewardPerToken() public view returns (uint256) {
        if (_totalSupply == 0) {
            return rewardPerTokenStored;
        }
        return
            rewardPerTokenStored + (((lastTimeRewardApplicable() - lastUpdateTime) * rewardRate * 1e18) / _totalSupply);
    }

This function would return the accumulated rewardPerTokenStored plus the linear increase of the reward. Since it depends on the denominator as _totalSupply, the whole multiplying will be divided by 1 wei which will inflate the rewardPerTokenStored astronomically. Also there isn't any preventive check for the user to withdraw it in the withdraw function. Thus, this scenario is applicable here:

  1. User stakes 1 wei
  2. The rewardDistributor transfers 1 ether reward tokens to the contract and calls notifyRewardAmount() with 1 ether
  3. Some time passes (e.g. 200 seconds)
  4. At this time the rewardRate is incorrectly inflated and the user can withdraw and drain the rewards

Proof of Concept

This test shows the attack vector explained above:

    function testFirstUserInflationAttack() public {

        uint rewardRate = staking.rewardPerToken();
        console.log("Initial Reward Rate is: ", rewardRate);

        address attacker = vm.addr(50);
        stakingToken.mint(attacker, 1 ether);
        vm.startPrank(attacker);
        stakingToken.approve(address(staking), 100 ether);
        staking.stake(1 wei);
        vm.stopPrank();

        vm.warp(block.timestamp + 100);

        vm.startPrank(rewardDistributor);
        rewardToken.transfer(address(staking), 1 ether);
        staking.notifyRewardAmount(1 ether);
        vm.stopPrank();

        vm.warp(block.timestamp + 100);

        rewardRate = staking.rewardPerToken();
        console.log("Reward Rate After Stake is: ", rewardRate);
    }

The test result is:

Ran 1 test for test/MakerTest.t.sol:MakerTest
[PASS] testFirstUserInflationAttack() (gas: 278449)
Logs:
  Initial Reward Rate is:  0
  Reward Rate After Stake is:  165343915343900000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.45ms (286.80µs CPU time)

Impact

The first user can inflate the rewardPerTokenStored with staking just 1 wei and after some time he can drain the accumulated rewards.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider putting deposit limits in the contract StakingRewards

telome commented 1 month ago

Not an issue. Works as intended. If the user is the only staker, they get to receive the full farm reward, irrespective of their stake (1 wei or otherwise).