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.
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
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.
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.
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
);
}
}
xiaoming90
high
VaultAccountSecondaryDebtShareStorage.maturity
will be cleared prematurelySummary
VaultAccountSecondaryDebtShareStorage.maturity
will be cleared prematurely during liquidationVulnerability Detail
If both the
accountDebtOne
andaccountDebtTwo
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
VaultLiquidationAction.deleverageAccount
functionWithin the
VaultLiquidationAction.deleverageAccount
function, it will call the_reduceAccountDebt
function.Referring to the
_reduceAccountDebt
function below. Assume that thecurrencyIndex
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
is1
. ThennetUnderlyingDebtOne
parameter will be set to a non-zero value (depositUnderlyingInternal
) at Line 261 whilenetUnderlyingDebtTwo
parameter will be set to zero at Line 262. This is because, in Line 263 of the_reduceAccountDebt
function, thepr[0]
will be set to the prime rate, while thepr[1]
will be zero or empty. It will then proceed to call theVaultSecondaryBorrow.updateAccountSecondaryDebt
https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/VaultLiquidationAction.sol#L239
Within the
updateAccountSecondaryDebt
function, at Line 272, assume thataccountStorage.accountDebtTwo
is100
. However, sincepr[1]
is not initialized, theVaultStateLib.readDebtStorageToUnderlying
will return a zero value and set theaccountDebtTwo
to zero.Assume that the liquidator calls the
deleverageAccount
function to clear all the debt of thecurrencyIndex
secondary currency. Line 274 will be executed, andaccountDebtOne
will be set to zero.Note that at this point, both
accountDebtOne
andaccountDebtTwo
are zero. At Line 301, the_setAccountMaturity
will set theaccountStorage.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
The final state will be
VaultAccountSecondaryDebtShareStorage
as follows:maturity
andaccountDebtOne
are zeroaccountDebtTwo
= 100https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/global/Types.sol#L551
Firstly, it does not make sense to have
accountDebtTwo
but no maturity in storage, which also means the vault account data is corrupted. Secondly, whenmaturity
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
functionThe root cause lies in the implementation of the
_reduceAccountDebt
function. SinceliquidateVaultCashBalance
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 followingsettleSecondaryBorrow
function will always revert. SincestoredMaturity == 0
butaccountDebtTwo
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
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.