Notional Finance

Description

There is code in VaultConfiguration. transferUnderlyingToVaultDirect that handles tokens with transfer fees when transferring borrow currencies to the vault:

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L330-L355

function transferUnderlyingToVaultDirect(
    VaultConfig memory vaultConfig,
    address transferFrom,
    uint256 depositAmountExternal
) internal returns (uint256) {
    if (depositAmountExternal == 0) return 0;

    Token memory assetToken = TokenHandler.getAssetToken(vaultConfig.borrowCurrencyId);
    Token memory underlyingToken = assetToken.tokenType == TokenType.NonMintable ?
        assetToken :
        TokenHandler.getUnderlyingToken(vaultConfig.borrowCurrencyId);

    address vault = vaultConfig.vault;
    if (underlyingToken.tokenType == TokenType.Ether) {
        require(msg.value == depositAmountExternal, "Invalid ETH");
        // Forward all the ETH to the vault
        GenericToken.transferNativeTokenOut(vault, msg.value);

        return msg.value;
    } else if (underlyingToken.hasTransferFee) {
        // In this case need to check the balance of the vault before and after
        uint256 balanceBefore = underlyingToken.balanceOf(vault);
        GenericToken.safeTransferFrom(underlyingToken.tokenAddress, transferFrom, vault, depositAmountExternal);
        uint256 balanceAfter = underlyingToken.balanceOf(vault);

        return balanceAfter.sub(balanceBefore);

However, Notional Strategy Vaults are hardcoded not to allow borrow currencies with transfer fees, either for primary or secondary currencies.

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L164

require(!assetToken.hasTransferFee && !underlyingToken.hasTransferFee);

contracts-v2/contracts/external/actions/VaultAction.sol:L67

require(!assetToken.hasTransferFee && !underlyingToken.hasTransferFee);

Recommendation

Consider removing this code to clarify and emphasize the intent of these vaults not to have borrow currencies with transfer fees.

Description

The Notional Strategy Vaults need to get whitelisted and have specific Notional parameters set in order to interact with the rest of the Notional system. This is done through VaultAction.updateVault() where the owner address can provide a VaultConfigStorage calldata vaultConfig argument to either whitelist a new vault or change an existing one. While this is to be performed by a trusted privileged actor (the owner), and it could be assumed they are careful with their updates, the contracts themselves don’t perform enough checks on the validity of the parameters, either in isolation or when compared against the existing vault state. Below are examples of arguments that should be better checked.

borrowCurrencyId

The borrowCurrencyId parameter gets provided to TokenHandler.getAssetToken() and TokenHandler.getUnderlyingToken() to retrieve its associated TokenStorage object and verify that the currency doesn’t have transfer fees.

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L162-L164

Token memory assetToken = TokenHandler.getAssetToken(vaultConfig.borrowCurrencyId);
Token memory underlyingToken = TokenHandler.getUnderlyingToken(vaultConfig.borrowCurrencyId);
require(!assetToken.hasTransferFee && !underlyingToken.hasTransferFee);

However, these calls retrieve data from the mapping from storage which returns an empty struct for an unassigned currency ID. This would pass the check in the last require statement regarding the transfer fees and would successfully allow to set the currency even if isn’t actually registered in Notional. The recommendation would be to check that the returned TokenStorage object has data inside of it, perhaps by checking the decimals on the token.

In the event that this is a call to update the configuration on a vault instead of whitelisting a whole new vault, this would also allow to switch the borrow currency without checking that the existing borrow and lending accounting has been cleared. This could cause accounting issues. A check for existing debt before swapping the borrow currency IDs is recommended.

liquidationRate and minCollateralRatioBPS

To ensure that the system doesn’t have bad debt, it employs a liquidation engine that depends on a few parameters, in particular the vault’s liquidationRate that incentivises liquidators and minCollateralRatioBPS that determines when an account can be liquidated.minCollateralRationBPS+100% (since the collateral ratio is calculated starting from 0% not 100%) would need to be greater than liquidationRate (that is calculated from 100%) or the system could run into problems liquidating small accounts entering the vault. There is an edge case during liquidation where if the account is below the minimum collateral ratio but doesn’t have to be liquidated fully, the leftover position from that account would be too small for liquidators to profitably liquidate (due to gas costs) as per another configuration parameter minAccountBorrowSize. In this edge case, the system would set the whole account to be liquidated and determine that the liquidator deposit required would be equal to that account’s total debt, which would be normally seen as vaultAccount.fCash. The liquidator in this case would roughly receive as much value as vaultAccount.fCash*liquidationRate denominated in that vault account’s vaultAccount.vaultShares, which is the existing assets of that vault account. In fact the liquidator gets:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L274-L283

uint256 vaultSharesToLiquidator;
{
    vaultSharesToLiquidator = vaultAccount.tempCashBalance.toUint()
        .mul(vaultConfig.liquidationRate.toUint())
        .mul(vaultAccount.vaultShares)
        .div(vaultShareValue.toUint())
        .div(uint256(Constants.RATE_PRECISION));
}

vaultAccount.vaultShares = vaultAccount.vaultShares.sub(vaultSharesToLiquidator);

Where vaultAccount.tempCashBalance has the liquidator deposit, which in this case would be the account’s debt and equal to vaultAccount.fCash. However, since we know that this account is being liquidated, we know that fCash*(1+minCollateralRationBPS) >= vaultShareValue. Similarly, assuming the liquidation rate was set incorrectly as defined in the beginning of this section, i.e. liquidationRate > (1+minCollateralRationBPS), we can determine that fCash*(liquidationRate) > vaultShareValue as well. Therefore, we will get some number vaultSharesToLiquidator=X*vaultAccount.vaultShares, where X=(vaultAccount.tempCashBalance*vaultConfig.liquidationRate)/(vaultShareValue) and X>1, so the result will be vaultSharesToLiquidator>vaultAccount.vaultShares, which will cause a revert once the liquidator shares get subtracted from that vault account’s vault share balance. This will cause the account to remain in the system until the account is possibly insolvent , potentially causing bad debt. The recommendation would be to check that the liquidation rate is less than the minimum collateral ratio, of course in the appropriate denomination (i.e. do minCollateral+1) and precision.

maxBorrowMarketIndex

The current Strategy Vault implementation does not allow for idiosyncratic cash because it causes issues during exits as there are no active markets for the account’s maturity. Therefore, the configuration shouldn’t be set with maxBorrowMarketIndex >=3 as that would open up the 1 Year maturity for vault accounts that could cause idiosyncratic fCash. The recommendation would be to add that check.

secondaryBorrowCurrencies

Similarly to the borrowCurrencyId, there are few checks that actually determine that the secondaryBorrowCurrencies[] given are actually registered in Notional. This is, however, more inline with how some vaults are supposed to work as they may have no secondary currencies at all, such as when the secondaryBorrowCurrencies[] id is given as 0. In the event that this is a call to update the configuration on a vault instead of whitelisting a whole new vault, this would also allow to switch the secondary borrow currency without checking that the existing borrow and lending accounting has been cleared. For example, the VaultAction.updateSecondaryBorrowCapacity() function could be invoked on the new set of secondary currencies and simply increase the borrow there. This could cause accounting issues. A check for existing debt before swapping the borrow currency IDs is recommended.

Description

There are a few places in the code where division by zero may occur but isn’t handled.

Examples

If the vault settles at exactly 0 value with 0 remaining strategy token value, there may be an unhandled division by zero trying to divide claims on the settled assets:

contracts-v2/contracts/internal/vaults/VaultAccount.sol:L424-L436

int256 settledVaultValue = settlementRate.convertToUnderlying(residualAssetCashBalance)
    .add(totalStrategyTokenValueAtSettlement);

// If the vault is insolvent (meaning residualAssetCashBalance < 0), it is necessarily
// true that totalStrategyTokens == 0 (meaning all tokens were sold in an attempt to
// repay the debt). That means settledVaultValue == residualAssetCashBalance, strategyTokenClaim == 0
// and assetCashClaim == totalAccountValue. Accounts that are still solvent will be paid from the
// reserve, accounts that are insolvent will have a totalAccountValue == 0.
strategyTokenClaim = totalAccountValue.mul(vaultState.totalStrategyTokens.toInt())
    .div(settledVaultValue).toUint();

assetCashClaim = totalAccountValue.mul(residualAssetCashBalance)
    .div(settledVaultValue);

If a vault account is entirely insolvent and its vaultShareValue is zero, there will be an unhandled division by zero during liquidation:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L274-L281

uint256 vaultSharesToLiquidator;
{
    vaultSharesToLiquidator = vaultAccount.tempCashBalance.toUint()
        .mul(vaultConfig.liquidationRate.toUint())
        .mul(vaultAccount.vaultShares)
        .div(vaultShareValue.toUint())
        .div(uint256(Constants.RATE_PRECISION));
}

If a vault account’s secondary debt is being repaid when there is none, there will be an unhandled division by zero:

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L661-L666

VaultSecondaryBorrowStorage storage balance =
    LibStorage.getVaultSecondaryBorrow()[vaultConfig.vault][maturity][currencyId];
uint256 totalfCashBorrowed = balance.totalfCashBorrowed;
uint256 totalAccountDebtShares = balance.totalAccountDebtShares;

fCashToLend = debtSharesToRepay.mul(totalfCashBorrowed).div(totalAccountDebtShares).toInt();

While these cases may be unlikely today, this code could be reutilized in other circumstances later that could cause reverts and even disrupt operations more frequently.

Recommendation

Handle the cases where the denominator could be zero appropriately.

Description

From the client’s specifications for the strategy vaults, we know that accounts should be able to increase their leveraged positions before maturity. This property will not hold for the vaults that require borrowing a secondary currency to enter a position. When an account opens its position in such vault for the first time, the VaultAccountSecondaryDebtShareStorage.maturity is set to the maturity an account has entered. When the account is trying to increase the debt position, an accounts current maturity will be checked, and since it is not set to 0, as in the case where an account enters the vault for the first time, nor it is smaller than the new maturity passed by an account as in case of a rollover, the code will revert.

Examples

contracts-v2/contracts/external/actions/VaultAction.sol:L226-L228

if (accountMaturity != 0) {
    // Cannot roll to a shorter term maturity
    require(accountMaturity < maturity);

Recommendation

In order to fix this issue, we recommend that < is replaced with <= so that account can enter the vault maturity the account is already in as well as the future once.

Description

Some of the Notional Strategy Vaults may allow for secondary currencies to be borrowed as part of the same strategy. For example, a strategy may allow for USDC to be its primary borrow currency as well as have ETH as its secondary borrow currency.

In order to enter the vault, a user would have to deposit depositAmountExternal of the primary borrow currency when calling VaultAccountAction.enterVault(). This would allow the user to borrow with leverage, as long as the vaultConfig.checkCollateralRatio() check on that account succeeds, which is based on the initial deposit and borrow currency amounts. This collateral ratio check is then performed throughout that user account’s lifecycle in that vault, such as when they try to roll their maturity, or when liquidators try to perform collateral checks to ensure there is no bad debt.

However, in the event that the vault has a secondary borrow currency as well, that additional secondary debt is not calculated as part of the checkCollateralRatio() check. The only debt that is being considered is the vaultAccount.fCash that corresponds to the primary borrow currency debt:

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L313-L319

function checkCollateralRatio(
    VaultConfig memory vaultConfig,
    VaultState memory vaultState,
    VaultAccount memory vaultAccount
) internal view {
    (int256 collateralRatio, /* */) = calculateCollateralRatio(
        vaultConfig, vaultState, vaultAccount.account, vaultAccount.vaultShares, vaultAccount.fCash

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L278-L292

function calculateCollateralRatio(
    VaultConfig memory vaultConfig,
    VaultState memory vaultState,
    address account,
    uint256 vaultShares,
    int256 fCash
) internal view returns (int256 collateralRatio, int256 vaultShareValue) {
    vaultShareValue = vaultState.getCashValueOfShare(vaultConfig, account, vaultShares);

    // We do not discount fCash to present value so that we do not introduce interest
    // rate risk in this calculation. The economic benefit of discounting will be very
    // minor relative to the added complexity of accounting for interest rate risk.

    // Convert fCash to a positive amount of asset cash
    int256 debtOutstanding = vaultConfig.assetRate.convertFromUnderlying(fCash.neg());

Whereas the value of strategy tokens that belong to that user account are being calculated by calling IStrategyVault(vault).convertStrategyToUnderlying() on the associated strategy vault:

contracts-v2/contracts/internal/vaults/VaultState.sol:L314-L324

function getCashValueOfShare(
    VaultState memory vaultState,
    VaultConfig memory vaultConfig,
    address account,
    uint256 vaultShares
) internal view returns (int256 assetCashValue) {
    if (vaultShares == 0) return 0;
    (uint256 assetCash, uint256 strategyTokens) = getPoolShare(vaultState, vaultShares);
    int256 underlyingInternalStrategyTokenValue = _getStrategyTokenValueUnderlyingInternal(
        vaultConfig.borrowCurrencyId, vaultConfig.vault, account, strategyTokens, vaultState.maturity
    );

contracts-v2/contracts/internal/vaults/VaultState.sol:L296-L311

function _getStrategyTokenValueUnderlyingInternal(
    uint16 currencyId,
    address vault,
    address account,
    uint256 strategyTokens,
    uint256 maturity
) private view returns (int256) {
    Token memory token = TokenHandler.getUnderlyingToken(currencyId);
    // This will be true if the the token is "NonMintable" meaning that it does not have
    // an underlying token, only an asset token
    if (token.decimals == 0) token = TokenHandler.getAssetToken(currencyId);

    return token.convertToInternal(
        IStrategyVault(vault).convertStrategyToUnderlying(account, strategyTokens, maturity)
    );
}

From conversations with the Notional team, it is assumed that this call returns the strategy token value subtracted against the secondary currencies debt, as is the case in the Balancer2TokenVault for example. In other words, when collateral ratio checks are performed, those strategy vaults that utilize secondary currency borrows would need to calculate the value of strategy tokens already accounting for any secondary debt. However, this is a dependency for a critical piece of the Notional controller’s strategy vaults collateral checks.

Therefore, even though the strategy vaults’ code and logic would be vetted before their whitelisting into the Notional system, they would still remain an external dependency with relatively arbitrary code responsible for the liquidation infrastructure that could lead to bad debt or incorrect liquidations if the vaults give inaccurate information, and thus potential loss of funds.

Recommendation

Specific strategy vault implementations using secondary borrows were not in scope of this audit. However, since the core Notional Vault system was, and it includes secondary borrow currency functionality, from the point of view of the larger Notional system it is recommended to include secondary debt checks within the Notional controller contract to reduce external dependency on the strategy vaults’ logic.

Description

As was previously mentioned some strategies require borrowing one or two secondary currencies. All secondary currencies have to be whitelisted in the VaultConfig.secondaryBorrowCurrencies. Borrow operation on secondary currencies is performed in the borrowSecondaryCurrencyToVault(...) function. Due to a require statement in that function, vaults will only be able to borrow secondary currencies if both of the currencies are whitelisted in VaultConfig.secondaryBorrowCurrencies. Considering that many strategies will have just one secondary currency, this will prevent those strategies from borrowing any secondary assets.

Examples

contracts-v2/contracts/external/actions/VaultAction.sol:L214

require(currencies[0] != 0 && currencies[1] != 0);

Recommendation

We suggest that the && operator is replaced by the || operator. Ideally, an additional check will be performed that will ensure that values in argument arrays fCashToBorrow, maxBorrowRate, and minRollLendRate are passed under the same index as the whitelisted currencies in VaultConfig.secondaryBorrowCurrencies.

contracts-v2/contracts/external/actions/VaultAction.sol:L202-L208

function borrowSecondaryCurrencyToVault(
    address account,
    uint256 maturity,
    uint256[2] calldata fCashToBorrow,
    uint32[2] calldata maxBorrowRate,
    uint32[2] calldata minRollLendRate
) external override returns (uint256[2] memory underlyingTokensTransferred) {

Description

One of the actions allowed in Notional Strategy Vaults is to roll an account’s maturity to a later one by borrowing from a later maturity and repaying that into the debt of the earlier maturity.

However, this could cause an issue if the vault is at maximum capacity at the time of the roll. When an account performs this type of roll, the new borrow would have to be more than the existing debt simply because it has to at least cover the existing debt and pay for the borrow fees that get added on every new borrow. Since the whole vault was already at max borrow capacity before with the old, smaller borrow, this process would revert at the end after the new borrow as well once the process gets to VaultAccount.updateAccountfCash and VaultConfiguration.updateUsedBorrowCapacity:

contracts-v2/contracts/internal/vaults/VaultConfiguration.sol:L243-L257

function updateUsedBorrowCapacity(
    address vault,
    uint16 currencyId,
    int256 netfCash
) internal returns (int256 totalUsedBorrowCapacity) {
    VaultBorrowCapacityStorage storage cap = LibStorage.getVaultBorrowCapacity()[vault][currencyId];

    // Update the total used borrow capacity, when borrowing this number will increase (netfCash < 0),
    // when lending this number will decrease (netfCash > 0).
    totalUsedBorrowCapacity = int256(uint256(cap.totalUsedBorrowCapacity)).sub(netfCash);
    if (netfCash < 0) {
        // Always allow lending to reduce the total used borrow capacity to satisfy the case when the max borrow
        // capacity has been reduced by governance below the totalUsedBorrowCapacity. When borrowing, it cannot
        // go past the limit.
        require(totalUsedBorrowCapacity <= int256(uint256(cap.maxBorrowCapacity)), "Max Capacity");

The result is that users won’t able to roll while the vault is at max capacity. However, users may exit some part of their position to reduce their borrow, thereby reducing the overall vault borrow capacity, and then could execute the roll. A bigger problem would occur if the vault configuration got updated to massively reduce the borrow capacity, which would force users to exit their position more significantly with likely a much smaller chance at being able to roll.

Recommendation

Document this case so that users can realise that rolling may not always be an option. Perhaps consider adding ways where users can pay a small deposit, like on enterVault, to offset the additional difference in borrows and pay for fees so they can remain with essentially the same size position within Notional.

Description

During the rollover of the strategy position into a longer maturity, several things happen:

• Funds are borrowed from the longer maturity to pay off the debt and fees of the current maturity.
• Strategy tokens that are associated with the current maturity are moved to the new maturity.
• Any additional funds provided by the account are deposited into the strategy into a new longer maturity.

In reality, due to the AMM nature of the protocol, the funds borrowed from the new maturity could exceed the debt the account has in the current maturity, resulting in a non-zero vaultAccount.tempCashBalance. In that case, those funds will be deposited into the strategy. That would happen even if there are no external funds supplied by the account for the deposit.

It is possible that the dust in the temporary account balance will not cover the gas cost of triggering a full deposit call of the strategy.

Examples

contracts-v2/contracts/internal/vaults/VaultState.sol:L244-L246

uint256 strategyTokensMinted = vaultConfig.deposit(
    vaultAccount.account, vaultAccount.tempCashBalance, vaultState.maturity, additionalUnderlyingExternal, vaultData
);

Recommendation

We suggest that additional checks are introduced that would check that on rollover vaultAccount.tempCashBalance + additionalUnderlyingExternal > 0 or larger than a certain threshold like minAccountBorrowSize for example.

Description

The Notional Strategy Vaults utilise collateral to allow leveraged borrowing as long as the account passes the checkCollateralRatio check that ensures the overall account value is at least minCollateralRatio greater than its debts. If the account doesn’t have sufficient collateral, it goes through a liquidation process where some of the collateral is sold to liquidators for the account’s borrowed currency in attempt to improve the collateral ratio. However, if the account is severely undercollateralised, the entire account position is liquidated and given over to the liquidator:

contracts-v2/contracts/internal/vaults/VaultAccount.sol:L282-L289

int256 depositRatio = maxLiquidatorDepositAssetCash.mul(vaultConfig.liquidationRate).div(vaultShareValue);

// Use equal to so we catch potential off by one issues, the deposit amount calculated inside the if statement
// below will round the maxLiquidatorDepositAssetCash down
if (depositRatio >= Constants.RATE_PRECISION) {
    maxLiquidatorDepositAssetCash = vaultShareValue.divInRatePrecision(vaultConfig.liquidationRate);
    // Set this to true to ensure that the account gets fully liquidated
    mustLiquidateFullAmount = true;

Here, the liquidator will need to deposit exactly maxLiquidatorDepositAssetCash=vaultShareValue/liquidationRate in order to get all of account’s assets, i.e. all of vaultShareValue in the form of vaultAccount.vaultShares. In fact, later this deposit will be set in vaultAccount.tempCashBalance:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L361-L380

int256 maxLiquidatorDepositExternal = assetToken.convertToExternal(maxLiquidatorDepositAssetCash);

// NOTE: deposit amount external is always positive in this method
if (depositAmountExternal < maxLiquidatorDepositExternal) {
    // If this flag is set, the liquidator must deposit more cash in order to liquidate the account
    // down to a zero fCash balance because it will fall under the minimum borrowing limit.
    require(!mustLiquidateFull, "Must Liquidate All Debt");
} else {
    // In the other case, limit the deposited amount to the maximum
    depositAmountExternal = maxLiquidatorDepositExternal;
}

// Transfers the amount of asset tokens into Notional and credit it to the account's temp cash balance
int256 assetAmountExternalTransferred = assetToken.transfer(
    liquidator, vaultConfig.borrowCurrencyId, depositAmountExternal
);

vaultAccount.tempCashBalance = vaultAccount.tempCashBalance.add(
    assetToken.convertToInternal(assetAmountExternalTransferred)
);

Then the liquidator will get:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L274-L281

uint256 vaultSharesToLiquidator;
{
    vaultSharesToLiquidator = vaultAccount.tempCashBalance.toUint()
        .mul(vaultConfig.liquidationRate.toUint())
        .mul(vaultAccount.vaultShares)
        .div(vaultShareValue.toUint())
        .div(uint256(Constants.RATE_PRECISION));
}

And if (except for precision and conversions) vaultAccount.tempCashBalance=maxLiquidatorDepositAssetCash=vaultShareValue/liquidationRate, then vaultSharesToLiquidator = (vaultAccount.tempCashBalance * liquidationRate * vaultAccount.vaultShares) / (vaultShareValue) becomes vaultSharesToLiquidator = ((vaultShareValue/liquidationRate)* liquidationRate * vaultAccount.vaultShares) / (vaultShareValue) = vaultAccount.vaultShares

In other words, the liquidator needed to deposit exactly vaultShareValue/liquidationRate to get all vaultAccount.vaultShares. However, the liquidator deposit (what would be in vaultAccount.tempCashBalance) needs to cover all of that account’s debt, i.e. vaultAccount.fCash. At the end of the liquidation process, the vault account has its fCash and tempCash balances updated:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L289-L290

int256 fCashToReduce = vaultConfig.assetRate.convertToUnderlying(vaultAccount.tempCashBalance);
vaultAccount.updateAccountfCash(vaultConfig, vaultState, fCashToReduce, vaultAccount.tempCashBalance.neg());

contracts-v2/contracts/internal/vaults/VaultAccount.sol:L77-L88

function updateAccountfCash(
    VaultAccount memory vaultAccount,
    VaultConfig memory vaultConfig,
    VaultState memory vaultState,
    int256 netfCash,
    int256 netAssetCash
) internal {
    vaultAccount.tempCashBalance = vaultAccount.tempCashBalance.add(netAssetCash);

    // Update fCash state on the account and the vault
    vaultAccount.fCash = vaultAccount.fCash.add(netfCash);
    require(vaultAccount.fCash <= 0);

While the vaultAccount.tempCashBalance gets cleared to 0, the vaultAccount.fCash amount only gets to vaultAccount.fCash = vaultAccount.fCash.add(netfCash), and netfCash=fCashToReduce = vaultConfig.assetRate.convertToUnderlying(vaultAccount.tempCashBalance), which, based on the constraints above essentially becomes:

vaultAccount.fCash=vaultAccount.fCash+vaultConfig.assetRate.convertToUnderlying(assetToken.convertToExternal(vaultShareValue/vaultConfig.liquidationRate))

However, later this account is set on storage, and, considering it is going through 100% liquidation, the account will necessarily be below minimum borrow size and will need to be at vaultAccount.fCash==0.

contracts-v2/contracts/internal/vaults/VaultAccount.sol:L52-L62

function setVaultAccount(VaultAccount memory vaultAccount, VaultConfig memory vaultConfig) internal {
    mapping(address => mapping(address => VaultAccountStorage)) storage store = LibStorage
        .getVaultAccount();
    VaultAccountStorage storage s = store[vaultAccount.account][vaultConfig.vault];

    // The temporary cash balance must be cleared to zero by the end of the transaction
    require(vaultAccount.tempCashBalance == 0); // dev: cash balance not cleared
    // An account must maintain a minimum borrow size in order to enter the vault. If the account
    // wants to exit under the minimum borrow size it must fully exit so that we do not have dust
    // accounts that become insolvent.
    require(vaultAccount.fCash == 0 || vaultConfig.minAccountBorrowSize <= vaultAccount.fCash.neg(), "Min Borrow");

The case where vaultAccount.fCash>0 is taken care of by taking any extra repaid value and assigning it to the protocol, zeroing out the account’s balances:

contracts-v2/contracts/external/actions/VaultAccountAction.sol:L293

if (vaultAccount.fCash > 0) vaultAccount.fCash = 0;

The case where vaultAccount.fCash < 0 is however not addressed, and instead the process will revert. This will occur whenever the vaultShareValue discounted with the liquidation rate is less than the fCash debt after all the conversions between external and underlying accounting. So, whenever the below is true, the account will not be liquidate-able. fCash>vaultShareValue/liquidationRate

This is an issue because the account is still technically solvent even though it is undercollateralized, but the current implementation would simply revert until the account is entirely insolvent (still without liquidation options) or its balances are restored enough to be liquidated fully.

Consider implementing a dynamic liquidation rate that becomes smaller the closer the account is to insolvency, thereby encouraging liquidators to promptly liquidate the accounts.

Description

Some strategy vaults utilize borrowing one currency, swapping it for another, and then using the new currency somewhere to generate yield. For example, the CrossCurrencyfCash strategy vault could borrow USDC, swap it for DAI, and then deposit that DAI back into Notional if the DAI lending interest rates are greater than USDC borrowing interest rates. However, during vault settlement the assets would need to be swapped back into the original borrow currency.

Since these vaults control the borrowed assets that go only into white-listed strategies, the Notional system allows users to borrow multiples of their posted collateral and claim the yield from a much larger position. As a result, these strategy vaults would likely have significant funds being borrowed and managed into these strategies.

However, as mentioned above, these strategies usually utilize a trading mechanism to swap borrowed currencies into whatever is required by the strategy, and these trades may be quite large. In fact, the BaseStrategyVault implementation contains functions that interact with Notional’s trading module to assist with those swaps:

strategy-vaults/contracts/vaults/BaseStrategyVault.sol:L100-L127

/// @notice Can be used to delegate call to the TradingModule's implementation in order to execute
/// a trade.
function _executeTrade(
    uint16 dexId,
    Trade memory trade
) internal returns (uint256 amountSold, uint256 amountBought) {
    (bool success, bytes memory result) = nProxy(payable(address(TRADING_MODULE))).getImplementation()
        .delegatecall(abi.encodeWithSelector(ITradingModule.executeTrade.selector, dexId, trade));
    require(success);
    (amountSold, amountBought) = abi.decode(result, (uint256, uint256));
}

/// @notice Can be used to delegate call to the TradingModule's implementation in order to execute
/// a trade.
function _executeTradeWithDynamicSlippage(
    uint16 dexId,
    Trade memory trade,
    uint32 dynamicSlippageLimit
) internal returns (uint256 amountSold, uint256 amountBought) {
    (bool success, bytes memory result) = nProxy(payable(address(TRADING_MODULE))).getImplementation()
        .delegatecall(abi.encodeWithSelector(
            ITradingModule.executeTradeWithDynamicSlippage.selector,
            dexId, trade, dynamicSlippageLimit
        )
    );
    require(success);
    (amountSold, amountBought) = abi.decode(result, (uint256, uint256));
}

Although some strategies may manage stablecoin <-> stablecoin swaps that typically would incur low slippage, large size trades could still suffer from low on-chain liquidity and end up getting frontrun and “sandwiched” by MEV bots or other actors, thereby extracting maximum amount from the strategy vault swaps as slippage permits. This could be especially significant during vaults’ settlements, that can be initiated by anyone, as lending currencies may be swapped in large batches and not do it on a per-account basis. For example with the CrossCurrencyfCash vault, it can only enter settlement if all strategy tokens (lending currency in this case) are gone and swapped back into the borrow currency:

strategy-vaults/contracts/vaults/CrossCurrencyfCashVault.sol:L141-L143

if (vaultState.totalStrategyTokens == 0) {
    NOTIONAL.settleVault(address(this), maturity);
}

As a result, in addition to the risk of stablecoins’ getting off-peg, unfavorable market liquidity conditions and arbitrage-seeking actors could eat into the profits generated by this strategy as per the maximum allowed slippage. However, during settlement the strategy vaults don’t have the luxury of waiting for the right conditions to perform the trade as the borrows need to repaid at their maturities.

So, the profitability of the vaults, and therefore users, could suffer due to potential low market liquidity allowing high slippage and risks of being frontrun with the chosen strategy vaults’ currencies.

Recommendation

Ensure that the currencies chosen to generate yield in the strategy vaults have sufficient market liquidity on exchanges allowing for low slippage swaps.

Description

Cross currency strategy currently takes lend and borrow currencies as the initialization arguments. Due to the way strategy and TradingModule are implemented, the strategy will not operate correctly if lend and borrow currencies are the same. Despite those arguments being passed exclusively by the Notional team, there is still a possibility of incorrect arguments being used.

Examples

strategy-vaults/contracts/vaults/CrossCurrencyfCashVault.sol:L77-L82

function initialize(
    string memory name_,
    uint16 borrowCurrencyId_,
    uint16 lendCurrencyId_,
    uint64 settlementSlippageLimit_
) external initializer {

Recommendation

We suggest adding a require check in the initialization function of the CrossCurrencyfCashVault.sol that will ensure that lend and borrow currencies are different.