DeFiFoundry
20,000 USDC
View results
Submission Details
Severity: medium
Invalid

Potential Loss of Staked Tokens Due to Incorrect Handling in unstakeAll Function

Summary

Potential Loss of Staked Tokens Due to Incorrect Handling in unstakeAll Function

Vulnerability Details

Based on the current implementation of the unstake function it is not possible for users to unstake immediately, if (dr.epoch == 0 || currentEpoch - epoch <= lockCycle) continue; The unstakeAll function in the staking contract has a critical flaw that can result in the unintentional loss of staked tokens for the latest epoch. The function attempts to zero out the dr.staked value even when a user’s stake is within the latest/current epoch. This results in the user's tokens being effectively erased without any transfer or refund, leading to a permanent loss of funds.

this issue can affect all users who stake in the latest epoch and then use the unstakeAll function, expecting their funds to be safely handled. Instead, their stake may be nullified, and they could lose their tokens without any way to recover them.

// 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; // @audit current epoch?
}

setting dr.staked = 0; means that if a user has staked in the latest epoch all the stake will be set to 0

Proof of Concept (PoC):

Staking Tokens in the Current Epoch:

  • Assume the current epoch is Epoch X.

  • A user stakes 100 FJORD tokens during Epoch X.

  • The user's dr.staked is updated with 100 FJORD tokens for Epoch X.

  • The user decides to call the unstakeAll function in the same Epoch X.

  • The unstakeAll function iterates over all active deposits.

  • When it reaches the deposit for Epoch X, it checks if the epoch is within the lock cycle using the condition currentEpoch - epoch <= lockCycle.
    Because this condition is true, the function should skip any unstake operation for Epoch X.

  • However, due to flawed logic, it sets dr.staked = 0 when dr.vestedStaked == 0. now user has no stake in the latest epoch even though he staked initially

  • The user's stake of 100 FJORD tokens is zeroed out (dr.staked = 0), but since the function skips the unstake operation, the user does not receive any tokens back.

  • The 100 FJORD tokens are effectively lost, as they are neither locked nor returned to the user.

Impact

loss of tokens

Tools Used

manual review

Recommendations

To prevent this loss of funds, the logic in the unstakeAll function should be modified to ensure that stakes in the current epoch are not zeroed out if they are still within the lock cycle. Specifically, the condition that sets dr.staked = 0 should include a check to verify that the epoch being processed is not the current epoch.

Here’s an updated version of the relevant logic:

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

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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