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

Unnecessary Zero Amount External calls in unstakeAll()

robinx33

Summary

When a user calls the stake() function, it will give them the activeDeposit, with that active deposit they can go through the unstakeAll(), but since they have not met the lockcycle requirement, totalStakedAmount won't get added with dr.staked, If that was the user's only activeDeposit or 1-3 (early) of them, The function will continue and begin adding zero values to (userData[msg.sender].totalStaked ,totalStaked) and also send zero amount transfers/external calls + Unnecessary events (including duplicate events) on onUnstaked and unstakeAll

Vulnerability Details

https://github.com/Cyfrin/2024-08-fjord/blob/main/src/FjordStaking.sol#L570

There is no Zero Amount check after the iteration is done inside the function

/// @notice Unstake from all epochs.
/// @dev This function allows users to unstake from all the epochs at once,
/// while also claiming all the pending rewards.
/// @return totalStakedAmount The total amount that has been unstaked.
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;
}
}
@> // no check here for totalStakedAmount being zero
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)
);
}

The if (activeDeposits.length == 0) revert NoActiveDeposit() is meant to remove the users who have never staked (Have no activedeposits), but there should also be a revert option for legit users trying to claim early to save their costs and clarity.

Impact

Users may become confused if they attempt to unstake tokens and the transaction appears successful without any feedback that it was not meant to be that way, Without an error message if they don't see their funds going through, they might try to call the function again which would also waste their costs with the external calls, its better to revert early if you have zero amount.

Likelihood: Medium ( users will call unstakeAll early)

Impact: Medium (occur pointless costs from the user)

Tools Used

Manual Review

Recommendations

Consider adding a check in unstakeAll() that if the totalStakedAmount is still zero after the For Loop, it should revert

This should also help Events in Providing Accurate Information

+ if(totalStakedAmount == 0) revert NoAmountToTransfer();
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)
);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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