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

Loss of funds for a user due to incorrect state updates while unstaking

PerseusLabs

Summary

In the staking contract, users can withdraw their staked tokens after a 6-epoch waiting period if unstaking from any of the past epochs, or immediately if unstaking from the current epoch. However, a critical issue arises when users attempt to unstake tokens from earlier epochs while they have previously staked in the current epoch. In this scenario, the contract fails to correctly update the unredeemedEpoch variable. This malfunction results in users losing access to their staked funds and accumulated rewards.

Vulnerability Details

FjordStaking.sol#L478

FjordStaking.sol#L540

The problem arises in the FjordStaking::unstake() and FjordStaking::_unstakeVested() when user requests unstake of fully amount (dr.staked and dr.vestedStaked will be 0) from any of the previous epochs, while previously staked in the current epoch. More precisely, in this scenario, userData[userAddress].unredeemedEpoch is set to 0, even tough user is unstaking from previous epoch and not the current one.

Specifically, when a user attempts to unstake tokens from an earlier epoch (i.e., any epoch other than the current one) after having staked additional tokens in the current epoch, the contract erroneously resets unredeemedEpoch to 0.

The root cause of the issue lies in the logic that does not differentiate between unstaking from the current epoch and previous epochs, leading to the unintended reset of the unredeemedEpoch variable.

PoC

Place the following test in test/unit/unstake.t.sol:

function testLossOfFunds() public {
address user=makeAddr("user");
deal(address(token),user,2 ether);
​
uint256 epoch = fjordStaking.currentEpoch();
console.log("First stake epoch:", epoch);
​
vm.startPrank(user);
token.approve(address(fjordStaking), 1 ether);
fjordStaking.stake(1 ether);
vm.stopPrank();
​
vm.startPrank(minter);
for (uint256 i = 0; i < 7; i++) {
vm.warp(vm.getBlockTimestamp() + fjordStaking.epochDuration());
fjordStaking.addReward(1 ether);
}
vm.stopPrank();
​
uint256 epochAfter = fjordStaking.currentEpoch();
console.log("Second stake epoch: ", epochAfter); //After the second stake, user also unstakes firstly stacked tokens
​
vm.startPrank(user);
token.approve(address(fjordStaking), 1 ether);
fjordStaking.stake(1 ether); // so user stakes tokens in current epoch and then unstakes from the first epoch
fjordStaking.unstake(1, 1 ether);
vm.stopPrank();
​
vm.warp(vm.getBlockTimestamp() + 7 * fjordStaking.epochDuration());
vm.prank(minter);
fjordStaking.addReward(1 ether);
epochAfter = fjordStaking.currentEpoch();
​
vm.prank(user);
vm.expectRevert();
// the next line reverts due to arithmetic underflow in the following line
// <https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L472>
fjordStaking.unstake(8,1 ether);
}
​

Impact

The impact of this vulnerability is significant, as it prevents users from accessing their staked tokens and accumulated rewards which will remain locked in the FjordStaking contract. This means that users lose access to newly staked tokens and any rewards that will be earned in the future epochs, potentially leading to a substantial financial loss.

Tools Used

Manual code review / Foundry tests

Recommendations

Add checks that will take into account that unredeemedEpoch should be set to 0 only if the currentEpoch == _epoch :

function unstake(uint16 _epoch, uint256 _amount)
external
checkEpochRollover
redeemPendingRewards
returns (uint256 total)
{
(...)
if (dr.staked == 0 && dr.vestedStaked == 0) {
// no longer a valid unredeemed epoch
- if (userData[msg.sender].unredeemedEpoch == currentEpoch) {
+ if (userData[msg.sender].unredeemedEpoch == currentEpoch && _epoch == currentEpoch) {
userData[msg.sender].unredeemedEpoch = 0;
}
delete deposits[msg.sender][_epoch];
_activeDeposits[msg.sender].remove(_epoch);
}
(...)
}
​
function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
(...)
​
if (dr.vestedStaked == 0 && dr.staked == 0) {
// instant unstake
- if (userData[streamOwner].unredeemedEpoch == currentEpoch) {
+ if (userData[streamOwner].unredeemedEpoch == currentEpoch && data.epoch == currentEpoch) {
userData[streamOwner].unredeemedEpoch = 0;
}
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
(...)
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months 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.