ShadowForce - Cannot permissionless settle the vault account if the user use a blacklisted account

[sherlock-admin]

ShadowForce

medium

Cannot permissionless settle the vault account if the user use a blacklisted account

Summary

Cannot permissionless settle the vault account if the user use a blacklisted account

Vulnerability Detail

In VaultAccoutnAction.sol, one of the critical function is

    /// @notice Settles a matured vault account by transforming it from an fCash maturity into
    /// a prime cash account. This method is not authenticated, anyone can settle a vault account
    /// without permission. Generally speaking, this action is economically equivalent no matter
    /// when it is called. In some edge conditions when the vault is holding prime cash, it is
    /// advantageous for the vault account to have this called sooner. All vault account actions
    /// will first settle the vault account before taking any further actions.
    /// @param account the address to settle
    /// @param vault the vault the account is in
    function settleVaultAccount(address account, address vault) external override nonReentrant {
        requireValidAccount(account);
        require(account != vault);

        VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
        VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vaultConfig);

        // Require that the account settled, otherwise we may leave the account in an unintended
        // state in this method because we allow it to skip the min borrow check in the next line.
        (bool didSettle, bool didTransfer) = vaultAccount.settleVaultAccount(vaultConfig);
        require(didSettle, "No Settle");

        vaultAccount.accruePrimeCashFeesToDebt(vaultConfig);

        // Skip Min Borrow Check so that accounts can always be settled
        vaultAccount.setVaultAccount({vaultConfig: vaultConfig, checkMinBorrow: false});

        if (didTransfer) {
            // If the vault did a transfer (i.e. withdrew cash) we have to check their collateral ratio. There
            // is an edge condition where a vault with secondary borrows has an emergency exit. During that process
            // an account will be left some cash balance in both currencies. It may have excess cash in one and
            // insufficient cash in the other. A withdraw of the excess in one side will cause the vault account to
            // be insolvent if we do not run this check. If this scenario indeed does occur, the vault itself must
            // be upgraded in order to facilitate orderly exits for all of the accounts since they will be prevented
            // from settling.
            IVaultAccountHealth(address(this)).checkVaultAccountCollateralRatio(vault, account);
        }
    }

as the comment suggests, this function should be called permissionless

and the comment is, which means there should not be able to permissionless reject account settlement

/// will first settle the vault account before taking any further actions.

this is calling

  (bool didSettle, bool didTransfer) = vaultAccount.settleVaultAccount(vaultConfig);

which calls

    /// @notice Settles a matured vault account by transforming it from an fCash maturity into
    /// a prime cash account. This method is not authenticated, anyone can settle a vault account
    /// without permission. Generally speaking, this action is economically equivalent no matter
    /// when it is called. In some edge conditions when the vault is holding prime cash, it is
    /// advantageous for the vault account to have this called sooner. All vault account actions
    /// will first settle the vault account before taking any further actions.
    /// @param account the address to settle
    /// @param vault the vault the account is in
    function settleVaultAccount(address account, address vault) external override nonReentrant {
        requireValidAccount(account);
        require(account != vault);

        VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
        VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vaultConfig);

        // Require that the account settled, otherwise we may leave the account in an unintended
        // state in this method because we allow it to skip the min borrow check in the next line.
        (bool didSettle, bool didTransfer) = vaultAccount.settleVaultAccount(vaultConfig);
        require(didSettle, "No Settle");

basically this calls

        // Calculates the net settled cash if there is any temp cash balance that is net off
        // against the settled prime debt.
        bool didTransferPrimary;
        (accountPrimeStorageValue, didTransferPrimary) = repayAccountPrimeDebtAtSettlement(
            vaultConfig.primeRate,
            primeVaultState,
            vaultConfig.borrowCurrencyId,
            vaultConfig.vault,
            vaultAccount.account,
            vaultAccount.tempCashBalance,
            accountPrimeStorageValue
        );

calling

    function repayAccountPrimeDebtAtSettlement(
        PrimeRate memory pr,
        VaultStateStorage storage primeVaultState,
        uint16 currencyId,
        address vault,
        address account,
        int256 accountPrimeCash,
        int256 accountPrimeStorageValue
    ) internal returns (int256 finalPrimeDebtStorageValue, bool didTransfer) {
        didTransfer = false;
        finalPrimeDebtStorageValue = accountPrimeStorageValue;

        if (accountPrimeCash > 0) {
            // netPrimeDebtRepaid is a negative number
            int256 netPrimeDebtRepaid = pr.convertUnderlyingToDebtStorage(
                pr.convertToUnderlying(accountPrimeCash).neg()
            );

            int256 netPrimeDebtChange;
            if (netPrimeDebtRepaid < accountPrimeStorageValue) {
                // If the net debt change is greater than the debt held by the account, then only
                // decrease the total prime debt by what is held by the account. The residual amount
                // will be refunded to the account via a direct transfer.
                netPrimeDebtChange = accountPrimeStorageValue;
                finalPrimeDebtStorageValue = 0;

                int256 primeCashRefund = pr.convertFromUnderlying(
                    pr.convertDebtStorageToUnderlying(netPrimeDebtChange.sub(accountPrimeStorageValue))
                );
                TokenHandler.withdrawPrimeCash(
                    account, currencyId, primeCashRefund, pr, false // ETH will be transferred natively
                );
                didTransfer = true;
            } else {
                // In this case, part of the account's debt is repaid.
                netPrimeDebtChange = netPrimeDebtRepaid;
                finalPrimeDebtStorageValue = accountPrimeStorageValue.sub(netPrimeDebtRepaid);
            }

the token withdrawal logic above try to push ETH to accout

TokenHandler.withdrawPrimeCash(
    account, currencyId, primeCashRefund, pr, false // ETH will be transferred natively
);

this is calling

  function withdrawPrimeCash(
        address account,
        uint16 currencyId,
        int256 primeCashToWithdraw,
        PrimeRate memory primeRate,
        bool withdrawWrappedNativeToken
    ) internal returns (int256 netTransferExternal) {
        if (primeCashToWithdraw == 0) return 0;
        require(primeCashToWithdraw < 0);

        Token memory underlying = getUnderlyingToken(currencyId);
        netTransferExternal = convertToExternal(
            underlying, 
            primeRate.convertToUnderlying(primeCashToWithdraw) 
        );

        // Overflow not possible due to int256
        uint256 withdrawAmount = uint256(netTransferExternal.neg());
        _redeemMoneyMarketIfRequired(currencyId, underlying, withdrawAmount);

        if (underlying.tokenType == TokenType.Ether) {
            GenericToken.transferNativeTokenOut(account, withdrawAmount, withdrawWrappedNativeToken);
        } else {
            GenericToken.safeTransferOut(underlying.tokenAddress, account, withdrawAmount);
        }

        _postTransferPrimeCashUpdate(account, currencyId, netTransferExternal, underlying, primeRate);
    }

note the function call

if (underlying.tokenType == TokenType.Ether) {
    GenericToken.transferNativeTokenOut(account, withdrawAmount, withdrawWrappedNativeToken);
} else {
    GenericToken.safeTransferOut(underlying.tokenAddress, account, withdrawAmount);
}

if the token type is not ETHER,

we are transfer the underlying ERC20 token to the account

GenericToken.safeTransferOut(underlying.tokenAddress, account, withdrawAmount);

the token in-scoped is

ERC20:  Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc.

USDC is common token that has blacklisted

if the account is blacklisted, the transfer would revert and the account cannot be settled!

Impact

what are the impact,

per comment

/// will first settle the vault account before taking any further actions.

if that is too vague, I can list three, there are more!

1. there are certain action that need to be done after the vault settlement, for example, liqudation require the vault settlement first

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

2. there are case that require force vault settlement, actually one example is notional need to force the settle the vault during migration! (this is just the case to show user should be able to permissionless reject settlement)

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/internal/balances/TokenHandler.sol#L241

Tool used

Manual Review

Recommendation

maybe let admin bypass the withdrawPrimeCash and force settle the account to not let settlement block further action!

[jeffywu]

Valid issue, transfers during supposedly permissionless settlement can indeed cause issues.

[jeffywu]

Fixed: https://github.com/notional-finance/contracts-v2/pull/135

[0xleastwood]

@0xleastwood + @xiaoming9090: PR 135 attempted to fix the issue by removing the transfer of excess assets from the settlement. It will store the excess assets to be refunded in s.primaryCash so that it can be read to vaultAccount.tempCashBalance later when exiting the vault (VaultAccountAction.exitVault). The transfer of excess assets will be performed during the exit vault.

However, in an edge case where a liquidator deleverages another account and happens to have primary cash in their vault account, their s.primaryCash (excess cash) will be wiped.

Following is the function call flow for reference: deleverageAccount -> _transferVaultSharesToLiquidator -> liquidator.setVaultAccount -> Effect: liquidator's s.primaryCash = 0

[0xleastwood]

@0xleastwood + @xiaoming9090: Verified fix as per comments in #207. Fixed in https://github.com/notional-finance/contracts-v2/pull/135