xiaoming90 - Account Collateral Ratio check is skipped even though there might be residual secondary debt

[sherlock-admin]

xiaoming90

high

Account Collateral Ratio check is skipped even though there might be residual secondary debt

Summary

Account Collateral Ratio check is skipped even though there might be residual secondary debt. This allows vault accounts with some residual debt in secondary currencies to fully exited

Vulnerability Detail

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

File: VaultAccountAction.sol
271:         if (vaultAccount.accountDebtUnderlying == 0 && vaultAccount.vaultShares == 0) {
272:             // If the account has no position in the vault at this point, set the maturity to zero as well
273:             vaultAccount.maturity = 0;
274:         }
275:         vaultAccount.setVaultAccount({vaultConfig: vaultConfig, checkMinBorrow: true});
276: 
277:         // It's possible that the user redeems more vault shares than they lend (it is not always the case
278:         // that they will be increasing their collateral ratio here, so we check that this is the case). No
279:         // need to check if the account has exited in full (maturity == 0).
280:         if (vaultAccount.maturity != 0) {
281:             IVaultAccountHealth(address(this)).checkVaultAccountCollateralRatio(vault, account);
282:         }

Assume a vault account still has some debt in secondary currencies (accountDebtOne and accountDebtTwo). In Line 271, the code will ignore them and only considers the debt in primary currency (accountDebtUnderlying). Then, it will proceed to set the maturity of the vault account to zero.

As a result, the checkVaultAccountCollateralRatio at the end of the function (In Line 281) will be skipped, and it fails to detect that there is still some debt in secondary currencies not repaid in this vault account. The vault account will be fully exited after the transaction.

Impact

If a vault account with some residual debt in secondary currencies fully exited, it will leave bad debt with the protocol, potentially threatening the protocol's solvency.

Code Snippet

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

Tool used

Manual Review

Recommendation

On the Notional end, it is recommended to "double-check" that all secondary debts are cleared (set to zero) before marking this vault account as "fully-exit" and allowing it to skip the final checkVaultAccountCollateralRatio check.

Consider checking that all secondary debts of a vault account are cleared before executing a full exit.

+ int256 accountDebtOne;
+ int256 accountDebtTwo;

+ if (vaultConfig.hasSecondaryBorrows()) {
+ 	(/* */, accountDebtOne, accountDebtTwo) = VaultSecondaryBorrow.getAccountSecondaryDebt(vaultConfig, account, pr);
+ }

- if (vaultAccount.accountDebtUnderlying == 0 && vaultAccount.vaultShares == 0) {
+ if (vaultAccount.accountDebtUnderlying == 0 && vaultAccount.vaultShares == 0 && accountDebtOne == 0 && accountDebtTwo == 0) {
	// If the account has no position in the vault at this point, set the maturity to zero as well
	vaultAccount.maturity = 0;
}
vaultAccount.setVaultAccount({vaultConfig: vaultConfig, checkMinBorrow: true});

In addition, Strategy Vault and Notional have been designed to be as "isolated" as possible to minimize collateral damage if something happens to one of them. Therefore, trust should be minimized between them, and this check should be performed to ensure that all secondary debts are cleared before a full exit.

Duplicate of #184