Lines of code

Vulnerability details

Impact

beforeWithdraw() call syncRewards() cause the number of “nextRewards” to be incorrect .
if a large amount is withdraw() at the end of the cycle, then the next cycle reward will incorrectly increase by the corresponding amount

Proof of Concept

when call sfrxETH.sol -> withdrew() there are the following steps:
1.sfrxETH.withdraw() will call beforeWithdraw()
2.in sfrxETH -> beforeWithdraw(): first call super.beforeWithdraw() to step 3
3.in xERC4626 -> beforeWithdraw() , this step will do : storedTotalAssets -= amount
4.go back sfrxETH -> beforeWithdraw(),then call syncRewards()
5.in syncRewards() will do: (asset.balance not reduce and storedTotalAssets_ is reduce)

    function syncRewards() public virtual {
    ...
       uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;
       //**attention： asset.balanceOf(address(this)) not reduce and storedTotalAssets_ is reduce in step 3**//


       lastRewardAmount = nextRewards.safeCastTo192(); 
    ...

6.after syncRewards() go back withdraw() will do asset.safeTransfer() to reduce "asset.balanceOf(address(this))”

So that nextRewards has more amount

Recommended Mitigation Steps

Call syncRewards() before super.beforeWithdraw(), this does not affect the number of burn shares, because before calling beforeWithdraw(), shares are already stored in temporary variables in withdraw()

sfrxETH.sol

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

The text was updated successfully, but these errors were encountered:

All reactions