search

sherlock-audit / 2023-12-flatmoney-judging

9 stars 8 forks source link

ni8mare - Incorrect implementation of `checkSkewMax` in the `announceStableWithdraw` function #225

Closed sherlock-admin closed 5 months ago

sherlock-admin commented 5 months ago

ni8mare

high

Incorrect implementation of checkSkewMax in the announceStableWithdraw function

Summary

The current usage of checkSkewMax while announcing the withdrawal of collateral tokens using the announceStableWithdraw function is not correct.

Vulnerability Detail

checkSkewMax is defined as follows:

    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");

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

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

_additionalSkew is added to the sizeOpenedTotal (numerator), whenever a user adds more leverage to his position. This implementation of checkSkewMax makes sense in a function like announceLeverageOpen. But, this implementation is wrong for the function announceStableWithdraw.

In announceStableWithdraw, it is used as vault.checkSkewMax({additionalSkew: expectedAmountOut});, where expectedAmountOut is the number of collateral tokens a user gets when they burn their LP tokens. So, note that the collateral tokens are decreased from the protocol when the user withdraws. Additional skew to the leverage is not added when the user withdraws their collateral from the protocol. Hence, this value needs to be subtracted from the stableCollateralTotal(denominator) instead of being added to the sizeOpenedTotal (numerator). For announceStableWithdraw, the longSkewFraction should be defined in the following way:

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

Impact

checkSkewMax is an important invariant of the system checking that the system is not too skewed towards the longs. Its incorrect usage breaks the invariant.

Code Snippet

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

https://github.com/sherlock-audit/2023-12-flatmoney/blob/bba4f077a64f43fbd565f8983388d0e985cb85db/flatcoin-v1/src/FlatcoinVault.sol#L294C1-L308C1

Tool used

Manual Review

Recommendation

For announceStableWithdraw, define the longSkewFraction in a similar fashion as the following:

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

Duplicate of #193

sherlock-admin commented 5 months ago

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/273.