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

Mismatch Between Pledge Call and Completion Can Lock Funds Without Reward, Leading to Potential User Losses

joicygiore

Summary

There is no difference in rewards between users who complete their stake one second before the end of the current epoch and those who stake at the beginning of the current epoch. The calculation of rewards only begins in the next epoch after the stake() function is called.A user aiming to maximize their rewards may choose to execute stake() just before an epoch ends. However, due to the time difference between the call and the actual execution, there is a risk that the stake() transaction could be completed after the epoch has flipped. In such a case, the user’s funds would be locked for nearly an entire epoch without receiving any reward.

Vulnerability Details

The FjordStaking::_checkEpochRollover() function is responsible for rolling over to the next epoch. The relevant code snippet is provided below, with the critical parts marked by @>. This function calls FjordStaking::getEpoch() to determine the current epoch. The epoch rollover relies on the calculation uint16((_timestamp - startTime) / epochDuration), which rounds down to the nearest integer multiple of the epoch duration. As a result, There is no difference in rewards between users who complete the pledge one second before the end of the current epoch and those who complete the pledge at the beginning of the current epoch.

// FjordStaking::_checkEpochRollover()
function _checkEpochRollover() internal {
@> uint16 latestEpoch = getEpoch(block.timestamp);
​
​
@> if (latestEpoch > currentEpoch) {
//Time to rollover
currentEpoch = latestEpoch;
//SNIP...
​
}
}
// FjordStaking::getEpoch()
function getEpoch(uint256 _timestamp) public view returns (uint16) {
if (_timestamp < startTime) return 0;
@> return uint16((_timestamp - startTime) / epochDuration) + 1;
}

Consider the following scenario:

  1. Alice stakes her tokens at the beginning of an epoch.

  2. Bob stakes his tokens just one second before the epoch ends.
    Despite the vast difference in their staking durations, both Alice and Bob will be locked for 6 epochs and will receive the same rewards. Bob's effective staking time is 604,799 seconds less than Alice's, But there is no difference in rewards they receive.

Poc

Please add the code to test/unit/unstake.t.sol and execute it:

function test_stake_one_second() public {
// epoch 1
vm.prank(minter);
fjordStaking.addReward(1e18);
// cache alice's staking time
uint256 alice_stake_time = block.timestamp;
// alice calls stake()
vm.prank(alice);
fjordStaking.stake(10 ether);
// Adjust the time to 1 second to enter epoch 2
vm.warp(block.timestamp + (7 days - 1));
// cache bob's staking time
uint256 bob_stake_time = block.timestamp;
// bob calls stake()
deal(address(token), bob, 10000 ether);
vm.startPrank(bob);
token.approve(address(fjordStaking), 10000 ether);
fjordStaking.stake(10 ether);
vm.stopPrank();
// Adjust time to enter epoch 2
vm.warp(block.timestamp + 1);
vm.prank(minter);
fjordStaking.addReward(1e18);
// epoch 3-7
for (uint256 i = 0; i < 5; i++) {
// Add reward and move to next 1 epoch
_addRewardAndEpochRollover(1 ether, 1); // epoch 3 - 7
}
​
// One second before entering epoch 8, the call to unStake() fails.
vm.warp(block.timestamp + (7 days - 1));
console.log("currnetEpoch:",fjordStaking.getEpoch(block.timestamp),"calls unStake() fails");
vm.startPrank(alice);
vm.expectRevert(FjordStaking.UnstakeEarly.selector); // if (7 - 1 <= 6) revert UnstakeEarly();
fjordStaking.unstake(1, 10 ether);
vm.stopPrank();
vm.startPrank(bob);
vm.expectRevert(FjordStaking.UnstakeEarly.selector);
fjordStaking.unstake(1, 10 ether);
vm.stopPrank();
​
// Entering epoch 8, successfully unstaking
vm.warp(block.timestamp + 1);
vm.prank(minter);
fjordStaking.addReward(1e18);
uint256 alice_unstake_time = block.timestamp;
uint256 bob_unstake_time = block.timestamp;
console.log("currnetEpoch:",fjordStaking.getEpoch(block.timestamp),"calls unStake() success");
vm.prank(alice);
fjordStaking.unstake(1, 10 ether);
vm.prank(bob);
fjordStaking.unstake(1, 10 ether);
​
// calculate and output the staking duration
uint256 alice_staking_duratio = alice_unstake_time - alice_stake_time;
uint256 bob_staking_duratio = bob_unstake_time - bob_stake_time;
console.log("alice staking duratio:",alice_staking_duratio);
console.log("bob staking duration:",bob_staking_duratio);
assertEq(alice_staking_duratio - bob_staking_duratio, 7 days - 1);
​
// Output `unclaimedRewards`
(,uint256 alice_unclaimedRewards,,) = fjordStaking.userData(alice);
(,uint256 bob_unclaimedRewards,,) = fjordStaking.userData(bob);
console.log("alice's unclaimedRewards:", alice_unclaimedRewards);
console.log("bob's unclaimedRewards:", bob_unclaimedRewards);
}
// [PASS] test_stake_one_second() (gas: 915410)
// Logs:
// currnetEpoch: 7 calls unStake() fails
// currnetEpoch: 8 calls unStake() success
// alice staking duratio: 4233600
// bob staking duration: 3628801
// alice's unclaimedRewards: 2666666666666666650
// bob's unclaimedRewards: 2666666666666666650

output:

[PASS] test_stake_one_second() (gas: 915410)
Logs:
currnetEpoch: 7 calls unStake() fails
currnetEpoch: 8 calls unStake() success
alice staking duratio: 4233600
bob staking duration: 3628801
alice's unclaimedRewards: 2666666666666666650
bob's unclaimedRewards: 2666666666666666650

Code Snippet

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L691-L724
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L330-L333

Impact

Consider the following scenario:

A user aiming to maximize their rewards may choose to execute stake() just before an epoch ends. However, due to the time difference between the call and the actual execution, there is a risk that the stake() transaction could be completed after the epoch has flipped. In such a case, the user’s funds would be locked for nearly an entire epoch without receiving any reward. Network congestion could exacerbate this issue, cause users to lose funds in disguise. Should an epoch flip check be added? If the epoch has advanced beyond the user's expectation, the call could be aborted or rescheduled to avoid locking funds without rewards, ensuring the user's expectations are met.

Tools Used

Manual Review

Recommendations

Consider redesigning the staking reward system for the current epoch. If it is already determined that no rewards will be granted regardless of the staking time within the same epoch, it is recommended to add checks in all staking-related functions. For example:

When the user calls stake(), include a logic check with a bool parameter to confirm whether to continue execution if the current epoch has flipped. If the user sets the parameter to true, the execution proceeds as normal. If set to false and the epoch has flipped, the transaction could be aborted or rolled back to avoid unintended fund locking.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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