AuditorPraise - `BBLiquidation.liquidteBadDebt()`, `BBLiquidation.liquidate()`, `SGLLiquidation.liquidateBadDebt()`, `SGLLiquidation.liquidate()` might use stale exchangeRate for liquidations if oracle reverts for any reason.

[sherlock-admin2]

AuditorPraise

medium

BBLiquidation.liquidteBadDebt(), BBLiquidation.liquidate(), SGLLiquidation.liquidateBadDebt(), SGLLiquidation.liquidate() might use stale exchangeRate for liquidations if oracle reverts for any reason.

Summary

The issue lies in how the catch side of the try-catch block is handled.

Vulnerability Detail

Here in Market._updateOracleRateForLiquidations() there's an issue with how the catch side of the try-catch block is handled.

 function _updateOracleRateForLiquidations() internal {
        try oracle.get(oracleData) returns (bool _updated, uint256 _exchangeRate) {
            if (_updated && _exchangeRate > 0) {
                exchangeRate = _exchangeRate; //update cached rate
                rateTimestamp = block.timestamp;
            } else {
                _exchangeRate = exchangeRate; //use stored rate
                if (_exchangeRate == 0) revert ExchangeRateNotValid();
            }
        } catch {
            if (exchangeRate == 0) revert ExchangeRateNotValid();
        }
    }

The catch block only reverts if exchangeRate state var is 0, which won't be effective considering the exchangeRate may have been updated before.

1. Now the issue is that if the catch block runs for any reason it means that the call try oracle.get(oracleData){ failed, so exchange rate won't be updated in these liquidation functions. Stale exchangeRate will be used in liquidations because the catch block won't revert if exchangeRate has been set previously

2. Also within the try block if update is false or _exchangeRate is 0 making the else statement run, _exchangeRate is set to exchangeRate state var without checking itsrateTimestamp against rateValidDuration. so exchange rates that are beyond rateValidDuration could be used in liquidations too.

   Impact

   BBLiquidation.liquidteBadDebt() , BBLiquidation.liquidate(), SGLLiquidation.liquidateBadDebt(), SGLLiquidationliquidate() might use stale exchangeRate for liquidations if oracle reverts for any reason and no one may even notice, considering that it doesn't revert

   Code Snippet

   https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/Market.sol#L437

   Tool used

Manual Review

Recommendation

1. make the catch block of the try-catch used in _updateOracleRateForLiquidations() to revert when the call to oracle fails.
2. Also in the try block if update is false or _exchangeRate is 0 making the else statement run, check the rateTimestamp of exchangeRate state var against rateValidDuration before setting _exchangeRate to exchangeRate state var

[cryptotechmaker]

Invalid; we purposely want the liquidation to run even if oracle call fails

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

looks like low probability of hapenning and impact is dos of one txn

[dmitriia]

Escalate Both 11 and 30 are not valid: liquidation not requiring live rate is a feature introduced as a C4-1026 mitigation in PR 324, not a bug. Also, 11 and 30 are dups between themselves, aren't dups to 79 (which is about handling exchange rate on risk increasing operations, not for liquidation).

[sherlock-admin2]

Escalate Both 11 and 30 are not valid: liquidation not requiring live rate is a feature introduced as a C4-1026 mitigation in PR 324, not a bug. Also, 11 and 30 are dups between themselves, aren't dups to 79 (which is about handling exchange rate on risk increasing operations, not for liquidation).

You've created a valid escalation!

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.

[nevillehuang]

@dmitriia Agree, thanks for bringing up additional information.

[cvetanovv]

I agree with the escalation of @dmitriia #11 and #30 being invalidated.

[cvetanovv]

Planning to accept the escalation and invalidate the issue.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• dmitriia: accepted