ShadowForce - Lack of check for liquidator address when deleveraging result in either griefing or loss of fund

[sherlock-admin]

ShadowForce

high

Lack of check for liquidator address when deleveraging result in either griefing or loss of fund

Summary

Lack of check for liquidator address when deleveraging result in loss of fund

Vulnerability Detail

When we are liquidating asset we are calling the function authenticate-deleverage check

    function deleverageAccount(
        address account,
        address vault,
        address liquidator,
        uint16 currencyIndex,
        int256 depositUnderlyingInternal
    ) external payable nonReentrant override returns (
        uint256 vaultSharesToLiquidator,
        int256 depositAmountPrimeCash
    ) {
        require(currencyIndex < 3);
        (
            VaultConfig memory vaultConfig,
            VaultAccount memory vaultAccount,
            VaultState memory vaultState
        ) = _authenticateDeleverage(account, vault, liquidator);

this is calling

    /// @notice Authenticates a call to the deleverage method
    function _authenticateDeleverage(
        address account,
        address vault,
        address liquidator
    ) private returns (
        VaultConfig memory vaultConfig,
        VaultAccount memory vaultAccount,
        VaultState memory vaultState
    ) {
        // Do not allow invalid accounts to liquidate
        requireValidAccount(liquidator);
        require(liquidator != vault);

        // Cannot liquidate self, if a vault needs to deleverage itself as a whole it has other methods 
        // in VaultAction to do so.
        require(account != msg.sender);
        require(account != liquidator);

        vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
        require(vaultConfig.getFlag(VaultConfiguration.DISABLE_DELEVERAGE) == false);

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

        vaultAccount = VaultAccountLib.getVaultAccount(account, vaultConfig);

if this check pass, in deleverageAccount, we are calling depositUnderlyingExternal function, this is an important

        uint16 currencyId = vaultConfig.borrowCurrencyId;
        if (currencyIndex == 1) currencyId = vaultConfig.secondaryBorrowCurrencies[0];
        else if (currencyIndex == 2) currencyId = vaultConfig.secondaryBorrowCurrencies[1];

        Token memory token = TokenHandler.getUnderlyingToken(currencyId);
        // Excess ETH is returned to the liquidator natively
        (/* */, depositAmountPrimeCash) = TokenHandler.depositUnderlyingExternal(
            liquidator, currencyId, token.convertToExternal(depositUnderlyingInternal), pr, false 
        );

which calling the code below, this is a important because it either use the ETH sent along with this transaciton or pull fund from liquidator account

    /// @notice Deposits an amount of underlying tokens to mint prime cash
    /// @param account account to transfer tokens from
    /// @param currencyId the associated currency id
    /// @param _underlyingExternalDeposit the amount of underlying tokens to deposit
    /// @param primeRate the current accrued prime rate
    /// @param returnNativeTokenWrapped if true, return excess msg.value ETH payments as WETH
    /// @return actualTransferExternal the actual amount of tokens transferred in external precision
    /// @return netPrimeSupplyChange the amount of prime supply created
    function depositUnderlyingExternal(
        address account,
        uint16 currencyId,
        int256 _underlyingExternalDeposit,
        PrimeRate memory primeRate,
        bool returnNativeTokenWrapped
    ) internal returns (int256 actualTransferExternal, int256 netPrimeSupplyChange) {
        uint256 underlyingExternalDeposit = _underlyingExternalDeposit.toUint();
        if (underlyingExternalDeposit == 0) return (0, 0);

        Token memory underlying = getUnderlyingToken(currencyId);
        if (underlying.tokenType == TokenType.Ether) {
            // Underflow checked above
            if (underlyingExternalDeposit < msg.value) {
                // Transfer any excess ETH back to the account
                GenericToken.transferNativeTokenOut(
                    account, msg.value - underlyingExternalDeposit, returnNativeTokenWrapped
                );
            } else {
                require(underlyingExternalDeposit == msg.value, "ETH Balance");
            }

            actualTransferExternal = _underlyingExternalDeposit;
        } else {
            // In the case of deposits, we use a balance before and after check
            // to ensure that we record the proper balance change.
            actualTransferExternal = GenericToken.safeTransferIn(
                underlying.tokenAddress, account, underlyingExternalDeposit
            ).toInt();
        }

        netPrimeSupplyChange = _postTransferPrimeCashUpdate(
            account, currencyId, actualTransferExternal, underlying, primeRate
        );
    }

what is the account parameter, account parameter is the liquidator

// Cannot liquidate self, if a vault needs to deleverage itself as a whole it has other methods 
// in VaultAction to do so.
require(account != msg.sender);
require(account != liquidator);

vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
require(vaultConfig.getFlag(VaultConfiguration.DISABLE_DELEVERAGE) == false);

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

if (vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE)) is on,

we are checking

if (vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE)) {
	require(msg.sender == vault);
} else {
	require(msg.sender == liquidator);
}

if vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE) is flag return false, we are checking

require(msg.sender == liquidator);

this looks right because if the underlyinig token is not ETH, token is pulled from liquidator account

but if vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE) is flag return true, we only check

require(msg.sender == vault);

but are not checking which account is liquidator

liquidator can just reject the deleverage by removing the allowance

OR in the worst case, as long as the liquidator given allowance to the smart contract, the liquidator is pulled from his account to cover the debt, which is result in loss of fund

Impact

Lack of check for liquidator address when deleveraging result in either griefing or loss of fund

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/actions/VaultLiquidationAction.sol#L63

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/actions/VaultLiquidationAction.sol#L90

Tool used

Manual Review

Recommendation

even if we check msg.sender is vault, we have to implement some sort of check to validate the liquidator address

Duplicate of #215