roguereddwarf - BaseVault.sol: Users are unable to close loan when oracle is stale or disabled so they cannot withdraw their collateral

[sherlock-admin]

roguereddwarf

medium

BaseVault.sol: Users are unable to close loan when oracle is stale or disabled so they cannot withdraw their collateral

Summary

When users try to withdraw their collateral this triggers a call to _getCollPrice which reverts if the oracle is stale or disabled.

This is a problem because users will not be able to withdraw their collateral in this case.
When a user has fully reduced his debt meaning debt=0, then he should be able to withdraw his collateral without the need for a valid price from the oracle.

Even the emergencyClosePosition function needs a valid price which should not be the case.

Vulnerability Detail

When withdrawing collateral from a loan (either via the BaseVault.modifyPosition or BaseVault.emergencyClosePosition function) the BaseVault._modifyPosition function will be called.

When collateral is withdrawn, mustCheckHealth is set to true:
https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L313-L321

This means that _checkAccountHealth will be called:
https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L325-L327

Downstream this then calls _getOraclePrice which reverts if there is no valid price:
https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L205-L207

Impact

Users are unable to withdraw their collateral when the oracle is stale or disabled even if they have no debt.

Code Snippet

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L276-L328

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/BaseVault.sol#L198-L208

Tool used

Manual Review

Recommendation

In the BaseVault._modifyPosition function, the checkAccountHealth function (which needs a valid price downstream) should only be called when userDetails[_account].debt > 0:

diff --git a/taurus-contracts/contracts/Vault/BaseVault.sol b/taurus-contracts/contracts/Vault/BaseVault.sol
index 33f87f0..a8f3f9a 100644
--- a/taurus-contracts/contracts/Vault/BaseVault.sol
+++ b/taurus-contracts/contracts/Vault/BaseVault.sol
@@ -322,7 +322,7 @@ abstract contract BaseVault is SwapHandler, UUPSUpgradeable {
             }
         }
 
-        if (mustCheckHealth) {
+        if (mustCheckHealth && userDetails[_account].debt > 0) {
             _checkAccountHealth(_account);
         }
     }

Duplicate of #184

[roguereddwarf]

Escalate for 10 USDC

This is a valid issue. Also the duplicate #184 is a valid issue.

In #184 the sponsor commented:

The best we can do is ensure that oracles never become stale in periods of volatility. We have done so with the GmxYieldAdapter, whose oracle cannot ever become stale.

However the protocol will support different oracles and they cannot be sure that an oracle cannot become stale.
In case the oracle becomes stale we have both shown how this leads to bad consequences for the users resulting in a loss of funds.

We also showed that there are ways to mitigate this so the above statement by the sponsor that there isn't anything to do about it is not true.

Similar issues have been accepted in the past, e.g. sherlock-audit/2022-11-isomorph-judging#69

[sherlock-admin]

Escalate for 10 USDC

This is a valid issue. Also the duplicate #184 is a valid issue.

In #184 the sponsor commented:

The best we can do is ensure that oracles never become stale in periods of volatility. We have done so with the GmxYieldAdapter, whose oracle cannot ever become stale.

However the protocol will support different oracles and they cannot be sure that an oracle cannot become stale.
In case the oracle becomes stale we have both shown how this leads to bad consequences for the users resulting in a loss of funds.

We also showed that there are ways to mitigate this so the above statement by the sponsor that there isn't anything to do about it is not true.

Similar issues have been accepted in the past, e.g. sherlock-audit/2022-11-isomorph-judging#69

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.

[hrishibhat]

Escalation rejected

In addition to Sponsor comment from #184
Given that in case of user having no debt a reverting oracle price will not allow for collateral withdrawal temporarily.
There is no loss of funds and given the temporary impact of the issue, it can be considered a low

[sherlock-admin]

Escalation rejected

In addition to Sponsor comment from #184
Given that in case of user having no debt a reverting oracle price will not allow for collateral withdrawal temporarily.
There is no loss of funds and given the temporary impact of the issue, it can be considered a low

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[iHarishKumar]

https://github.com/protokol/taurus-contracts/pull/133