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

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

AllTooWell

Summary

unredeemedEpoch is set to zero due to a false if condition in unstake and _unstakeVested function. Set unredeemedEpoch to 0 wrongly will lead to the deposits at currentEpoch can't translate into user's totalStaked. User will get no reward.

Vulnerability Details

FjordStaking.sol::unstake

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);
}

FjordStaking.sol::_unstakeVested

if (dr.vestedStaked == 0 && dr.staked == 0) {
// instant unstake
if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
userData[streamOwner].unredeemedEpoch = 0;
}
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}

From code above, unredeemedEpoch is set to 0 under the following conditions:

  1. The user fully unstake at this epoch(not necessarily currentEpoch).

  2. The unredeemedEpoch is equal to currentEpoch which means the user stake at currentEpoch.

In summary, if user stake at currentEpoch first and then fully unstake at any epoch, unredeemedEpoch is set to 0.
It's ok to set unredeemedEpoch to 0 if unstake epoch is currentEpoch. But it's wrong to to set unredeemedEpoch to 0 if unstake epoch isn't currentEpoch. It means the unstake on other epoch(not currentEpoch) affect the state of currentEpoch. The unredeemedEpoch is 0 which leads the if condition in _redeem is not meet. The deposits at currentEpoch will not translate into user's totalStaked.

FjordStaking.sol::_redeem

function _redeem(address sender) internal {
//1. Get user data
UserData storage ud = userData[sender];
​
ud.unclaimedRewards +=
calculateReward(ud.totalStaked, ud.lastClaimedEpoch, currentEpoch - 1);
ud.lastClaimedEpoch = currentEpoch - 1;
​
@ 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);
}
}

Impact

Set unredeemedEpoch to 0 wrongly will lead to the deposits at currentEpoch can't translate into user's totalStaked. User will get no reward.

POC

Paste code below in test/unit/unstake.t.sol.

function test_POC() public {
// before this, address(this) already stakes 10 ether, check afterSetup() function
​
// total added reward is 7 ether over 7 epochs
_addRewardAndEpochRollover(1 ether, 7);
assertEq(fjordStaking.currentEpoch(), 8);
​
// at epoch 8, stake 1 ether
fjordStaking.stake(1 ether);
// check userData
(uint256 totalStaked_1,, uint16 unredeemedEpoch_1, uint16 lastClaimedEpoch_1) =
fjordStaking.userData(address(this));
assertEq(totalStaked_1, 10 ether);
assertEq(unredeemedEpoch_1, 8);
assertEq(lastClaimedEpoch_1, 7);
​
// at epoch 8, fully unstake epoch 1
uint256 total = fjordStaking.unstake(1, 10 ether);
assertEq(total, 10 ether);
assertEq(fjordStaking.totalStaked(), 0 ether);
​
// check staking position
(uint16 epoch, uint256 staked,) = fjordStaking.deposits(address(this), 1);
assertEq(epoch, 0);
assertEq(staked, 0);
(uint16 epoch_2, uint256 staked_2,) = fjordStaking.deposits(address(this), 8);
assertEq(epoch_2, 8);
assertEq(staked_2, 1 ether);
​
// check userData
(uint256 totalStaked_2,, uint16 unredeemedEpoch_2, uint16 lastClaimedEpoch_2) =
fjordStaking.userData(address(this));
assertEq(totalStaked_2, 0 ether);
assertEq(unredeemedEpoch_2, 0);
assertEq(lastClaimedEpoch_2, 7);
​
// total added reward is 7 ether over 7 epochs
_addRewardAndEpochRollover(7 ether, 7);
assertEq(fjordStaking.currentEpoch(), 15);
​
// at epoch 15, stake 1 ether
fjordStaking.stake(1 ether);
​
// check staking position
(uint16 epoch_3, uint256 staked_3,) = fjordStaking.deposits(address(this), 8);
assertEq(epoch_3, 8);
assertEq(staked_3, 1 ether);
(uint16 epoch_4, uint256 staked_4,) = fjordStaking.deposits(address(this), 15);
assertEq(epoch_4, 15);
assertEq(staked_4, 1 ether);
​
// check userData
(uint256 totalStaked_3,, uint16 unredeemedEpoch_3, uint16 lastClaimedEpoch_3) =
fjordStaking.userData(address(this));
assertEq(totalStaked_3, 0 ether);
assertEq(unredeemedEpoch_3, 15);
assertEq(lastClaimedEpoch_3, 14);
​
// check total stake and total new stake
assertEq(fjordStaking.totalStaked(), 1 ether);
assertEq(fjordStaking.newStaked(), 1 ether);
}

At epoch 15, user's lastClaimedEpoch is 14 and user's deposit amount at epoch 8 is 1 ether. However user's totalStaked is 0 at epoch 15. But totalStaked of stake contract is 1 ether at epoch 15.

Tools Used

manual and foundry

Recommendations

FjordStaking.sol::unstake

// no longer a valid unredeemed epoch
- if (userData[msg.sender].unredeemedEpoch == currentEpoch) {
+ if (userData[msg.sender].unredeemedEpoch == _epoch) {
userData[msg.sender].unredeemedEpoch = 0;
}

FjordStaking.sol::_unstakeVested

// instant unstake
- if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
+ if (userData[streamOwner].unredeemedEpoch == data.epoch) {
userData[streamOwner].unredeemedEpoch = 0;
}
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.