xiaoming90 - Partial liquidations are not possible

[sherlock-admin]

xiaoming90

high

Partial liquidations are not possible

Summary

Due to an incorrect implementation of VaultValuation.getLiquidationFactors(), Notional requires that a liquidator reduces an account's debt below minBorrowSize. This does not allow liquidators to partially liquidate a vault account into a healthy position and opens up the protocol to an edge case where an account is always ineligible for liquidation.

Vulnerability Detail

While VaultValuation.getLiquidationFactors() might allow for the resultant outstanding debt to be below the minimum borrow amount and non-zero, deleverageAccount() will revert due to checkMinBorrow being set to true. Therefore, the only option is for liquidators to wipe the outstanding debt entirely but users can set up their vault accounts such that that maxLiquidatorDepositLocal is less than each of the vault currency's outstanding debt.

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultValuation.sol#L240-L270

File: VaultValuation.sol
240:         int256 maxLiquidatorDepositLocal = _calculateDeleverageAmount(
241:             vaultConfig,
242:             h.vaultShareValueUnderlying,
243:             h.totalDebtOutstandingInPrimary.neg(),
244:             h.debtOutstanding[currencyIndex].neg(),
245:             minBorrowSize,
246:             exchangeRate,
247:             er.rateDecimals
248:         );
249: 
250:         // NOTE: deposit amount is always positive in this method
251:         if (depositUnderlyingInternal < maxLiquidatorDepositLocal) {
252:             // If liquidating past the debt outstanding above the min borrow, then the entire
253:             // debt outstanding must be liquidated.
254: 
255:             // (debtOutstanding - depositAmountUnderlying) is the post liquidation debt. As an
256:             // edge condition, when debt outstanding is discounted to present value, the account
257:             // may be liquidated to zero while their debt outstanding is still greater than the
258:             // min borrow size (which is normally enforced in notional terms -- i.e. non present
259:             // value). Resolving this would require additional complexity for not much gain. An
260:             // account within 20% of the minBorrowSize in a vault that has fCash discounting enabled
261:             // may experience a full liquidation as a result.
262:             require(
263:                 h.debtOutstanding[currencyIndex].sub(depositUnderlyingInternal) < minBorrowSize,
264:                 "Must Liquidate All Debt"
265:             );
266:         } else {
267:             // If the deposit amount is greater than maxLiquidatorDeposit then limit it to the max
268:             // amount here.
269:             depositUnderlyingInternal = maxLiquidatorDepositLocal;
270:         }

If depositUnderlyingInternal >= maxLiquidatorDepositLocal, then the liquidator's deposit is capped to maxLiquidatorDepositLocal. However, maxLiquidatorDepositLocal may put the vault account's outstanding debt below the minimum borrow amount but not to zero.

However, because it is not possible to partially liquidate the account's debt, we reach a deadlock where it isn't possible to liquidate all outstanding debt and it also isn't possible to liquidate debt partially. So even though it may be possible to liquidate an account into a healthy position, the current implementation doesn't always allow for this to be true.

Impact

Certain vault positions will never be eligible for liquidation and hence Notional may be left with bad debt. Liquidity providers will lose funds as they must cover the shortfall for undercollateralised positions.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/external/actions/VaultLiquidationAction.sol#L76-L82

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/external/actions/VaultAccountHealth.sol#L260-L264

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultValuation.sol#L240-L270

Tool used

Manual Review

Recommendation

VaultValuation.getLiquidationFactors() must be updated to allow for partial liquidations.

File: VaultValuation.sol
251:         if (depositUnderlyingInternal < maxLiquidatorDepositLocal) {
252:             // If liquidating past the debt outstanding above the min borrow, then the entire
253:             // debt outstanding must be liquidated.
254: 
255:             // (debtOutstanding - depositAmountUnderlying) is the post liquidation debt. As an
256:             // edge condition, when debt outstanding is discounted to present value, the account
257:             // may be liquidated to zero while their debt outstanding is still greater than the
258:             // min borrow size (which is normally enforced in notional terms -- i.e. non present
259:             // value). Resolving this would require additional complexity for not much gain. An
260:             // account within 20% of the minBorrowSize in a vault that has fCash discounting enabled
261:             // may experience a full liquidation as a result.
262:             require(
263:                 h.debtOutstanding[currencyIndex].neg().sub(depositUnderlyingInternal) >= minBorrowSize,
                         || h.debtOutstanding[currencyIndex].neg().sub(depositUnderlyingInternal) == 0
264:                 "Must Liquidate All Debt"
265:             );
266:         } else {
267:             // If the deposit amount is greater than maxLiquidatorDeposit then limit it to the max
268:             // amount here.
269:             depositUnderlyingInternal = maxLiquidatorDepositLocal;
270:         }

[T-Woodward]

This is true. This is an issue when a vaultAccount is borrowing from the prime maturity, but not an issue when an account is borrowing from an fCash maturity.

[0xleastwood]

Escalate for 10 USDC

We are able to set up a vault account in a way such that liquidating an amount equal or up to maxLiquidatorDepositLocal will leave the resultant currency debt above minBorrowSize and hence the liquidation will fail. Therefore, we would be in a deadlock position where a user is unable to sufficiently liquidate a vault of considerable size and hence the vault could accrue bad debt. While this only applies to prime vaults, high severity is still justified here because it is not possible to liquidate a vault if these conditions are held.

[sherlock-admin]

Escalate for 10 USDC

We are able to set up a vault account in a way such that liquidating an amount equal or up to maxLiquidatorDepositLocal will leave the resultant currency debt above minBorrowSize and hence the liquidation will fail. Therefore, we would be in a deadlock position where a user is unable to sufficiently liquidate a vault of considerable size and hence the vault could accrue bad debt. While this only applies to prime vaults, high severity is still justified here because it is not possible to liquidate a vault if these conditions are held.

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.

[Trumpero]

Agree with the escalation that this should be a high issue.

[hrishibhat]

Result:
High
Unique
Considering this a high issue based points raised in the issue and the escalation

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xleastwood: accepted

[jeffywu]

Fixed: notional-finance/contracts-v2#132

[xiaoming9090]

@0xleastwood + @xiaoming9090 : Verified. Fixed in PR notional-finance/contracts-v2#132