Inability to Liquidate Certain Positions Due to Erroneous Stable Collateral Update

Summary

The protocol encounters an issue where it inaccurately calculates the Profit and Loss (PnL) during liquidation, leading to an incorrect update of the stable collateral. This miscalculation prevents the liquidation of specific positions due to the failure of invariant checks that are conducted at the liquidation's conclusion.

Vulnerability Detail

The process of liquidation is designed to redistribute the remaining margin of a liquidated position (if any) to the Liquidity Providers (LPs) by augmenting their stable collateral. However, a flaw exists within the liquidate() function where it inaccurately accounts for the PnL of the position being liquidated. This error disrupts the liquidation process, as it fails the invariant checks that are essential for completing the liquidation.

The issue lies in how the liquidate() function processes the remaining and settled margins:

    if (settledMargin > 0) {

        // ...

        // Adjust the stable collateral total to account for user's remaining margin.
        // If the remaining margin is greater than 0, this goes to the LPs.
        // Note that {`remainingMargin` - `profitLoss`} is the same as {`marginDeposited` + `accruedFunding`}.
>>      vault.updateStableCollateralTotal(int256(remainingMargin) - positionSummary.profitLoss);

        // Send the liquidator fee to the caller of the function.
        // If the liquidation fee is greater than the remaining margin, then send the remaining margin.
        vault.sendCollateral(msg.sender, liquidatorFee);
    } else {
        // If the settled margin is -ve then the LPs have to bear the cost.
        // Adjust the stable collateral total to account for user's profit/loss and the negative margin.
        // Note: We are adding `settledMargin` and `profitLoss` instead of subtracting because of their sign (which will be -ve).
>>      vault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);
    }

This flawed approach fails to recognize that the PnL has already been considered during the updateGlobalPositionData() function call, leading to an incorrect stable collateral update by subtracting the PnL from the remaining margin, effectively nullifying the PnL's impact.

A practical scenario illustrating this flaw:

Bob initiates a position with a 10 rETH margin and a 100 rETH leverage at a price of 1000 USD per rETH.
The price drops to 914 USD per rETH, rendering Bob's position eligible for liquidation.
The global PnL adjustments, through the updateGlobalPositionData() function, apply losses to Bob's position, impacting the stable collateral.
Upon liquidation, Bob's remaining margin stands at 0.6 rETH, which should ideally augment the stable collateral. However, the flawed calculation adds the entire 10 rETH due to the erroneous PnL subtraction.
This miscalculation causes the invariant checks to fail during liquidation, preventing the liquidation process from executing.

Impact

This flaw will prevent the protocol from liquidating positions when necessary due to incorrect stable collateral updates.

Proof of Concept

The vulnerability can be demonstrated through the following test script added to Liquidate.t.sol, executable via the command: forge test --match-test test_dos_liquidation.

function test_dos_liquidation() public {
    // No funding fees
    vaultProxy.setMaxFundingVelocity(0);

    // Deposit stable collateral
    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: 120e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });
    // Open a first position, we will liquidate it later
    uint256 tokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 10e18,
        additionalSize: 100e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });
    // Create a 2nd position to update PnL later
    uint256 tokenId2 = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 10e18,
        additionalSize: 10e18,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    // Price goes down to make the first position liquidatable
    skip(10 days);
    setWethPrice(914e8);

    // Update PnL through withdrawing some margin from 2nd position
    announceAndExecuteLeverageAdjust({
        tokenId: tokenId2,
        traderAccount: alice,
        keeperAccount: keeper,
        marginAdjustment: -1e18,
        additionalSizeAdjustment: 0,
        oraclePrice: 914e8,
        keeperFeeAmount: 0
    });

    // The first position should be liquidated
    assertTrue(liquidationModProxy.canLiquidate(tokenId));

    // The first position cannot be actually liquidated due to invariant checks
    vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.InvariantViolation.selector, "stableCollateralPerShareLiquidation"));
    liquidationModProxy.liquidate(tokenId);
}

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LiquidationModule.sol#L133 https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LiquidationModule.sol#L142

Tool used

Manual Review

Recommendation

To resolve this issue, it is imperative to revise the liquidate() function, ensuring it accurately updates the stable collateral without negating the PnL's effect, thereby restoring the liquidation process's integrity.

Duplicate of #180

sherlock-admin commented 5 months ago

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

takarez commented:

valid: the position cannot be liquidated high(8)

sherlock-admin commented 4 months ago

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

sherlock-audit / 2023-12-flatmoney-judging

santipu_ - Inability to Liquidate Certain Positions Due to Erroneous Stable Collateral Update #230