Frax Ether Liquid Staking contest - Critical's results

Lines of code

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

Vulnerability details

Impact

The withdrawal amount will be counted as part of the surplus asset balance mistakenly if block.timestamp >= rewardsCycleEnd.

Proof of Concept

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

    function beforeWithdraw(uint256 assets, uint256 shares) internal override {
        super.beforeWithdraw(assets, shares); // call xERC4626's beforeWithdraw first
        if (block.timestamp >= rewardsCycleEnd) { syncRewards(); } 
    }

storedTotalAssets will be deducted by the withdrawal amount in xERC4626.sol#beforeWithdraw().

At sfrxETH.sol#L50, when block.timestamp >= rewardsCycleEnd, syncRewards() will be called AFTER storedTotalAssets -= amount; (xERC4626.sol#L67).

https://github.com/corddry/ERC4626/blob/6cf2bee5d784169acb02cc6ac0489ca197a4f149/src/xERC4626.sol#L65-L101

In syncRewards(), storedTotalAssets will be used to calculate the rewards.

All surplus asset balance of the contract over the internal balance becomes queued for the next cycle.

uint256 storedTotalAssets_ = storedTotalAssets;
uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; // SSTORE

Tools Used

Recommended Mitigation Steps

Consider calling syncRewards() before call xERC4626's beforeWithdraw.