Reentrancy flag is not supported when exiting vault

Summary

When the vault attempts to re-enter the exitVault() function, it will always revert due to the flash-loan/MEV mitigation control.

Vulnerability Detail

When the enterVault(), rollVaultPosition(), and exitVault() functions are called, they trigger the settleAccountOrAccruePrimeCashFees() function.

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L487

File: VaultAccount.sol
487:     function settleAccountOrAccruePrimeCashFees(
488:         VaultAccount memory vaultAccount,
489:         VaultConfig memory vaultConfig
490:     ) internal returns (bool didSettle) {
..SNIP..
496:         // If the account did not settle but is in the prime cash maturity, assess a fee.
497:         if (!didSettle && vaultAccount.maturity == Constants.PRIME_CASH_VAULT_MATURITY) {
498:             // The prime cash fee is deducted from the tempCashBalance
499:             vaultConfig.assessVaultFees(
500:                 vaultAccount,
501:                 vaultConfig.primeRate.convertFromUnderlying(vaultAccount.accountDebtUnderlying).neg(),
502:                 vaultAccount.maturity,
503:                 block.timestamp
504:             );
505:         }
506:     }

The settleAccountOrAccruePrimeCashFees() function will in turn call the assessVaultFees() function followed by the calculateVaultFees() function.

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L257

File: VaultConfiguration.sol
236:     function calculateVaultFees(
237:         VaultConfig memory vaultConfig,
238:         VaultAccount memory vaultAccount,
239:         int256 primeCashBorrowed,
240:         uint256 maturity,
241:         uint256 blockTime
242:     ) internal pure returns (int256 netTotalFee) {
..SNIP..
257:             vaultAccount.lastUpdateBlockTime = blockTime;

Within the calculateVaultFees function, it will update vaultAccount.lastUpdateBlockTime to block.timestamp at Line 257 above.

The purpose of enabling the re-entrancy flag on a vault is to allow the vault to callback to Notional for a second time to carry out the necessary actions (e.g. enterVault, rollVaultPosition, and exitVault).

However, it was observed that the re-entrancy flag would not work for the exitVault function. Assume the following scenario:

1) Someone calls the enterVault, rollVaultPosition, and exitVault functions 2) The settleAccountOrAccruePrimeCashFees function will be called, and vaultAccount.lastUpdateBlockTime is set to block.timestamp 3) Notional pass the control to the Strategy Vault 4) The Strategy Vault performs some actions and callback to Notional's exitVault function 5) The exitVault function will always revert because the require statement at Line 242 require(vaultAccount.lastUpdateBlockTime + Constants.VAULT_ACCOUNT_MIN_TIME <= block.timestamp); will always be false. 6) Note that vaultAccount.lastUpdateBlockTime has been set to block.timestamp earlier. So the condition can be evaluated to require(block.timestamp + Constants.VAULT_ACCOUNT_MIN_TIME <= block.timestamp), which will always be false.

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

File: VaultAccountAction.sol
224:     function exitVault(
225:         address account,
226:         address vault,
227:         address receiver,
228:         uint256 vaultSharesToRedeem,
229:         uint256 lendAmount,
230:         uint32 minLendRate,
231:         bytes calldata exitVaultData
232:     ) external payable override nonReentrant returns (uint256 underlyingToReceiver) {
233:         VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
234:         vaultConfig.authorizeCaller(account, VaultConfiguration.ONLY_VAULT_EXIT);
235: 
236:         // If the vault allows further re-entrancy then set the status back to the default
237:         if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
238:             reentrancyStatus = _NOT_ENTERED;
239:         }
240: 
241:         VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vaultConfig);
242:         require(vaultAccount.lastUpdateBlockTime + Constants.VAULT_ACCOUNT_MIN_TIME <= block.timestamp);

Impact

Some strategy vaults are designed to rely on the ability to re-enter Notional to function properly. Without this ability, those vault would be broken potentially causing a wide range of issues such as being unable to enter/deposit/exit/redeem or stuck assets.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L487

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L257

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

Tool used

Manual Review

Recommendation

Consider allowing the vault to bypass the flash-loan/MEV mitigation control if the call comes from the vault itself.

function exitVault(
..SNIP..
    // If the vault allows further re-entrancy then set the status back to the default
    if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
        reentrancyStatus = _NOT_ENTERED;
    }

    VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vaultConfig);
+   if (msg.sender != vaultConfig.vault) {
        require(vaultAccount.lastUpdateBlockTime + Constants.VAULT_ACCOUNT_MIN_TIME <= block.timestamp);
+   }

It is important that verify that it does not cause a security implication on the Strategy Vault side before applying this change.

sherlock-audit / 2023-03-notional-judging

xiaoming90 - Reentrancy flag is not supported when exiting vault #188