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

`_unstakeVested` will delete a user normal stake and brick his funds

pyro

Impact

  1. Direct loss of funds for users who withdraw their NFTs

Summary

_unstakeVested deletes a user's stake (not a vested one) when they unstake their NFT.

Vulnerability Details

When calling stake or stakeVested, these functions always set userData.unredeemedEpoch to currentEpoch.

userData[msg.sender].unredeemedEpoch = currentEpoch;

This is necessary because the next time the user interacts with the contract, they trigger _redeem (through the redeemPendingRewards modifier). If userData.lastClaimedEpoch is set, the system will account for the user's stake, reward it, increase userData.totalStaked by it. Note that this only occurs in the next epoch because ud.unredeemedEpoch < currentEpoch.

if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) {
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
​
ud.unclaimedRewards += calculateReward(
deposit.staked + deposit.vestedStaked, ud.unredeemedEpoch, currentEpoch - 1
);
​
ud.unredeemedEpoch = 0;
ud.totalStaked += (deposit.staked + deposit.vestedStaked);
}

If userData[msg.sender].unredeemedEpoch is accidentally set to 0 before _redeem is triggered in the next epoch, userData.totalStaked will not increase, and the user's stake will be esentially be deleted.

This is exactly what happens inside _unstakeVested. The function incorrectly assumes that the NFT is being withdrawn in the same epoch it was staked, causing any normal or vested stakes in this epoch to be deleted.

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
NFTData storage data = _streamIDs[streamOwner][_streamID];
​
// Note that `dr` is using `data.epoch` and not the current epoch
// `data.epoch` is the epoch this original NFT was staked in
DepositReceipt storage dr = deposits[streamOwner][data.epoch];
if (amount > data.amount) revert InvalidAmount();
​
bool isFullUnstaked = data.amount == amount;
uint16 epoch = data.epoch;
​
dr.vestedStaked -= amount;
if (currentEpoch != data.epoch) {
totalStaked -= amount;
totalVestedStaked -= amount;
userData[streamOwner].totalStaked -= amount;
} else {
newStaked -= amount;
newVestedStaked -= amount;
}
​
// If there are 0 stakes for that epoch,
// userData[streamOwner].unredeemedEpoch will be set to 0
// even thought that `deposits[streamOwner][currentEpoch]` may be > 0
if (dr.vestedStaked == 0 && dr.staked == 0) {
if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
userData[streamOwner].unredeemedEpoch = 0;
}
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
}

Example:

  1. A user stakes an NFT at epoch X.

  2. At epoch X+10, the user stakes 1000 tokens using stake.

  3. stake sets userData.unredeemedEpoch = X+10 and deposits[msg.sender][X+10] = 1000.

  4. The user withdraws their NFT, and userData.unredeemedEpoch is set to 0.

  5. In the next epoch, when the user interacts with the contract to trigger _redeem, their totalStaked balance is not increased.

As a result, the user's funds are stuck inside the contract without even generating rewards. The user can call unstake after the lock period, but since totalStaked is still 0, and unstake decreases it, the TX would revert:

dr.staked -= _amount;
if (currentEpoch != _epoch) {
totalStaked -= _amount;
// The user's totalStaked will be 0
userData[msg.sender].totalStaked -= _amount;
} else {
newStaked -= _amount;
}

Root Cause

The second if condition inside _unstakeVested incorrectly assumes that the user staked during the current epoch and sets his unredeemedEpoch to 0.

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

Tools Used

Manual review.

Recommendations

Do not reset unredeemedEpoch inside _unstakeVested. If the user has 0 vestedStaked and 0 staked, then _redeem would not perform any action.

if (dr.vestedStaked == 0 && dr.staked == 0) {
- if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
- userData[streamOwner].unredeemedEpoch = 0;
- }
​
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
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.