Fjord Token Staking

Summary

The FjordStaking contract implements an epoch-based staking system supporting regular and vested token staking via Sablier. It includes mechanisms for stake management, reward distribution, and early unstaking penalties. However, the contract's reward distribution mechanism introduces a significant delay in reward accrual for new stakers, leading to an uneven distribution of rewards that favors early participants.

Vulnerability Details

The core issue in the FjordStaking contract lies in how new stakes are handled and how rewards are calculated across epochs.

When a user stakes tokens, their stake is initially recorded in newStaked:

The newStaked value is then only added to the totalStaked when a new epoch starts:

However, the pendingRewardsPerToken is calculated only over the totalStaked value:

Finally, the actual unclaimedReward calculation happens in the redeem(...) function and is based on the difference between rewardsPerToken for the epoch between it is being calculated. Users have a separate userData structure, which also holds totalStaked amount, which again is only updated after a successful redeem:

From the above we can come to the following scenario:

1. Alice stakes1e18 tokens in epoch 1. Her newStaked = 1e18 and totalStaked = 0. Her lastClaimEpoch is set to 0 and the ud.unredeemedEpoch, lastEpochRewarded is also 0.

2. No new rewards are added in this epoch and a new epoch starts.

3. Bob now stakes 1e18, so the _checkEpochRollover now indicates a change in epochs and sets the rewardPerToken[epoch1] = 0 (as totalStaked is still 0, we don't have pendingReward calculations so the rewardPerToken is not changed, and it is initially 0).

4. Alice's newStaked is moved to totalStaked.

5. The redeem() function doesn't make any changes for Bob, as if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) is false, so his UserData.totalStaked does not indicate a change.

6. Bob's lastClaimedEpoch is set to 1 and the lastEpochRewarded is set to 1.

7. 1e18 tokens are added as a reward to this epoch. No reward calculations happen as if (latestEpoch > currentEpoch) will be false in _checkEpochRollover().

8. A new epoch starts.

9. No new stakes happen and a new reward of 1e18 is added. lastClaimedEpoch now changes the protocol state.

10. The currentBalance will be 4e18, with totalStaked = 1e18, newStaked = 1e18, so the pendingRewards = 2e18.

11. This sets the rewardPerToken[epoch2] = 2e18 and sets the lastEpochRewarded = 2.

12. Bob's newStaked goes to the totalStaked = 2e18.

13. A new epoch starts and Alice and Bob want to claim their rewards.

14. claimRewards(...) also invokes _checkEpochRollover(...), where we do not have changes in the currentBalanceand staked amounts, so nowpendingRewards = 0, which sets the rewardPerToken[epoch3] = 2e18, as it is set to equal rewardPerToken[lastEpochRewarded] + pendingRewardsPerToken. lastEpochRewardedis now set to3`.

15. In the rewards calculation for Alice we have:

1. Now for Bob we have:

1. From the above we can see that Alice has claimed all of the rewards and Bob has nothing left to claim and his claimRewards(...) call will revert.

2. It will only be on the next reward addition, that Bob will start accumulating rewards and that the proportion of rewards becomes equal between users.

Impact

Early stakers gain an unfair advantage over users who start staking at a later stage, which could lead to reduced participation in the protocol.

Tools Used

Manual review

Recommendations

This issue is not that straightforward to fix, as it will require some functionalities to change. Some ideas include:

1. Modify _checkEpochRollover() to include newStaked when calculating the pendingRewardsPerToken value.

2. Add a snapshot mechanism so that calculations can take in account user's stakes at every point in the epoch.