hansfriese - Lyra vault underestimates the collateral value

[github-actions]

hansfriese

medium

Lyra vault underestimates the collateral value

Summary

Lyra vault subtracts the withdrawal fee while calculating the collateral value in USD, and it does not match the actual Lyra Pool implementation.

Vulnerability Detail

The user's collateral value is estimated using the function priceCollateralToUSD() at Vault_Lyra.sol#L77 as follows.

function priceCollateralToUSD(bytes32 _currencyKey, uint256 _amount) public view override returns(uint256){
        //The LiquidityPool associated with the LP Token is used for pricing
    ILiquidityPoolAvalon LiquidityPool = ILiquidityPoolAvalon(collateralBook.liquidityPoolOf(_currencyKey));
    //we have already checked for stale greeks so here we call the basic price function.
    uint256 tokenPrice = LiquidityPool.getTokenPrice();
    uint256 withdrawalFee = _getWithdrawalFee(LiquidityPool);
    uint256 USDValue  = (_amount * tokenPrice) / LOAN_SCALE;
    //we remove the Liquidity Pool withdrawalFee
    //as there's no way to remove the LP position without paying this.
    uint256 USDValueAfterFee = USDValue * (LOAN_SCALE- withdrawalFee)/LOAN_SCALE;
    return(USDValueAfterFee);
}

So it is understood that the withdrawal fee is removed to get the reasonable value of the collateral.
But according to the Lyra Pool implementation, the token price used for withdrawal is calculated using the function _getTotalBurnableTokens.
And the function _getTotalBurnableTokens is as belows.

function _getTotalBurnableTokens()
    internal
    returns (
      uint tokensBurnable,
      uint tokenPriceWithFee,
      bool stale
    )
  {
    uint burnableLiquidity;
    uint tokenPrice;
    (tokenPrice, stale, burnableLiquidity) = _getTokenPriceAndStale();

    if (optionMarket.getNumLiveBoards() != 0) {
      tokenPriceWithFee = tokenPrice.multiplyDecimal(DecimalMath.UNIT - lpParams.withdrawalFee);
    } else {
      tokenPriceWithFee = tokenPrice;//@audit withdrawalFee is not applied if there are no live borads
    }

    return (burnableLiquidity.divideDecimal(tokenPriceWithFee), tokenPriceWithFee, stale);
  }

From the code, it is clear that the withdrawal fee is subtracted only when the related option market has live boards.
Because Vault_Lyra.sol applies a withdrawal fee all the time to price the collateral, it means the user's collateral is under-valued.

Impact

User's collaterals are under-valued than reasonable and might get to a liquidatable status sooner than expected. A liquidator can abuse this to get an unfair profit by liquidating the user's collateral with the under-estimated value and withdrawing it from the Lyra pool without paying a withdrawal fee.

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Lyra.sol#L77

Tool used

Manual Review

Recommendation

Make sure to apply withdrawal fee consistent to how Lyra pool does.

[kree-dotcom]

Currently this fix looks like it will be quite a lot of alterations. If there is time and it doesn't introduce too many changes for Sherlock to check we will try to fix it.

The reason is the call added must be made to the optionMarket contract of each collateral, this address is stored as an internal address in liquidityPool and other contracts, therefore to access it we must add another field to Lyra collateral in the collateral book, this would involve altering all collateralBook functions to expect another field as well as other vaults and systems to adhere to this larger model.

Alternatively we can add another mapping to the Vault_Lyra to store the OptionsMarkets and an admin only function to add new ones but this feels like poor design.

Leaving this issue unfixed is unlikely to cause large problems. LiquidityTokens should only have zero live boards if an optionMarket is closed (usually to be depreciated) with zero live boards the collateral should be earning no interest for the owner and therefore they would likely desire to close their loan and redeem their collateral and so not be in a situation to be liquidated.

[IAm0x52]

Sponsor has acknowledged and accepted the risk without fixing.