Lyra Finance

The _doTrade and addCollateral functions from OptionMarket contract allow updating collateral on positions, even if the board is settled. As a consequence, unexpected situations may happen:

• The added collateral could be forever stuck in ShortCollateral contract and the liquidity pool would never get it.

• If the result is not favorable for users once a board is settled, they can reduce their collateral and negatively affect the liquidity of the protocol.

• Even in some edge scenarios of volatility, the collateral on positions can be reduced to less than expected.

Code Location

addCollateral function will update position's collateral without previously verifying if board is settled:

OptionMarket.sol

function addCollateral(uint positionId, uint amountCollateral) external nonReentrant notGlobalPaused {
  int pendingCollateral = SafeCast.toInt256(amountCollateral);
  OptionType optionType = optionToken.addCollateral(positionId, amountCollateral);
  _routeUserCollateral(optionType, pendingCollateral);
}

\color{black}\color{white}When opening / closing a position, if trade amount is 0, _doTrade function won't verify is board is settled and will return earlier, which allows updating position's collateral:

OptionMarket.sol

// don't engage AMM if only collateral is added/removed
if (trade.amount == 0) {
  if (expectedAmount != 0) {
    revert TradeIterationsHasRemainder(address(this), iterations, expectedAmount, 0, 0);
  }
  return (0, 0, 0, new OptionMarketPricer.TradeResult[](0));
}

if (board.frozen) {
  revert BoardIsFrozen(address(this), board.id);
}
if (board.expiry < block.timestamp) {
    revert BoardExpired(address(this), board.id, board.expiry, block.timestamp);

SOLVED: The issue was fixed in the following commits:

• 056017961c89f0800eec2dfd5bc559a591b68aae
• d8d2e902c6d368313d9e04bec40c21fbf70b870b

When closing a position long call, the _maybeExchangeBase function is called with the argument revertBuyOnInsufficientFunds set to false. As a consequence, the liquidity pool will be able to swap sUSD for sETH without limits and could eventually run out of sUSD tokens.

This situation could affect some relevant operations such as withdrawal, premium payment, settlement, etc.

Proof of Concept:

Initial liquidity info for the test:

The attacker opens a long call position of 120 sETH. There is a big difference between lockedCollateral.base and sETH balance because of \vulnref{DIFFERENCES BETWEEN LOCKED COLLATERAL AND SETH BALANCE ARE NOT ADEQUATELY CAPPED}:

Because of the difference, the owner decides to set maxFeePaid = MAX_UINT to enable sETH repurchase.

On the other hand, a user withdraws sUSD from liquidity pool. The image shows liquidity info after withdrawal:

Finally, the attacker closes the 82.6 sETH long call position:

Since the swap is not limited by any parameter, the protocol uses all available sUSD. In the end, the new sUSD balance is almost 0 (0.29037... in the example):

Code Location

LiquidityPool.sol

(uint quoteSpent, uint baseReceived) = synthetixAdapter.exchangeToExactBaseWithLimit(
  exchangeParams,
  address(optionMarket),
  amountBase,
  revertBuyOnInsufficientFunds ? freeLiquidity : type(uint).max
);
emit BasePurchased(quoteSpent, baseReceived);

SOLVED: The issue was fixed in commit d8d2e902c6d368313d9e04bec40c21fbf70b870b. With the update to the _getLiquidity function in the LiquidityPool contract, this attack vector is not feasible.

The differences between lockedCollateral.base and sETH balance in the liquidity pool are not limited to opening long call positions. These differences could create distorted liquidity values with the following consequences:

• In some scenarios, the liquidity pool can run out of sUSD, which could affect some operations such as withdrawals, payment of premiums, settlement, etc. See \vulnref{LIQUIDITY POOL COULD RUN OUT OF SUSD TOKENS} for more details.

• Distorted liquidity values will erroneously affect protocol decisions, for example: more liquidity to withdraw, less amount to hedge, etc. See Proof of Concept below for more details.

Proof of Concept:

Initial situation for the test:

Scenario 1: quoteBaseFeeRate <= maxFeePaid The user opens a long call position of 100 sETH. Due to the swap, sETH balance and lockedCollateral.base have the \underline{same value}:

Scenario 2: quoteBaseFeeRate > maxFeePaid The user opens a long call position 100 sETH. Because there is no swap, \underline{differences} between sETH balance and lockedCollateral.base \underline{will increase}:

Comparative table of liquidity info between both scenarios:

Scenario 2 shows that freeLiquidity and burnableLiquidity have increased, but pendingDeltaLiquidity has decreased.

These distorted liquidity values (compared to Scenario 1) will erroneously affect protocol decisions, e.g: more liquidity to withdraw, less amount to hedge, etc.

Code Location

lockBase function increases the value of lockedCollateral.base and then calls _maybeExchangeBase:

LiquidityPool.sol

function lockBase(
  uint amount,
  SynthetixAdapter.ExchangeParams memory exchangeParams,
  uint freeLiquidity
) external onlyOptionMarket {
  lockedCollateral.base += amount;
  _maybeExchangeBase(exchangeParams, freeLiquidity, true);
  emit BaseLocked(amount, lockedCollateral.base);
}

\color{black}\color{white}_maybeExchangeBase function could return earlier without swapping, which creates a big difference between lockedCollateral.base and sETH balance:

LiquidityPool.sol

} else if (currentBaseBalance < lockedCollateral.base) {
  // Buy base for quote
  uint amountBase = lockedCollateral.base - currentBaseBalance;
  if (exchangeParams.quoteBaseFeeRate > lpParams.maxFeePaid) {
    uint estimatedExchangeCost = synthetixAdapter.estimateExchangeToExactBase(exchangeParams, amountBase);
    if (revertBuyOnInsufficientFunds && estimatedExchangeCost > freeLiquidity) {
      revert InsufficientFreeLiquidityForBaseExchange(
        address(this),
        amountBase,
        estimatedExchangeCost,
        freeLiquidity
      );
    }
    return;
  }

PARTIALLY SOLVED: Commit d8d2e902c6d368313d9e04bec40c21fbf70b870b partially fixes this security issue by not allowing the liquidity pool to run out of sUSD.

The _addNewStrikeToStrikeCache and _updateStrikeSkew functions of the OptionGreekCache contract update strikeSkewGWAV with the value of a new skew.

If this new skew is outside the gwavSkewFloor / gwavSkewCap range, strikeSkewGWAV will store a capped skew (not the actual value), which feeds the GWAV oracle inconsistent data.

Code Location

OptionGreekCache.sol

function _addNewStrikeToStrikeCache(
  OptionBoardCache storage boardCache,
  uint strikeId,
  uint strikePrice,
  uint skew
) internal {
  // This is only called when a new board or a new strike is added, so exposure values will be 0
  StrikeCache storage strikeCache = strikeCaches[strikeId];
  strikeCache.id = strikeId;
  strikeCache.strikePrice = strikePrice;
  strikeCache.skew = skew;
  strikeCache.boardId = boardCache.id;

  emit StrikeCacheUpdated(strikeCache);

  strikeSkewGWAV[strikeId]._initialize(
    _max(_min(skew, greekCacheParams.gwavSkewCap), greekCacheParams.gwavSkewFloor),
    block.timestamp
  );

OptionGreekCache.sol

function _updateStrikeSkew(
  OptionBoardCache storage boardCache,
  StrikeCache storage strikeCache,
  uint newSkew
) internal {
strikeCache.skew = newSkew;

strikeSkewGWAV[strikeCache.id]._write(
  _max(_min(newSkew, greekCacheParams.gwavSkewCap), greekCacheParams.gwavSkewFloor),
  block.timestamp
);

\textbf{RISK ACCEPTED:} The Lyra team accepted the risk of this finding and stated that feeding the GWAV oracle with a capped skew value (different from the one cached) in edge cases is intentional behavior of the protocol.

When the owner creates a new board, the addBoard function from OptionGreekCache contract is called. This function triggers _updateGlobalLastUpdatedAt, which sets the value of minUpdatedAtPrice to 0.

As a consequence, users will not be able to withdraw from the liquidity pool (processWithdrawalQueue) because _canProcess will always return false until someone explicitly calls the updateBoardCachedGreeks function to update the cache with the actual values.

Code Location

addBoard function triggers _updateGlobalLastUpdatedAt:

OptionGreekCache.sol

boardCache.expiry = board.expiry;
boardCache.iv = board.iv;
boardCache.updatedAt = block.timestamp;
emit BoardCacheUpdated(boardCache);
boardIVGWAV[board.id]._initialize(board.iv, block.timestamp);
emit BoardIvUpdated(boardCache.id, board.iv, globalCache.maxIvVariance);

liveBoards.push(board.id);

for (uint i = 0; i < strikes.length; i++) {
 _addNewStrikeToStrikeCache(boardCache, strikes[i].id, strikes[i].strikePrice, strikes[i].skew);
}

_updateGlobalLastUpdatedAt();

\color{black}\color{white}_updateGlobalLastUpdatedAt function sets minUpdatedAtPrice to 0:

OptionGreekCache.sol

for (uint i = 1; i < liveBoards.length; i++) {
 boardCache = boardCaches[liveBoards[i]];
 if (boardCache.updatedAt < minUpdatedAt) {
   minUpdatedAt = boardCache.updatedAt;
 }
 if (boardCache.updatedAtPrice < minUpdatedAtPrice) {
   minUpdatedAtPrice = boardCache.updatedAtPrice;
 }
 if (boardCache.updatedAtPrice > maxUpdatedAtPrice) {
   maxUpdatedAtPrice = boardCache.updatedAtPrice;
 }
 if (boardCache.maxSkewVariance > maxSkewVariance) {
   maxSkewVariance = boardCache.maxSkewVariance;
 }
 if (boardCache.ivVariance > maxIvVariance) {
   maxIvVariance = boardCache.ivVariance;
 }
}

globalCache.minUpdatedAt = minUpdatedAt;
globalCache.minUpdatedAtPrice = minUpdatedAtPrice;

SOLVED: The issue was fixed in commit 1e04d54b12c4faf0378b54b67f93d5de2b7c6e68.

The updateCacheAndGetTradeResult function in the OptionMarketPricer contract contains the following inaccurate validity checks:

• newSkew includes min / max values in its validity check. As a consequence, the function will revert incorrectly when dealing with edge values.

• pricing.callDelta does not include min / max values in its validity check. As a consequence, the function will not revert when dealing with edge values, as it should.

This issue is categorized as informational because it could cause the aforementioned function to not work as expected in edge cases.

Code Location

OptionMarketPricer.sol

// If it is a force close and skew ends up outside the "abs min/max" thresholds
if (
  trade.tradeDirection != OptionMarket.TradeDirection.LIQUIDATE &&
  (newSkew <= tradeLimitParams.absMinSkew || newSkew >= tradeLimitParams.absMaxSkew)
) {
   revert ForceCloseSkewOutOfRange(

OptionMarketPricer.sol

// delta must fall BELOW the min or ABOVE the max to allow for force closes
if (
  pricing.callDelta > tradeLimitParams.minForceCloseDelta &&
  pricing.callDelta < (int(DecimalMath.UNIT) - tradeLimitParams.minForceCloseDelta)
) {
   revert ForceCloseDeltaOutOfRange(

\textbf{ACKNOWLEDGED:} The Lyra team acknowledged this finding.

The following operations in OptionMarket contract affect boards, even if they are already expired:

• Base IV can be set on an expired board
• Skew can be set on a strike from an expired board
• Strikes can be added on an expired board

It is worth noting that this issue is classified as informational because it does not affect the settlement process, but could cause the owner to spend more gas unnecessarily if they mistakenly interact with an expired board.

Code Location

OptionMarket.sol

function setBoardBaseIv(uint boardId, uint baseIv) external onlyOwner {
  OptionBoard storage board = optionBoards[boardId];
  if (board.id != boardId) {
   revert InvalidBoardId(address(this), boardId);
  }
  if (baseIv == 0) {
   revert ExpectedNonZeroValue(address(this), NonZeroValues.BASE_IV);
  }
  if (!board.frozen) {
   revert BoardNotFrozen(address(this), boardId);
  }

  board.iv = baseIv;
  greekCache.setBoardIv(boardId, baseIv);
  emit BoardBaseIvSet(boardId, baseIv);

OptionMarket.sol

function setStrikeSkew(uint strikeId, uint skew) external onlyOwner {
  Strike storage strike = strikes[strikeId];
  if (strike.id != strikeId) {
   revert InvalidStrikeId(address(this), strikeId);
  }
  if (skew == 0) {
   revert ExpectedNonZeroValue(address(this), NonZeroValues.SKEW);
  }

  OptionBoard memory board = optionBoards[strike.boardId];
  if (!board.frozen) {
   revert BoardNotFrozen(address(this), board.id);
  }

  strike.skew = skew;
  greekCache.setStrikeSkew(strikeId, skew);
  emit StrikeSkewSet(strikeId, skew);
} 

OptionMarket.sol

function addStrikeToBoard(
  uint boardId,
  uint strikePrice,
  uint skew
) external onlyOwner {
  OptionBoard storage board = optionBoards[boardId];
  if (board.id != boardId) revert InvalidBoardId(address(this), boardId);
  Strike memory strike = _addStrikeToBoard(board, strikePrice, skew);
  greekCache.addStrikeToBoard(boardId, strike.id, strikePrice, skew);
}

\textbf{ACKNOWLEDGED:} The Lyra team acknowledged this finding.

In the following loops, the i variable is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Proof of Concept:

For example, based on the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

Code Location

LiquidityPool.sol

function processDepositQueue(uint limit) external nonReentrant {
    (uint tokenPrice, bool stale, ) = _getTokenPriceAndStale();

    for (uint i = 0; i < limit; i++) {
      QueuedDeposit storage current = queuedDeposits[queuedDepositHead];
      if (!_canProcess(current.depositInitiatedTime, lpParams.depositDelay, stale, queuedDepositHead)) {
        return;
      }

LiquidityPool.sol

function processWithdrawalQueue(uint limit) external nonReentrant {
    for (uint i = 0; i < limit; i++) {
      (uint totalTokensBurnable, uint tokenPriceWithFee, bool stale) = _getTotalBurnableTokens();

Other resources affected

 GWAV: L#136
 OptionGreekCache: L#332, 348, 352, 726, 816, 878, 913, 922, 1000
 OptionMarket: L#236, 393, 415, 458, 744, 972, 1009
 OptionToken: L#300, 509, 591, 605, 616
 ShortCollateral: L#172

SOLVED: The issue was fixed in commit 1e04d54b12c4faf0378b54b67f93d5de2b7c6e68.