The Liquidity Pool will lack margin of Synthetix perpetual market, if `liquidationBufferRatio` of contract `PerpsV2MarketSettings` from Synthetix is updated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L764-L771

Vulnerability details

Impact

The Liquidity Pool can lack the margin of PerpMarket if liquidationBufferRatio of contract PerpsV2MarketSettings from Synthetix is greater than 1e18. Then the delay orders of the pool can not be executed and the position of the pool might be liquidated.

Proof of concept

• Function _calculateMargin returns the margin amount needed to transfer with the specific size of PerpMarket.

  function _calculateMargin(int256 size) internal view returns (uint256 margin) {
  (uint256 spotPrice, bool isInvalid) = baseAssetPrice();
  
  require(!isInvalid || spotPrice > 0);
  
  uint256 absSize = size.abs();
  margin = absSize.mulDivDown(spotPrice, futuresLeverage);
  }

• When LiquidityPool executes a delayed order of Synthetix, function _updatePositionMargin from PerpsV2Market of Synthetix will be called. It will check the margin of the new position in the perps market:

  uint liqPremium = _liquidationPremium(position.size, price);
  uint liqMargin = _liquidationMargin(position.size, price).add(liqPremium);
  _revertIfError(
  (margin < _minInitialMargin()) ||
      (margin <= liqMargin) ||
      (_maxLeverage(_marketKey()) < _abs(_currentLeverage(position, price, margin))),
  Status.InsufficientMargin
  );

• Function _liquidationMargin returns the maximum of margin that will be liquidated with the position size of perps market. Then the new margin must to be greater than _liquidationMargin.

  function _liquidationMargin(int positionSize, uint price) internal view returns (uint lMargin) {
  uint liquidationBuffer = _abs(positionSize).multiplyDecimal(price).multiplyDecimal(_liquidationBufferRatio());
  return liquidationBuffer.add(_liquidationFee(positionSize, price));
  }

• To calculate the _liquidationMargin, PerpsV2Martket use the variable _liquidationBufferRatio as the scale of _abs(positionSize).multiplyDecimal(price) (value of the position). This variable has getter and setter functions in the contract PerpsV2MarketSettings. You can find this contract at https://optimistic.etherscan.io/address/0x09793Aad1518B8d8CC72FDd356479E3CBa7B4Ad1#code.

• _liquidationBufferRatio is 1e16 (1%) now but can be changed in the future, and can become market-specific (I asked Synthetix team and they said it will be changed in a couple of weeks, but I didn't know how it will be changed).

• Since function _calculateMargin in contract LiquidityPool doesn't consider this minimum required margin (to not be liquidated), LiquidityPool can lack the margin of perps market in the future.

• Scenario:

  1. futuresLeverage of LiquidityPool is applied to be 5.Then function _calculateMargin returns 1/5 (20%) amount of position value (position value = size * spotPrice)
  2. _liquidationBufferRatio is set to be 3e17 (30%) in PerpsV2MarketSettings contract. Then it requires a margin >= 30% of the position value when updating a position.
  3. LiquidityPool's margin is not enough for its position in PerpsMarket. Then the delay orders of the pool can't be executed and the position of the pool in PerpsMarket can be liquidated

     Tool used

Manual Review

Recommended Mitigation Steps

Should calculate _liquidationMargin from PerpsMarket using the current _liquidationBufferRatio from PerpsV2MarketSettings contract, to set the minimum margin in function _calculateMargin

[mubaris]

Do you even know what liquidation margin is? It can't be above 1e18. futuresLeverage is under the control of the admin and we expect to keep it at respectable values like 2 where Synthetix provides 25x leverage and it is expected to set to 100x in the future

[c4-sponsor]

mubaris marked the issue as sponsor disputed

[c4-sponsor]

mubaris requested judge review

[JustDravee]

From the warden's submission above:

if liquidationBufferRatio of contract PerpsV2MarketSettings from Synthetix is greater than 1e18

_liquidationBufferRatio is 1e16 (1%) now but can be changed in the future, and can become market-specific (I asked Synthetix team and they said it will be changed in a couple of weeks, but I didn't know how it will be changed).

ChatGPT to the Rescue:

The liquidation buffer ratio is a metric used in cryptocurrency trading to determine the level of risk associated with holding a leveraged position. When trading with leverage, a trader borrows funds to increase their trading position, and the liquidation buffer ratio represents the amount of collateral a trader must hold to avoid being liquidated in the event of a market downturn.

The liquidation buffer ratio is calculated by dividing the collateral held by the trader by the notional value of their leveraged position. For example, if a trader has $10,000 worth of collateral and a leveraged position with a notional value of $100,000, their liquidation buffer ratio would be 10% (10,000 / 100,000).

If the value of the trader's position falls below a certain threshold determined by the exchange, the trader's position will be automatically liquidated to repay the borrowed funds, which can result in significant losses. Maintaining a sufficient liquidation buffer ratio can help traders manage their risk and avoid liquidation.

Hence the starting hypothesis is indeed implausible

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[huuducsc]

I made a typing mistake in the impact assessment, where the number 1e18 should be 1e16 (1%). This is the current value of liquidationBufferRatio in Synthetix perps, although it may change in the future. In the proof of concept, I used 3e17 (30%) as an example, and it is a valid value for _liquidationBufferRatio.

[mubaris]

Realistically, the protocol would be using 2-3x leverage (unlike 5x mentioned by the warden). Synthetix changing params takes at least a week and they announce it via SIPs. In that time, we can always reduce the leverage or add additional margin. Also anything above 10% liquidation buffer is absurd.

[rivalq]

Yeah this scenario can happen but only after many what-ifs.

[c4-sponsor]

rivalq marked the issue as sponsor confirmed

[c4-judge]

JustDravee marked the issue as selected for report