0x52 - All collateral in Velodrome vault will be permantly locked if either asset in liquidity pair stays outside of min/max price

[github-actions]

0x52

medium

All collateral in Velodrome vault will be permantly locked if either asset in liquidity pair stays outside of min/max price

Summary

The oracles used have a built in safeguard to revert the transaction if the queried asset is outside of a defined price range. The issue with this is that every vault interaction requires the underlying collateral to be valued. If one of the assets in the pair goes outside it's immutable range then the entire vault will be frozen and all collateral will be permanently stuck.

Vulnerability Detail

function getOraclePrice(IAggregatorV3 _priceFeed, int192 _maxPrice, int192 _minPrice) public view returns (uint256 ) {
    (
        /*uint80 roundID*/,
        int signedPrice,
        /*uint startedAt*/,
        uint timeStamp,
        /*uint80 answeredInRound*/
    ) = _priceFeed.latestRoundData();
    //check for Chainlink oracle deviancies, force a revert if any are present. Helps prevent a LUNA like issue
    require(signedPrice > 0, "Negative Oracle Price");
    require(timeStamp >= block.timestamp - HEARTBEAT_TIME , "Stale pricefeed");

    //@audit revert if price is outside of immutable bounds
    require(signedPrice < _maxPrice, "Upper price bound breached");
    require(signedPrice > _minPrice, "Lower price bound breached");
    uint256 price = uint256(signedPrice);
    return price;
}

The lines above are called each time and asset is priced. If the oracle returns outside of the predefined range then the transaction will revert.

    uint256 outstandingisoUSD = isoUSDdebt - _USDToVault;
    //@audit contract prices withdraw collateral
    uint256 colInUSD = _calculateProposedReturnedCapital(_collateralAddress, _loanNFTs, _partialPercentage);
    if(outstandingisoUSD >= TENTH_OF_CENT){ //ignore debts less than $0.001
        uint256 collateralLeft = totalCollateralValue(_collateralAddress, msg.sender) - colInUSD;
        uint256 borrowMargin = (outstandingisoUSD * minOpeningMargin) / LOAN_SCALE;
        require(collateralLeft > borrowMargin , "Remaining debt fails to meet minimum margin!");
    }

When closing a loan the vault attempts to price the users collateral. Since this is the only way for a user to remove collateral is to call closeLoan, if the price of either asset in the LP goes outside of its bounds then all user deposits will be lost.

Impact

Entire vault will be frozen and all collateral will be permanently stuck

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Velo.sol#L527-L587

Tool used

Manual Review

Recommendation

If a user is closing their entire loan then there is no need to check the value of the withdraw collateral because there is no longer any debt to collateralize. Move the check inside the inequality to allow the closeLoan to always function:

    uint256 outstandingisoUSD = isoUSDdebt - _USDToVault;
-   uint256 colInUSD = _calculateProposedReturnedCapital(_collateralAddress, _loanNFTs, _partialPercentage);
+   uint256 colInUSD;
    if(outstandingisoUSD >= TENTH_OF_CENT){ //ignore debts less than $0.001
+       uint256 colInUSD = _calculateProposedReturnedCapital(_collateralAddress, _loanNFTs, _partialPercentage);
        uint256 collateralLeft = totalCollateralValue(_collateralAddress, msg.sender) - colInUSD;
        uint256 borrowMargin = (outstandingisoUSD * minOpeningMargin) / LOAN_SCALE;
        require(collateralLeft > borrowMargin , "Remaining debt fails to meet minimum margin!");
    }

[kree-dotcom]

Proposed solution no longer works as the solution to issue #161 changed line 553 in Vault_Velo.sol to if((outstandingisoUSD > 0) && (colInUSD > 0)){ meaning that the calculation of colInUSD must occur before entering the if clause.

However with issue #145 fixed this problem is less terminal as now we update the minPrice and maxPrice with each call.
We then have two situations in which the minPrice or maxPrice protection is triggered:

1. Slow approach, i.e. like ETH reaching $1,000,000 over the span of a year. It is likely that Chainlink would update the aggregator maxPrice before it was reached and so this situation does not pose a problem.
2. Flash crash, a collateral is hacked or manipulated so that it drops below the minPrice faster than Chainlink can react and so the collateral is now stuck for our system. While this situation is not solved the assets backing the loan are now likely worthless anyway. This does not address the situation when the price rises suddenly. In that case the collateral will become inaccessible for a short while, it is assumed that if the price persists Chainlink would then alter the maxPrice and so collateral would become accessible again.

Therefore this has become a "won't fix" issue

[IAm0x52]

The risk of this issue has been greatly reduced by the fixes made for #145. The risk of a flash crash has been acknowledged by sponsor and they have accepted this risk, without a fix.