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) {
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) {
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 {
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) {
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
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
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 {
vm.prank(alice);
fjordStaking.stake(1 ether);
skip(fjordStaking.epochDuration());
vm.prank(bob);
fjordStaking.stake(1 ether);
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
skip(fjordStaking.epochDuration());
vm.startPrank(alice);
fjordStaking.stake(1 ether);
fjordStaking.unstake(1, 1 ether);
vm.stopPrank();
skip(fjordStaking.epochDuration());
vm.prank(bob);
fjordStaking.stake(1 ether);
skip(fjordStaking.epochDuration());
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:
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.unclaimedRewards
will not be increased.
function _redeem(address sender) internal {
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) {
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
>> 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);
}
...
}