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

Inconsistent Handling of dr.staked in the `unstakeAll` Function

Summary

dr.staked is not handled correctly, leading to incorrect calculations of the total staked amount.

Vulnerability Details

The current implementation of the unstakeAll function could lead to incorrect calculations of the total staked amount, particularly when a user stakes and then tries to unstake within the same epoch. This issue arises because the function includes the most recent stake (from the current epoch) in the total staked amount calculation, even though the function’s logic prevents users from unstaking their latest stake due to a lock cycle check. This inconsistency could lead to incorrect token transfers and a misrepresentation of the actual staked amounts.

Proof of Concept:

The unstakeAll function is designed to prevent users from unstaking their tokens within the same epoch they were staked in. This is achieved through a check that ensures users can only unstake from epochs that are at least one epoch old:

if (dr.epoch == 0 || currentEpoch - epoch <= lockCycle) continue;

However, the dr.staked value, which represents the total amount staked by a user in a specific epoch, includes all stakes, including those made in the current epoch. When a user stakes in the latest epoch, dr.staked is updated to include the newly staked amount:

if (dr.epoch == 0) {
dr.staked = _amount;
dr.epoch = currentEpoch;
_activeDeposits[msg.sender].add(currentEpoch);
} else {
dr.staked += _amount;
}

This means that dr.staked reflects the total staked amount, including the most recent stake from the current epoch. However, when calculating the totalStakedAmount across all epochs, the function includes this latest stake, despite the fact that users are not allowed to unstake from the current epoch due to the lock cycle:

totalStakedAmount += dr.staked; // `dr.staked` includes the latest stake from the user

As a result, the totalStakedAmount erroneously includes the staked amount from the latest epoch. When the function proceeds to reduce the totalStaked amount:

totalStaked -= totalStakedAmount;

It inaccurately subtracts a value that includes the latest stake, leading to an incorrect calculation of the totalStaked. Furthermore, when the final transfer occurs, the user receives a totalStakeAmount that wrongly includes the stake from the latest epoch, which should have been excluded based on the lock cycle check.

Impact

incorrect calculations of the total staked amount.

Tools Used

manual review

Recommendations

To address this issue, the function should ensure that the dr.staked value does not include the amount staked in the current epoch when calculating totalStakedAmount. This can be achieved by adjusting the logic to exclude the latest stake from the calculation if it falls within the current epoch.

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.