Squilliam - Malicious stakers will cause loss of reward funds for honest participants

[sherlock-admin2]

Squilliam

High

Malicious stakers will cause loss of reward funds for honest participants

Summary

The flawed reward calculation in the StakingRewards contract, which fails to account for time-weighted average stakes, will cause a significant loss of reward tokens for honest stakers. This flaw enables malicious users to briefly stake large amounts, withdraw most of their stake, and still earn rewards as if they had maintained their maximum stake throughout the entire period. Malicious actors can repeat this process over time and on different wallet addresses to claim disproportionately large rewards while maintaining minimal long-term stake.

Root Cause

In StakingRewards.sol, the earned() and rewardPerToken() functions do not correctly account for changes in a user's staked amount over time. The reward calculation is based on the current stake multiplied by the difference in rewardPerToken() since the last update, rather than considering the time-weighted average of the user's stake. This means that:

1. In rewardPerToken():

   return rewardPerTokenStored + (((lastTimeRewardApplicable() - lastUpdateTime) * rewardRate * 1e18) / _totalSupply);

   The calculation uses the current _totalSupply, not accounting for how it might have changed over the period.

2. In earned():

   return ((_balances[account] * (rewardPerToken() - userRewardPerTokenPaid[account])) / 1e18) + rewards[account];

   This multiplies the current balance by the entire change in rewardPerToken(), even if the user's balance changed during that period.

This approach allows users to benefit from high reward rates achieved during periods of large stakes, even after they've withdrawn most of their stake.

StakingRewards::earned(): https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol?=plain#L92-L94

StakingRewards::rewardPerToken(): https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol?=plain#L84-L90

Internal pre-conditions

The StakingRewards contract needs to be deployed and active. The reward pool needs to be funded with a significant amount of reward tokens. The staking period needs to be ongoing.

External pre-conditions

None specific to this vulnerability.

Attack Path

1. Attacker observes the current reward rate and total staked amount.
2. Attacker stakes a large amount of tokens, significantly increasing their share of the total stake.
3. Attacker waits for a short period, allowing rewards to accrue based on their large stake.
4. Attacker withdraws most of their stake, leaving only a minimal amount.
5. The contract continues to calculate rewards based on the attacker's current (small) stake, but using the high rewardPerToken value that was inflated by their previously large stake.
6. Attacker repeats this process multiple times during the staking period.
7. At the end of the staking period, the attacker claims their disproportionately large rewards.

Impact

This vulnerability leads to a significant loss of funds for honest users and undermines the integrity of the staking mechanism:

Direct Loss of Rewards(Funds) for Honest Users: Honest stakers receive fewer reward tokens(funds) than they should based on their stake and duration. The difference between expected rewards in a fair system and actual received rewards represents a direct loss of funds for honest participants. This loss is quantifiable in terms of the reward tokens not received that they should of received.

Cumulative Financial Impact: Over time, the loss for long-term stakers can be substantial. The consistent underpayment of rewards compounds, resulting in significant financial losses for dedicated participants.

Market Pressure: If widely exploited, the vulnerability could lead to increased selling pressure on the reward token as exploiters claim and potentially sell their disproportionate rewards. This could indirectly lead to a decrease in the reward token's value, further impacting honest users' returns.

Undermining Token Supply Reduction: The vulnerability negates the intended reduction of circulating supply. Users can stake tokens, quickly withdraw and sell them, while still earning rewards. This increases token velocity instead of reducing it, potentially leading to: Higher price volatility contrary to the protocol's stability objectives Increased selling pressure on the token - Increased selling pressure causes losses for the protocol, dev team, investors, and stakers.

Reasoning for High Severity Rating:

1. Assuming a user stakes 10,000 MKR (worth $10 million at $1000/MKR) for a month, expecting 1% rewards (100 MKR or $100,000). Due to this vulnerability, if exploiters collectively manipulate the pool such that honest users only receive 50% of their expected rewards, this user would lose 50 MKR ($50,000), which is a 50% loss on their expected rewards and 0.5% of their total staked value. This loss significantly exceeds the 5% threshold for high severity.

2. This vulnerability can be exploited continuously throughout the staking period.

3. Given the assumed $250 million maximal locked MKR, if this vulnerability leads to a 10% reduction in staking participation due to lost trust and reduced rewards for honest participants, it would result in a $25 million loss in locked value for the protocol, far exceeding the 5% threshold for high severity.

4. This vulnerability is inherent in the reward calculation mechanism and can be exploited under normal operating conditions, without requiring any specific external factors or unusual states.

PoC

Add the following test functions to StakingRewards.t.sol, in the StakingRewardsTest contract:

 function testPartialWithdrawalsAndRewardCalculations() public {
        uint256 rewardAmount = 1000 * WAD;
        uint256 stakingAmount = 100 * WAD;
        uint256 duration = 7 days;

        initFarm(stakingAmount, rewardAmount, duration);

        // Skip half the duration
        skip(duration / 2);

        // Calculate earned rewards
        uint256 earnedBefore = rewards.earned(address(this));

        // Withdraw half the staked amount
        uint256 withdrawAmount = stakingAmount / 2;
        rewards.withdraw(withdrawAmount);

        // Check balances
        assertEq(gem.balanceOf(address(this)), withdrawAmount, "Should have withdrawn half of staked amount");
        assertEq(
            rewards.balanceOf(address(this)), stakingAmount - withdrawAmount, "Should have half of staked amount left"
        );

        // Skip the rest of the duration
        skip(duration / 2);

        // Calculate earned rewards after partial withdrawal
        uint256 earnedAfter = rewards.earned(address(this));

        // The earned amount after should be less than double the earned amount before,
        // because we reduced our stake halfway through
        assert(earnedAfter < earnedBefore * 2);
        assert(earnedAfter > earnedBefore);

        // Withdraw remaining and claim rewards
        rewards.exit();

        // Check final balances
        assertEq(gem.balanceOf(address(this)), stakingAmount, "Should have withdrawn all staked tokens");
        assertEq(rewardGem.balanceOf(address(this)), earnedAfter, "Should have claimed all rewards");
    }

    function testDetailedPartialWithdrawalsAndRewardCalculations() public {
        uint256 rewardAmount = 1000 * WAD;
        uint256 stakingAmount = 100 * WAD;
        uint256 duration = 7 days;

        console.log("Initial setup:");
        console.log("Reward amount:", rewardAmount);
        console.log("Staking amount:", stakingAmount);
        console.log("Duration:", duration);

        initFarm(stakingAmount, rewardAmount, duration);

        console.log("After initFarm:");
        console.log("Total supply:", rewards.totalSupply());
        console.log("Reward rate:", rewards.rewardRate());

        // Skip half the duration
        skip(duration / 2);

        console.log("\nHalf duration passed:");
        console.log("Current timestamp:", block.timestamp);
        console.log("Period finish:", rewards.periodFinish());

        // Calculate earned rewards
        uint256 earnedBefore = rewards.earned(address(this));
        console.log("Earned before withdrawal:", earnedBefore);

        // Withdraw half the staked amount
        uint256 withdrawAmount = stakingAmount / 2;
        rewards.withdraw(withdrawAmount);

        console.log("\nAfter partial withdrawal:");
        console.log("Withdrawn amount:", withdrawAmount);
        console.log("Remaining stake:", rewards.balanceOf(address(this)));
        console.log("Total supply:", rewards.totalSupply());

        // Skip the rest of the duration
        skip(duration / 2);

        console.log("\nEnd of duration:");
        console.log("Current timestamp:", block.timestamp);
        console.log("Period finish:", rewards.periodFinish());

        // Calculate earned rewards after partial withdrawal
        uint256 earnedAfter = rewards.earned(address(this));
        console.log("Earned after full duration:", earnedAfter);

        console.log("\nComparison:");
        console.log("Earned before * 2:", earnedBefore * 2);
        console.log("Earned after:", earnedAfter);

        // Instead of using assert, we'll use require with a detailed message
        require(
            earnedAfter < earnedBefore * 2,
            string(
                abi.encodePacked(
                    "Earned after (",
                    vm.toString(earnedAfter),
                    ") should be less than double earned before (",
                    vm.toString(earnedBefore * 2),
                    ")"
                )
            )
        );

        require(
            earnedAfter > earnedBefore,
            string(
                abi.encodePacked(
                    "Earned after (",
                    vm.toString(earnedAfter),
                    ") should be greater than earned before (",
                    vm.toString(earnedBefore),
                    ")"
                )
            )
        );

        // Withdraw remaining and claim rewards
        rewards.exit();

        console.log("\nAfter exit:");
        console.log("Staking token balance:", gem.balanceOf(address(this)));
        console.log("Reward token balance:", rewardGem.balanceOf(address(this)));

        assertEq(gem.balanceOf(address(this)), stakingAmount, "Should have withdrawn all staked tokens");
        assertEq(rewardGem.balanceOf(address(this)), earnedAfter, "Should have claimed all rewards");
    }

    function testRootCauseOfPartialWithdrawalVulnerability() public {
        uint256 rewardAmount = 1000 * WAD;
        uint256 stakingAmount = 100 * WAD;
        uint256 duration = 7 days;

        // Setup two identical farms
        StakingRewards rewardsA = new StakingRewards(address(this), address(this), address(rewardGem), address(gem));
        StakingRewards rewardsB = new StakingRewards(address(this), address(this), address(rewardGem), address(gem));

        // Initialize both farms
        rewardsA.setRewardsDuration(duration);
        rewardsB.setRewardsDuration(duration);

        rewardGem.mint(rewardAmount * 2);
        rewardGem.transfer(address(rewardsA), rewardAmount);
        rewardsA.notifyRewardAmount(rewardAmount);
        rewardGem.transfer(address(rewardsB), rewardAmount);
        rewardsB.notifyRewardAmount(rewardAmount);

        gem.mint(stakingAmount * 2);
        gem.approve(address(rewardsA), stakingAmount);
        gem.approve(address(rewardsB), stakingAmount);

        rewardsA.stake(stakingAmount);
        rewardsB.stake(stakingAmount);

        // Fast forward to mid-duration
        vm.warp(block.timestamp + duration / 2);

        // For rewardsB, perform a partial withdrawal
        rewardsB.withdraw(stakingAmount / 2);

        console.log("Mid-duration state:");
        console.log("rewardsA staked:", rewardsA.balanceOf(address(this)));
        console.log("rewardsB staked:", rewardsB.balanceOf(address(this)));
        console.log("rewardsA earned:", rewardsA.earned(address(this)));
        console.log("rewardsB earned:", rewardsB.earned(address(this)));

        // Fast forward to end of duration
        vm.warp(block.timestamp + duration / 2);

        console.log("\nEnd of duration state:");
        console.log("rewardsA staked:", rewardsA.balanceOf(address(this)));
        console.log("rewardsB staked:", rewardsB.balanceOf(address(this)));
        console.log("rewardsA earned:", rewardsA.earned(address(this)));
        console.log("rewardsB earned:", rewardsB.earned(address(this)));

        // Both should claim their rewards
        rewardsA.getReward();
        uint256 rewardsAClaimed = rewardGem.balanceOf(address(this));
        rewardGem.transfer(address(0x1), rewardsAClaimed); // Reset balance for next check
        rewardsB.getReward();
        uint256 rewardsBClaimed = rewardGem.balanceOf(address(this));

        console.log("\nAfter claiming rewards:");
        console.log("rewardsA claimed:", rewardsAClaimed);
        console.log("rewardsB claimed:", rewardsBClaimed);

        // Assert that the rewards are incorrectly the same
        assertEq(
            rewardsAClaimed, rewardsBClaimed, "Rewards should be different but are the same, indicating a vulnerability"
        );

        // Assert that rewardsB (partial withdrawal) has earned more than it should have
        require(
            rewardsBClaimed > (rewardAmount * 3) / 4,
            "Partial withdrawal farm earned less than expected, vulnerability not present"
        );
    }

    function testMultiplePartialWithdrawals() public {
        // Setup initial staking and rewards
        uint256 initialStake = 100 * WAD;
        uint256 rewardAmount = 1000 * WAD;
        uint256 duration = 7 days;
        initFarm(initialStake, rewardAmount, duration);

        // User A: No withdrawals
        // User B: Multiple partial withdrawals
        address userA = address(1);
        address userB = address(2);

        // Stake for both users
        vm.startPrank(userA);
        setupStakingToken(initialStake);
        rewards.stake(initialStake);
        vm.stopPrank();

        vm.startPrank(userB);
        setupStakingToken(initialStake);
        rewards.stake(initialStake);
        vm.stopPrank();

        // Perform multiple withdrawals for User B
        uint256[] memory withdrawalTimes = new uint256[](3);
        withdrawalTimes[0] = duration / 4;
        withdrawalTimes[1] = duration / 2;
        withdrawalTimes[2] = (3 * duration) / 4;

        for (uint256 i = 0; i < withdrawalTimes.length; i++) {
            vm.warp(block.timestamp + withdrawalTimes[i]);
            vm.prank(userB);
            rewards.withdraw(initialStake / 10); // Withdraw 10% each time
        }

        // Warp to end of duration
        vm.warp(block.timestamp + duration);

        // Compare rewards
        uint256 rewardsA = rewards.earned(userA);
        uint256 rewardsB = rewards.earned(userB);

        console.log("User A rewards:", rewardsA);
        console.log("User B rewards:", rewardsB);

        // User B should have earned significantly less than User A
        assert(rewardsB < rewardsA);
        // But in reality, they might be close or equal due to the vulnerability
    }

    function testExtremeWithdrawal() public {
        uint256 largeStake = 1000000 * WAD;
        uint256 tinyStake = 1 * WAD;
        uint256 rewardAmount = 1000 * WAD;
        uint256 duration = 7 days;

        initFarm(largeStake + tinyStake, rewardAmount, duration);

        // User A: Stakes large amount and immediately withdraws most
        address userA = address(1);
        address userB = address(2);

        deal(address(gem), userA, largeStake);
        deal(address(gem), userB, tinyStake);

        vm.startPrank(userA);
        gem.approve(address(rewards), largeStake);
        rewards.stake(largeStake);
        rewards.withdraw(largeStake - tinyStake);
        vm.stopPrank();

        // User B: Stakes tiny amount
        vm.startPrank(userB);
        gem.approve(address(rewards), tinyStake);
        rewards.stake(tinyStake);
        vm.stopPrank();

        // Warp to end of duration
        vm.warp(block.timestamp + duration);

        uint256 rewardsA = rewards.earned(userA);
        uint256 rewardsB = rewards.earned(userB);

        console.log("User A rewards (large stake, then withdraw):", rewardsA);
        console.log("User B rewards (tiny stake):", rewardsB);

        // User A and B should have very similar rewards due to the vulnerability
        assertApproxEqRel(rewardsA, rewardsB, 1e16); // 1% tolerance
    }

Run these tests with the following: forge test --mt testPartialWithdrawalsAndRewardCalculations -vvv forge test --mt testDetailedPartialWithdrawalsAndRewardCalculations -vvv forge test --mt testRootCauseOfPartialWithdrawalVulnerability-vvv forge test --mt testMultiplePartialWithdrawals -vvv forge test --mt testExtremeWithdrawal -vvv

These tests reveal the following about the vulnerability: testPartialWithdrawalsAndRewardCalculations: This test failed with an assertion error, indicating that the earned rewards after a partial withdrawal were not less than double the rewards earned before the withdrawal, as expected. This suggests that the reward calculation is not correctly adjusting for partial withdrawals.

testDetailedPartialWithdrawalsAndRewardCalculations: This test provides more detailed logging, and it clearly shows the issue: Before withdrawal (mid-duration), the earned amount was 499999999999999867200. After the full duration, even with half the stake withdrawn, the earned amount was 999999999999999734400. This is exactly double the mid-duration amount, despite having only half the stake for the second half of the duration.

testRootCauseOfPartialWithdrawalVulnerability: This test passes, but it demonstrates the vulnerability: Two identical farms are set up, but one (rewardsB) has a partial withdrawal midway. At the end of the duration, both farms show the same earned amount (999999999999999734400). Both farms claim the same reward amount, despite rewardsB having half the stake for half the duration.

testMultiplePartialWithdrawals(): This test passed, but it reveals the vulnerability. User B, who made multiple partial withdrawals, still earned 89.8% of what User A earned (who maintained their full stake). This is much higher than expected, given that User B had less stake for a significant portion of the time. It shows that the contract is not correctly adjusting rewards for partial withdrawals.

testExtremeWithdrawal(): This test result clearly demonstrates the vulnerability in the StakingRewards contract: User A initially staked a large amount and then immediately withdrew almost all of it, leaving only a tiny stake. User B staked only a tiny amount for the entire duration. Both users ended up with exactly the same rewards (999997000008999).

The vulnerability lies in how the contract calculates rewards after partial withdrawals. It appears that when a user partially withdraws their stake, the contract does not properly adjust the reward calculation. As a result:

1. Users who partially withdraw continue to earn rewards as if they had their full stake for the entire duration.
2. This allows users to game the system by staking a large amount, waiting for some time, then withdrawing most of their stake while still earning rewards on the full amount.
3. The total rewards distributed could exceed the intended amount, unfairly distributing rewards, and greatly rewarding malicious users more than honest users.

Mitigation

Implement one or more of the following:

1. Implement Time-Weighted Staking: Modify the reward calculation mechanism to use a time-weighted average of a user's stake. Track each user's stake changes over time, storing timestamps and amounts for each stake and withdrawal action. Update the earned() function to calculate rewards based on the time-weighted average stake rather than the current stake.

2. Introduce Reward Vesting: Implement a vesting period for rewards to discourage short-term staking behavior. Rewards earned could be released linearly over a set period (e.g., 30 days) after they are accrued. Early withdrawal of stakes could result in forfeiting unvested rewards or some type of penalty.

3. Snapshots for Reward Calculations: Implement a system of periodic snapshots of user stakes. Calculate rewards based on these snapshots rather than instantaneous balances. This approach can help mitigate the impact of rapid stake/unstake actions.

4. Dynamic Reward Rate Adjustment: Implement a mechanism that adjusts the reward rate based on the total staked amount and its stability over time. This could help balance rewards when there are significant fluctuations in the staking pool.

5. Incremental Reward Distribution: Instead of accumulating rewards and allowing claims at any time, distribute rewards incrementally (e.g., daily or weekly). This approach can help ensure that rewards more accurately reflect a user's stake over time.

6. Two-Tiered Staking System: Create two staking tiers: a flexible tier with lower rewards and a locked tier with higher rewards. The locked tier would require tokens to be staked for a minimum period, enforcing long-term participation for maximum rewards.

[sunbreak1211]

It isn't clear if the submission is referring to the dilution of stakers due to large stakers entering the farm or to an incorrect calculation of rewards. If the former, this is by design and is intrinsic to how the farm operates. If the latter, it seems the submitter may be confused as to what the rewardPerToken() represents. It is a global normalized reward accumulator that increases more slowly during periods of high total supply (high dilution) only to increase again at a faster rate once the total supply has reduced. There is nothing incorrect about multiplying that value by a staker's current balance to calculate the accumulated reward of a staker.