SBSecurity - When skewFractionMax is lowered, liquidity providers will not be able to withdraw.

[sherlock-admin2]

SBSecurity

medium

When skewFractionMax is lowered, liquidity providers will not be able to withdraw.

Summary

When skewFractionMax is being lowered by admin, certain liquidity providers will have their tokens locked, without a way to be withdrawn.

For example, when skewFractionMax is lowered from 120% to 100%, only the users who have enough amount of collateral tokens to bring the skew fraction below the max will be able to unblock the withdrawals.

Vulnerability Detail

The skew factor is checked either when liquidity provider withdraws or trader opens a leveraged position.

It is used to prevent the system from being skewed towards the longs as we can see:

/// @notice Asserts that the system will not be too skewed towards longs after additional skew is added (position change).
    /// @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");

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

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

In simpler words when skew factor is set to 120% (taken from the deployment config) maximum leveraged long positions with a total size of 120e18 can be opened for every 100e18 of deposited stable collateral tokens (UNIT).

This can be problematic, especially when skewFraction is being lowered.

Current skewFraction = 120% in bullish scenarios, then skewFraction is being lowered to 100%, the remaining 20% UNIT LP tokens are locked in the system and there is no way to be withdrawn unless UNIT LP holder, having enough tokens to bring it to 100% deposits or big leveraged positions are closed, but the second scenario is assumed to not happen given the increasing price of the rETH and leveraged traders profiting.

Additionally, leveraged positions will be bricked too because every single wei increasing the skewFraction will revert.

Impact

Liquidity providers will be locked inside the protocol without way to withdraw their funds, unless there are other depositors willing to decrease the skewFraction, but then their funds will be locked, because of the other LPs withdrawing and bringing the fraction back to the max value.

Leveraged trading positions will be DoS because the skew will be at the upper limit, without way to be lowered unless someone intentionally lock his funds by depositing in the LP vault.

Code Snippet

DelayedOrder.sol

function announceStableWithdraw(
    uint256 withdrawAmount,
    uint256 minAmountOut,
    uint256 keeperFee
) external whenNotPaused {
  ...More code
      vault.checkSkewMax({additionalSkew: expectedAmountOut});
    }

DelayedOrder.sol

function announceLeverageOpen(
    uint256 margin,
    uint256 additionalSize,
    uint256 maxFillPrice,
    uint256 keeperFee
) external whenNotPaused {
    ...More code
    vault.checkSkewMax({additionalSkew: additionalSize});

Tool used

Manual Review

Recommendation

As this is a tricky situation to handle we are proposing to checking the current skewFraction when updating it in the setter function:

/// @notice Setter for the maximum leverage total skew fraction.
    /// @dev This ensures that stable LPs are not too short by capping long trader total open interest.
    ///      Note that `_skewFractionMax` should include 18 decimals.
    /// @param _skewFractionMax The maximum limit of total leverage long size vs stable LP.
    function setSkewFractionMax(uint256 _skewFractionMax) public onlyOwner {
        if (_skewFractionMax < 1e18) revert FlatcoinErrors.InvalidSkewFractionMax(_skewFractionMax);

        skewFractionMax = _skewFractionMax;
    }

That way the whole situation will be prevented and no funds will be locked, but it opens another griefing opportunity for the UNIT LP holders to deny the lowering of the skewFraction, because the overall amount of fees that they receive will be lowered also.

[sherlock-admin]

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

takarez commented:

invalid: admin will do the needful

[nevillehuang]

Invalid based on the following sherlock rule, given maximum skew is admin controlled.

5.3 An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.