Rysk

The OptionExchange contract’s redeem() function calls _swapExactInputSingle() with minimum output set to 0, making it susceptible to a front-running/sandwich attack when collateral is being liquidated. It is recommended that a minimum representing an acceptable loss on the swap is used instead.

// OptionExchange::redeem
function redeem(address[] memory _series) external {
  _onlyManager();
  uint256 adLength = _series.length;
  for (uint256 i; i < adLength; i++) {
    // ... Dedaub: Code omitted for brevity.

    if (otokenCollateralAsset == collateralAsset) {
	// ... Dedaub: Code omitted for brevity.
    } else {
      // Dedaub: Minimum output set to 0. Susceptible to sandwich attacks.
      uint256 redeemableCollateral =
        _swapExactInputSingle(redeemAmount, 0, otokenCollateralAsset);
      SafeTransferLib.safeTransfer(
        ERC20(collateralAsset),address(liquidityPool),redeemableCollateral
      );
      emit RedemptionSent(
        redeemableCollateral, collateralAsset, address(liquidityPool)
      );
    }
  }
}

The VolatilityFeed contract uses the SABR model to compute the implied volatility of an option series. This model uses a number of parameters which are regularly updated by a keeper through the updateSabrParameters() function. It is possible for an attacker to front-run this update, transact with the LiquidityPool at the old price and then transact back with the LiquidityPool at the new price (computed in advance) if the difference is profitable.

The function quoteOptionPrice of the BeyondPricer contract retrieves the implied volatility from the function VolatilityFeed::getImpliedVolatility(). However, the returned value is not accompanied by a timestamp that can be used by the quoteOptionPrice() function to determine whether the value is stale or not. Since the implied volatility returned is affected by a keeper, which is responsible for updating the parameters of the underlying SABR model, it is recommended that staleness checks are implemented in order to avoid providing wrong implied volatility values.

The BeyondPrice contract gets the price of the underlying token via the function _getUnderlyingPrice(), which consults a Chainlink price feed for the price.

// BeyondPrice::_getUnderlyingPrice
function _getUnderlyingPrice(address underlying, address _strikeAsset)
  internal view returns (uint256)
{
  return PriceFeed(protocol.priceFeed()).
    getNormalizedRate(underlying, _strikeAsset);
}

However, when trying to obtain the same price in the function _getCollateralRequirements(), the addressBook is used to get the price feed from an Oracle implementing the IOracle interface.

// BeyondPrice::_getCollateralRequirements
function getCollateralRequirements(
  Types.OptionSeries memory _optionSeries, uint256 _amount
) internal view returns (uint256) {
  IMarginCalculator marginCalc =
    IMarginCalculator(addressBook.getMarginCalculator());

  return
    marginCalc.getNakedMarginRequired(
      _optionSeries.underlying,
      _optionSeries.strikeAsset,
      _optionSeries.collateral,
      _amount / SCALE_FROM,
      _optionSeries.strike / SCALE_FROM, // assumes in e18
      IOracle(addressBook.getOracle()).getPrice(_optionSeries.underlying),
      _optionSeries.expiration,
      18, // always have the value return in e18
      _optionSeries.isPut
    );
}

The same addressBook technique is used in the getCollateral() function of the OptionRegistry contract and in the checkVaultHealth() function of the Option registry contract.

It is recommended that this is refactored to use the Chainlink feed in order to avoid a situation where different prices for the underlying are obtained by different parts of the code.

In the OptionExchange contract’s _handleDHVBuyback() function, a division is used before a multiplication operation at lines 925 and 932. It is recommended to use multiplication prior to division operations to avoid a possible loss of precision in the calculation. Alternatively, the mulDiv function of the PRBMath library could be used.

The implied volatility used by the BeyondPricer contract to price options is determined by the SABR model. However, the SABR model is a function of several parameters set by bots controlled by the Rysk team. This means that the Rysk team has the ability to affect option prices through the control of these parameters.

The OptionRegistry’s redeem() function is not access controlled and calls the OpynInteractions library contract’s redeem() function, which interacts with the GammaController and the option and collateral tokens. Dedaub’s static analysis tools warned about a potential reentrancy risk. Our manual inspection identified no such immediate risk, but as the tokens supported are not strictly defined and a future version of the code could potentially make such an attack possible, it is advisable to add a reentrancy guard around OptionRegistry’s redeem() function.

The OptionRegistry::open() function performs the assignment vaultIds[series] = vaultId_ on line 271. But this can be moved into the if block starting at line 255, since the vaultId_ only changes value if this if block is executed.

// OpenRegistry::open
function open(
  address _series,
  uint256 amount,
  uint256 collateralAmount
) external returns (bool, uint256) {
  _isLiquidityPool();
  // make sure the options are ok to open
  Types.OptionSeries memory series = seriesInfo[_series];
  // assumes strike in e8
  if (series.expiration <= block.timestamp) {
    revert AlreadyExpired();
  }
  // ... Dedaub: Code omitted for brevity.
  if (vaultId_ == 0) {
    vaultId_ = (controller.getAccountVaultCounter(address(this))) + 1;
    vaultCount++;
  }
  // ... Dedaub: Code omitted for brevity.
  // Dedaub: Below assignment can be moved inside the above block.
  vaultIds[_series] = vaultId_;
  // returns in collateral decimals
  return (true, collateralAmount);
}

The OptionExchange’s _swapExactInputSingle() function definition is annotated with several misleading comments. For instance, it mentions that _amountIn has to be in WETH when it can support any collateral token. It also mentions that _assetIn is the stablecoin that is bought, when it is in fact the collateral that is swapped. The description of the function, which reads “function to sell exact amount of WETH to decrease delta” is incorrect.

// OptionExchange::_swapExactInputSingle
/** @notice function to sell exact amount of wETH to decrease delta
 *  @param _amountIn the exact amount of wETH to sell
 *  @param _amountOutMinimum the min amount of stablecoin willing to
          receive. Slippage limit.
 *  @param _assetIn the stablecoin to buy
 *  @return the amount of usdc received
 */
function _swapExactInputSingle(
  uint256 _amountIn,
  uint256 _amountOutMinimum,
  address _assetIn) internal returns (uint256) {
  // ... Dedaub: Code omitted for brevity.
}

The division of the _amount by 2, mentioned in the code comment, does not appear in the code. It appears that this comment corresponds to a previous version of the codebase and it should be removed.

//BeyondPricer::_getSlippageMultiplier
function _getSlippageMultiplier(
  uint256 _amount,
  int256 _optionDelta,
  int256 _netDhvExposure,
  bool _isSell
) internal view returns (uint256 slippageMultiplier) {
  // divide _amount by 2 to obtain the average exposure throughout the tx.
  // Dedaub: The above comment is not relevant any more.
  // ... Dedaub: Code omitted for brevity.
}

The formula of the SABR model that is responsible for computing the implied volatility (https://web.math.ku.dk/~rolf/SABR.pdf formula (2.17a)) is an approximate one. It is not clear to us if this value will always be non-negative as it should be. For example, for absolute values of ρ close to 1 and large values of v, the last term of this formula, and probably the whole value of the implied volatility will be negative.

The execution of VolatilityFeed::getImpliedVolatility will revert if the value returned by lognormalVol() is non-negative, to protect the protocol from using this absurd value. Nevertheless, if this keeps happening for a while, the protocol will be unable to price the options and therefore will be unable to work.

This issue could be avoided either by a careful choice of the SABR parameters by the protocol’s keepers or by using an alternative volatility feed in case this happens.

In BeyondPricer::quoteOptionPrice() a check that _optionseries.expiration >= block.timestamp is missing. If the function is called to price an option series with a past expiration date, it will return an absurd result. We suggest adding a check that would revert the execution with an appropriate message in case the condition is not satisfied.

Function OptionExchange::_checkHash, which returns if an option series is approved or not, is defined as public. However, the starting underscore in “_checkHash” implies that this functionality should not be exposed externally (via the public modifier) creating an inconsistency, even though it is probably useful/necessary to the users of the protocol.

Whenever a user wants to buy an amount of options, first it is checked if the long exposure of the protocol to this option series is positive. If this is the case, then the protocol first sells the options it holds, to decrease its long exposure, and if they are not enough, then the Liquidity pool writes extra options to reach the amount requested by the user. The problem is that the _buyOption function, in the case the Liquidity pool is called to write these extra options, returns only this extra amount, and not the total amount sold to the user.

The code of the BeyondPricer, OptionExchange and OptionCatalogue contracts is compiled with the floating pragma >=0.8.0, and the OptionRegistry contract is compiled with the floating pragma >=0.8.9. It is recommended that the compiler version is fixed to a specific version and that this is kept consistent amongst source files.

The code of the BeyondPricer, OptionExchange and OptionCatalogue contracts is compiled with the floating pragma >=0.8.0, and the OptionRegistry contract is compiled with the floating pragma >=0.8.9. Versions 0.8.0 and 0.8.9 in particular, have some known bugs, which we do not believe affect the correctness of the contracts.