Fjord Token Staking

Relevant Github Link

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L315-L318
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L419
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L375

Summary

The main issue with inconsistent state during epoch rollover is that transactions occurring around the rollover period might not be correctly attributed to the right epoch. This can lead to unfair reward distribution for users.

Vulnerability Details

Issue: The checkEpochRollover modifier is invoked before executing the main logic of the stake, stakeVested function. If an epoch rollover occurs, it updates several global state variables such as currentEpoch, totalStaked, and newStaked. However, the state of the user’s DepositReceipt is updated later in the stakeVested function. This sequence could lead to inconsistencies if the function is re-entered or if other operations depend on the previous epoch's data.

Scenario: Alice and Bob (among other users) invoke FjordStaking.stake() or FjordStaking.stakeVested() towards the end of an epoch.

Sequence of Events:

1. Alice spends some money and triggers a transaction just before the epoch ends.

2. The system starts processing Alice's transaction, which causes the epoch to roll over (move from epoch 5 to epoch 6).

3. Bob also spends money around the same time, and his transaction is processed right after Alice's. However, because the epoch has just rolled over, Bob's points would be counted in the new epoch (epoch 6), even though he intended them to be part of epoch 5.

Initial Setup:
currentEpoch = 5
totalStaked = 1000 FJORD
newStaked = 100 FJORD (from recent stakes)
totalRewards = 200 FJORD
rewardPerToken[5] = 0.2 FJORD
User A staked 800 FJORD during epoch 5, and User B staked 200 FJORD.

Phase 1: Before Epoch Rollover Begins
The contract is stable, and rewards are being calculated and distributed as expected.

Phase 2: Start of Epoch Rollover (Inconsistent State)
Action: The contract begins to transition from epoch 5 to epoch 6.

Calculations:

The contract calculates the pending rewards for epoch 5:

Let's assume:
currentBalance = 1350 FJORD;
totalVestedStaked = 150 FJORD;
newVestedStaked = 50 FJORD
Pending rewards:

Partial Update:

The contract starts updating rewardPerToken and other state variables:
rewardPerToken[6] is being calculated but not yet finalized.
totalStaked is being updated to include newStaked.
The contract sets newStaked = 0 to reset for the new epoch.

Critical Moment: User C decides to stake 100 FJORD during this transition phase.

Phase 3: User Action During Inconsistent State

Impact:

User C’s stake is processed with incomplete state updates.
If the contract uses outdated totalStaked or rewardPerToken values:
The rewards for epoch 6 might be incorrectly distributed.
totalStaked would temporarily not reflect the true total (e.g., it could still be 1000 FJORD instead of 1100 FJORD).
rewardPerToken[6] might not include the proper rewards, resulting in an unfair reward distribution.

Calculation Error:

Suppose the contract hasn't yet updated totalStaked when User C stakes:
The contract could distribute rewards for epoch 6 as if totalStaked was still 1000 FJORD, not 1100 FJORD.
User C receives fewer rewards than they should, or earlier stakers receive more than they deserve.
Phase 4: After Epoch Rollover Completes
Final State:

The contract completes the rollover, finalizing rewardPerToken[6] and updating totalStaked.
But since User C's stake was processed during the transition, their rewards would be miscalculated.

Potential Precursor Problems:

Race Conditions: The vulnerability highlights a race condition where multiple users interact with the contract around the same time, causing the epoch to roll over unexpectedly for some users.
High Activity Periods: This issue could be exacerbated during periods of high activity, where many users are staking around the same time.
Inadequate Synchronization: The lack of proper synchronization or batching of epoch rollovers could lead to inconsistent states.

Impact

Risk: The contract could end up in an inconsistent state (relative to the intended current states) if multiple transactions involving different users or streams occur simultaneously during an epoch rollover.

Tools Used

Manual Review

Recommendations

Any state dependencies between epoch rollovers and user-specific updates should be clearly separated and handled atomically, or consider locking certain operations during a rollover. Possible mitigations are as follows:

1. Separation of Concerns (SoC): Separate Staking from Epoch Rollover: Separate the logic for staking from the epoch rollover to ensure that the two processes don’t interfere with each other.

1. Atomic Operations (Most feasible): Ensure that epoch rollovers and stake updates happen atomically, so that any pending stakes are correctly attributed to the right epoch.

2. Grace Periods: Add an outsideGracePeriod modifier to block actions during the sensitive period around epoch transitions, preventing race conditions and unintended behavior.