code423n4 commented 2 years ago

Lines of code

LOC: https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L26

Vulnerability details

Description

When protocols hand out rewards to staked tokens, they must be careful to do so without leaving a large MEV opportunity, otherwise a bot could sandwich the increase of token value by minting shares and immediately redeeming them for a larger underlying amount. By doing this with a large sum of funds, attackers can enjoy a large % of the rewards while minimizing the profits of stakers considerably. xERC4626 addresses this issue and is designed to release staking rewards linearly during some defined cycle length. totalAssets() returns the previous matured assets added to the unlockedRewards from the current cycle:

uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_); // reward * cycle elapsed time / cycle total time
return storedTotalAssets_ + unlockedRewards;

syncRewards() should be triggered externally and updates the cycle parameters:

uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
…
lastSync = timestamp;
rewardsCycleEnd = end;

syncRewards() rounds timestamp to the next multiple of rewardsCycleLength and marks that timestamp as end of unlocked rewards. The problem is that timestamp could already be very close to the next rewardsCycleLength multiple, and therefore the release could be extremely sharp (Up to the entire reward amount released in a single block). syncRewards() is permissionless by design, so attacker can wait for timestamp to be close to the target and then call syncRewards(). This will only work if no one else called syncRewards() to update the current cycle.

Impact

Attacker can theoretically consume up to the whole reward amount in a single block.

Proof of Concept

Assume rewardCycleLength = 1000 seconds, current assets = 200 frxETH, current shares = 150 sfrxETH, reward = 20 frxETH, next block timestamp = 19999

Attacker calls syncRewards() -> end = ((19999 + 1000) / 1000 ) * 1000 = 20000
Attacker mints 150 sfrxETH shares , which costs 200 frxETH
Attacker waits 1 block
Attacker redeems 150 sfrxETH shares, for half of the total assets = (200 + 200 + 20) / 2 = 210
Attacker profits 10 ETH (50% of the reward) from a single block. Staking APY is cut in half. The more attacker deposits, the larger the % of reward consumed.

Tools Used

Manual audit

Recommended Mitigation Steps

syncRewards() should add a check: timestamp % rewardsCycleLength <= rewardsCycleLength * SYNC_THRESHOLD / SYNC_THRESHOLD_FACTOR, with a reasonable threshold. Since syncRewards() is used in beforeWithdraw, it's best to not revert but instead return a false value if we have missed the current cycle.

FortisFortuna commented 2 years ago

From @denett syncRewards should be called by us at the beginning of each period, or we need to automatically call it before deposits/withdrawals.