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

The `unstakeAll()` function can delete active `dr.staked` tokens because only check for `dr.vestedStaked == 0`

StraawHaat

Summary

The unstakeAll() function can delete dr.staked tokens because only check for dr.vestedStaked == 0. This happens in this code block:

// no vested staked and stake is 0 then delete the deposit
if (dr.vestedStaked == 0) {
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
}

You can see that there is no check if dr.staked == 0 as in the unstake() function.

Vulnerability Details

The unstakeAll function allow users to unstake all their staked FJORD tokens from all active epochs at once. This function simplifies the unstaking process by allowing users to withdraw all their staked tokens without having to manually specify each epoch individually.

function unstakeAll()
external
checkEpochRollover
redeemPendingRewards
returns (uint256 totalStakedAmount)
{
uint256[] memory activeDeposits = getActiveDeposits(msg.sender);
if (activeDeposits.length == 0) revert NoActiveDeposit();
​
for (uint16 i = 0; i < activeDeposits.length; i++) {
uint16 epoch = uint16(activeDeposits[i]);
DepositReceipt storage dr = deposits[msg.sender][epoch];
​
if (dr.epoch == 0 || currentEpoch - epoch <= lockCycle) continue;
totalStakedAmount += dr.staked;
// no vested staked and stake is 0 then delete the deposit
if (dr.vestedStaked == 0) { /
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
} else {
// still have vested staked, then only delete the staked
dr.staked = 0;
}
}
​
totalStaked -= totalStakedAmount;
userData[msg.sender].totalStaked -= totalStakedAmount;
​
fjordToken.transfer(msg.sender, totalStakedAmount);
points.onUnstaked(msg.sender, totalStakedAmount);
​
// emit event
emit UnstakedAll(
msg.sender, totalStakedAmount, activeDeposits, getActiveDeposits(msg.sender)
);
}

We're interested in the following code block:

// no vested staked and stake is 0 then delete the deposit
if (dr.vestedStaked == 0) {
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
}

The current logic checks if there are no vested tokens staked (dr.vestedStaked == 0). If this condition is true, it deletes the deposit from deposits[msg.sender][epoch] and removes the epoch from _activeDeposits.

The problem here is that the current code only checks for dr.vestedStaked == 0 but does not check dr.staked == 0, there could be a scenario where dr.staked is non-zero. This could lead to an incomplete cleanup, where the deposit record is deleted even though there are still non-vested tokens (dr.staked > 0) associated with that epoch.

Impact

The user will lose access to their staked tokens if the deposit record is deleted prematurely.
This would not only result in a loss of tokens but also prevent the user from earning rewards on those tokens and from unstaking them in the future.

Tools Used

Visual Studio Code

Recommendations

The logic should check both dr.staked == 0 and dr.vestedStaked == 0 before deleting the deposit.

if (dr.vestedStaked == 0 && dr.staked == 0) {
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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