sherlock-audit / 2024-06-leveraged-vaults-judging

9 stars 8 forks source link

eeyore - Liquidation of an insolvent account should be permitted if it results in a solvent account and a derisked Notional protocol #53

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

eeyore

Medium

Liquidation of an insolvent account should be permitted if it results in a solvent account and a derisked Notional protocol

Summary

The Notional protocol is taking all the risk on itself by preventing the liquidation of insolvent accounts in the BaseStakingVault contract.

Vulnerability Detail

In the BaseStakingVault, the deleverageAccount() function prematurely denies the liquidation of an account if it is already insolvent. In such situations, Notional takes all the risk, either by hoping the account becomes solvent again or by waiting a prolonged period for the forced withdrawal request to mature to withdraw staked tokens.

This risk can be mitigated by allowing the liquidation of such accounts and rechecking if the account is solvent or fully liquidated after the liquidation. This ensures there is no bad debt or risk for Notional.

Only if no liquidators are willing to derisk Notional, Notional can then take this risk on itself.

Example of how it should work:

  1. An asset de-pegs, making the account insolvent.
  2. A liquidator sees an opportunity, assuming the underlying staked token will not lose value over time, and happily liquidates the insolvent account, taking the bad debt risk on itself.
  3. Notional is left with a solvent account, with no risk or bad debt.

Impact

All the risk of an insolvent account is taken by Notional. If the staked token also loses value over time, the bad debt for Notional protocol can increase. In the best-case scenario, Notional will only break even.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/BaseStakingVault.sol#L227

Tool used

Manual Review

Recommendation

Perform the insolvency check after the deleverageAccount() and _splitWithdrawRequest() calls within the deleverageAccount() function. If the account is solvent and the collateral ratio is improved, allow for such liquidation and transfer the risk to the liquidator.

function deleverageAccount(
    address account,
    address vault,
    address liquidator,
    uint16 currencyIndex,
    int256 depositUnderlyingInternal
) external payable override virtual returns (
    uint256 vaultSharesFromLiquidation,
    int256 depositAmountPrimeCash
) {
    require(msg.sender == liquidator);
    _checkReentrancyContext();

    // Do not allow liquidations if the result will be that the account is insolvent. This may occur if the
    // short term de-peg of an asset causes a bad debt to accrue to the protocol. In this case, we should be
    // able to execute a forced withdraw request and wait for a full return on the staked token.
    (VaultAccountHealthFactors memory healthBefore, /* */, /* */) = NOTIONAL.getVaultAccountHealthFactors(
        account, vault
    );
-   require(0 <= healthBefore.collateralRatio, "Insolvent");

    // Executes the liquidation on Notional, vault shares are transferred from the account to the liquidator
    // inside this process.
    (vaultSharesFromLiquidation, depositAmountPrimeCash) = NOTIONAL.deleverageAccount{value: msg.value}(
        account, vault, liquidator, currencyIndex, depositUnderlyingInternal
    );

    // Splits any withdraw requests, if required. Will revert if the liquidator cannot absorb the withdraw
    // request because they have another active one.
    _splitWithdrawRequest(account, liquidator, vaultSharesFromLiquidation);

    (VaultAccountHealthFactors memory healthAfter, /* */, /* */) = NOTIONAL.getVaultAccountHealthFactors(
        account, vault
    );

+   require(0 <= healthAfter.collateralRatio, "Insolvent");
    // Ensure that the health ratio increases as a result of liquidation, this is similar the solvency check
    // above. If an account ends up in a worse collateral position due to the liquidation price we are better
    // off waiting until the withdraw request finalizes.
    require(healthBefore.collateralRatio < healthAfter.collateralRatio, "Collateral Decrease");
}
sherlock-admin4 commented 2 months ago

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

0xmystery commented:

Intended design