DeFiFoundry
20,000 USDC
View results
Submission Details
Severity: high
Valid

A User will lose their staking rewards for current epoch and the rewards will be stuck in the `FjordStaking` contract when the user unstakes completely the position from the previous epoch

Summary

The incorrect update of unredeemedEpoch at FjordStaking:L481,L543 will cause loss and stuck of staking rewards in current epoch for a user as the user unstakes completely the position from the previous epoch.

Vulnerability Details

When a user unstakes completely a position (both normal Fjord tokens and vested Fjord tokens) from the previous epoch, there is an update of unredeemedEpoch

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L480-L482
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L542-L544

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) {
>> 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) {
>> userData[streamOwner].unredeemedEpoch = 0;
}
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
...
}

The unredeemedEpoch will be set to zero when the user unstakes completely from the previous epoch and they have a staking position in the current epoch. By setting unredeemedEpoch to zero, the current position can not accure staking rewards if the rewards are added in the future epoch (note that, this position can only accrue rewards if the rewards are added in the currentEpoch+1) because of the check at

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L737

function _redeem(address sender) internal {
//1. Get user data
UserData storage ud = userData[sender];
ud.unclaimedRewards +=
calculateReward(ud.totalStaked, ud.lastClaimedEpoch, currentEpoch - 1);
ud.lastClaimedEpoch = currentEpoch - 1;
>> if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) {
// 2. Calculate rewards for all deposits since last redeemed, there will be only 1 pending unredeemed epoch
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
// 3. Update last redeemed and pending rewards
ud.unclaimedRewards += calculateReward(
deposit.staked + deposit.vestedStaked, ud.unredeemedEpoch, currentEpoch - 1
);
ud.unredeemedEpoch = 0;
ud.totalStaked += (deposit.staked + deposit.vestedStaked);
}
}

Add this PoC to test/PoC/POC.t.sol

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity =0.8.21;
import "../FjordStakingBase.t.sol";
contract StakeRewardScenarios is FjordStakingBase {
function afterSetup() internal override {
deal(address(token), bob, 10000 ether);
vm.prank(bob);
token.approve(address(fjordStaking), 10000 ether);
}
function testPoC() public {
// First epoch
vm.prank(alice);
fjordStaking.stake(1 ether);
skip(fjordStaking.epochDuration());
// Second epoch
vm.prank(bob);
fjordStaking.stake(1 ether);
// Skip 6 epochs
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
// Eighth epoch
vm.startPrank(alice);
// Alice stakes in the current epoch
fjordStaking.stake(1 ether);
// This is where the bug occurs
// `userData[alice].unredeemedEpoch` will be set to zero
fjordStaking.unstake(1, 1 ether);
vm.stopPrank();
skip(fjordStaking.epochDuration());
// Ninth epoch
vm.prank(bob);
fjordStaking.stake(1 ether);
skip(fjordStaking.epochDuration());
// End of the ninth epoch / Start of the tenth epoch
// The protocol add rewards
// The rewards should be distributed from the eighth epoch to the first epoch
vm.prank(minter);
fjordStaking.addReward(2 ether);
{
vm.expectRevert(FjordStaking.NothingToClaim.selector);
vm.prank(alice);
fjordStaking.claimReward(false);
}
{
vm.prank(bob);
fjordStaking.claimReward(false);
(uint256 epoch, uint256 amount) = fjordStaking.claimReceipts(bob);
console.log("Bob's rewards: %e", amount);
}
}
}

Logs:

Bob's rewards: 1e18

The protocol added 2e18 reward tokens, Bob only received 1e18 tokens and Alice received nothing. The correct behavior should be Bob receives 1e18 tokens and Alice receives 1e18 tokens.

Note that, the rewards form Alice will be stuck in the FjordStaking contract forever, no one can claim the rewards, the protocol can not redistribute the rewards. Because during epoch rollover (the ninth epoch in the above example), the amount of FJORD tokens in the position of current epoch of Alice is still accounted in newStaked, which will added to totalStaked. Then when the protocol add the staking rewards (start of the tenth epoch in the above example), the rewards will be distributed to totalStaked, which includes Alice's position.

Impact

  • The user, who unstakes the position from previous epoch, will lose their staking rewards for the current epoch.

  • The rewards will be stuck in the FjordStaking contract and lost forever.

Tools Used

Manual Review.

Recommendations

Having the unredeemedEpoch not set to zero even when a user unstakes completely from the position in the current epoch is not a problem, because deposit.staked + deposit.vestedStaked will be zero in that case, and then ud.unclaimedRewardswill not be increased.

function _redeem(address sender) internal {
//1. Get user data
UserData storage ud = userData[sender];
ud.unclaimedRewards +=
calculateReward(ud.totalStaked, ud.lastClaimedEpoch, currentEpoch - 1);
ud.lastClaimedEpoch = currentEpoch - 1;
if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) {
// 2. Calculate rewards for all deposits since last redeemed, there will be only 1 pending unredeemed epoch
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
// 3. Update last redeemed and pending rewards
>> ud.unclaimedRewards += calculateReward(
deposit.staked + deposit.vestedStaked, ud.unredeemedEpoch, currentEpoch - 1
);
ud.unredeemedEpoch = 0;
ud.totalStaked += (deposit.staked + deposit.vestedStaked);
}
}

Because of that, our recommendation is not set unredeemedEpoch to zero in any case

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) {
- 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) {
- userData[streamOwner].unredeemedEpoch = 0;
- }
delete deposits[streamOwner][data.epoch];
_activeDeposits[streamOwner].remove(data.epoch);
}
...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

3n0ch Submitter
10 months ago
inallhonesty Lead Judge
10 months ago
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.