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

The `RewardAdded` event might emit the wrong parameters

greed

Summary

The FjordStaking::addReward() function is supposed to be triggered at the end of an epoch to update the currentEpoch and pulling Fjord Tokens that will be rewarded to stakers for the particular epoch.

It emits the RewardAdded with the previousEpoch parameter and is supposed to be "the last action of the epoch".

This previousEpoch parameter represents the epoch value before the epoch update.

https://github.com/Cyfrin/2024-08-fjord/blob/main/src/FjordStaking.sol#L751-L768

/// @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();
​
emit RewardAdded(previousEpoch, msg.sender, _amount);
}

Vulnerability Details

The currentEpoch update is performed by the _checkEpochRollover() function when the epoch period of 1 week has passed.

This internal function is triggered by the checkEpochRollover modifier that is called by all the user functions in order for the state to be updated regularly.

This means users have the ability to modify the value currentEpoch when they perform their staking operations.

The protocol documentation states that addReward() "must be the action to trigger update epoch and the last action of the epoch".

On Ethereum, it is impossible to order the transactions in a block from a user perspective, meaning a user transaction can be executed right before addReward() (either by frontrunning or by pure luck).

Here is a scenario that describes this situation and results in the RewardAdded event to emit the wrong previousEpoch value.

  • the protocol is in its 5th epoch, meaning currentEpoch equals 5

  • 1 week pass and now the protocol should be in its 6th epoch, however currentEpoch must be updated by addReward()

  • the Fjord multisig executes addReward() expecting RewardAdded to emit 5 as the value of previousEpoch.

  • a user frontruns this transaction and calls stake() which will trigger _checkEpochRollover() and update currentEpoch to be equal to 6.

  • now the addReward() call occurs and will cache the currentEpoch in uint16 previousEpoch = currentEpoch; which will be equal to 6

  • the _checkEpochRollover() will be executed but won't modify currentEpoch

  • the RewardAdded will emit previousEpoch as 6 instead of the expected 5.

Impact

Internal and external systems that rely heavily on Fjord events.

Tools Used

Manual review

Recommendations

In addReward() make sure the call to _checkEpochRollover() actually modified currentEpoch and emit the right previousEpoch consequently.

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();
​
+ if(currentEpoch == previousEpoch) previousEpoch--;
emit RewardAdded(previousEpoch, msg.sender, _amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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