Summary

A vulnerability exists in the staking mechanism where users can lose their pending rewards if they stake additional shares without first claiming their rewards. This occurs because the system resets the user's lastValuePerShare to the current valuePerShare during staking, effectively discarding any unclaimed rewards earned since their last interaction.

Vulnerability Details

Root Cause

The issue arises in the stake function, specifically during the reward accumulation process:

1. When a user stakes, the system calculates their pending rewards using the formula:

   https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/market-making/leaves/Distribution.sol#L115

   valueChange = (current valuePerShare - lastValuePerShare) * actor.shares

2. After calculating the pending rewards, the system updates the user's lastValuePerShare to the current valuePerShare.

3. If the user does not claim their pending rewards before staking again, the rewards are lost, as the lastValuePerShare is reset without transferring the earned rewards to the user.

Affected Code

The vulnerability is present in the following functions:

1. _updateLastValuePerShare:

   https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/market-making/leaves/Distribution.sol#L93

   function _updateLastValuePerShare(

   Data storage self,

   Actor storage actor,

   UD60x18 newActorShares

   )

   private

   returns (SD59x18 valueChange)

   {

   valueChange = _getActorValueChange(self, actor);

   actor.lastValuePerShare = newActorShares.eq(UD60x18_ZERO) ? int256(0) : self.valuePerShare;

   }

2. stake:

   https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/market-making/branches/VaultRouterBranch.sol#L404

   function stake(uint128 vaultId, uint128 shares) external {

   // Accumulate pending rewards

   wethRewardDistribution.accumulateActor(actorId);

   ​

   // Update staked shares

   UD60x18 updatedActorShares = ud60x18(actor.shares).add(ud60x18(shares));

   wethRewardDistribution.setActorShares(actorId, updatedActorShares);

   }

accumulateActor(actorId) return the reward of the user but this value is not used.

Example Scenario

1. User A stakes 10 shares at valuePerShare = 0.1.

2. Over time, valuePerShare increases to 0.2.

3. User A stakes again without claiming rewards:

   • Pending rewards: (0.2 - 0.1) * 10 = 1 WETH.

   • lastValuePerShare is updated to 0.2.

   • The 1 WETH reward is lost because it was not claimed before staking.

user call stake -> wethRewardDistribution.accumulateActor(actorId); -> _updateLastValuePerShare(self, actor, ud60x18(actor.shares)); -> valueChange = _getActorValueChange(self, actor);

Impact

• Loss of User Funds: Users can lose their earned rewards if they stake additional shares without claiming rewards first.

• User Frustration: Users may be unaware of the need to claim rewards before staking, leading to frustration and loss of trust in the protocol.

• Financial Loss: Depending on the size of the rewards and the number of affected users, this vulnerability could result in significant financial losses.

Tools Used

• Manual Code Review: The vulnerability was identified through a detailed review of the staking and reward distribution logic.

• Scenario Testing: Simulated user interactions to understand the impact of staking without claiming rewards.

Recommendations

1. Automatically Claim Pending Rewards During Staking

Modify the stake function to automatically claim pending rewards before updating the user's shares or revert when user have pending rewards.