shealtielanz

medium

params.oracleSlippagePercentOrLimit for static trades is not checked.

Summary

Static Trades might be settled with a large slippage causing a loss of assets as the oracleSlippagePercentOrLimit limit is not bounded, and can exceed the Constants.SLIPPAGE_LIMIT_PRECISION threshold.

Vulnerability Detail

static trades allow any params.oracleSlippagePercentOrLimit without check mating it, on the call to _executeTradeWithStaticSlippage()
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L152C1-L176C6

    function _executeTradeWithStaticSlippage(
        ITradingModule tradingModule,
        TradeParams memory params,
        address sellToken,
        address buyToken,
        uint256 amount
    ) internal returns (uint256 amountSold, uint256 amountBought) {
        require(
            params.tradeType == TradeType.EXACT_IN_SINGLE ||
            params.tradeType == TradeType.EXACT_IN_BATCH
        );

        Trade memory trade = Trade(
            params.tradeType,
            sellToken,
            buyToken,
            amount,
            params.oracleSlippagePercentOrLimit,
            block.timestamp, // deadline
            params.exchangeData
        );

        // Execute trade using the absolute slippage limit set by `oracleSlippagePercentOrLimit`
        (amountSold, amountBought) = trade._executeTrade(params.dexId, tradingModule);
    }

this function is used during reward reinvestment trades.

Impact

Trade can settle with a high slippage.

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L152C1-L176C6

Tool used

Manual Review

Recommendation

Consider restricting the slippage limit when executing static trades.

Bauer

high

Risk of reward loss and incomplete exit handling in emergencyExit() function

Summary

The emergencyExit() function currently lacks a mechanism to claim rewards during the exit process, potentially resulting in the loss of rewards.

Vulnerability Detail

The SingleSidedLPVaultBase.emergencyExit() function is designed for emergency situations, allowing the strategy to exit the pool and redeem a specified amount of pool assets.
The protocol currently executes _unstakeAndExitPool() in the emergencyExit() function, but it does not retrieve the rewards obtained during the exit. This design could pose a risk in situations where third-party protocols are compromised or during other emergency scenarios, potentially leading to the loss of rewards.

 function emergencyExit(
        uint256 claimToExit, bytes calldata /* data */
    ) external override onlyRole(EMERGENCY_EXIT_ROLE) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        if (claimToExit == 0) claimToExit = state.totalPoolClaim;

        // By setting min amounts to zero, we will accept whatever tokens come from the pool
        // in a proportional exit. Front running will not have an effect since no trading will
        // occur during a proportional exit.
        _unstakeAndExitPool(claimToExit, new uint256[](NUM_TOKENS()), true);

        state.totalPoolClaim = state.totalPoolClaim - claimToExit;
        state.setStrategyVaultState();

        emit EmergencyExit(claimToExit);
        _lockVault();
    }

Impact

Rewards may be at risk of loss.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L480-L496

Tool used

Manual Review

Recommendation

Perform the operation of claimRewardTokens() and reinvestReward() internally within the emergencyExit() function

AuditorPraise

high

borrowedCurrencyAmount gotten from calling _redeemFromNotional() in BaseStrategyVault.redeemFromNotional() will be inflated because primary token gotten from executing trades on external exchanges is not transferred back to the vaults from the Trading Module

Summary

primary tokens gotten from executing trades on external exchanges is not transferred back to the vaults from the Trading Module, this will cause issues in the BaseStrategyVault.redeemFromNotional() function

Vulnerability Detail

The vaults will use TradingModule.sol to execute trades on Dexes.

They will sell a certain amount of sellToken for buyToken(primary token).

The issue is that after the vaults use the TradingModule contract to execute trades on Dexes, the primary tokens(buy Token) gotten from executing trades on external exchanges is not transferred back to the vaults from the Trading Module

Now amount of primary token gotten from executing trades on external exchanges will reflect in borrowedCurrencyAmount but it's not really transferred back into the vaults, so borrowedCurrencyAmount will always be inflated above the balance of _UNDERLYING_TOKEN that is really in the vault resulting in issues here

The main logic of this vulnerability here is that borrowedCurrencyAmount will be inflated by the amount of primary token gotten from executing trades on external exchanges but the primary tokens aren't sent back into the vaults.

SO the primary tokens gotten from the executing trades on external exchanges won't reflect in the balance of the _UNDERLYING_TOKEN.

Impact

borrowedCurrencyAmount gotten from calling _redeemFromNotional() in BaseStrategyVault.redeemFromNotional() will be inflated because primary token gotten from executing trades on external exchanges is not transferred back to the vaults from the Trading Module

BaseStrategyVault.redeemFromNotional() may always revert because borrowedCurrencyAmount will always be inflated above the balance of _UNDERLYING_TOKEN that is really in the vault here

transferToNotional may always be greater than balance of _UNDERLYING_TOKEN causing reverts due to insufficient balance

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L204

Tool used

Manual Review

Recommendation

transfer the primary token gotten from executing trades on external exchanges back into the vaults

shealtielanz

high

Incorrect computing of the Invariant due to rounding differences

Summary

The invariant used within BalancerSpotPrice.sol to compute the spot price is not aligned with the Balancer's ComposableStablePool due to rounding differences. The spot price is used to verify if the pool has been manipulated before executing certain key vault actions (e.g. settle vault, reinvest rewards). In the worst-case scenario, it might potentially fail to detect the pool has been manipulated as the spot price computed might be inaccurate.

Vulnerability Detail

The BalancerSpotPrice.sol relies on the old version of the StableMath._calculateInvariant that allows the caller to specify if the computation should round up or down via the roundUp parameter.
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/math/StableMath.sol#L28C1-L85C6

function _calculateInvariant(
        uint256 amplificationParameter,
        uint256[] memory balances,
        bool roundUp
    ) internal pure returns (uint256) {
        /**********************************************************************************************
        // invariant                                                                                 //
        // D = invariant                                                  D^(n+1)                    //
        // A = amplification coefficient      A  n^n S + D = A D n^n + -----------                   //
        // S = sum of balances                                             n^n P                     //
        // P = product of balances                                                                   //
        // n = number of tokens                                                                      //
        *********x************************************************************************************/

        unchecked {
            // We support rounding up or down.
            uint256 sum = 0;
            uint256 numTokens = balances.length;
            for (uint256 i = 0; i < numTokens; i++) {
                sum = sum.add(balances[i]);
            }
            if (sum == 0) {
                return 0;
            }

            uint256 prevInvariant = 0;
            uint256 invariant = sum;
            uint256 ampTimesTotal = amplificationParameter * numTokens;

            for (uint256 i = 0; i < 255; i++) {
                uint256 P_D = balances[0] * numTokens;
                for (uint256 j = 1; j < numTokens; j++) {
                    P_D = Math.div(Math.mul(Math.mul(P_D, balances[j]), numTokens), invariant, roundUp);
                }
                prevInvariant = invariant;
                invariant = Math.div(
                    Math.mul(Math.mul(numTokens, invariant), invariant).add(
                        Math.div(Math.mul(Math.mul(ampTimesTotal, sum), P_D), _AMP_PRECISION, roundUp)
                    ),
                    Math.mul(numTokens + 1, invariant).add(
                        // No need to use checked arithmetic for the amp precision, the amp is guaranteed to be at least 1
                        Math.div(Math.mul(ampTimesTotal - _AMP_PRECISION, P_D), _AMP_PRECISION, !roundUp)
                    ),
                    roundUp
                );

                if (invariant > prevInvariant) {
                    if (invariant - prevInvariant <= 1) {
                        return invariant;
                    }
                } else if (prevInvariant - invariant <= 1) {
                    return invariant;
                }
            }
        }

        revert CalculationDidNotConverge();
    }

Within the BalancerSpotPrice._calculateStableMathSpotPrice() function, the StableMath._calculateInvariant() is computed rounding up.
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/BalancerSpotPrice.sol#L78C1-L97C6

    function _calculateStableMathSpotPrice(
        uint256 ampParam,
        uint256[] memory scalingFactors,
        uint256[] memory balances,
        uint256 scaledPrimary,
        uint256 primaryIndex,
        uint256 index2
    ) internal pure returns (uint256 spotPrice) {
        // Apply scale factors
        uint256 secondary = balances[index2] * scalingFactors[index2] / BALANCER_PRECISION;

        uint256 invariant = StableMath._calculateInvariant(
            ampParam, StableMath._balances(scaledPrimary, secondary), true // round up
        );
..SNIP..
    }

The issue is that Balancer ComposableStablePool uses a newer version of the StableMath library where the StableMath._calculateInvariant function always rounds down.
https://etherscan.io/address/0xa13a9247ea42d743238089903570127dda72fe44#code#F16#L57

    function _calculateInvariant(uint256 amplificationParameter, uint256[] memory balances)
        internal
        pure
        returns (uint256)
    {
        /**********************************************************************************************
        // invariant                                                                                 //
        // D = invariant                                                  D^(n+1)                    //
        // A = amplification coefficient      A  n^n S + D = A D n^n + -----------                   //
        // S = sum of balances                                             n^n P                     //
        // P = product of balances                                                                   //
        // n = number of tokens                                                                      //
        **********************************************************************************************/

        // Always round down, to match Vyper's arithmetic (which always truncates).

        uint256 sum = 0; // S in the Curve version
        uint256 numTokens = balances.length;
        for (uint256 i = 0; i < numTokens; i++) {
            sum = sum.add(balances[i]);
        }
        if (sum == 0) {
            return 0;
        }
        ..SNIP..

Thus, Notional round up when calculating the invariant while Balancer's ComposableStablePool round down when calculating the invariant. This inconsistency will result in a different invariant

• You can futher down the line see that BalancerSpotPrice._calculateStableMathSpotPrice() is called by BalancerSpotPrice.getComposableSpotPrices() to calculate spot prices which is called by BalancerComposableAuraVaul._checkPriceAndCalculateValue() to get balances and spot price that would be later used to calculate the LP tokens.
• BalancerComposableAuraVaul._checkPriceAndCalculateValue() is used to calculate the exchange rate, oracle prices and strategy for an underlying token in SingleSidedLPVaultBase.sol.
• reinvestReward() usesBalancerComposableAuraVaul._checkPriceAndCalculateValue() to check if the spot prices are in line with the oracle values.

Impact

The invariant is used to compute the spot price to verify if the pool has been manipulated before executing certain key vault actions (e.g. settle vault, reinvest rewards). If the inputted invariant is inaccurate, the spot price computed might not be accurate and might not match the actual spot price of the Balancer Pool. In the worst-case scenario, it might potentially fail to detect the pool has been manipulated and the trade proceeds to execute against the manipulated pool leading to a loss of assets.

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/BalancerSpotPrice.sol#L78C1-L97C6
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/math/StableMath.sol#L28C1-L85C6
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/BalancerSpotPrice.sol#L49C1-L75C10

Tool used

Manual Review, Past contest report for Notional -> sherlock-audit/2022-12-notional-judging#17

Recommendation

To avoid any discrepancy in the result, ensure that the StableMath library used by Balancer's ComposableStablePool and Notional's BalancerSpotPrice. and the implementation of the StableMath functions is the same between them.

Duplicate of #77

ginlee

medium

No validation for primaryIndex may lead to unexpected amountOut for user

Summary

Vulnerability Detail

Since executeRedemptionTrades is a public function, anyone can call with random primaryIndex

 address primaryToken = address(tokens[primaryIndex])

if primaryIndex is larger than tokens.length, instead of reverting, primaryToken will be set to 0 address， it will be passed as fourth param in _executeDynamicSlippageTradeExactIn

if (exitBalances[i] > 0) {
                (/* */, uint256 amountBought) = _executeDynamicSlippageTradeExactIn(
                    Deployments.TRADING_MODULE, t, address(tokens[i]), primaryToken, exitBalances[i]
                );

In _executeDynamicSlippageTradeExactIn, fourth param is buyToken and is used to calculate trade, which may lead to unexpected amountOut for user

 function _executeDynamicSlippageTradeExactIn(
        ITradingModule tradingModule,
        TradeParams memory params,
        address sellToken,
        address buyToken,
        uint256 amount
    ) internal returns (uint256 amountSold, uint256 amountBought)

Impact

If the transaction is executed in some way with the buyToken as a zero address, funds may be directed to an unpredictable destination, or the transaction logic may operate in an unintended manner.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L59-L87
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L119-L147

Tool used

Manual Review

Recommendation

It is necessary to ensure that primaryIndex does not exceed the bounds of the tokens array

require(primaryIndex < tokens.length, "primaryIndex out of bounds");

ke1caM

medium

Initialization can be front-run in CrossCurrencyVault.

Summary

initialize function in CrossCurrencyVault can be front-run.

Vulnerability Detail

function initialize(
        string memory name_,
        uint16 borrowCurrencyId_,
        uint16 lendCurrencyId_
    ) external initializer {
        __INIT_VAULT(name_, borrowCurrencyId_);

        LEND_CURRENCY_ID = lendCurrencyId_;
        (/* */, Token memory underlyingToken) = NOTIONAL.getCurrency(lendCurrencyId_);

        LEND_ETH = underlyingToken.tokenType == TokenType.Ether;
        IERC20 tokenAddress = IERC20(underlyingToken.tokenAddress);
        LEND_UNDERLYING_TOKEN = tokenAddress;
        LEND_DECIMALS = TokenUtils.getDecimals(address(tokenAddress));
        BORROW_DECIMALS = TokenUtils.getDecimals(address(_underlyingToken()));
    }

initialize function lacks access control which means that anyone can call it. This function is used to set the important state variables that enable correct functionality of the protocol. User could front-run admin's / owner's transaction and initialize it with different parameters.

Impact

Protocol functionality could be disrupted due to missing access control. User can front-run initialization of the smart contract.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L73-L88

Tool used

Manual Review

Recommendation

Add onlyNotionalOwner modifer to initialize function to restrict addresses other than owner from calling this function.

bin2chen

medium

reinvestReward() generates dust totalPoolClaim causing vault abnormal

Summary

If the first user deposits too small , due to the round down, it may result in 0 shares, which will result in 0 shares no matter how much is deposited later.
In National, this situation will be prevented by setting a minimum borrow size and a minimum leverage ratio.
However, reinvestReward() does not have this restriction, which may cause this problem to still exist, causing the vault to enter an abnormal state.

Vulnerability Detail

The calculation of the shares of the vault is as follows:

    function _mintVaultShares(uint256 lpTokens) internal returns (uint256 vaultShares) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
@>          vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim;
        }

        // Updates internal storage here
        state.totalPoolClaim += lpTokens;
        state.totalVaultSharesGlobal += vaultShares.toUint80();
        state.setStrategyVaultState();

If the first deposit is too small, due to the conversion to INTERNAL_TOKEN_PRECISION, the precision is lost, resulting in vaultShares=0. Subsequent depositors will enter the second calculation, but totalVaultSharesGlobal=0, so vaultShares will always be 0.

To avoid this situation, Notional has restrictions.

hey guys, just to clarify some rounding issues stuff on vault shares and the precision loss. Notional will enforce a minimum borrow size and a minimum leverage ratio on users which will essentially force their initial deposits to be in excess of any dust amount. so we should not really see any tiny deposits that result in rounding down to zero vault shares. If there was rounding down to zero, the account will likely fail their collateral check as the vault shares act as collateral and the would have none.
there is the possibility of a dust amount entering into depositFromNotional in a valid state, that would be due to an account "rolling" a position from one debt maturity to another. in this case, a small excess amount of deposit may come into the vault but the account would still be forced to be holding a sizeable position overall due to the minium borrow size.

in reinvestReward(), not this limit

    function reinvestReward(
        SingleSidedRewardTradeParams[] calldata trades,
        uint256 minPoolClaim
    ) external whenNotLocked onlyRole(REWARD_REINVESTMENT_ROLE) returns (
        address rewardToken,
        uint256 amountSold,
        uint256 poolClaimAmount
    ) {
        // Will revert if spot prices are not in line with the oracle values
        _checkPriceAndCalculateValue();

        // Require one trade per token, if we do not want to buy any tokens at a
        // given index then the amount should be set to zero. This applies to pool
        // tokens like in the ComposableStablePool.
        require(trades.length == NUM_TOKENS());
        uint256[] memory amounts;
        (rewardToken, amountSold, amounts) = _executeRewardTrades(trades);

        poolClaimAmount = _joinPoolAndStake(amounts, minPoolClaim);

        // Increase LP token amount without minting additional vault shares
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
@>      state.totalPoolClaim += poolClaimAmount;
        state.setStrategyVaultState();

        emit RewardReinvested(rewardToken, amountSold, poolClaimAmount);
    }

From the above code, we know that reinvestReward() will increase totalPoolClaim, but will not increase totalVaultSharesGlobal.

This will cause problems in the following scenarios:

1. The current vault has deposits.
2. Rewards have been generated, but reinvestReward() has not been executed.
3. The bot submitted the reinvestReward() transaction. but step 4 execute first
4. The users took away all the deposits totalPoolClaim = 0, totalVaultSharesGlobal=0.
5. At this time reinvestReward() is executed, then totalPoolClaim > 0, totalVaultSharesGlobal=0.
6. Other users' deposits will fail later

It is recommended that reinvestReward() add a judgment of totalVaultSharesGlobal>0.

Note: If there is a malicious REWARD_REINVESTMENT_ROLE, it can provoke this issue by donating reward token and triggering reinvestReward() before the first depositor appears.

Impact

reinvestReward() generates dust totalPoolClaim causing vault abnormal

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L385

Tool used

Manual Review

Recommendation

    function reinvestReward(
        SingleSidedRewardTradeParams[] calldata trades,
        uint256 minPoolClaim
    ) external whenNotLocked onlyRole(REWARD_REINVESTMENT_ROLE) returns (
        address rewardToken,
        uint256 amountSold,
        uint256 poolClaimAmount
    ) {
        // Will revert if spot prices are not in line with the oracle values
        _checkPriceAndCalculateValue();

        // Require one trade per token, if we do not want to buy any tokens at a
        // given index then the amount should be set to zero. This applies to pool
        // tokens like in the ComposableStablePool.
        require(trades.length == NUM_TOKENS());
        uint256[] memory amounts;
        (rewardToken, amountSold, amounts) = _executeRewardTrades(trades);

        poolClaimAmount = _joinPoolAndStake(amounts, minPoolClaim);

        // Increase LP token amount without minting additional vault shares
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
+       require(state.totalVaultSharesGlobal > 0 ,"invalid shares");
        state.totalPoolClaim += poolClaimAmount;
        state.setStrategyVaultState();

        emit RewardReinvested(rewardToken, amountSold, poolClaimAmount);
    }

0xpep7

medium

Potential reversion in the Curve2TokenConvexVault.emergencyExit function when the Curve pool is killed, leading to vault locking failure

Summary

If the underlying Curve pool's self.is_killed becomes true, the emergencyExit() function in Curve2TokenConvexVault may fail to execute, preventing the intended withdrawal and locking of the vault. This vulnerability could result in the loss of funds and compromise the emergency protection mechanism of the vault.

Vulnerability Detail

The Curve2TokenConvexVault.emergencyExit() function is used to trigger an emergency exit on the vault which involves withdrawing the LP tokens in a Curve pool by calling _unstakeAndExitPool with isSingleSided = true. The _unstakeAndExitPool then proceeds to remove the LP tokens from the Curve pool via remove_liquidity_one_coin.

// File: leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol
480:    function emergencyExit(
        ...
489:        _unstakeAndExitPool(claimToExit, new uint256[](NUM_TOKENS()), true); // <= FOUND: always reverts if the Curve pool is killed
        ...
494:        emit EmergencyExit(claimToExit);
495:        _lockVault(); // <= FOUND: unable to lock the vault if _unstakeAndExitPool reverts
496:    }

// File: leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol
66:    function _unstakeAndExitPool(
        ...
80:        if (isSingleSided) {
81:            // Redeem single-sided
82:            exitBalances[_PRIMARY_INDEX] = pool.remove_liquidity_one_coin( // <= FOUND
83:                poolClaim, int8(_PRIMARY_INDEX), _minAmounts[_PRIMARY_INDEX]
84:            );
            ...
95:    }

The root cause of the vulnerability is that the emergencyExit() function relies on the remove_liquidity_one_coin function in the underlying Curve pool contract. If the Curve pool is killed (self.is_killed is true), calling remove_liquidity_one_coin always reverts at the assertion below, leading to the failure of the emergency exit process.

Curve Pool Contract:

670: def remove_liquidity_one_coin(_token_amount: uint256, i: int128, min_amount: uint256):
671:     """
672:     Remove _amount of liquidity all in a form of coin i
673:     """
674:     assert not self.is_killed  # dev: is killed

Impact

There are two impacts that should be discussed in this case:

1. Funds remain locked in the Curve pool until the BaseStrategyVault.redeemFromNotional function is called by Notional to redeem tokens proportionally via remove_liquidity instead. However, this still defeats the core functionality of the emergencyExit function.
2. The _lockVault can never be called due to reversion of _unstakeAndExitPool which leads to the associated protection mechanisms, such as locking the vault, cannot be executed. In the event of an emergency, where it is crucial to locking the vault, the inability to call _lockVault() effectively leaves the vault vulnerable to unknown potential exploits. This should be a Med severity based on Sherlock docs's Section V.2

Code Snippet

https://github.com/sherlock-audit/2023-10-notional-0xpep/tree/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L489

Tool used

Manual Review

Recommendation

It is recommended to enhance the emergencyExit() function to handle the scenario where the Curve pool is killed. The function should accept an additional parameter, isKilled, and adjust the call to _unstakeAndExitPool accordingly, ensuring that emergency exits can still be performed even if the underlying Curve pool is in a killed state.

Note: Similar report was previously accepted as Med severity.

Duplicate of #96

wangxx2026

medium

Loss of judgment on withdrawal asset length and slippage length leads to redemption failure

Summary

Loss of judgment on withdrawal asset length and slippage length leads to redemption failure

Vulnerability Detail

In the redemption of BalancerComposableAuraVault and BalancerWeightedAuraVault, the parameter transfer path for slippage judgment is:

sequenceDiagram
    redeemFromNotional->>_redeemFromNotional: data
    _redeemFromNotional->>_unstakeAndExitPool: data.decode
    _unstakeAndExitPool->>_exitPoolExactBPTIn: minAmounts
    _exitPoolExactBPTIn->>BALANCER_VAULT: exitPool.amounts

In this process, it is never judged whether the length of minAmountsde is the same as the length of assets. In the BALANCER_VAULT code and the document, it is clearly required that the length of the slippage array needs to be the same as the length of the redeemed assets.

When depositing assets, there is a clear judgment. This may be due to the negligence of the developer.

Impact

Loss of judgment on withdrawal asset length and slippage length leads to redemption failure

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/balancer/mixins/BalancerPoolMixin.sol#L188-L198

Tool used

Manual Review

Recommendation

Add

require(assets.length == amounts.length);

judgment before requesting, and ensure the order is consistent

ginlee

medium

Can't use block.timestamp as deadline since it can be manipulated

Summary

Normally in trading, users will specify a deadline parameter that enforces a time limit by which the transaction must be executed. Without a deadline parameter, the transaction may sit in the mempool and be executed at a much later time potentially resulting in a worse price for the user. Protocols shouldn't set the deadline to block.timestamp as a validator can hold the transaction and the block it is eventually put into will be block.timestamp, so this offers no protection.

Vulnerability Detail

 Trade memory trade = Trade(
            params.tradeType,
            sellToken,
            buyToken,
            amount,
            0, // No absolute slippage limit is set here
            block.timestamp, // deadline       
            params.exchangeData
        );

In Trade, block.timestamp is passed as deadline for this transaction, may result in a worse price for the user

Impact

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L140
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L170
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L177
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L248

Tool used

Manual Review

Recommendation

Protocols should allow users to set expiration deadlines by themselves

Duplicate of #115

djanerch

medium

Gas limit DoS via unbounded operations

Summary

If a function requires more gas than the block gas limit to complete its execution, it will inevitably fail. These vulnerabilities typically occur in loops that iterate over dynamic data structures.

Vulnerability Detail

Certain functions in contracts take arrays as input and iterate over them without checking their sizes. This oversight can lead to reaching the block gas limit and resulting in a reverted transaction.

Impact

Functions vulnerable to gas limits can become uncallable, potentially locking funds or freezing the contract state.

Code Snippet

• StrategyUtils.sol 27-55

• StrategyUtils.sol 59-87

Tool Used

Manual Review

Recommendation

To ensure that functions like these are bounded and prevent array exhaustion, include proper input validation mechanisms in your smart contract. Follow these general guidelines:

1. Check Array Length:

   • Before iterating over arrays, verify that the length of the array is within reasonable bounds to prevent exhaustion. Utilize the require statement for this purpose.

   function executeDepositTrades(uint256[] memory amounts) external {
       require(amounts.length <= MAX_ARRAY_LENGTH, "Array length exceeds maximum");
       // rest of the function
   }

   Define MAX_ARRAY_LENGTH as a constant with an appropriate value.

2. Limit Iteration:

   • Use a for loop to iterate over the array elements, ensuring that the loop index is incremented properly within the loop body. Avoid using unbounded loops relying on external conditions.

   function executeRedemptionTrades(uint256[] memory exitBalances) external {
       require(exitBalances.length <= MAX_ARRAY_LENGTH, "Inner array length exceeds maximum");
       // rest of the loop body
   }

   Ensure that inner arrays are also bounded.

3. Gas Limit Consideration:

   • Recognize that large arrays or nested loops can consume a significant amount of gas, and there's a gas limit for each Ethereum block. If the array size or computation is too large, the function might fail to execute. Consider breaking down the task into smaller transactions if necessary.

Always tailor these validations to your specific use case and the constraints of your smart contract. Adjust the MAX_ARRAY_LENGTH and other parameters based on your system's requirements and limitations.

Duplicate of #32

shealtielanz

medium

_mintVaultShares() in SingleSidedLPVaultBase.sol can revert unexpectedly, causing DOS to deposits from notional.

Summary

the call to _mintVaultShares() can revert for a certain vault share greater than type(uint80).max on the call to .toUint80 due to overflow.

Vulnerability Detail

The SingleSidedLPVaultBase contract during the execution of _depositFromNotional() function calls the internal function _mintVaultShares().
Inside this function could be seen casting uint256 typed local variables to uint80 type:
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L229C1-L240C64

    function _mintVaultShares(uint256 lpTokens) internal returns (uint256 vaultShares) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            //@audit the percison used to divide is above the normal percios and would lead to low amounts
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim; //@audit in a situation we have a low enough ppoltoclaim the vault shares can go above uint80.
        }

        // Updates internal storage here
        state.totalPoolClaim += lpTokens;
        state.totalVaultSharesGlobal += vaultShares.toUint80();
        state.setStrategyVaultState();
..SNIP..

This casting could be considered safe only based on the assumption that value of vaultShares returned would always be less than uint80.maxValue.
However, we could see that this assumption would be wrong in case of low enough totalPoolClaim amount in the pool.

        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            //@audit the percison used to divide is above the normal percios and would lead to low amounts
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim; //@audit in a situation we have a low enough ppoltoclaim the vault shares can go above uint80.
        }

With a small state.totalPoolClaim where state.totalPoolClaim != 0 , big lpTokens and state.totalVaultSharesGlobal values, the calculation could result in a vaultShares that is greater than uint80.maxValue.
This will lead to an overflow on the line state.totalVaultSharesGlobal += vaultShares.toUint80(); causing a revert, resulting to DOS on the call to deposit from notional.

Impact

This would brick deposits from notional on the call to _depositFromNotional()

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L229C1-L240C64
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L195C1-L223C43
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L240

Tool used

Manual Review

Recommendation

Consider changing the state.totalVaultSharesGlobal field type from uint80 to uint256.

dany.armstrong90

high

CrossCurrencyVault.sol#_redeemfCash: IWrappedfCash#redeem function call will revert.

Summary

Since the parameter types and numbers of IWrappedfCash#redeem function call in CrossCurrencyVault.sol#_redeemfCash does not coincide with function declaration, function calls will revert.

Vulnerability Detail

CrossCurrencyVault.sol#_redeemfCash function is as follows.

File: CrossCurrencyVault.sol
278:     function _redeemfCash(bool isETH, uint256 maturity, uint256 vaultShares) private {
279:         IWrappedfCash wfCash = getWrappedFCashAddress(maturity);
280:         uint256 assets = wfCash.redeem(vaultShares, address(this), address(this));
281: 
282:         if (isETH) WETH.withdraw(assets);
283:     }

On the other hand, IWrappedfCash.sol#redeem function declaration is as follows.

File: IWrappedfCash.sol
34:     function redeem(uint256 amount, RedeemOpts memory data) external;
35:     function redeemToAsset(uint256 amount, address receiver, uint32 maxImpliedRate) external;
36:     function redeemToUnderlying(uint256 amount, address receiver, uint32 maxImpliedRate) external;

Since the parameter types and numbers of IWrappedfCash#redeem function call does not coincide with function declaration, function calls will revert.

Impact

When maturity < Constants.PRIME_CASH_VAULT_MATURITY, redeemFromNotional and convertVaultSharesToPrimeMaturity function call to CrossCurrencyVault will revert at any time.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L280

Tool used

Manual Review

Recommendation

Ensures that the parameter types and numbers of IWrappedfCash#redeem function call coincides with function declaration.

Vagner

medium

getOraclePrice in SingleSidedLPVaultBase.sol does not check if the sequencer is down for Arbitrum/Optimism

Summary

Chainlink recommends that all Optimistic L2 oracles consult the Sequencer Uptime Feed to ensure that the sequencer is live before trusting the data returned by the oracle, but this is not done in the getOraclePrice used in the SingleSidedLPVaultBase.sol by calling TRADING_MODULE contract.

Vulnerability Detail

As you can see _getOraclePairPrice calls , getOraclePrice on the TRADING_MODULE
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L323
which checks for the values that the chainlink oracle provides for those 2 assets passed as arguments,
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L237-L261
but nowhere it is checked if the L2 sequencer is down, in the case of Arbitrum and Optimism, as recommended by Chainlink https://docs.chain.link/data-feeds/l2-sequencer-feeds.
This could lead to stale prices and wrong values, which would hurt the protocol.

Impact

Medium, since the prices returned could be stale if the sequencer is down.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L237-L261

Tool used

Manual Review

Recommendation

Check if the sequencer is down for Arbitrum and Optimism, as per Chainlink recommendations.

Duplicate of #2

AuditorPraise

high

missing payable keyword on TradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage(), will cause VAULTS to be unable to execute trades on external exchanges via the trading module whenever ETH is the sell Token

Summary

The vaults will be executing trades on external exchanges via TradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage() and ETH could be among the tokens to trade for primary token BUT the tradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage() lack the payable keyword.

Vulnerability Detail

tradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage() won't be able to receive ETH (Whenever ETH is sell token) because they lack the payable keyword.

This can cause reverts in some of the key functions of the vaults like:

• depositFromNotional()
• redeemFromNotional()
• reinvestReward()

Impact

vaults will be unable to execute trades on external exchanges via the trading module whenever ETH is the sell Token

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L127

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L169

Tool used

Manual Review

Recommendation

Add the payable keyword to TradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage()

Duplicate of #51

mstpr-brainbot

high

Yield can be sandwiched

Summary

During a reinvestment, the total claims of the pool increase, but the shares remain unaffected. Consequently, users who hold vault shares experience an increase in LP token claims on their shares. However, the reinvestment call is susceptible to being sandwiched. If a user deposits just before the reinvestment and exits immediately afterward, that user pockets the profit without being an actual vault shareholder. This action disrupts the yield of other vault shareholders.

Vulnerability Detail

Let's consider a scenario where the total vault shares amount to 100 tokens, and there are 100 LP tokens in the vault. When the reinvestment manager triggers a reinvestment, buying an additional 10 LP tokens, Alice, an MEV expert, notices this transaction in the mempool. She swiftly enters the vault just before the reinvestment occurs, mints tokens at a 1:1 ratio, and immediately after the reinvestment function, Alice exits the vault, claiming the yield for herself. This maneuver adversely affects other vault holders, resulting in them receiving lower yields, while Alice gains an unfair advantage by being in the vault for only one block.

Moreover, Alice can scale up this attack significantly. By depositing a large number of tokens via flash loans, she can acquire a substantial share from the vault. As she holds the majority of the shares, Alice receives the bulk of the yield. Consequently, other vault shareholders receive minimal yield, significantly diminishing their returns.

Impact

High since it can block the yield

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L385-L411

Tool used

Manual Review

Recommendation

Bauer

high

Funds Loss Risk in Exchange Rate Calculation

Summary

The getExchangeRate() function poses a risk of funds loss within the protocol, as the current price of a vault share may be overestimated, leading to potential financial discrepancies during exchanges.

Vulnerability Detail

The function Curve2TokenConvexVault._checkPriceAndCalculateValue() fetches balances from CURVE_POOL using CURVE_POOL.balances(0) and CURVE_POOL.balances(1), then uses get_dy to obtain the price of one unit of the primary token converted to the secondary token.

    function _checkPriceAndCalculateValue() internal view override returns (uint256 oneLPValueInPrimary) {
        uint256[] memory balances = new uint256[](2);
        balances[0] = ICurvePool(CURVE_POOL).balances(0);
        balances[1] = ICurvePool(CURVE_POOL).balances(1);

        // The primary index spot price is left as zero.
        uint256[] memory spotPrices = new uint256[](2);
        uint256 primaryPrecision = 10 ** PRIMARY_DECIMALS;
        uint256 secondaryPrecision = 10 ** SECONDARY_DECIMALS;

        // `get_dy` returns the price of one unit of the primary token
        // converted to the secondary token. The spot price is in secondary
        // precision and then we convert it to POOL_PRECISION.
        spotPrices[SECONDARY_INDEX] = ICurvePool(CURVE_POOL).get_dy(
            int8(_PRIMARY_INDEX), int8(SECONDARY_INDEX), primaryPrecision
        ) * POOL_PRECISION() / secondaryPrecision;

        return _calculateLPTokenValue(balances, spotPrices);
    }

Finally, the function invokes an internal function _calculateLPTokenValue() to determine the value of one LP token based on the obtained balances and spot prices from the Curve pool. The oneLPValueInPrimary is calculated as follows.

  uint256 tokenClaim = balances[i] * POOL_PRECISION() / totalSupply;
            if (i == PRIMARY_INDEX()) {
                oneLPValueInPrimary += tokenClaim;
            } else {
......
 oneLPValueInPrimary += (tokenClaim * POOL_PRECISION() * primaryDecimals) / 
                    (price * secondaryDecimals);
}

However, the issue arises from the fact that CurvePool(CURVE_POOL).balances() can be manipulated by malicious actors.

Bad actors could potentially deposit funds into CURVE_POOL in advance, artificially inflating the balances. As a result, when calculating the LP token value, the protocol would derive a value greater than expected due to the manipulated balances.

Therefore, in the getExchangeRate() function, the current price of a vault share will be greater than expected, as indicated in the following code snippet.

function getExchangeRate(uint256 /* maturity */) external view override returns (int256) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        uint256 oneLPValueInPrimary = _checkPriceAndCalculateValue();
        // If inside an emergency exit, just report the one LP value in primary since the total
        // pool claim will be 0
        if (state.totalVaultSharesGlobal == 0 || isLocked()) {
            return oneLPValueInPrimary.toInt();
        } else {
            uint256 lpTokensPerVaultShare = (uint256(Constants.INTERNAL_TOKEN_PRECISION) * state.totalPoolClaim)
                / state.totalVaultSharesGlobal;
            return (oneLPValueInPrimary * lpTokensPerVaultShare / POOL_PRECISION()).toInt();
        }
    }

Furthermore, in the reinvestReward() function, the invocation of _checkPriceAndCalculateValue() may lead to a revert, preventing the successful execution of the reinvestment process, possibly due to the the same issue.

Impact

Resulting in a loss of funds within the protocol during exchanges.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol#L99-L100

Tool used

Manual Review

Recommendation

shealtielanz

high

TradingUtils._executeTrade() will leak ETH to WETH

Summary

If sellToken is ETH, and using Uniswap for the dex, and it is exact out trade, too much is deposited to the WETH and does not withdraw the excess amount. It will give wrong amountSold value as well as accounting error.

Vulnerability Detail

trade.sellToken is ETH and using Uniswap as dex, WETH should be used instead of ETH as Uniswap does not support ETH. There for TradingUtils wraps the ETH to WETH before trading.

If the trade would be exact out, the amount trade.limit will be deposited to WETH instead of the trade.amount. However, because it is exact out, not all ETH deposited will be traded. In the current implementation, there is no logic to recover the excess deposit.

As the TradingUtils::_executeInternal, which uses the TradingUtils::_executeTrade will calculate the amountSold based on the balance of ETH, it will return the trade.limit as the amountSold, thus resulting in accounting error.

Impact

amountSold will reflect not the amount really sold, rather the trade.limit. It is unclear whether the excess amount of ETH, which is deposited for WETH can be recovered.

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol#L143C1-L151C10
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol#L28C1-L63C6

Tool used

Manual Review, exactly same issue was found in the notional past audit but the fix isn't implemented in this current codebase,
link here

Recommendation

In the _executeTrade, if the sellToken is ETH and it is exact out trade, recover excess deposit

Bauer

high

Potential Stranded Rewards in Aura Pool for Newly Added Tokens

Summary

The introduction of new rewards to the Aura pool may result in the tokens becoming stranded within the protocol, rendering them inaccessible for withdrawal.

Vulnerability Detail

If a new reward token is added to the Aura pool, calling getReward() will introduce the new reward token into the protocol. However, when the REWARD_REINVESTMENT_ROLE executes reinvestReward(), there is a check in _isInvalidRewardToken() that verifies the token against the ones specified during initialization.

function _isInvalidRewardToken(address token) internal override view returns (bool) {
        return (
            token == TOKEN_1 ||
            token == TOKEN_2 ||
            token == TOKEN_3 ||
            token == TOKEN_4 ||
            token == TOKEN_5 ||
            token == address(AURA_BOOSTER) ||
            token == address(AURA_REWARD_POOL) ||
            token == address(Deployments.WETH)
        );
    }

Since the newly added reward token is not part of the initial specification, the validation fails. As a result, the token cannot be exchanged for pool tokens and cannot be withdrawn, remaining indefinitely within the protocol.

Impact

The introduction of new rewards to the Aura pool may result in the tokens becoming stranded within the protocol。

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L385-L411

Tool used

Manual Review

Recommendation

AuditorPraise

medium

The convertVaultSharesToPrimeMaturity() function lacks the whenNotLocked() modifier

Summary

All key external functions in the vault are supposed to have the whenNotLocked() modifier, in case of an emergency but convertVaultSharesToPrimeMaturity() doesn't

Vulnerability Detail

From the convertVaultSharesToPrimeMaturity() function an account can withdraw settled fCash to underlying from the fCash wrapper and deposit back into Notional as Prime Cash from Notional contract.

The issue is that this feature should also be restricted in the case of an emergency.

When there is an emergency the vault has to be fully locked, i.e no funds entering and no funds leaving the vaults.

Moreover the SingleSidedLPVaultBase.emergencyExit() will lock the vault so that no entries, exits or valuations of vaultShares can be performed, but since the convertVaultSharesToPrimeMaturity() function lacks the whenNotLocked() modifier, there will still be exits and valuations of vaultShares in the locked vault.

Impact

An account can withdraw settled fCash to underlying from the fCash wrapper and deposit back into Notional as Prime Cash from Notional contract even when there's an emergency that requires the vaults to be fully locked.

The vault is locked so that no entries, exits or valuations of vaultShares can be performed but the intention for which the vaults were locked can be ruined via convertVaultSharesToPrimeMaturity() function as it lacks the whenNotLocked() modifier.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L208

Tool used

Manual Review

Recommendation

Add the whenNotLocked() modifier to the convertVaultSharesToPrimeMaturity() function

mstpr-brainbot

medium

Restoring the vault can result in big losses if balances are changed

Summary

When a vault is in a paused state, it indicates an emergency situation where an emergency withdrawal has been executed. This withdrawal removes the balanced liquidity from all coins of the underlying tokens. Restoring the vault involves adding liquidity using the current balances of the vault, assuming that whatever tokens were withdrawn from the pool during the emergency exit will be balanced when deposited back into the pool during restoration. However, changes in pool balances can occur when the vault decides to restore, potentially resulting in the vault receiving fewer LP tokens based on the current balances.

Vulnerability Detail

Let's consider an example where the underlying Curve pool utilized by the vault contains a total of 100 tokens, with 20 tokens being X and the remaining 80 tokens being Y within the pool. For simplicity, let's assume that half of the LP tokens are held by the vault.

At a certain point, an emergency withdraw is initiated by the emergency role holder, causing the vault to remove its balanced liquidity. Given that the vault possesses half of the liquidity, the withdrawn amounts are 10 X and 40 Y.

After some time, the emergency situation is resolved, and the administrator intends to restore the vault. However, the underlying pool composition has changed, now consisting of 80% X and 20% Y, while maintaining the same total token sum. In this scenario, restoring the vault solely with the existing balances might not be the most prudent choice. This assumption is made based on the possibility of external markets where the vault's underlying tokens can be exchanged and added to the pool more efficiently, aiming to maximize the LP.

Aside from the above example, it is also possible for an attacker to manipulate the pool balances to give Notional a lesser LP share hence lesser value on the strategy token.

Impact

The above scenarios are considering the total sum of tokens are not changed but only the ratios did. It is also possible that the sum of tokens has changed which would lead to Notional to get higher or lower LP token amounts. However, this is not something that Notional admin can control so that's why I didn't include that part in above. However, considering sums are same or very near to what it was when emergency withdraw is called, just because the lack of selling optionality vault will have less LP tokens and users will have less LP token claims. That's why I interpreted this as medium.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L480-L525

Tool used

Manual Review

Recommendation

Allow the notional owner to sell some tokens to external markets just as it allows in deposits. Notional admin is trusted and also executeDepositTrades are well protected so it does make sense to allow restoreVault sell tokens when adding LP back to pool.

Duplicate of #82

mstpr-brainbot

medium

Reward token can be an underlying token and it would not be possible to reinvest rewards

Summary

Some pools can have reward tokens as same as one of the underlying token. In this case Notional reward investor role will not be able to compound the rewards properly and the rewards will be stuck in the contract.

Vulnerability Detail

As Notional considers all pools containing one of the underlying assets usable, certain pools may have reward tokens identical to one of the underlying tokens. However, compounding these rewards might not be feasible due to the checks in the executeRewardTrades function.

Consider the scenario where Notional selects the rETH-wstETH single-sided vault for its ETH, which is a strong candidate. Yet, additional rewards from the Curve gauge might also be in wstETH. Lido has contemplated rewarding some pools similarly, such as the stETH-ETH pool, where they provide wstETH instead of LDO. If such a situation arises, wherein wstETH is both an underlying asset and a reward source from the Convex gauge, rewardInvestors won't be able to claim and reinvest those wstETH rewards due to the following check:
if (_isInvalidRewardToken(rewardToken)) revert Errors.InvalidRewardToken(rewardToken);
and this is the invalidRewardToken

function _isInvalidRewardToken(address token) internal override view returns (bool) {
        return (
            token == TOKEN_1 ||
            token == TOKEN_2 ||
            token == address(CURVE_POOL_TOKEN) ||
            token == address(CONVEX_REWARD_POOL) ||
            token == address(CONVEX_BOOSTER) ||
            token == Deployments.ALT_ETH_ADDRESS
        );
    }

This function validates that selling rewards, which are also underlying tokens, isn't possible.

Impact

1-Reward tokens can be underlying tokens and notional has no control over it
2- Since all curve pools can be used, any pool that has crv-cvx-bal pairs also be used and they are reward tokens

Considering above points, I think this is a valid medium

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L385-L427
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L422
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/curve/ConvexStakingMixin.sol#L60-L69

Tool used

Manual Review

Recommendation

Make a flag that tells the contract reward token is also an underlying token of the pool. Vault contract should never have idle tokens as long as it is not paused and when its paused reinvesting rewards are not possible. Hence, I think it is safe to say that allowing reward investors to sell reward/underlying tokens in general

Duplicate of #84

eeshenggoh

medium

No Protection of Uninitialized Implementation Contracts From Attacker

Summary

The inclusion of the _disableInitializers function in the abstract contract serves as a proactive security measure, providing a foundation for controlled initialization behavior in the contract's inheritance hierarchy. Although the function may not be currently employed, its presence establishes a robust design pattern, preparing the contract for potential future scenarios where preventing further initialization becomes essential.

Vulnerability Detail

The vulnerability mitigated by using _disableInitializers relates to the possibility of an attacker exploiting reinitialization, potentially compromising the contract's intended behavior. However all contracts which implements AccessControlUpgradeable & Initializable do not call _disableInitializers in the constructors Without the proper use of _disableInitializers, the contract might be susceptible to unexpected reinitialization, leading to unanticipated consequences and potential security risks.
To further prove my point in the contracts/proxy/utils/Initializable.sol it states the following:

 * Avoid leaving a contract uninitialized.
 * An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation
 * contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke
 * the {_disableInitializers} function in the constructor to automatically lock it when it is deployed:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6bc1173c8e37ca7de2201a0230bb08e395074da1/contracts/proxy/utils/Initializable.sol#L41C1-L46C3

Impact

Failure to use _disableInitializers could allow an attacker to exploit the contract's initialization process, leading to unexpected behavior, security vulnerabilities, or even a complete compromise of the contract's functionality. This vulnerability could impact the security and reliability of the contract, potentially putting user funds or sensitive operations at risk.

Code Snippet

  **All Vaults Affected**
  * leveraged-vaults/contracts/vaults/*

Tool used

Manual Review

Recommendation

For contracts that inherit BaseStrategyVault, do include the following

abstract contract BaseStrategyVault is Initializable, IStrategyVault, AccessCont
     /// @notice Set the NOTIONAL address on deployment
     constructor(NotionalProxy notional_, ITradingModule tradingModule_) initializer {
         // Make sure we are using the correct Deployments lib
+        //@audit-issue Prevent further re-initialization of the implementation contract
+        _disableInitializers();
         uint256 chainId = 42161;
         //assembly { chainId := chainid() }
         require(Deployments.CHAIN_ID == chainId);

For contracts that do not inherit BaseStrategyVault do include _disableInitializers() in constructor

shealtielanz

medium

Unexpected behavior for UniV3Adapter, and ZeroExAdapter when msgValue is not zero

Summary

Unexpected behavior for UniV3Adapter, and ZeroExAdapter when msgValue is not zero.

Vulnerability Detail

UniV3Adapter, and ZeroExAdapter are assuming msgValue is zero. But the caller logic is shared between BalancerV2Adapter and CurveAdapter which support positive msgValue. So, there is a chance that UniV3Adapter, and ZeroExAdapter will be given positive msgValue. Since they don't handled positive msgValue cases, it will return an unexpected behavior.

Impact

If msgValue is not zero, UniV3Adapter, and ZeroExAdapter may execute unexpected behavior, also some trades will be impossible in Notional

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/adapters/UniV3Adapter.sol#L89C1-L112C2
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/adapters/ZeroExAdapter.sol#L12C1-L25C6

Tool used

Manual Review

Recommendation

Unexpected behavior for UniV3Adapter, and ZeroExAdapter when msgValue is not zero

 function getExecutionData(address from, Trade calldata trade) 
     internal view returns ( 
         address spender, 
         address target, 
         uint256 msgValue, 
         bytes memory executionCallData 
     ) 
 {
   ...
   require(msgValue == 0, "Positive msgValue not allowed");
   ...
 }

Vagner

high

BalancerWeightedAuraVault.sol wrongly assumes that all of the weighted pools uses totalSupply

Summary

BalancerWeightedAuraVault.sol which is used specifically for weighted balancer pools wrongly uses totalSupply all the time to get the total supply of the pool, but this would not be true for newer weighted pools.

Vulnerability Detail

Balancer pools have different methods to get their total supply of minted LP tokens, which is also specified in the docs here
https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#getting-bpt-supply .
The docs specifies the fact that totalSupply is only used for older stable and weighted pools, and should not be used without checking it first, since the newer pools have pre-minted BPT and getActualSupply should be used in that case. Most of the time, the assumption would be that only the new composable stable pools uses the getActualSupply, but that is not the case, since even the newer weighted pools have and uses the getActualSupply. To give you few examples of newer weighted pools that uses getActualSupply
https://etherscan.io/address/0x9f9d900462492d4c21e9523ca95a7cd86142f298
https://etherscan.io/address/0x3ff3a210e57cfe679d9ad1e9ba6453a716c56a2e
https://etherscan.io/address/0xcf7b51ce5755513d4be016b0e28d6edeffa1d52a
the last one also being on Aura finance. Because of that, the interaction with newer weighted pools and also the future weighted pools would not be accurate since the wrong total supply is used and calculations like _mintVaultShares and _calculateLPTokenValue would both be inaccurate, which would hurt the protocol and the users.

Impact

Impact is a high one since the calculation of shares and the value of the LP tokens is very important for the protocol, and any miscalculations could hurt the protocol and the users a lot.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerWeightedAuraVault.sol#L1-L94

Tool used

Manual Review

Recommendation

Since the protocol would want to interact with multiple weighted pools, try to check first if the weighted pool you are interacting with is a newer one and uses the getActualSupply or if it is an older one and uses totalSupply, in that way the protocol could interact with multiple pools in the long run.

Vagner

medium

Most of the composable stable pools uses wstETH on the mainnet, which is would not work with the current codebase because of _getOraclePairPrice

Summary

Most of the composable stable pool on Balancer uses wstETH as one of the tokens, but that would not be compatible with BalancerComposableAuraVault.sol because there is no mainnet oracle compatible with wstETH, so the function _getOraclePairPrice used in _calculateLPTokenValue would not be possible to be used.

Vulnerability Detail

As you can see on the balancer composable stable pools page for mainnet
https://app.balancer.fi/#/ethereum
most of the pools, and especially the ones with the biggest values, uses wstETH as one of their assets. The problem with the current codebase is that wstETH would not be compatible with the current _getOraclePairPrice function. As you can see _checkPriceAndCalculateValue calls _calculateLPTokenValue
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L104
which calls _getOraclePairPrice
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L351
if the assets provided is not the primary index one. Now if we look at the _getOraclePairPrice function, it calls getOraclePrice on the TRADING_MODULE contract
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L323
which uses the chainlink oracle to get the latest round data for those 2 assets provided in the _getOraclePairPrice function
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L237-L261
but since wstETH is not supported on any mainnet oracle, the whole function could not be used. To be more specifically, there will be issues when anytime when _checkPriceAndCalculateValue, which is on reinvestReward , getExchangeRate
and convertStrategyToUnderlying functions in SingleSidedLPVaultBase.sol.

Impact

Impact is a medium one since reinvesting and other functionalities would not be possible, in those cases.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L322-L327

Tool used

Manual Review

Recommendation

Use another way of checking the price of wstETH, since avoiding interacting with it would make using most of the composable pool impossible.

squeaky_cactus

medium

chainId of Arbitum breaks Mainnet and Optimism deployments

Summary

The constructor of BaseStrategyVault requires the chain id matches that from Deployments.CHAIN_ID (the value assigned is 42161, the Arbitum chain id).

Although Deployments.sol is out of scope, in scope Deployments.CHAIN_ID is used to control program flow, deciding which chain specific functions are invoked on external contracts. The outcome always being the same, irrespective of the chain deployed onto, only the Arbitum path is ever used.

Vulnerability Detail

The constructor of BaseStrategyVault contains the setting of chainId to 42161 (Arbitum chain).

    constructor(NotionalProxy notional_, ITradingModule tradingModule_) initializer {
        // Make sure we are using the correct Deployments lib
        uint256 chainId = 42161;
        //assembly { chainId := chainid() }
        require(Deployments.CHAIN_ID == chainId);

        NOTIONAL = notional_;
        TRADING_MODULE = tradingModule_;
    }

Throughout the code the constant Deployments.CHAIN_ID is used, rather than retrieving the chain id, resulting in the
flow control gates always choosing the Arbitum path.

An example is Curve2TokenConvexVault._joinPoolAndStake, where Deployments.CHAIN_ID will always be Arbitum (as its enforced in the constructor), resultingly the Mainnet path will never be used, even on Mainnet.

        if (Deployments.CHAIN_ID == Constants.CHAIN_ID_MAINNET) {
            success = IConvexBooster(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens, true);
        } else if (Deployments.CHAIN_ID == Constants.CHAIN_ID_ARBITRUM) {
            success = IConvexBoosterArbitrum(CONVEX_BOOSTER).deposit(CONVEX_POOL_ID, lpTokens);
        }

Given the commented out inline Yul { chainId := chainid() }, this might have been a work-around for dev/testing,
but this being the code under audit, I'd feel remissed in omitting it.

Impact

The Notional Update 4 ReadMe states the deployment chains that need supporting:

Q: On what chains are the smart contracts going to be deployed?

Arbitrum, Mainnet, Optimism

Requirements on any mismatch between Deployments.sol and chain id from Discord channel notional-update-4-nov-18:

Jeff Wu | Notional:

Getting a few questions about the constructor in the BaseStrategyVault and the check against chainid.
There are some constants in Deployments.sol which are chain specific.
The constructor should revert if the contract is being deployed to a mismatched chain from the Deployments.sol file it is compiled with.

The constructor does not revert when there is a mismatched chain from the Deployments.sol, failing the requirement.

The control flow effected when deployed on:

• Mainnet: an unusable Curve2TokenConvexVault would be deployed, with the stake, unstake and claim rewards
  functions that will all revert.
• Optimism: Curve2TokenConvexVault can be deployed (against the intention in the code against unsupported chains), with
  reverting stake, claim and reward functions.

Deployments.sol holds chain specific addresses (although some addresses are common across chains),
Mainnet and Optimism deployments would be using the Arbitum address, some would be incorrect, resulting
in failing calls to routers and wrapped staked ether.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L58-L61

Tool used

Manual Review

Recommendation

In the constructor of BaseStrategyVault check the id of the chain being deployed on against the Deployments.sol being used.

-        uint256 chainId = 42161;
-        //assembly { chainId := chainid() }
-        require(Deployments.CHAIN_ID == chainId);
+        require(Deployments.CHAIN_ID == block.chainId);

Duplicate of #73

shealtielanz

medium

setting deadline to block.timestamp can be very problematic when trades are in the mempool.

Summary

in StrategyUtils.sol during execution of dynamic and static trades it sets the deadline to block.timestamp automatically, this is bad as setting to block.stamp allows a miner/bundler to hold a trade in the mempool and execute it at anytime of their wanting which can be used to cause grief traders due to their trades being executed at a time they do not want which can lead them to accrue losses. this can also be used to sandwich trades to make gain.

Vulnerability Detail

you can see in StrategyUtils.sol _executeDynamicSlippageTradeExactIn() and _executeTradeWithStaticSlippage() the deadlin for the trades are set to block.timestamp.

   function _executeDynamicSlippageTradeExactIn(
..SNIP..

        Trade memory trade = Trade(
            params.tradeType,
            sellToken,
            buyToken,
            amount,
            0, // No absolute slippage limit is set here
            block.timestamp, // deadline
            params.exchangeData
        );
..SNIP..
    function _executeTradeWithStaticSlippage(
..SNIP..

        Trade memory trade = Trade(
            params.tradeType,
            sellToken,
            buyToken,
            amount,
            params.oracleSlippagePercentOrLimit,
            block.timestamp, // deadline
            params.exchangeData
        );
..SNIP..

Impact

This allows trades to be vulnerable to MEV attacks and trades can settle in unfavourable times that will lead to loses to Traders.

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L134C1-L142C11
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L164C1-L172C11
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L248
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L177

Tool used

Manual Review

Recommendation

allow callers specify their custom deadlines

Duplicate of #115

coffiasd

medium

Checking for an invalid reward token could potentially lead to a executeRewardTrades DOS

Summary

Checking for an invalid reward token could potentially lead to a executeRewardTrades DOS.

Vulnerability Detail

Bot can claim reward tokens from reward pool and then reinvest them as LP tokens which are donated to all vault users。This requires the following 2-step process ：

• 1.claimRewardTokens
• 2.reinvestReward

There exist a _isInvalidRewardToken to ensure reward token is not in list of pool tokens

    function _isInvalidRewardToken(address token) internal override view returns (bool) {
        return (
            token == TOKEN_1 ||
            token == TOKEN_2 ||
            token == TOKEN_3 ||
            token == TOKEN_4 ||
            token == TOKEN_5 ||
            token == address(AURA_BOOSTER) ||
            token == address(AURA_REWARD_POOL) ||
            token == address(Deployments.WETH)
        );
    }

The reward token present is the BAL token, which is claimed from the ARUA reward pool 。 However it is possible that the BAL token is inside the pool .

Here is a list of pools from AURA:

And I believe there is a possibility of adding more pools in the future that include BAL。If this type of pool encounters a revert during the reinvestReward operation in the bot, it could lead to a denial-of-service (DOS).

And the same with curve

coins[0] in above LP is CRV 0xD533a949740bb3306d119CC777fa900bA034cd52

Reward token from rewardPool is also CRV 0xD533a949740bb3306d119CC777fa900bA034cd52

Impact

bot can not invoke reinvestReward due to revert

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L38#L49

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/curve/ConvexStakingMixin.sol#L60#L69

    function _isInvalidRewardToken(address token) internal override view returns (bool) {
        return (
            token == TOKEN_1 ||
            token == TOKEN_2 ||
            token == TOKEN_3 ||
            token == TOKEN_4 ||
            token == TOKEN_5 ||
            token == address(AURA_BOOSTER) ||
            token == address(AURA_REWARD_POOL) ||
            token == address(Deployments.WETH)
        );
    }

Tool used

Manual Review

Recommendation

ensure BAL token is not in the pool

Duplicate of #84

ke1caM

medium

block.timestamp offers no protection as a deadline for a swap

Summary

block.timestamp will have the value of whichever block the txn is inserted into, hence the txn can be held indefinitely by malicious validators.

Vulnerability Detail

Block proposers have advance knowledge of whether they will be suggesting individual blocks or a sequence of blocks ahead of time. In this situation, a dishonest validator could delay a transaction and carry it out at a more advantageous block number.

Impact

Transaction can be validated at further blocks on less favorable terms.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L177

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L248

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L140

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L170

Tool used

Manual Review

Recommendation

Allow sender of the transaction to parameterize deadline parameter of a trade / swap.

Duplicate of #115

AuditorPraise

high

if isSingleSided bool is false, execution of trades back to the primary token on external exchanges will fail due to non-transfer of the balance of the other tokens to be sold for primary token into the trading Module

Summary

the balance of the other tokens to be sold for primary token is never transferred into the trading module, hence trading module has nothing to trade with.

Vulnerability Detail

SingleSidedLPVaultBase._redeemFromNotional() uses StrategyUtils.executeRedemptionTrade() to execute redemption trades whenever isSingleSided bool is false

Now the issue is that the balance of the other tokens to be sold for primary token is never transferred into the trading module, so the trading module has no funds to trade with on the external exchanges.

Here's the flow of a redemption trade:

• BaseStrategyVault.redeemFromNotional() calls SingleSidedLPVaultBase._redeemFromNotional() here

• SingleSidedLPVaultBase._redeemFromNotional() uses StrategyUtils.executeRedemptionTrades for execution of trades on external exchanges here

• StrategyUtils.executeRedemptionTrades calls _executeDynamicSlippageTradeExactIn() here

• _executeDynamicSlippageTradeExactIn() makes the call to TradeHandler._executeTrade() here

• The main issue lies here in the TradeHandler._executeTrade().
  The TradeHandler._executeTrade() just makes a delegate call to the trading module without transferring the amount of sellToken to trade into the trading module.

Impact

if isSingleSided bool is false, execution of trades back to the primary token on external exchanges will fail due to non-transfer of the balance of the other tokens to be sold for primary token into the trading Module

SingleSidedLPVaultBase._redeemFromNotional() will always revert due to lack of funds to execute trades on trading module as the funds were never transferred into the trading module.

Code Snippet

Tool used

Manual Review

Recommendation

Have the vaults transfer amount of sellToken to execute trades with into the TradingModule.sol.

djanerch

medium

Functions can reach block gas-limit via unbounded operations

Summary

Smart contract vulnerabilities, particularly related to gas limit issues, can have severe consequences. One critical vulnerability arises when functions demand more gas than the block limit allows, commonly occurring in loops that iterate over dynamic data structures.

Vulnerability Detail

Certain smart contract functions are at risk due to inadequate size validation of arrays, particularly in loops iterating over dynamic data structures. Neglecting to check input array sizes before execution can lead to computational requirements exceeding the block gas limit, resulting in transaction failures and state rollback. This vulnerability is especially crucial in decentralized finance (DeFi) applications where precise function execution is vital.

Impact

Gas limit vulnerabilities extend beyond transaction failures, potentially causing funds locking, contract freezing, operational disruptions, and broader economic consequences for users and the ecosystem.

1. Funds Locking: Users may struggle to access or withdraw funds, leading to financial losses and trust erosion.

2. Contract Freezing: Gas limit failures can freeze the contract's state, affecting DApps relying on its functionality.

3. Operational Disruption: Disruptions in DeFi protocols may inconvenience users and harm the project's reputation.

4. Economic Consequences: Gas-related failures can have economic repercussions for users, investors, and the ecosystem.

Code Snippet

• StrategyUtils.sol 27-55
• StrategyUtils.sol 59-87

Tool Used

Manual Review

Recommendation

To strengthen smart contracts against gas limit vulnerabilities and mitigate their impact, consider implementing the following measures:

1. Check Array Length:

   • Validate array lengths using the require statement before iterating to prevent exhaustion.

   function executeDepositTrades(uint256[] memory amounts) external {
       require(amounts.length <= MAX_ARRAY_LENGTH, "Array length exceeds maximum");
       // rest of the function
   }

2. Limit Iteration:

   • Utilize a for loop with proper indexing to iterate over array elements, ensuring bounds are respected.

   function executeRedemptionTrades(uint256[] memory exitBalances) external {
       require(exitBalances.length <= MAX_ARRAY_LENGTH, "Inner array length exceeds maximum");
       // rest of the loop body
   }

3. Gas Limit Consideration:

   • Be aware of gas limits for each Ethereum block. For extensive computations, consider breaking down tasks into smaller transactions.

Bauer

high

reinvestReward() will not function as intended

Summary

The protocol initializes by retrieving tokens only from BALANCER_VAULT.getPoolTokens(), without specifying a reward token during initialization. However, in the reinvestReward() function, the protocol attempts to exchange the reward token for pool tokens, and there is a check that the reward token must be one of the tokens specified during initialization. Since the reward token is not specified during initialization, this check fails, preventing the execution of reinvestReward.

Vulnerability Detail

The reinvestReward() function is responsible for reinvesting rewards into the protocol.

  function reinvestReward(
        SingleSidedRewardTradeParams[] calldata trades,
        uint256 minPoolClaim
    ) external whenNotLocked onlyRole(REWARD_REINVESTMENT_ROLE) returns (
        address rewardToken,
        uint256 amountSold,
        uint256 poolClaimAmount
    ) {
        // Will revert if spot prices are not in line with the oracle values
        _checkPriceAndCalculateValue();

        // Require one trade per token, if we do not want to buy any tokens at a
        // given index then the amount should be set to zero. This applies to pool
        // tokens like in the ComposableStablePool.
        require(trades.length == NUM_TOKENS());
        uint256[] memory amounts;
        (rewardToken, amountSold, amounts) = _executeRewardTrades(trades);

The issue lies in the _isInvalidRewardToken() check within the _executeRewardTrades() function. This check ensures that the rewardToken used in the reward trading process is a valid reward token, i.e., one of the tokens specified during the protocol initialization.

    function _isInvalidRewardToken(address token) internal override view returns (bool) {
        return (
            token == TOKEN_1 ||
            token == TOKEN_2 ||
            token == TOKEN_3 ||
            token == TOKEN_4 ||
            token == TOKEN_5 ||
            token == address(AURA_BOOSTER) ||
            token == address(AURA_REWARD_POOL) ||
            token == address(Deployments.WETH)
        );
    }

However, the rewardToken obtained during the reinvestReward function execution is not part of the initially specified reward tokens, the _isInvalidRewardToken() check will fail.

As a result, the protocol won't be able to execute the reinvestReward() function successfully because the reward token obtained from the trades is not considered valid

Impact

The protocol won't be able to execute the reinvestReward() function successfully

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L38-L49
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/BalancerPoolMixin.sol#L84-L102

Tool used

Manual Review

Recommendation

The recommended fix is to specify the reward token during initialization.

Bauer

medium

Bypassing access controls for REWARD_REINVESTMENT_ROLE in claimRewardTokens()

Summary

The claimRewardTokens() function, is designed to be accessible only to entities with the REWARD_REINVESTMENT_ROLE.However ,anyone can claim Convex rewards for any account, bypassing the intended role-based access control.

Vulnerability Detail

The SingleSidedLPVaultBase.claimRewardTokens() function is intended to allow the claiming of rewards, restricted to entities with the REWARD_REINVESTMENT_ROLE. However, there is a potential vulnerability as anyone can claim Convex rewards for any account, bypassing the intended role-based access control.

function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
    uint256 reward = earned(_account);
    if (reward > 0) {
        rewards[_account] = 0;
        rewardToken.safeTransfer(_account, reward);
        IDeposit(operator).rewardClaimed(pid, _account, reward);
        emit RewardPaid(_account, reward);
    }

    //also get rewards from linked rewards
    if(_claimExtras){
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).getReward(_account);
        }
    }
    return true;
}

Impact

Bypass the intended role-based access control.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L376-L378

Tool used

Manual Review

Recommendation

djanerch

high

Front-Running and Sandwich Attack Vulnerability in BalancerComposableAuraVault.sol and BalancerWeightedAuraVault.sol

Summary

The absence of time-locks or equivalent mechanisms in the staking functions can expose vulnerabilities to front-running and sandwich attacks.

Vulnerability Detail

This vulnerability arises due to the lack of protective measures in the staking functions, allowing malicious actors to exploit transaction order and manipulate the state of the Balancer pool.

Impact

Scenario:

1. Victim's Transaction (Original Intent):

   • The victim aims to stake LP tokens using the _joinPoolAndStake function.
   • The victim broadcasts the transaction to the network.

2. Attacker Observes Victim's Transaction:

   • The attacker monitors pending transactions and identifies the victim's transaction awaiting mining.

3. Front-Run Attack:

   • The attacker rapidly submits a transaction with a higher gas price, invoking the same _joinPoolAndStake function before the victim's transaction is mined.
   • The attacker's transaction is prioritized, leading to the staking of LP tokens in the Balancer pool.

4. Victim's Transaction Mined:

   • The victim's transaction is mined, but the Balancer pool now holds fewer available LP tokens due to the attacker's transaction.
   • The victim's LP tokens are staked, potentially impacting rewards due to the altered state of the pool.

5. Sandwich Attack:

   • With the LP tokens staked, the attacker swiftly submits another transaction to unstake LP tokens and exit the pool, aiming to exploit the changed state caused by their initial front-run transaction.
   • The attacker's second transaction, with a higher gas price, is swiftly mined.

6. Profit for the Attacker:

   • The attacker successfully unstakes LP tokens and exits the pool, potentially profiting from the changed state and taking advantage of the victim's transaction.

In this scenario, the attacker strategically employs transaction sequencing to front-run the victim, manipulate the Balancer pool's state, and execute a sandwich attack for potential financial gain.

Code Snippet

BalancerComposableAuraVault.sol:
(https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L34#L58)
(https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L60#L88)

BalancerWeightedAuraVault.sol:
(https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerWeightedAuraVault.sol#L36#L50)
(https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerWeightedAuraVault.sol#L52#L74)

Tool used

Manual Review

Recommendation

To mitigate such attacks, it is recommended to implement the following safeguards:

• Utilize time-locks or similar mechanisms to restrict the execution window for certain operations.
• Implement checks to verify that the caller is the intended user.
• Encourage users to set appropriate gas fees to minimize the likelihood of front-running.

shealtielanz

medium

EXACT_IN trades will be prone to extremely high slippage.

Summary

TradingUtils.sol _getLimitAmount() function sets the min amount for a EXACT_IN trade type to zero, allowing trades of that sort to settle with extremely high slippage up to zero amount.

Vulnerability Detail

you can see in _getLimitAmount()

    function _getLimitAmount(
        address from,
        TradeType tradeType,
        address sellToken,
        address buyToken,
        uint256 amount,
        uint32 slippageLimit,
        uint256 oraclePrice,
        uint256 oracleDecimals
    ) internal view returns (uint256 limitAmount) {
        uint256 sellTokenDecimals = 10 **
            (
                sellToken == Deployments.ETH_ADDRESS
                    ? 18
                    : IERC20(sellToken).decimals()
            );
        uint256 buyTokenDecimals = 10 **
            (
                buyToken == Deployments.ETH_ADDRESS
                    ? 18
                    : IERC20(buyToken).decimals()
            );

        if (tradeType == TradeType.EXACT_OUT_SINGLE || tradeType == TradeType.EXACT_OUT_BATCH) {
            // type(uint32).max means no slippage limit
            if (slippageLimit == type(uint32).max) {
                return sellToken == Deployments.ETH_ADDRESS
                    ? address(from).balance
                    : IERC20(sellToken).balanceOf(from);
            }
            // For exact out trades, we need to invert the oracle price (1 / oraclePrice)
            // We increase the precision before we divide because oraclePrice is in
            // oracle decimals
            oraclePrice = (oracleDecimals * oracleDecimals) / oraclePrice;
            // For exact out trades, limitAmount is the max amount of sellToken the DEX can
            // pull from the contract
            limitAmount =
                ((oraclePrice + 
                    ((oraclePrice * uint256(slippageLimit)) /
                        Constants.SLIPPAGE_LIMIT_PRECISION)) * amount) / 
                oracleDecimals;

            // limitAmount is in buyToken precision after the previous calculation,
            // convert it to sellToken precision
            limitAmount = (limitAmount * sellTokenDecimals) / buyTokenDecimals;
        } else {
            // type(uint32).max means no slippage limit
            if (slippageLimit == type(uint32).max) {
                return 0;
            }

this function is used to set the limit amount of trade in the pools and vaults, where the limit amount of a trade is set to 0, trade can lose their value to the max.

Impact

This will leave EXACT_IN trades vulnerable to extreme slippage attacks and settle at huge loses, whenever the slippageLimit == type(uint32).max)

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol#L175C1-L225C14

Tool used

Manual Review

Recommendation

revert instead of return 0.

AuditorPraise

high

Execution of trades on external exchanges will fail whenever ETH is buyToken because TradingModule.sol contract lacks a payable fallback/receive function hence it won’t be able to receive ETH sent back from the dexes during the execution of trades.

Summary

the TradingModule.sol contract lacks a payable fallback/receive function

Vulnerability Detail

A payable fallback function is needed for handling transactions that are sent to the contract without specifying a particular function to call.

A payable receive function is also needed for a contract to be able to handle incoming ETH.

Whenever dexes are sending back the ETH bought during the execution of trades, The TradingModule.sol contract won't be able to receive it because it lacks a payable fallback/receive function.

Vaults won't be able to execute trades on external exchanges via the trading module whenever ETH is the primary token (i.e buyToken).

Impact

Vaults won't be able to execute trades on external exchanges via the trading module whenever ETH is the primary token (i.e buyToken).

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L23

Tool used

Manual Review

Recommendation

Add a payable fallback/receive function to TradingModule.sol

mstpr-brainbot

medium

Some curve pools can not be used as a single sided strategy

Summary

For single sided curve strategy contest docs says that any curve pool should be ok to be used. However, some pools are not adaptable to the Curve2TokenPoolMixin abstract contract.

Vulnerability Detail

The contract assumes the existence of three types of pool scenarios. In one scenario, the pool address itself serves as the LP token. In another scenario, the LP token is obtained by querying the lp_token() or token() function. However, in some cases where potential integration with Notional is possible, certain pools lack a direct method to retrieve the underlying LP token. For instance, the sETH-ETH pool, which presents a promising option for a single-sided strategy in ETH vaults, does not offer public variables to access the underlying LP token. Although the pool contract contains the token variable that returns the LP token of the pool, this variable is not publicly accessible. Consequently, in such cases, querying the LP token directly from the pool is not feasible, and it becomes necessary to provide the LP token address as input.

Here the example contracts where neither of the options are available:
sETH-ETH: https://etherscan.io/address/0xc5424b857f758e906013f3555dad202e4bdb4567
hBTC-WBTC: https://etherscan.io/address/0x4ca9b3063ec5866a4b82e437059d2c43d1be596f

Impact

All curve pools that can be used as a single sided strategy for notional leveraged vaults considered to be used but some pools are not adaptable thus I will label this as medium.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/curve/Curve2TokenPoolMixin.sol#L73-L78

Tool used

Manual Review

Recommendation

Curve contracts are pretty different and it is very hard to make a generic template for it. I would suggest make the LP token also an input and remove the necessary code for setting it in the constructor. Since the owner is trusted this should not be a problem.

mstpr-brainbot

medium

Some curve pools will not be able to remove liquidity single sided

Summary

Some curve pools uses the remove_liquidity_one_coin function with uint256 but the notional code assumes it is always int128 which if the pool to be used is using uint256 then the removing single sided liquidity is impossible.

Vulnerability Detail

First this is the interface that the Notional uses when removing single sided liquidity from curve:

interface ICurve2TokenPool is ICurvePool {
    function remove_liquidity(uint256 amount, uint256[2] calldata _min_amounts) external returns (uint256[2] memory);
    function remove_liquidity_one_coin(uint256 _token_amount, int128 i, uint256 _min_amount) external returns (uint256);
}

so it uses uint256, int128 and uint256 as inputs. Let's see some of the pools that notional can potentially use. One can be the rETH-ETH pool for ETH leveraged vault. This is how the remove_liquidity_one_coin function is built:

@external
@nonreentrant('lock')
def remove_liquidity_one_coin(token_amount: uint256, i: uint256, min_amount: uint256,
                              use_eth: bool = False, receiver: address = msg.sender) -> uint256:

as we can see it uses uint256, uint256 , uint256. In Notionals interface above it uses int128 for the I variable but the actual pool uses uint256. This will fail because casting an interface input from int128 to uint256 is not supported in solidity.

Impact

Pool can be deployed without a problem. However, in production the removing single sided liquidity will not work. Also, Notional team states that every "potential" curve pool can be used and the above example is rETH-ETH which is a very good candidate for ETH strategy. Thus, I'll label this as medium.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol#L78-L84

Tool used

Manual Review

Recommendation

Make a flag that tells the contract that the pool uses remove_liquidity_one_coin with uint256 inputs.

0xDemon

medium

Some Token May Reverted on Large Approvals or Transfers

Summary

Some ERC 20 tokens can revert if the approval or transfer exceeds uint96. This can trigger unexpected behavior during the initial approval of collateral tokens from the user until the stacking process.

Vulnerability Detail

In the AuraStakingMixin.sol contract there is an approve function that is called and the max value is uint256 . This function aims to approve tokens as collateral from user to vault and from vault to stacking. Some tokens (e.g. UNI, COMP) revert if the value passed to approve or transfer is larger than uint96. Below is the codebase for UNI & COMP tokens in the approve function section :

function approve(address spender, uint rawAmount) external returns (bool) {
        uint96 amount;
        if (rawAmount == uint(-1)) {
            amount = uint96(-1);
        } else {
            amount = safe96(rawAmount, "Comp::approve: amount exceeds 96 bits");
        }

        allowances[msg.sender][spender] = amount;

        emit Approval(msg.sender, spender, amount);
        return true;
    }

function transfer(address dst, uint rawAmount) external returns (bool) {
        uint96 amount = safe96(rawAmount, "Comp::transfer: amount exceeds 96 bits");
        _transferTokens(msg.sender, dst, amount);
        return true;
    }

Both of the tokens have special case logic in approve that sets allowance to type(uint96).max . While the number of approvals is max uint256, this may cause issues with systems that expect the value passed to approve to be reflected in the allowances mapping.

Impact

Revert on Large Approvals or Transfers when execute the approve function that called on initialization

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L51-L60

Tool used

Manual review

Recommendation

Consider creating a special approve or transfer function that has a max uint96 for both of tokens if the protocol still wants to receive these tokens as collateral.

Duplicate of #93

Vagner

high

If the pool used in Curve2TokenConvexVault.sol is killed emergencyExit would revert 100% of times, blocking this important functionality

Summary

If the pool used in Curve2TokenConvexVault.sol gets killed, which happens regularly, emergencyExit would not be possible, which could hurt the protocol in case of an emergency.

Vulnerability Detail

emergencyExit can be used by the emergency exit role to trigger an emergency exit on the vault, to protect the users and lock the vault, until the emergency passes, as can be seen here
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L475-L479
this would call _unstakeAndExitPool with the isSingleSided bolean to true
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L489
but for the case of Curve2TokenConvexVault.sol this could be a big problem if the pool used is killed. As you can see here, in the case where isSingleSided is true, it will try to redeem single-sided
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol#L80-L84
by calling remove_liquidity_one_coin on the curve pool. The problem arise that, if the pool is killed, remove_liquidity_one_coin would not be possible and revert 100%
https://etherscan.io/address/0xdcef968d416a41cdac0ed8702fac8128a64241a2#code#L709
only remove_liquidity will function if a pool is killed, as can be seen here
https://etherscan.io/address/0xdcef968d416a41cdac0ed8702fac8128a64241a2#code#L510
This would mean that emergency exits would fail, which could pose a real security threat for those cases, since in case of an emergency, the protocol can't use emergencyExit.

Impact

Impact is a high one since emergencyExit could not be possible to be used in those cases.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/Curve2TokenConvexVault.sol#L80-L84

Tool used

Manual Review

Recommendation

Take a different approach for emergency exits, so the protocol would have way of protecting itself, in case of emergencies for Curve2TokenConvexVault.sol.

Duplicate of #96

wangxx2026

medium

Historical pooltoken will still decrease the allowance of (owner, spender) even when the allowance is set to type (uint256).max

Summary

Historical pooltoken will still decrease the allowance of (owner, spender) even when the allowance is set to type (uint256).max

Vulnerability Detail

While it's not defined in the EIP-20, it's a common implementation (see both OpenZeppelin and Solmate) that the transferFrom function of an ERC20 token does not decrease the allowance of the spender when such allowance has been set to the max value type(uint256).max.

Although the latest BalancerPoolToken implementation has already implemented support, many old ones still do not support it, such as the following two:

poolId:
0xade4a71bb62bec25154cfc7e6ff49a513b491e81000000000000000000000497
0xade4a71bb62bec25154cfc7e6ff49a513b491e81000000000000000000000497

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);
        _approve(
            sender,
            msg.sender,
            _allowances[sender][msg.sender].sub(amount, Errors.ERC20_TRANSFER_EXCEEDS_ALLOWANCE)
        );
        return true;
    }

The code for approve is now implemented as follows. Once set, it cannot be set again.

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L173-L181

    function initialize(InitParams calldata params) external override initializer onlyNotionalOwner {
        // Initialize the base vault
        __INIT_VAULT(params.name, params.borrowCurrencyId);

        // Settings are validated in setStrategyVaultSettings
        VaultStorage.setStrategyVaultSettings(params.settings);

        _initialApproveTokens();
    }

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L52-L60

    function _initialApproveTokens() internal override {
        (IERC20[] memory tokens, /* */) = TOKENS();
        for (uint256 i; i < tokens.length; i++) {
            tokens[i].checkApprove(address(Deployments.BALANCER_VAULT), type(uint256).max);
        }

        // Approve Aura to transfer pool tokens for staking
        POOL_TOKEN().checkApprove(address(AURA_BOOSTER), type(uint256).max);
    }

Impact

Due to the wide range of tokens supported by Valut,
at some point in the future, the stake operation will revert because the spender would not have enough allowance anymore.

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L56

bool success = AURA_BOOSTER.deposit(AURA_POOL_ID, lpTokens, true);

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/BalancerWeightedAuraVault.sol#L48

bool success = AURA_BOOSTER.deposit(AURA_POOL_ID, lpTokens, true);

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L52-L60
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L173-L181

Tool used

Manual Review

Recommendation

Add a method to reset allowance

dany.armstrong90

high

CrossCurrencyVault.convertStrategyToUnderlying: The return value is miscalculated.

Summary

In CrossCurrencyVault.sol#convertStrategyToUnderlying function, when calculating the underlyingValue value, decimal of rate instead of precision is used.
As a result, the return value underlyingValue has become enormously large, so that Notional misjudges that vault user is fully collatoralized, and bad debt is generated.

Vulnerability Detail

The CrossCurrencyVault.sol#convertStrategyToUnderlying function is as follows.

File: CrossCurrencyVault.sol
107:     function convertStrategyToUnderlying(
108:         address /* account */,
109:         uint256 vaultShares,
110:         uint256 maturity
111:     ) public override view returns (int256 underlyingValue) {
112:         int256 pvExternalUnderlying;
...
128: 
129:         // Returns the oracle price between the lend and borrow tokens.
130:         IERC20 underlyingToken = _underlyingToken();
131:         (int256 rate, int256 rateDecimals) = TRADING_MODULE.getOraclePrice(
132:             address(LEND_UNDERLYING_TOKEN), address(underlyingToken)
133:         );
134:         int256 borrowPrecision = int256(10**BORROW_DECIMALS);
135:         int256 lendPrecision = int256(10**LEND_DECIMALS);
136: 
137:         // Convert this back to the borrow currency, external precision
138:         // (pv (lend decimals) * borrowDecimals * rate) / (rateDecimals * lendDecimals)
139:         return (pvExternalUnderlying * borrowPrecision * rate) /
140:             (rateDecimals * lendPrecision);
141:     }

In line 139, rate is divided by rateDecimal instead of ratePrecision = 10 ** rateDecimal.
For example, if rateDecimal == 10, the return value underlyingValue is amplified by a factor of 10 ** rateDecimal / rateDecimal == 1e9.
Due to SingleSidedLPVaultBase.sol#L304-L305, this function is used to check the collatoralization rate of vault user by Notional.
Thus Notional misjudges that vault user is fully collatoralized and bad debt is generated.

Impact

Notional misjudges that vault user is fully collatoralized and bad debt is generated.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L140

Tool used

Manual Review

Recommendation

Modify the code CrossCurrencyVault.sol#L139-L140 as follows.

        return (pvExternalUnderlying * borrowPrecision * rate) /
            (10 ** rateDecimals * lendPrecision);

cu5t0mPe0

high

Slight errors in calculations allow users to steal other people's property

Summary

When the user deposits, since there is no slippage protection, the funds deposited by the user only need not be divisible, then the totalPoolClaim/totalPoolClaim will become larger. This effect is similar to artificially created bonus funds.

vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim

In this way, those who join the treasury later will actually receive fewer shares.

Vulnerability Detail

Assume Constants.INTERNAL_TOKEN_PRECISION is 100 and POOL_PRECISION is 10000.

1. Malicious user A deposits 1600000099 and will receive 16000000 shares. At this time, the totalPoolClaim value is 16000000.

2. User B deposits 10,000, and the calculated shares are 99.999993. Rounding down is 99 shares.

3. At this time, malicious user A is withdrawing money, and the amount obtained will be 1600000198.9987688. The actual amount obtained is 1600000198. At this time, user A has obtained part of user B's deposit.

If a malicious user keeps increasing the totalPoolClaim value through mev in this way, the shares obtained by other users will be reduced even more.

Impact

Malicious users can steal other people’s funds

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L235-L235

Tool used

Manual Review

Recommendation

Users should add slippage protection when obtaining shares. When the shares obtained exceed the slippage range, the transaction will be cancelled.

Vagner

high

_checkReentrancyContext should be called in more instances than already is, to protect against read-only reentrancy

Summary

_checkReentrancyContext used in BalancerComposableAuraVault.sol and BalancerWeightedAuraVault.sol is used to protect against reentrancy, as per balancer recommendation, but it is called only in one instance in deleverageAccount function, which is not enough.

Vulnerability Detail

As per balancer recommendation, it is important to protect against the reentrancy vulnerability found in multiple balancer pools, explained in details here https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345
The only case where _checkReentrancyContext is used right not is only in the deleverageAccount function, as can be seen here
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L217-L229
but there is also another very important case, that the _checkReentrancyContext should be called, and that is every time when getActualSupply is called. As can be seen in the NatSpec of getActualSupply of a random composable stable pool
https://etherscan.io/address/0x42ed016f826165c2e5976fe5bc3df540c5ad0af7#code#F3#L1104
it is specified that it is very important for the reentrancy check to be called every time before getActualSupply is called, for this function to be safe to call, but this is not done in any of the contracts provided, as can be seen here
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L108-L110
This could lead to reentrancy vulnerability, explained on the balancer forum, being exploited and that would hurt the protocol and the users.

Impact

Impact is a high one, since the damage could be very high.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/BalancerComposableAuraVault.sol#L108-L110

Tool used

Manual Review

Recommendation

Call _checkReentrancyContext before calling getActualSupply, so the function call would be protected from reentrancy.

shealtielanz

high

_deleverageVaultAccount() in FlashLiquidatorBase.sol is prone to read-only reentrancy whenever actionParams.useVaultDeleverage is false allowing for reentrancy

Summary

_deleverageVaultAccount() in FlashLiquidatorBase.sol is used when handling liquidations, the issue is that when actionParams.useVaultDeleverage is equal to false is doesn't check against read only reentrancy which will allow for reentrancy on balancer and curve pools.

Vulnerability Detail

Let me start by saying the devs are well aware of this issue(read-only reentrancy in balancer and curve pools) as you can see in you can see _deleverageVaultAccount()
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/utils/FlashLiquidatorBase.sol#L136C1-L175C6

    function _deleverageVaultAccount(LiquidationParams memory params) private {
        (uint16 currencyIndex, int256 maxUnderlying) = _getOptimalDeleveragingParams(params.account, params.vault);
        require(maxUnderlying > 0);


        DeleverageVaultAccountParams memory actionParams = abi.decode(params.actionData, (DeleverageVaultAccountParams));
        uint256 vaultSharesFromLiquidation;
        if (actionParams.useVaultDeleverage) {
            (
                vaultSharesFromLiquidation, /* */ 
            ) = IStrategyVault(params.vault).deleverageAccount{value: address(this).balance}(
                params.account, 
                params.vault, 
                address(this), 
                currencyIndex,
                maxUnderlying
            );
        } else {
            (
                vaultSharesFromLiquidation, /* */ 
            ) = NOTIONAL.deleverageAccount{value: address(this).balance}(
                params.account, 
                params.vault, 
                address(this), 
                currencyIndex,
                maxUnderlying
            );
        }


        if (0 < vaultSharesFromLiquidation) {
            NOTIONAL.exitVault(
                address(this), 
                params.vault, 
                address(this), 
                vaultSharesFromLiquidation,
                0, 
                0, 
                actionParams.redeemData
            );
        }
    }

when actionParams.useVaultDeleverage = true the call to IStrategyVault(params.vault).deleverageAccount is shown as
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L217C1-L229C6

    function deleverageAccount(
        address account,
        address vault,
        address liquidator,
        uint16 currencyIndex,
        int256 depositUnderlyingInternal
    ) external payable returns (uint256 vaultSharesFromLiquidation, int256 depositAmountPrimeCash) {
        require(msg.sender == liquidator);
        _checkReentrancyContext();
        return NOTIONAL.deleverageAccount{value: msg.value}(
            account, vault, liquidator, currencyIndex, depositUnderlyingInternal
        );
    }

You can see it checks against reentrancy on the call to _checkReentrancyContext(); before NOTIONAL.deleverageAccount() is called.

• for balancer pools
  https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/BalancerPoolMixin.sol#L131C3-L136C6

    /// @notice the re-entrancy context is checked during liquidation to prevent read-only
    /// reentrancy on all balancer pools.
    function _checkReentrancyContext() internal override {
        IBalancerVault.UserBalanceOp[] memory noop = new IBalancerVault.UserBalanceOp[](0);
        Deployments.BALANCER_VAULT.manageUserBalance(noop);
    }

• for curve pools
  https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/curve/Curve2TokenPoolMixin.sol#L101C1-L106C6

    function _checkReentrancyContext() internal override {
        // We need to set the LP token amount to 1 for Curve V2 pools to bypass
        // the underflow check
        uint256[2] memory minAmounts;
        ICurve2TokenPool(address(CURVE_POOL)).remove_liquidity(IS_CURVE_V2 ? 1 : 0, minAmounts);
    }

However whenever actionParams.useVaultDeleverage = False the call to _deleverageVaultAccount() is skipped and it calls NOTIONAL.deleverageAccount() directly without checking against the reentrancy that can happen during liquidations.

Impact

whenever actionParams.useVaultDeleverage = False liquidations are prone to reentrancy allowing exploits on balancer and curve pools.

Code Snippet

• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/utils/FlashLiquidatorBase.sol#L136C1-L175C6
• https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L217C1-L229C6

Tool used

Manual Review, curve pool exploit

Recommendation

depending on the pool the liquidation happens in(curve or balancer) the corresponding _checkReentrancyContext() should be called before the call to NOTIONAL.deleverageAccount()

    function _deleverageVaultAccount(LiquidationParams memory params) private {
        (uint16 currencyIndex, int256 maxUnderlying) = _getOptimalDeleveragingParams(params.account, params.vault);
        require(maxUnderlying > 0);


        DeleverageVaultAccountParams memory actionParams = abi.decode(params.actionData, (DeleverageVaultAccountParams));
        uint256 vaultSharesFromLiquidation;
        if (actionParams.useVaultDeleverage) {
            (
                vaultSharesFromLiquidation, /* */ 
            ) = IStrategyVault(params.vault).deleverageAccount{value: address(this).balance}(
                params.account, 
                params.vault, 
                address(this), 
                currencyIndex,
                maxUnderlying
            );
        } else {
+    _checkReentrancyContext();
            (
                vaultSharesFromLiquidation, /* */ 
            ) = NOTIONAL.deleverageAccount{value: address(this).balance}(
                params.account, 
                params.vault, 
                address(this), 
                currencyIndex,
                maxUnderlying
            );
        }


        if (0 < vaultSharesFromLiquidation) {
            NOTIONAL.exitVault(
                address(this), 
                params.vault, 
                address(this), 
                vaultSharesFromLiquidation,
                0, 
                0, 
                actionParams.redeemData
            );
        }
    }

Bauer

high

Sandwich attack in reinvestReward() Function

Summary

The reinvestReward() function is susceptible to a sandwich attack, where malicious users can exploit the lack of additional vault share minting to prematurely withdraw rewards. This occurs by executing depositFromNotional() before reinvestReward(), acquiring LP tokens, and later using redeemFromNotional() to exchange LP tokens for tokens. A bad actor can receive rewards without the need for a specified waiting period after depositing funds.

Vulnerability Detail

The calculation for vaultShares in the SingleSidedLPVaultBase._mintVaultShares() is determined by multiplying the number of LP tokens (lpTokens) acquired by the global total of vault shares (state.totalVaultSharesGlobal).

    function _mintVaultShares(uint256 lpTokens) internal returns (uint256 vaultShares) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim;
        }

The calculation for poolClaim in the SingleSidedLPVaultBase._redeemVaultShares function is determined by multiplying the number of vaultShares by the total value of assets in the pool (state.totalPoolClaim), and then dividing the result by the global total of vault shares (state.totalVaultSharesGlobal).

    function _redeemVaultShares(uint256 vaultShares) internal returns (uint256 poolClaim) {
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        // Will revert on divide by zero, which is the correct behavior
        poolClaim = (vaultShares * state.totalPoolClaim) / state.totalVaultSharesGlobal;

        state.totalPoolClaim -= poolClaim;
        // Will revert on underflow if vault shares is greater than total shares global
        state.totalVaultSharesGlobal -= vaultShares.toUint80();
        state.setStrategyVaultState();
    }

This SingleSidedLPVaultBase.reinvestReward() function is designed to handle the reinvestment of rewards obtained from a strategy.
In this function, where the LP token amount is increased without minting additional vault shares, there is a vulnerability to sandwich attack.

  poolClaimAmount = _joinPoolAndStake(amounts, minPoolClaim);

        // Increase LP token amount without minting additional vault shares
        StrategyVaultState memory state = VaultStorage.getStrategyVaultState();
        state.totalPoolClaim += poolClaimAmount;
        state.setStrategyVaultState();

Malicious actors can monitor transactions within the trading pool, execute a deposit operation using depositFromNotional() before the reinvestReward() function is called, acquire LP tokens, and subsequently invoke redeemFromNotional() after the execution of reinvestReward() to exchange LP tokens for tokens. The issue arises because reinvestReward() only updates the LP token amount without minting additional vault shares. This allows users to exploit the sandwich attack, enabling them to promptly withdraw the corresponding rewards gained from the attack.

Impact

It leads to unfair advantages and financial losses for other users within the trading pool.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L385-L411

Tool used

Manual Review

Recommendation

The recommendation is to allow users to redeem their funds only after a specified waiting period following the deposit

Duplicate of #48

ginlee

medium

Users invest lpTokens but receives no vaultShares in return

Summary

Vulnerability Detail

Within the SingleSidedLPVaultBase._mintVaultShares function, it was observed that if the numerator is smaller than the denominator, the vaultShares will be zero.

if (state.totalPoolClaim == 0) {
            // Vault Shares are in 8 decimal precision
            vaultShares = (lpTokens * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / POOL_PRECISION();
        } else {
            vaultShares = (lpTokens * state.totalVaultSharesGlobal) / state.totalPoolClaim;
        }

Impact

When the vaultShares is zero, the function returns zero instead of reverting. Therefore, it is possible that users invest their lpTokens, but receives no vaultShares in return because the number of lpTokens input by the user is too small.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L231-L236

Tool used

Manual Review

Recommendation

Consider reverting if the vaultShares received is zero. This check has been implemented in many well-known vault designs

require(vaultShares > 0, "Failed to mint any vault shares");

Duplicate of #75

ZanyBonzy

medium

No check for active L2 Sequencer

Summary

Using Chainlink in L2 chains such as Arbitrum requires to check if the sequencer is down to avoid prices from looking like they are fresh although they are not according to their recommendation

Vulnerability Detail

The SingleSidedLPVaultBase and CrossCurrencyVault contracts make the getOraclePrice external call to the TradingModule contract. However, the getOraclePrice in the TradingModule makes no check to see if the sequencer is down.

Impact

If the sequencer goes down, the protocol will allow users to continue to operate at the previous (stale) rates and this can be leveraged by malicious actors to gain unfair advantage.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/SingleSidedLPVaultBase.sol#L323

    function _getOraclePairPrice(address base, address quote) internal view returns (uint256) {
        (int256 rate, int256 precision) = TRADING_MODULE.getOraclePrice(base, quote);
        require(rate > 0);
        require(precision > 0);
        return uint256(rate) * POOL_PRECISION() / uint256(precision);
    }

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L131

        (int256 rate, int256 rateDecimals) = TRADING_MODULE.getOraclePrice(

https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/trading/TradingModule.sol#L71C1-L77C6

    function getOraclePrice(address baseToken, address quoteToken)
        public
        view
        override
        returns (int256 answer, int256 decimals)
    {
        PriceOracle memory baseOracle = priceOracles[baseToken];
        PriceOracle memory quoteOracle = priceOracles[quoteToken];

        int256 baseDecimals = int256(10**baseOracle.rateDecimals);
        int256 quoteDecimals = int256(10**quoteOracle.rateDecimals);

        (/* */, int256 basePrice, /* */, uint256 bpUpdatedAt, /* */) = baseOracle.oracle.latestRoundData();
        require(block.timestamp - bpUpdatedAt <= maxOracleFreshnessInSeconds);
        require(basePrice > 0); /// @dev: Chainlink Rate Error

        (/* */, int256 quotePrice, /* */, uint256 qpUpdatedAt, /* */) = quoteOracle.oracle.latestRoundData();
        require(block.timestamp - qpUpdatedAt <= maxOracleFreshnessInSeconds);
        require(quotePrice > 0); /// @dev: Chainlink Rate Error

        answer =
            (basePrice * quoteDecimals * RATE_DECIMALS) /
            (quotePrice * baseDecimals);
        decimals = RATE_DECIMALS;
    }

Tool used

Manual Review

Recommendation

It is recommended to follow the Chailink example code

Bauer

medium

Token Approval Error: Incompatibility with type(uint256).max

Summary

Some tokens, like Uni, do not support type(uint256).max for unlimited approvals, leading to errors during approval.

Vulnerability Detail

The function AuraStakingMixin._initialApproveTokens() is designed to approve token transfers in bulk, ensuring that subsequent contract operations can be executed smoothly.

    function _initialApproveTokens() internal override {
        (IERC20[] memory tokens, /* */) = TOKENS();
        for (uint256 i; i < tokens.length; i++) {
            tokens[i].checkApprove(address(Deployments.BALANCER_VAULT), type(uint256).max);
        }

        // Approve Aura to transfer pool tokens for staking
        POOL_TOKEN().checkApprove(address(AURA_BOOSTER), type(uint256).max);
    }

Some tokens, such as Uni, do not support the value type(uint256).max for unlimited approvals,they only accept uint96(-1). This can lead to errors during the approval process when attempting to use the unsupported value.

Impact

This could result in the protocol being unable to initialize approvals.

Code Snippet

https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L52-L56

Tool used

Manual Review

Recommendation

Approve special tokens separately

Duplicate of #93