0x52 - Users are unable close or add to their Lyra vault positions when price is stale or circuit breaker is tripped

[github-actions]

0x52

high

Users are unable close or add to their Lyra vault positions when price is stale or circuit breaker is tripped

Summary

Users are unable close or add to their Lyra vault positions when price is stale or circuit breaker is tripped. This is problematic for a few reasons. First is that the circuit breaker can be tripped indefinitely which means their collateral could be frozen forever and they will be accumulating interest the entire time they are frozen. The second is that since they can't add any additional collateral to their loan, the loan may end up being underwater by the time the price is no longer stale or circuit breaker is no longer tripped. They may have wanted to add more assets and now they are liquidated, which is unfair as users who are liquidated are effectively forced to pay a fee to the liquidator.

Vulnerability Detail

function _checkIfCollateralIsActive(bytes32 _currencyKey) internal view override {
        
         //Lyra LP tokens use their associated LiquidityPool to check if they're active
         ILiquidityPoolAvalon LiquidityPool = ILiquidityPoolAvalon(collateralBook.liquidityPoolOf(_currencyKey));
         bool isStale;
         uint circuitBreakerExpiry;
         //ignore first output as this is the token price and not needed yet.
         (, isStale, circuitBreakerExpiry) = LiquidityPool.getTokenPriceWithCheck();
         require( !(isStale), "Global Cache Stale, can't trade");
         require(circuitBreakerExpiry < block.timestamp, "Lyra Circuit Breakers active, can't trade");
}

The above lines are run every time a user a user tries to interact with the vault. Currently this is overly restrictive and can lead to a lot of undesired situations, as explained in the summary.

Impact

Frozen assets, unfair interest accumulation and unfair liquidations

Code Snippet

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

Tool used

Manual Review

Recommendation

The contract is frozen when price is stale or circuit breaker is tripped to prevent price manipulation. While it should restrict a majority of actions there are a two that don't need any price validation. If a user wishes to close out their entire loan then there is no need for price validation because the user has no more debt and therefore doesn't need to maintain any level of collateralization. The other situation is if a user adds collateral to their vault and doesn't take out any more loans. In this scenario, the collateralization can only increase, which means that price validation is not necessary.

I recommend the following changes to closeLoan:

-   _checkIfCollateralIsActive(currencyKey);
    uint256 isoUSDdebt = (isoUSDLoanAndInterest[_collateralAddress][msg.sender] * virtualPrice) / LOAN_SCALE;
    require( isoUSDdebt >= _USDToVault, "Trying to return more isoUSD than borrowed!");
    uint256 outstandingisoUSD = isoUSDdebt - _USDToVault;
    if(outstandingisoUSD >= TENTH_OF_CENT){ //ignore leftover debts less than $0.001
+       //only need to check collateral value if user has remaining debt
+       _checkIfCollateralIsActive(currencyKey);
        uint256 collateralLeft = collateralPosted[_collateralAddress][msg.sender] - _collateralToUser;
        uint256 colInUSD = priceCollateralToUSD(currencyKey, collateralLeft); 
        uint256 borrowMargin = (outstandingisoUSD * minOpeningMargin) / LOAN_SCALE;
        require(colInUSD > borrowMargin , "Remaining debt fails to meet minimum margin!");
    }

I recommend removing liquidation threshold check from increaseCollateralAmount:

    //debatable check begins here 
-   uint256 totalCollat = collateralPosted[_collateralAddress][msg.sender] + _colAmount;
-   uint256 colInUSD = priceCollateralToUSD(currencyKey, totalCollat);
-   uint256 USDborrowed = (isoUSDLoanAndInterest[_collateralAddress][msg.sender] * virtualPrice) / LOAN_SCALE;
-   uint256 borrowMargin = (USDborrowed * liquidatableMargin) / LOAN_SCALE;
-   require(colInUSD >= borrowMargin, "Liquidation margin not met!");
    //debatable check ends here

[kree-dotcom]

The liquidation threshold for increaseCollateralAmount() has been removed as part of the fix for issue #229

The closeLoan() aspect has been fixed here kree-dotcom/isomorph@c144106

[IAm0x52]

Fixes look good. No collateral check if loan is fully repaid.