bitsurfer - Liquidations will be frozen, when the oracle go offline or a token's price dropping to zero

[sherlock-admin]

bitsurfer

medium

Liquidations will be frozen, when the oracle go offline or a token's price dropping to zero

Summary

Liquidations will be frozen, when the oracle go offline or a token's price dropping to zero

Vulnerability Detail

In certain exceptional scenarios, such as when oracles go offline/paused or token prices plummet to zero, liquidations will be temporarily suspended.

When liquidator want to liquidate() an account because of bad debt, it will check the _isLiquidatable() and loop over through the userEnteredMarkets to check if debtValue > liquidationCollateralValue for an account. To calculate this oracle is called to get the asset price.

If this call to oracle is corrupted due to offline/paused, the liquidate() call function will reverted. More over if the price is returning 0 it will be reverted because require(assetPrice > 0, "invalid price");

Impact

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.

Code Snippet

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/pool/IronBank.sol#L1065-L1092

File: IronBank.sol
499:         require(_isLiquidatable(borrower), "borrower not liquidatable");

File: IronBank.sol
1065:     function _isLiquidatable(address user) internal view returns (bool) {
...
1069:         address[] memory userEnteredMarkets = allEnteredMarkets[user];
1070:         for (uint256 i = 0; i < userEnteredMarkets.length; i++) {
...
1079:             uint256 assetPrice = PriceOracleInterface(priceOracle).getPrice(userEnteredMarkets[i]);
1080:             require(assetPrice > 0, "invalid price");
...
1090:         }
1091:         return debtValue > liquidationCollateralValue;
1092:     }

File: PriceOracle.sol
66:     function getPriceFromChainlink(address base, address quote) internal view returns (uint256) {
67:         (, int256 price,,,) = registry.latestRoundData(base, quote);
68:         require(price > 0, "invalid price");

Tool used

Manual Review

Recommendation

Implement a protective measure to mitigate this potential risk. For example, enclose any oracle get price function within a try-catch block, and incorporate fallback logic within the catch block to handle scenarios where access to the Chainlink oracle data feed is denied. Or use second oracle for backup situation.

[ibsunhub]

It doesn't make any sense to liquidate an asset whose price is 0. It won't add value to debt nor collateral. It just can't have a zero-price asset on a lending protocol.

We thought about creating a backing oracle to be ready to replace ChainLink if ChainLink's price feed goes wrong. However, it also creates a trust issue since we control the price of the backing oracle and we could switch to it at will. For now, we stick with ChainLink. If the price from ChainLink is inaccurate (including being 0), we pause the borrow / repay / liquidate on this token.

[0xffff11]

I do believe this is an actual issue. Oracle failure should not pause liquidations, Another fix I can recommend (having a fallback oracle is the best), it is that for every fetch, the last price is stored on a variable. If the oracle fails, the last price on the variable will be the one used instead.

[ibsunhub]

There are different failure for ChainLink price feed. If the price is not updated, retrieving the stale price from ChainLink is basically the same with the solution that stores a variable. Another concern is that we believe this fix (store the previous price on a variable) will not solve the root cause and will increase a significant amount of gas consumption for user operations (additional read / write storage during every price retrieval).

I think the best solution here is still a backing oracle. However, that's another topic to create a sub-system to deal with the trust issue and the availability issue.

[0xffff11]

Yes, I agree with your point, backing oracle should be the way to go. I think tellor oracles are very common as the fallback oracle. Might be worth checking those out. Therefore, I believe that you confirm that the issue is valid, right? @ibsunhub

[ib-tycho]

We maintain our position that this issue is not valid. It should be noted that prominent lending protocols like Compound v3 Comet, AAVE, and Euler do not implement fallback oracles or address Chainlink's potential downtime. While having fallback oracles can be beneficial, we consider them to be optional rather than essential for protocol functionality.

[0xffff11]

Issue will be kept as a medium at the moment because there is a chance of the price going to 0 and the scenario would be given.

[IAm0x52]

Escalate for 10 USDC

This should be low. I agree with the sponsor's comments. Chainlink will never show the price of the oracle as zero unless the price is truly zero. If the token really is worth 0 then there is no point in liquidating it anyways so that scenario is invalid. Second it is not a vulnerability that they don't have a backup oracle, that is simply a design choice and as mentioned many blue-chip protocols don't use backup oracles.

[sherlock-admin]

Escalate for 10 USDC

This should be low. I agree with the sponsor's comments. Chainlink will never show the price of the oracle as zero unless the price is truly zero. If the token really is worth 0 then there is no point in liquidating it anyways so that scenario is invalid. Second it is not a vulnerability that they don't have a backup oracle, that is simply a design choice and as mentioned many blue-chip protocols don't use backup oracles.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xffff11]

I agree with the escalation from the watson. Chainlink does not return 0 anymore if the price is not 0. And the fallback oracle, while it could be an improvement it is a design decision that most of big protocols don't even use. Low

[0x3agle]

I agree with the escalation from the watson. Chainlink does not return 0 anymore if the price is not 0. And the fallback oracle, while it could be an improvement it is a design decision that most of big protocols don't even use. Low

I agree that this scenario is not possible, but in my report (https://github.com/sherlock-audit/2023-05-ironbank-judging/issues/121), I have shown that if the Chainlink oracle is taken down by a multi-sig admin, then the contract does not have any way to handle that scenario and will always revert, causing a denial-of-service (DoS). Therefore, I believe that at least this case must be considered as valid. The suggested mitigation is to add a try-catch block.

Additionally, could you please explain why my issue was considered a duplicate of this one? Both issues involve different scenarios, but they require the same mitigation. One scenario may not be valid, but it is important to consider all the scenarios related to this part of the code.

[0xbitsurfer]

I wish IllIllI had submit this issue again in this contest, thus no one can argue

• https://github.com/sherlock-audit/2023-02-gmx-judging/issues/156

even the negative value issue does exist, and confirmed from previous contest

• https://github.com/sherlock-audit/2023-02-gmx-judging/issues/177

according to @hrishibhat : https://github.com/sherlock-audit/2023-02-gmx-judging/issues/156#issuecomment-1512787660

Although unlikely, a zero price should not break the liquidations. Considering this a valid medium

---

Another reason why submitter name should be blurred, @Evert0x @jacksanford1

[IAm0x52]

I don't know the context of the issues you've linked to but the asset referenced there are real world assets. Crypto assets can never go negative in price because they cannot have carrying costs. Ironbank lists crypto asset not RWA so low still seems appropriate to me. If GMX does not intend to list RWA then the issue you've linked should also have been invalid/low.

[hrishibhat]

Result: Low Has duplicates Agree with the comments on the escalation. If the price of the asset is zero then there is no point liquidating it.

Also, Iron bank works differently from gmx. Some context on the gmx issue tagged: The gmx issue is related to futures: oil futures did go negative, and a future is not a RWA. if something hits zero, the other side of the position should be able to exit the position at maximum possible value - they shouldn't be prevented from doing so due to a bug. Also there could be fee associated with it.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: accepted