Users Can Exceed Maximum Skew Due to Unsettled PnL

Summary

In the process of order execution, the system checks for maximum allowable skew prior to settling Profit and Loss (PnL), allowing users to potentially exceed the established skew limit due to the PnL not being factored into the calculation.

Vulnerability Detail

The Flat Money protocol maintains equilibrium by capping the skew at a predetermined maximum level, typically 120%, to ensure market stability. This cap is enforced through a series of actions such as withdrawing stable collateral, initiating or modifying a long position, all of which could potentially increase the skew beyond acceptable limits.

The function responsible for monitoring the maximum skew is 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);
    }
}

The vulnerability arises when stable collateral is withdrawn without settling the PnL (updateGlobalPositionData() not invoked), leading to an underestimation of the actual skew. Similarly, when a user opens or adjusts a long position, the skew check occurs before PnL settlement, potentially resulting in a skew calculation that does not accurately reflect the market's true state.

This discrepancy allows users to manipulate the skew to levels beyond the established maximum, disregarding the unsettled PnL.

Impact

The oversight of not including PnL in the maximum skew requirement calculations enables users to artificially inflate the skew beyond the set limit, posing a significant risk to the protocol's integrity.

Proof of Concept

The following test demonstrates the vulnerability and can be executed within Withdraw.t.sol using the command forge test --match-test test_skew_higher_than_max:

Before executing the test, make sure to have this line on top of the file in order for the test to work:

import {FlatcoinErrors} from "../../../src/libraries/FlatcoinErrors.sol";

function test_skew_higher_than_max() public {
    vm.startPrank(alice);

    // Max skew is 120%
    assertEq(vaultProxy.skewFractionMax(), 1.2e18);

    uint256 stableDeposit = 120e18;
    uint256 additionalSize = 120e18;

    // Deposit 120 rETH as stable collateral
    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: stableDeposit,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });
    // Open a 120 rETH long position
    uint256 tokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 100e18,
        additionalSize: additionalSize,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    // Current skew is 0%
    assertEq(vaultProxy.getVaultSummary().marketSkew, 0);

    // Price doubles
    setWethPrice(2000e8);

    // Right now, profit is 60 rETH (additionalSize / 2)
    // This means stableCollateralTotal will be 60 rETH (120 - 60)
    // Current skew is 200% (120 / 60) but PnL isn't settled yet
    // Now, nobody should be allowed to open new longs or to withdraw stable collateral
    assertEq(leverageModProxy.getMarketSummary().profitLossTotalByLongs, int256(additionalSize / 2));
    assertEq(stableModProxy.stableCollateralPerShare(), 0.5e18);

    // Despite the current skew being 200%, user can withdraw 20 rETH (40 shares)
    announceAndExecuteWithdraw({
        traderAccount: alice,
        keeperAccount: keeper,
        withdrawAmount: 40e18, // 40 shares == 20 rETH
        oraclePrice: 2000e8,
        keeperFeeAmount: 0
    });

    // We're going to withdraw some margin to update the PnL
    announceAndExecuteLeverageAdjust({
        tokenId: tokenId,
        traderAccount: alice,
        keeperAccount: keeper,
        marginAdjustment: -1e18,
        additionalSizeAdjustment: 0,
        oraclePrice: 2000e8,
        keeperFeeAmount: 0
    });

    // Now, the skew is 300% (120 / 40)
    // The user effectively increased the skew from 200% to 300%
    vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.MaxSkewReached.selector, 3e18));
    vaultProxy.checkSkewMax(0);
}

As demonstrated, the skew can be inflated from 200% to 300% due to the unaccounted PnL, despite the 120% cap.

Code Snippet

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

Tool used

Manual Review

Recommendation

To address this vulnerability, it is advised to settle the PnL immediately after order execution:

    function executeOrder(
        address account,
        bytes[] calldata priceUpdateData
    )
        external
        payable
        nonReentrant
        whenNotPaused
        updatePythPrice(vault, msg.sender, priceUpdateData)
        orderInvariantChecks(vault)
    {
        // Settle funding fees before executing any order.
        // This is to avoid error related to max caps or max skew reached when the market has been skewed to one side for a long time.
        // This is more important in case the we allow for limit orders in the future.
        vault.settleFundingFees();

+       (uint256 currentPrice, ) = IOracleModule(vault.moduleAddress(FlatcoinModuleKeys._ORACLE_MODULE_KEY)).getPrice();
+       vault.updateGlobalPositionData({ 
+           price: currentPrice,
+           marginDelta: 0,
+           additionalSizeDelta: 0
+       });

        // ...
    }

Duplicate of #143

sherlock-audit / 2023-12-flatmoney-judging

santipu_ - Users Can Exceed Maximum Skew Due to Unsettled PnL #129