hats-finance / VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f

LP token lending protocol
MIT License
2 stars 1 forks source link

**Liquidations will be frozen, when the chainlink oracle goes offline** #3

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @https://github.com/maarcweiss Submission hash (on-chain): 0x6bddb0c819575a51ced25c0fffb628214677f641f66c532e0c3b43e5a6b7d969 Severity: medium severity

Description: Liquidations will be frozen, when the chainlink oracle goes offline

In certain exceptional scenarios, such as when oracles go offline/paused liquidations and borrowing will be temporarily suspended.

When liquidator want to liquidationCall() (or borrower borrow() ), an account because of bad debt, it will check the _calculateAvailableCollateralToLiquidate() and loop over it, fetching during calculation vars.collateralPrice = oracle.getAssetPrice(collateralAsset); As there is no fallback if chainlink fails (due to offline/paused), the function call will revert and liquidations will be paused.

SEVERITY

Impact high, likelihood low = Medium

During critical periods, there is a risk that liquidations may not be feasible when they are most needed by the protocol. This can lead to a situation where the value of users' assets declines below their outstanding debts, effectively disabling any motivation for liquidation. Consequently, this can potentially push the protocol into insolvency, posing significant challenges and potential financial instability.

A LINK TO THE GITHUB ISSUE

https://github.com/VMEX-finance/vmex/blob/b0dc00c5dd6bdcac05827128d14dcdc730f19e1c/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#L208-L230

SOLUTION

Implement a protective measure to mitigate this potential risk. For example, enclose chainlink’s get price function within a try-catch block.

ksyao2002 commented 1 year ago

In a previous peer review (see https://mint-salesman-909.notion.site/VMEX-Peer-Review-6d3783a5c72c446cab7caf4d739f0889) we were recommended to revert in case Chainlink goes down to prevent the use of stale and invalid prices in case of bad market conditions. We always have the ability to set a fallback oracle that will take over in case chainlink goes down. A try catch block would be detrimental to the protocol as it allows users to perform borrows with stale or invalid oracle information.

maarcweiss commented 1 year ago

Hi @ksyao2002 the issue imo is valid and it is the best solution that currently exists. If chainlink fails, have a fallback oracle in the catch and store the last price to check deviation. The peer review was wrong and it is not the optimal solution. Here you have a nice guide for setting the fix up:

I recommend tellor as the fallback oracle in the catch statement.

Proof of the issue being a valid med:

I believe this is more than enough to validate the issue. If not try catch is implemented with fallback oracle logic, there will be a risk of any function fetching the price ex. liquidations, not being able to happen

ksyao2002 commented 1 year ago

Hi @maarcweiss thanks for your response. I do agree with the points you're bringing up, but I do also think this is an area of active discussion and there is no right or wrong answer. This is evidenced by us receiving different advice from other security researchers, and by the fact that many large and successful protocols (like Aave, compound, etc) consume chainlink data without the try catch block.

Regardless, we have actually incorporated the try-catch block, as it has been reported in other submissions (you have the first submission though, so you will get the bounty). However, I will argue that the severity should be low, because of the differing opinions, and since other submissions have marked it as low: https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/issues/29. Please let me know if you think that is fair.