code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The last user may not be able to withdraw all.

Proof of Concept

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

    function beforeWithdraw(uint256 amount, uint256 shares) internal virtual override {
        super.beforeWithdraw(amount, shares);
        storedTotalAssets -= amount;
    }

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

    function totalAssets() public view override returns (uint256) {
        // cache global vars
        uint256 storedTotalAssets_ = storedTotalAssets;
        uint192 lastRewardAmount_ = lastRewardAmount;
        uint32 rewardsCycleEnd_ = rewardsCycleEnd;
        uint32 lastSync_ = lastSync;

        if (block.timestamp >= rewardsCycleEnd_) {
            // no rewards or rewards fully unlocked
            // entire reward amount is available
            return storedTotalAssets_ + lastRewardAmount_;
        }

        // rewards not fully unlocked
        // add unlocked rewards to stored total
        uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
        return storedTotalAssets_ + unlockedRewards;
    }

https://github.com/transmissions11/solmate/blob/main/src/mixins/ERC4626.sol#L146-L150

    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
    }

When the last user tries to withdraw all, previewWithdraw() will return totalAssets() as the user owns all the assets.

However, part of the totalAssets() is the unlockedRewards. Therefore, xERC4626.sol#beforeWithdraw() will revert due to underflow as storedTotalAssets < amount when amount == totalAssets().

Tools Used

Recommended Mitigation Steps

Consider settling the unlocked rewards to storedTotalAssets before the withdrawal.

FortisFortuna commented 2 years ago

Already known bug https://code4rena.com/reports/2022-04-xtribe/#m-01-xerc4626sol-some-users-may-not-be-able-to-withdraw-until-rewardscycleend-the-due-to-underflow-in-beforewithdraw

0xean commented 2 years ago

closing as invalid....see README

code-423n4 / 2022-09-frax-findings