Fjord Token Staking

Summary

In FjordStaking and FjordPoints, the epoch times are calculated based on when the constructor is initialized(when the contracts are deployed). This leads to a mismatch in epoch time. Which allows users to stake and unstake immediately and still claim rewards.

Vulnerability Details

Assume that FjordPoints is deployed 20 blocks before FjordStaking. This will make the lastDistribution in FjordPoints vary from the startTimein FjordStaking by 20 blocks. This mismatch would lead to the subsequent epochs ending time to also vary by 20 blocks (since the epoch durations are the same i.e 1 week).

(https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L281-L293)

(https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordPoints.sol#L118-L120)

The problem arises because eventhough the FjordStaking epoch remains the same, the FjordPoints epoch gets updated to the next epoch.

FjordPoints: | --------------------X---|---Y----------------------|-------------------------|---------

FjordStaking: |---------------------------|-------------------------|------------------------|

(Here the vertical lines represent the epoch changes, the time between any 2 vertical lines is 1 week, and the mismatch in the first vertical line of both the contracts is due to different deployment times)

Scenario

1. User calls the stake() function at time = X (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L368-L391)

2. User calls the unstake() function at time = Y (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L449-L466)

   1. Note that the unstake can be done immediately since the epoch in FjordStaking hasnt changed

      // _epoch is same as current epoch then user can unstake immediately

      if (currentEpoch != _epoch) {

      // _epoch less than current epoch then user can unstake after at complete lockCycle

      if (currentEpoch - _epoch <= lockCycle) revert UnstakeEarly();

      }

3. The unstake() function internally calls the points.onUnstaked() function in FjordPoints. The onUnstaked() function updates the rewards in the FjordPoints contract, by calling the checkDistribution() and updatePendingPoints(user) functions.

   (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordPoints.sol#L214-L227)

   1. checkDistribution() : Here since the epoch has been updated in the FjordPoints contract, the rewards of the week are distributed. (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordPoints.sol#L232-L248)

      function distributePoints() public {

      // This check passes since the epoch duration has passed in this contract

      if (block.timestamp < lastDistribution + EPOCH_DURATION) {

      return;

      }

      // rest of the code

   2. updatePendingPoints(user) : Here the user's pending points is updated to include the reward for this week. (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordPoints.sol#L146C14-L153)

4. User calls the claimPoints() function in FjordPoints, and gets his FjordPoints as reward.

Therefore the user has received the reward for the complete week just by being staked for (Y - X ) time. This (Y - X) time can be equal to 1 block too. So the user is getting rewarded unfairly which is not the intention of the protocol.

Note: To substantiate the claim that both the contracts, FjordPoints and FjordStaking will not be deployed on the same block, i have attached below the links for their testnet deployments. Here we can see that they are deployed 10 blocks apart from each other.

FjordPoints (block number : 71179313): https://sepolia.arbiscan.io/address/0x5833678d5a7feb297ab472913fe155082580b044

FjordStaking (block number : 71179323): https://sepolia.arbiscan.io/address/0x3479951f4f6f422df170df1aedb3cd69ee472816

These contract addresses are got from their documentation: (2024-08-fjord/deployments/421614-fjord-arbitrum_sepolia.json at main · Cyfrin/2024-08-fjord (github.com)

POC

In the POC i have used the case where (Y - X) == 20 blocks. This is done for simplicity.

Steps to run

Replace the /test/FjordStakingBase.t.sol with the below code.

OUTPUT

This shows that Bob received his reward after just staking for 10 blocks

Impact

The user will never have to stake more than 1 block, and can still recieve all the FjordPoints of the week as reward.

Tools Used

Manual Review

Recommendations

There are multiple ways to mitigate this issue :

1. Make sure that both the contracts are deployed in the same transaction. (FjordPoints first, sicne FjordStaking needs address(FjordPoints))

2. Consider having the same variable for epoch/startTime in both the contracts. For example: FjordPoints epoch can be calculated after taking the startTime from FjordStaking.