xiaoming90 - VaultAccountSecondaryDebtShareStorage.maturity will be cleared prematurely

[sherlock-admin]

xiaoming90

high

VaultAccountSecondaryDebtShareStorage.maturity will be cleared prematurely

Summary

VaultAccountSecondaryDebtShareStorage.maturity will be cleared prematurely during liquidation

Vulnerability Detail

If both the accountDebtOne and accountDebtTwo of secondary currencies are zero, Notional will consider both debt shares to be cleared to zero, and the maturity will be cleared as well as shown below.

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultSecondaryBorrow.sol#L495

File: VaultSecondaryBorrow.sol
495:     function _setAccountMaturity(
496:         VaultAccountSecondaryDebtShareStorage storage accountStorage,
497:         int256 accountDebtOne,
498:         int256 accountDebtTwo,
499:         uint40 maturity
500:     ) private {
501:         if (accountDebtOne == 0 && accountDebtTwo == 0) {
502:             // If both debt shares are cleared to zero, clear the maturity as well.
503:             accountStorage.maturity = 0;
504:         } else {
505:             // In all other cases, set the account to the designated maturity
506:             accountStorage.maturity = maturity;
507:         }
508:     }

VaultLiquidationAction.deleverageAccount function

Within the VaultLiquidationAction.deleverageAccount function, it will call the _reduceAccountDebt function.

Referring to the _reduceAccountDebt function below. Assume that the currencyIndex reference to a secondary currency. In this case, the else logic in Line 251 will be executed. An important point to take note of that is critical to understand this bug is that only ONE of the prime rates will be set as it assumes that the other prime rate will not be used (Refer to Line 252 - 255). However, this assumption is incorrect.

Assume that the currencyIndex is 1. Then netUnderlyingDebtOne parameter will be set to a non-zero value (depositUnderlyingInternal) at Line 261 while netUnderlyingDebtTwo parameter will be set to zero at Line 262. This is because, in Line 263 of the _reduceAccountDebt function, the pr[0] will be set to the prime rate, while the pr[1] will be zero or empty. It will then proceed to call the VaultSecondaryBorrow.updateAccountSecondaryDebt

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/VaultLiquidationAction.sol#L239

File: VaultLiquidationAction.sol
239:     function _reduceAccountDebt(
240:         VaultConfig memory vaultConfig,
241:         VaultState memory vaultState,
242:         VaultAccount memory vaultAccount,
243:         PrimeRate memory primeRate,
244:         uint256 currencyIndex,
245:         int256 depositUnderlyingInternal,
246:         bool checkMinBorrow
247:     ) private {
248:         if (currencyIndex == 0) {
249:             vaultAccount.updateAccountDebt(vaultState, depositUnderlyingInternal, 0);
250:             vaultState.setVaultState(vaultConfig);
251:         } else {
252:             // Only set one of the prime rates, the other prime rate is not used since
253:             // the net debt amount is set to zero
254:             PrimeRate[2] memory pr;
255:             pr[currencyIndex - 1] = primeRate;
256: 
257:             VaultSecondaryBorrow.updateAccountSecondaryDebt(
258:                 vaultConfig,
259:                 vaultAccount.account,
260:                 vaultAccount.maturity,
261:                 currencyIndex == 1 ? depositUnderlyingInternal : 0,
262:                 currencyIndex == 2 ? depositUnderlyingInternal : 0,
263:                 pr,
264:                 checkMinBorrow
265:             );
266:         }
267:     }

Within the updateAccountSecondaryDebt function, at Line 272, assume that accountStorage.accountDebtTwo is 100. However, since pr[1] is not initialized, the VaultStateLib.readDebtStorageToUnderlying will return a zero value and set the accountDebtTwo to zero.

Assume that the liquidator calls the deleverageAccount function to clear all the debt of the currencyIndex secondary currency. Line 274 will be executed, and accountDebtOne will be set to zero.

Note that at this point, both accountDebtOne and accountDebtTwo are zero. At Line 301, the _setAccountMaturity will set the accountStorage.maturity = 0 , which clears the vault account's maturity.

An important point here is that the liquidator did not clear the accountDebtTwo. Yet, accountDebtTwo became zero in memory during the execution and caused Notional to wrongly assume that both debt shares had been cleared to zero.

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultSecondaryBorrow.sol#L256

File: VaultSecondaryBorrow.sol
256:     function updateAccountSecondaryDebt(
257:         VaultConfig memory vaultConfig,
258:         address account,
259:         uint256 maturity,
260:         int256 netUnderlyingDebtOne,
261:         int256 netUnderlyingDebtTwo,
262:         PrimeRate[2] memory pr,
263:         bool checkMinBorrow
264:     ) internal {
265:         VaultAccountSecondaryDebtShareStorage storage accountStorage = 
266:             LibStorage.getVaultAccountSecondaryDebtShare()[account][vaultConfig.vault];
267:         // Check maturity
268:         uint256 accountMaturity = accountStorage.maturity;
269:         require(accountMaturity == maturity || accountMaturity == 0);
270:         
271:         int256 accountDebtOne = VaultStateLib.readDebtStorageToUnderlying(pr[0], maturity, accountStorage.accountDebtOne); 
272:         int256 accountDebtTwo = VaultStateLib.readDebtStorageToUnderlying(pr[1], maturity, accountStorage.accountDebtTwo);
273:         if (netUnderlyingDebtOne != 0) {
274:             accountDebtOne = accountDebtOne.add(netUnderlyingDebtOne);

276:             _updateTotalSecondaryDebt(
277:                 vaultConfig, account, vaultConfig.secondaryBorrowCurrencies[0], maturity, netUnderlyingDebtOne, pr[0]
278:             );
279: 
280:             accountStorage.accountDebtOne = VaultStateLib.calculateDebtStorage(pr[0], maturity, accountDebtOne)
281:                 .neg().toUint().toUint80();
282:         }
283: 
284:         if (netUnderlyingDebtTwo != 0) {
285:             accountDebtTwo = accountDebtTwo.add(netUnderlyingDebtTwo);
286: 
287:             _updateTotalSecondaryDebt(
288:                 vaultConfig, account, vaultConfig.secondaryBorrowCurrencies[1], maturity, netUnderlyingDebtTwo, pr[1]
289:             );
290: 
291:             accountStorage.accountDebtTwo = VaultStateLib.calculateDebtStorage(pr[1], maturity, accountDebtTwo)
292:                 .neg().toUint().toUint80();
293:         }
294: 
295:         if (checkMinBorrow) {
296:             // No overflow on negation due to overflow checks above
297:             require(accountDebtOne == 0 || vaultConfig.minAccountSecondaryBorrow[0] <= -accountDebtOne, "min borrow");
298:             require(accountDebtTwo == 0 || vaultConfig.minAccountSecondaryBorrow[1] <= -accountDebtTwo, "min borrow");
299:         }
300: 
301:         _setAccountMaturity(accountStorage, accountDebtOne, accountDebtTwo, maturity.toUint40());
302:     }

The final state will be VaultAccountSecondaryDebtShareStorage as follows:

• maturity and accountDebtOne are zero
• accountDebtTwo = 100

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/global/Types.sol#L551

struct VaultAccountSecondaryDebtShareStorage {
    // Maturity for the account's secondary borrows. This is stored separately from
    // the vault account maturity to ensure that we have access to the proper state
    // during a roll borrow position. It should never be allowed to deviate from the
    // vaultAccount.maturity value (unless it is cleared to zero).
    uint40 maturity;
    // Account debt for the first secondary currency in either fCash or pCash denomination
    uint80 accountDebtOne;
    // Account debt for the second secondary currency in either fCash or pCash denomination
    uint80 accountDebtTwo;
}

Firstly, it does not make sense to have accountDebtTwo but no maturity in storage, which also means the vault account data is corrupted. Secondly, when maturity is zero, it also means that the vault account did not borrow anything from Notional. Lastly, many vault logic would break since it relies on the maturity value.

VaultLiquidationAction.liquidateVaultCashBalance function

The root cause lies in the implementation of the _reduceAccountDebt function. Since liquidateVaultCashBalance function calls the _reduceAccountDebt function to reduce the debt of the vault account being liquidated, the same issue will occur here.

Impact

Any vault logic that relies on the VaultAccountSecondaryDebtShareStorage's maturity value would break since it has been cleared (set to zero). For instance, a vault account cannot be settled anymore as the following settleSecondaryBorrow function will always revert. Since storedMaturity == 0 but accountDebtTwo is not zero, Line 399 below will always revert.

As a result, a vault account with secondary currency debt cannot be settled. This also means that the vault account cannot exit since a vault account needs to be settled before exiting, causing users' assets to be stuck within the protocol.

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultSecondaryBorrow.sol#L385

File: VaultSecondaryBorrow.sol
385:     function settleSecondaryBorrow(VaultConfig memory vaultConfig, address account) internal returns (bool) {
386:         if (!vaultConfig.hasSecondaryBorrows()) return false;
387: 
388:         VaultAccountSecondaryDebtShareStorage storage accountStorage = 
389:             LibStorage.getVaultAccountSecondaryDebtShare()[account][vaultConfig.vault];
390:         uint256 storedMaturity = accountStorage.maturity;
391: 
392:         // NOTE: we can read account debt directly since prime cash maturities never enter this block of code.
393:         int256 accountDebtOne = -int256(uint256(accountStorage.accountDebtOne));
394:         int256 accountDebtTwo = -int256(uint256(accountStorage.accountDebtTwo));
395:         
396:         if (storedMaturity == 0) {
397:             // Handles edge condition where an account is holding vault shares past maturity without
398:             // any debt position.
399:             require(accountDebtOne == 0 && accountDebtTwo == 0); 
400:         } else {

In addition, the vault account data is corrupted as there is a secondary debt without maturity, which might affect internal accounting and tracking.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/VaultLiquidationAction.sol#L239

Tool used

Manual Review

Recommendation

Fetch the prime rate of both secondary currencies because they are both needed within the updateAccountSecondaryDebt function when converting debt storage to underlying.

    function _reduceAccountDebt(
        VaultConfig memory vaultConfig,
        VaultState memory vaultState,
        VaultAccount memory vaultAccount,
        PrimeRate memory primeRate,
        uint256 currencyIndex,
        int256 depositUnderlyingInternal,
        bool checkMinBorrow
    ) private {
        if (currencyIndex == 0) {
            vaultAccount.updateAccountDebt(vaultState, depositUnderlyingInternal, 0);
            vaultState.setVaultState(vaultConfig);
        } else {
            // Only set one of the prime rates, the other prime rate is not used since
            // the net debt amount is set to zero
            PrimeRate[2] memory pr;
-           pr[currencyIndex - 1] = primeRate;
+	    pr = VaultSecondaryBorrow.getSecondaryPrimeRateStateful(vaultConfig);

            VaultSecondaryBorrow.updateAccountSecondaryDebt(
                vaultConfig,
                vaultAccount.account,
                vaultAccount.maturity,
                currencyIndex == 1 ? depositUnderlyingInternal : 0,
                currencyIndex == 2 ? depositUnderlyingInternal : 0,
                pr,
                checkMinBorrow
            );
        }
    }

[jeffywu]

Fixed: notional-finance/contracts-v2#127

[xiaoming9090]

@0xleastwood + @xiaoming9090 : Verified. Fixed in PR notional-finance/contracts-v2#127