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

Rewards are distributed incorrectly to the stakers.

jsmi

Summary

According to the comment, admin must call addReward() so that rewards are distributed immediately.
However, by chance or due to front-run, the rewards may not be distributed inside the addReward().
Therefore, rewards are distributed incorrectly per epoch to stakers.

Vulnerability Details

The relevant code of FjordStaking.addReward() is following.

/// @notice addReward should be called by master chef
752 /// must be only call if it's can trigger update next epoch so the total staked won't increase anymore
/// must be the action to trigger update epoch and the last action of the epoch
/// @param _amount The amount of tokens to be added as rewards.
function addReward(uint256 _amount) external onlyRewardAdmin {
//CHECK
if (_amount == 0) revert InvalidAmount();
​
//EFFECT
uint16 previousEpoch = currentEpoch;
​
//INTERACT
763 fjordToken.safeTransferFrom(msg.sender, address(this), _amount);
​
765 _checkEpochRollover();
​
emit RewardAdded(previousEpoch, msg.sender, _amount);
}

According to the comment of L752, admin must only call addReward() if it can trigger update next epoch. It ensures that the _amount of rewards are transferred from the admin to the contract in L763 and are distributed to stakers in the following _checkEpochRollover() of L765.

function _checkEpochRollover() internal {
uint16 latestEpoch = getEpoch(block.timestamp);
​
if (latestEpoch > currentEpoch) {
//Time to rollover
currentEpoch = latestEpoch;
​
if (totalStaked > 0) {
uint256 currentBalance = fjordToken.balanceOf(address(this));
​
// no distribute the rewards to the users coming in the current epoch
uint256 pendingRewards = (currentBalance + totalVestedStaked + newVestedStaked)
- totalStaked - newStaked - totalRewards;
uint256 pendingRewardsPerToken = (pendingRewards * PRECISION_18) / totalStaked;
totalRewards += pendingRewards;
for (uint16 i = lastEpochRewarded + 1; i < currentEpoch; i++) {
rewardPerToken[i] = rewardPerToken[lastEpochRewarded] + pendingRewardsPerToken;
emit RewardPerTokenChanged(i, rewardPerToken[i]);
}
} else {
for (uint16 i = lastEpochRewarded + 1; i < currentEpoch; i++) {
rewardPerToken[i] = rewardPerToken[lastEpochRewarded];
emit RewardPerTokenChanged(i, rewardPerToken[i]);
}
}
​
totalStaked += newStaked;
totalVestedStaked += newVestedStaked;
newStaked = 0;
newVestedStaked = 0;
​
lastEpochRewarded = currentEpoch - 1;
}
}

However, it may not always be the case.
If stakers call stake(), unstake(), stakeVested() or unstakeVested(), it calls internally _checkEpochRollover() which will update epoch to next epoch if possible.
Even though the admin calls addReward() at the begining of epoch, there can be a user's tx before admin's tx to update the epoch to next epoch. The more stakers exist, the higher the possibility will be.

The following Scenario is available:

  1. Assume that admin calls repeatedly addReward() by passing _amount = 10 ether at the begining of each epoch.

  2. At epoch 1, admin calls addReward(). Then 10 ether of rewards will be distributed at epoch 1.

  3. At epoch 2, user A calls addReward(). Unfortunately, a staker's stake() tx is executed before admin's addReward() tx. Since the current epoch is updated in staker's tx, the _amount of rewards of admin's tx can't be distributed at epoch 2.

  4. At epoch 3, admin calls addReward() again. Then 20 ether of rewards which are sum of rewards for epoch 2 and epoch 3 will be distributed at a time at epoch 3.

  5. As shown above, the rewards are distributed 10 ether for epoch 1, zero for epoch 2, and 20 ether for epoch 3. That is, the rewards are distributed incorrectly per epoch.

Note:
Exploiting this vulnerability, attacker can front-run the admin's tx and change the rewards distribution to be in favor of himself.

Impact

Rewards are distributed incorrectly to stakers.
Attacker can front-run the admin's tx and change the rewards distribution to be in favor of himself.

Tools Used

Manual Review

Recommendations

It is recommended to change the order of fjordToken.safeTransferFrom() and _checkEpochRollover() in FjordStaking.addReward().

/// @notice addReward should be called by master chef
- /// must be only call if it's can trigger update next epoch so the total staked won't increase anymore
/// must be the action to trigger update epoch and the last action of the epoch
/// @param _amount The amount of tokens to be added as rewards.
function addReward(uint256 _amount) external onlyRewardAdmin {
//CHECK
if (_amount == 0) revert InvalidAmount();
​
//EFFECT
uint16 previousEpoch = currentEpoch;
​
//INTERACT
- fjordToken.safeTransferFrom(msg.sender, address(this), _amount);
+ _checkEpochRollover();
​
- _checkEpochRollover();
+ fjordToken.safeTransferFrom(msg.sender, address(this), _amount);
​
emit RewardAdded(previousEpoch, msg.sender, _amount);
}

Then, the rewards will be distributed correctly per epoch regardless of the presence of staker's tx.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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