Permanent lock of all funds when the funding fees are bigger than total margin

Summary

The issue arises when the funding fees settled between LPs and long traders exceed the total margin deposited by traders and favours them. In such instances, the protocol erroneously zeroes out the total margin, consequently freezing all ongoing long positions. Moreover, this situation triggers a protocol invariant check that prevents LPs from withdrawing their stable collateral, effectively causing a complete lockup of funds within the system.

Vulnerability Detail

The protocol's standard operation involves settling funding fees as an initial step following any action, executed through the FlatcoinVault.settleFundingFees() function. This function is responsible for calculating and redistributing funding fees accrued since the last settlement among LPs and long traders.

Under some conditions where the funding fees owed by traders to LPs surpass the traders' total deposited margin, the protocol's response to this imbalance is to set the total margin to zero, aiming to prevent negative balances. However, this measure fails to consider scenarios where LPs owe funding fees to long traders, which should ideally increase the traders' margin. Instead, it indiscriminately zeroes the margin, unjustly locking long positions.

The flaw is encapsulated in the following code segment:

    // In the worst case scenario that the last position which remained open is underwater,
    // we set the margin deposited total to 0. We don't want to have a negative margin deposited total.
    _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
        ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
>>      : 0;

    _updateStableCollateralTotal(-_fundingFees);

This mechanism is designed to avoid a negative margin but it doesn't account cases where funding fees are bigger than the total margin but those fees are going to increase the margin, not decrease it.

Impact

The flaw leads to a scenario where, following the settlement of funding fees that favor long traders, the entire margin pool is zeroed out, effectively freezing all active positions. Additionally, the subsequent invariant check halts any attempts by LPs to retrieve their stable collateral, culminating in a total immobilization of funds within the protocol.

Proof of Concept

The following test can be pasted in Withdraw.t.sol and be run with the following command: forge test --match-test test_zero_total_margin.

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_zero_total_margin() public {
    // Set max funding velocity to 3%
    vm.startPrank(admin);
    vaultProxy.setMaxFundingVelocity(0.03e18);

    vm.startPrank(alice);

    uint256 marginDeposited = 4e18;

    // Deposit 200 rETH as stable collateral
    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: 200e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });
    // Deposit 4 rETH margin and 30 rETH additional size
    uint256 tokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: marginDeposited,
        additionalSize: 30e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    skip(3 days);
    setWethPrice(1000e8); // keep the price fresh

    // After 3 days, the funding fees are bigger than the margin deposited
    // Then, marginDepositedTotal will be set to zero
    int256 fundingFees = vaultProxy.settleFundingFees();
    assertGt(fundingFees, int256(marginDeposited));
    assertEq(vaultProxy.getGlobalPositions().marginDepositedTotal, 0);

    // Now, Alice is going to try closing the position
    uint256 keeperFee = mockKeeperFee.getKeeperFee();
    vm.prank(alice);
    delayedOrderProxy.announceLeverageClose(tokenId, 1000e8, keeperFee);
    skip(uint256(vaultProxy.minExecutabilityAge()));
    bytes[] memory priceUpdateData = getPriceUpdateData(1000e8);

    // Alice cannot close the position because total margin has been erroneously set to 0
    vm.expectRevert(FlatcoinErrors.InsufficientGlobalMargin.selector);
    delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);

    skip(uint256(vaultProxy.maxExecutabilityAge()) + 1);

    // Now, Alice is going to try withdrawing some stable collateral (10 shares out of 200)
    vm.prank(alice);
    delayedOrderProxy.announceStableWithdraw(10e18, 0, keeperFee);
    skip(uint256(vaultProxy.minExecutabilityAge()));
    priceUpdateData = getPriceUpdateData(1000e8);

    // Call reverts because an invariant violation caused by total margin being set to zero
    vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.InvariantViolation.selector,"collateralNet"));
    delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);
}

The test sequence involves depositing margins and collateral and simulating the accrual of large funding fees over a three-day period, leading to the erroneous zeroing of the total margin. Subsequent attempts by a trader to close a position or withdraw stable collateral are prevented by reverts due to the flawed margin handling and invariant checks.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L232-L234

Tool used

Manual Review

Recommendation

To rectify this issue, a refined conditional check is advised to discern whether funding fees are advantageous to long traders prior to any adjustment of the total margin. The proposed fix is as follows:

    // In the worst case scenario that the last position which remained open is underwater,
    // we set the margin deposited total to 0. We don't want to have a negative margin deposited total.
-   _globalPositions.marginDepositedTotal = (int256(_globalPositions.marginDepositedTotal) > _fundingFees)
-       ? uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees)
-       : 0;

+   if(_fundingFees < 0 && _globalPositions.marginDepositedTotal < uint256(_fundingFees * -1)) {
+       _globalPositions.marginDepositedTotal = 0;
+   } else {
+       _globalPositions.marginDepositedTotal = uint256(int256(_globalPositions.marginDepositedTotal) + _fundingFees);
+   }

This alteration ensures that the total margin is only zeroed out under appropriate circumstances, thereby mitigating the risk of unjust position lockups and fund immobilization.

Duplicate of #195

sherlock-audit / 2023-12-flatmoney-judging

santipu_ - Permanent lock of all funds when the funding fees are bigger than total margin #127