FlatcoinVault.sol#checkSkewMax function is called with error.

Summary

FlatcoinVault.sol#checkSkewMax function is called with wrong parameter in the DelayedOrder.sol#announceStableWithdraw function. So FlatcoinVault.sol#checkSkewMax will be malfunctioned.

Vulnerability Detail

FlatcoinVault.sol#checkSkewMax function is the following.

294:/// @notice Asserts that the system will not be too skewed towards longs after additional skew is added (position change).
295:/// @param _additionalSkew The additional skew added by either opening a long or closing an LP position.
    function checkSkewMax(uint256 _additionalSkew) public view {
        // check that skew is not essentially disabled
        if (skewFractionMax < type(uint256).max) {
            uint256 sizeOpenedTotal = _globalPositions.sizeOpenedTotal;

            if (stableCollateralTotal == 0) revert FlatcoinErrors.ZeroValue("stableCollateralTotal");

303:        uint256 longSkewFraction = ((sizeOpenedTotal + _additionalSkew) * 1e18) / stableCollateralTotal;

            if (longSkewFraction > skewFractionMax) revert FlatcoinErrors.MaxSkewReached(longSkewFraction);
        }
    }

From L295 and L303, we can see that _additionalSkew should be the amount of collateral to be added into long positions inside the pool. On the other hand, DelayedOrder.sol#announceStableWithdraw function is the following.

    function announceStableWithdraw(
        uint256 withdrawAmount,
        uint256 minAmountOut,
        uint256 keeperFee
    ) external whenNotPaused {
        uint64 executableAtTime = _prepareAnnouncementOrder(keeperFee);

        IStableModule stableModule = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY));
        uint256 lpBalance = IERC20Upgradeable(stableModule).balanceOf(msg.sender);

        if (lpBalance < withdrawAmount)
            revert FlatcoinErrors.NotEnoughBalanceForWithdraw(msg.sender, lpBalance, withdrawAmount);

        // Check that the requested minAmountOut is feasible
        {
124:        uint256 expectedAmountOut = stableModule.stableWithdrawQuote(withdrawAmount);

            if (keeperFee > expectedAmountOut) revert FlatcoinErrors.WithdrawalTooSmall(expectedAmountOut, keeperFee);

128:        expectedAmountOut -= keeperFee;

            if (expectedAmountOut < minAmountOut) revert FlatcoinErrors.HighSlippage(expectedAmountOut, minAmountOut);

132:        vault.checkSkewMax({additionalSkew: expectedAmountOut});
        }

Since announceStableWithdraw function decrease the stableCollateralTotal of FlatcoinVault.sol#L303, it should pass the amount of decreased stable collateral to FlatcoinVault.sol#checkSkewMax function. (See Recommendation) But now exepectedAmountOut of L132 is the amount of collateral which are refunded from pool to user. Thus FlatcoinVault.sol#checkSkewMax will be malfunctioned.

Impact

FlatcoinVault.sol#checkSkewMax will be malfunctioned. That is, the system may be too skewed towards longs after redeeming stables, or redeeming stables may be failed while system is not too skewed towards longs.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L132

Tool used

Manual Review

Recommendation

Add the amount of stable collateral to remove as a parameter to FlatcoinVault.sol#checkSkewMax function and Modify DelayedOrder.sol#announceStableWithdraw function to pass the stableCollateralAmount which corresponds to withdrawAmount. That is, modify FlatcoinVault.sol#checkSkewMax function as follows.

--  function checkSkewMax(uint256 _additionalSkew) public view {
++  function checkSkewMax(uint256 _additionalSkew, uint256 _removedStateCollateral) public view {
        // check that skew is not essentially disabled
        if (skewFractionMax < type(uint256).max) {
            uint256 sizeOpenedTotal = _globalPositions.sizeOpenedTotal;

            if (stableCollateralTotal == 0) revert FlatcoinErrors.ZeroValue("stableCollateralTotal");

--         uint256 longSkewFraction = ((sizeOpenedTotal + _additionalSkew) * 1e18) / stableCollateralTotal;
++         uint256 longSkewFraction = ((sizeOpenedTotal + _additionalSkew) * 1e18) / (stableCollateralTotal - _removedStateCollateral);

            if (longSkewFraction > skewFractionMax) revert FlatcoinErrors.MaxSkewReached(longSkewFraction);
        }
    }

Then modify DelayedOrder.sol#announceStableWithdraw function as follows.

    function announceStableWithdraw(
        uint256 withdrawAmount,
        uint256 minAmountOut,
        uint256 keeperFee
    ) external whenNotPaused {
++      uint256 stableCollateralAmount;
        ......
--          vault.checkSkewMax({additionalSkew: expectedAmountOut});
++          vault.checkSkewMax({additionalSkew: 0, removedStateCollateral: stableCollateralAmount});
        ......
    }

There are several places where vault.checkSkewMax function is called so all such calls should be updated with new signature.

Duplicate of #193

sherlock-admin commented 8 months ago

2 comment(s) were left on this issue during the judging contest.

0xLogos commented:

low, there's correct check in stable withdraw execution. After doing some math, you'll see that the correct check is more strict, so the announce function won't revert falsely.

takarez commented:

invalid