Fjord Token Staking
Fjord
DeFiFoundry
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

A DOS of unstakeAll will happen if the user stake and unstake,in the same epoch

0xphantom

Summary

When a user stakes or unstakes, their total staked amount is updated if and only if their unredeemed epoch is higher than 0 and less than the current epoch.

The problem occurs when a user has a stake amount in a previous epoch and then stakes another amount and unstakes the amount from the previous epoch in the same epoch.

When they stake the new amount, their unredeemed epoch will be set to the current epoch, and the total staked amount will be updated. However, when they unstake, the unredeemedEpoch will be set to 0 because of this line of code.

Vulnerability Details

Lets Imagine a user stake 1 fjord Token his unredeemedEpoch will be set to the current epoch as we can see here in the stake function (L-373) :

userData[msg.sender].unredeemedEpoch = currentEpoch;

After 8 weeks and 8 epochs, they decide to stake 1 more token. This code in the _redeem function (L-729) will be triggered:

if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) {
// 2. Calculate rewards for all deposits since last redeemed, there will be only 1 pending unredeemed epoch
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
​
// 3. Update last redeemed and pending rewards
ud.unclaimedRewards += calculateReward(
deposit.staked + deposit.vestedStaked, ud.unredeemedEpoch, currentEpoch - 1
);
​
ud.unredeemedEpoch = 0;
ud.totalStaked += (deposit.staked + deposit.vestedStaked);
}

As we can see, their totalStaked will be set to 1e18, and at the end of the call, the unredeemedEpoch will be set again to the current epoch. Let’s imagine that in the same epoch, they decide to unstake the first deposit.

They will call the unstake function (L-449), but because of this, the if statement will be triggered since the unredeemedEpoch equals the current epoch and the deposit is empty:

if (dr.staked == 0 && dr.vestedStaked == 0) {
// no longer a valid unredeemed epoch
if (userData[msg.sender].unredeemedEpoch == currentEpoch) {
userData[msg.sender].unredeemedEpoch = 0;
}
delete deposits[msg.sender][_epoch];
_activeDeposits[msg.sender].remove(_epoch);
}

The unredeemedEpoch will be set to 0. The problem occurs here because the deposit will never be added to the totalStaked, as the if statement in the _redeem function will prevent it.

So when they use unstakeAll, it will never work due to an underflow, where the sum of all the active deposits will be higher than the totalStaked.

userData[msg.sender].totalStaked -= totalStakedAmount;
​

You can run this test in the StakeRewardScenarios contract in the stakeReward.t.sol file

function test_DOS_UnstakeAll() public {
vm.warp(block.timestamp + 7286594);
vm.roll(block.number + 2);
fjordStaking.stake(1);
vm.warp(block.timestamp + 4803108);
vm.roll(block.number + 1);
fjordStaking.stake(1);
fjordStaking.unstake(13, 1);
vm.warp(block.timestamp + 3637164);
vm.roll(block.number + 1);
vm.expectRevert();
fjordStaking.unstakeAll();
​
}

Impact

The user will not be able to use the unstakeAll function and will not be able to unstake this deposit. He will not earn any reward for it.

Tools Used

Echidna

Recommendations

I think that the best way to avoid this revert is to remove the if statement in the unstake function. This is even more important because this check doesn’t exist in the unstakeAll function.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`unredeemedEpoch` is set to zero due to a false if condition in `unstake` and `_unstakeVested` function

Users that try to unstake tokens from an earlier epoch after they staked in the current epoch will have their ` unredeemedEpoch` set to 0, leading to them being unable to access the newly staked tokens. Impact: High – Tokens are lost Likelihood: Medium – It happens every time a user performs the respective sequence of calls. It’s not always but it’s also not a low likelihood scenario. It’s normal usage of the protocol, that doesn’t necessarily require special conditions.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.