liquidation will fail if the Seer or Oracle reverts instead of returning false

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/Seer.sol#L52-L55 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/OracleMulti.sol#L107-L118 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/modules/ModuleChainlinkMulti.sol#L58-L64 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/utils/ChainlinkUtils.sol#L52 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L309-L316 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L340-L341

Vulnerability details

Impact

Even though Market has a logic to use the old price information in the case of Oracle failure, it will not happen because the Seer will always revert, if the chainlink should return stale price informations. This mismatching behavior between two contracts (Oracle/Seer is reverting on failure, while the Market expects to get false instead) will result in liquidation failure.

When Seer.get() is called, the following functions are subsequently called:

• OracleMulti._readAll() (called from Seer.get() line 55 Seer.sol)
• ModuleChainlinkMulti._quoteChainlink() (called from OracleMulti._readAll() line 118 in OracleMulti.sol)
• ChainlinkUtils._readChainlinkFeed() (called from ModuleChainlinkMulti._quoteChainlink() line 64 in ModuleChainlinkMulti.sol)

The issue is that ChainlinkUtils._readChainlinkFeed() potentially reverts (line 52 in ChainlinkUtils.sol), if for example the condition for staleness is being triggered (line 51 in ChainlinkUtils.sol).

When a liquidator wants to liquidate a user in a market like BigBang, the liquidation tx can potentially fail due to the revert inside the Seer.sol oracle that was described above.

As an example, when calling BigBang.liquidate() the tx potentially fails if the market uses the Seer.sol oracle, because the subsequent call to Market.updateExchangeRate() (BigBang.sol line 316) can face a revert condition on line 341 in Market.sol, where the oracle.get() function is called. If the oracle is the Seer.sol a revert can potentially happen, for example due to staleness as described above, and the liquidation will fail.

Liquidations should never fail and instead use the old exchange rate - see BigBang.sol line 315 comment:

// Oracle can fail but we still need to allow liquidations

But the liquidation transaction can potentially fail when trying to fetch the new exchange rate via Market.updateExchangeRate() as shown above.

This issue applies for all markets (Singularity, BigBang) that are using the Seer.sol oracle, since the revert during the liquidation happens in the Market.sol contract from which all markets inherit.

Because of this issue user's collateral values may fall below their debt values, without being able to liquidate them, pushing the protocol into insolvency.

This is classified as high risk because liquidation is an essential functionality of the protocol.

This issue should be in scope because Seer.sol is in scope and the subsequent calls are all calls to (abstract) contracts (i.e. OracleMulti, ModuleChainlinkMulti, ChainlinkUtils) from which Seer.sol is inheriting.

This issue has very few similarities to another issue that I submitted, but the mitigation for this issue has to be done in a much different way than the other issue, thus I don't consider this issue to be a duplicate.

Proof of Concept

Here is a POC that shows that a liquidation tx will potentially revert when the Seer.get() is called according to the issue described above.

Note: When running the POC unit test it will revert with InvalidChainlinkRate which is caused by the call to await seer.get('0x00'); (last line of the POC):

https://gist.github.com/zzzitron/bd88e59a8bb96239708028ca5d20d8e4

Tools Used

Manual review

Recommended Mitigation Steps

Consider using try catch for the get call to the oracle inside Market.updateExchangeRate().

// Market.sol
340    function updateExchangeRate() public returns (bool updated, uint256 rate) {
341        try oracle.get("") returns (bool _updated, uint256 _rate) {
342             updated = _updated;
343             if (updated) {
344                 // ...
351             }
352             else {
353                 rate = exchangeRate;
354             }
355        } catch {
356             rate = exchangeRate; // oracle fail, still liquidate with old rate
357             updated = false;
358        }

Assessed type

Oracle

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as duplicate of #34

[c4-judge]

dmvt marked the issue as unsatisfactory: Out of scope

[zzzitron]

Please be aware that both Market.sol and Seer.sol are in scope. The issue here is not about the 3rd party oracle but the issue is that Market.sol and Seer.sol currently don't really fit together.

Market.updateExchangeRate() calls Seer.get(), expecting that Seer.get() will never revert. Instead of reverting, Seer.get() should return false if fetching the latest exchange rate fails. However Seer.get() always returns true and doesn't handle failure (e.g. when underlying oracle reverts) properly.

It doesn't matter which underlying oralce is being used, because Seer.sol is the wrapper contract, which should take care of managing and integrating the underlying oracle, which it currently doesn't, which is why we consider this issue in scope and valid.

Note: Chainlink oracle is mentioned in this report because this is just one example where the revert is forwarded to Market.sol instead of handled by Seer.sol. Which is why we recommend that either Market.sol or Seer.sol handles the revert. Quickly replacing the underlying oracle is not the solution, because the issue is that 2 in-scope contracts (Market.sol and Seer.sol) are not properly working together.

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[dmvt]

Agreed. This makes sense as a medium. High is too severe given that we are quite literally relying on an external factor. Thank you for the additional context.

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt marked the issue as selected for report