shealtielanz - EXACT_IN trades will be prone to extremely high slippage. about 2023-10-notional-judging

[shealtielanz]

Escalate

concerning

Out of Scope, issue does not link to how an inscope contract can be affected by finding found in an out of scope contract, and it is not the judges responsibility to help watson do so

Based on impact shown.

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

This library is clearly used under the hood whenever performing trades through out every vault.

used when executing dynamic trades in StrategyUtils(which is in scope) where _getLimitAmount() of tradingUtils is call by getLimitAmount of trading module which is used to get the trading limit amount during dynamic trades executeTradeWithDynamicSlippage() that is called through out StrategyUtils that is used by the vaults, links here here here

This issue is a valid med atleast, as all the exact in trades are prone to extreme slippage attacks during the conditions stated in the primary report

[shealtielanz]

considering

While the contracts in scope uses these functions, your original submission still lack description of how it will impact contracts in scope.

My main report stated the impact of the issue, and it's root, as the bug can be seen in the library used by notional, all contracts making use of it for trades will be affected.

concerning

I suggest you follow the contract flow (it is tedious) and point out to me where exactly a 0 slippage could take place that is not a user input.

In SingleSidedLPVaultBase.sol :
Flow 1
SingleSidedLPVaultBase.sol._depositFromNotional() -> StrategyUtils.executeDepositTrades() -> StrategyUtils._executeDynamicSlippageTradeExactIn() -> TradeHandler._executeTradeWithDynamicSlippage() -> TradingModule.executeTradeWithDynamicSlippage() -> TradingModule.getLimitAmount() -> TradingUtils._getLimitAmount()

Flow 2
SingleSidedLPVaultBase.sol._redeemFromNotional() -> StrategyUtils.executeRedemptionTrades() -> StrategyUtils._executeDynamicSlippageTradeExactIn() -> TradeHandler._executeTradeWithDynamicSlippage() -> TradingModule.executeTradeWithDynamicSlippage() -> TradingModule.getLimitAmount() -> TradingUtils._getLimitAmount()

on the call to StrategyUtils._executeDynamicSlippageTradeExactIn()
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L119C1-L147C6

• read inline comments.

    /// @notice Executes a trade that uses a dynamic slippage amount relative to the current
    /// oracle price.
    function _executeDynamicSlippageTradeExactIn(
        ITradingModule tradingModule,
        TradeParams memory params,
        address sellToken,
        address buyToken,
        uint256 amount
    ) internal returns (uint256 amountSold, uint256 amountBought) {
        // Can only do exact in trades
    //@audit exact in trades 
        require(
            params.tradeType == TradeType.EXACT_IN_SINGLE ||
            params.tradeType == TradeType.EXACT_IN_BATCH
        );
        // Ensure that the slippage percent is valid
       //@audtit checks to ensure the oracleSlippagePercentOrLimit  is within a reasonable range
        require(params.oracleSlippagePercentOrLimit <= Constants.SLIPPAGE_LIMIT_PRECISION);

        Trade memory trade = Trade(
            params.tradeType,
            sellToken,
            buyToken,
            amount,
            0, // No absolute slippage limit is set here
            block.timestamp, // deadline
            params.exchangeData
        );
    //@audit performs the single sided trade here the `dynamicSlippageLimit` for the trade is `params.oracleSlippagePercentOrLimit`
@>        (amountSold, amountBought) = trade._executeTradeWithDynamicSlippage(
            params.dexId, tradingModule, uint32(params.oracleSlippagePercentOrLimit)
        );
    }

The call to the Tradehander.sol is delegated to the TradingModule.sol here

TradingModule.executeTradeWithDynamicSlippage()
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/trading/TradingModule.sol#L127C1-L162C6

    /// @notice Executes a trade with a dynamic slippage limit based on chainlink oracles.
    /// @dev Expected to be called via delegatecall on the implementation directly. This means that
    /// the contract's calling context does not have access to storage (accessible via the proxy
    /// address).
    /// @param dexId the dex to execute the trade on
    /// @param trade trade object
    /// @param dynamicSlippageLimit the slippage limit in 1e8 precision
    /// @return amountSold amount of tokens sold
    /// @return amountBought amount of tokens purchased
    function executeTradeWithDynamicSlippage(
        uint16 dexId,
        Trade memory trade,
        uint32 dynamicSlippageLimit
    ) external override returns (uint256 amountSold, uint256 amountBought) {
        if (!PROXY.canExecuteTrade(address(this), dexId, trade)) revert InsufficientPermissions();
        if (trade.amount == 0) return (0, 0);

        // This method calls back into the implementation via the proxy so that it has proper
        // access to storage.
//@audit gets the trade.limit from `getLimitAmount()` which calls `TradingUtils._getLimitAmount()` under the hood.
 @>       trade.limit = PROXY.getLimitAmount(
            address(this),
            trade.tradeType,
            trade.sellToken,
            trade.buyToken,
            trade.amount,
            dynamicSlippageLimit
        );

        (
            address spender,
            address target,
            uint256 msgValue,
            bytes memory executionData
        ) = _getExecutionData(dexId, address(this), trade);
//@audit performs the trade with the trade params.
        return
            TradingUtils._executeInternal(
                trade,
                dexId,
                spender,
                target,
                msgValue,
                executionData
            );
    }

TradingModule.getLimitAmount() calls TradingUtils._getLimitAmount()
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/trading/TradingModule.sol#L279C1-L303C6

    function getLimitAmount(
..SNIP..
    ) external view override returns (uint256 limitAmount) {
..SNIP..
@audit calls `TradingUtils._getLimitAmount()` under the hood 
 @>       limitAmount = TradingUtils._getLimitAmount({
..SNIP..

in TradingUtils._getLimitAmount()
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/trading/TradingUtils.sol#L176C1-L238C6

  function _getLimitAmount(
..SNIP..
 ) internal view returns (uint256 limitAmount) {
   ..SNIP..
        if (tradeType == TradeType.EXACT_OUT_SINGLE || tradeType == TradeType.EXACT_OUT_BATCH) {
..SNIP..
        } else {
            // type(uint32).max means no slippage limit
@>            if (slippageLimit == type(uint32).max) {
                return 0
}
@>             // For exact in trades, limitAmount is the min amount of buyToken the contract
@>             // expects from the DEX
..SNIP..
        }
    }

you can see the comments:

• For exact in trades, limitAmount is the min amount of buyToken the contract expects from the DEX. Link here

The issue is that when slippageLimit == type(uint32).max, the _getLimitAmount() will return zero as the limitAmount(min amount to recieve from the DEX used) to be used for the trade, allowing for the exact in trade to settle at a large slippage.

• This affects the deposits from notional(_depositFromNotional()) and the redemptions from notional(_redeemFromNotional()), as they both call this function(_getLimitAmount()) to get the trade.limit of the exact in trades under the hood.
• Setting the minAmount to zero as the amount to receive from DEXs will leave such trades(exact in trades) prone to sandwich and slippage attacks.

[nevillehuang]

The flow stops at the utilization of executeTradeWithDynamicSlippage(), so I still don't see how this is a slippage issue unless you can prove to me that in somewhere in the codebase in notional, the slippage inputted to _getLimitAmount() is hardcoded to type(uint32).max. If not, this is simply users given choice to set their own input slippages.

[shealtielanz]

Thank you @nevillehuang
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/contracts/vaults/common/StrategyUtils.sol#L145

      (amountSold, amountBought) = trade._executeTradeWithDynamicSlippage(
            params.dexId, tradingModule, uint32(params.oracleSlippagePercentOrLimit)

The slippageLimit on the call is the oracleSlippagePercentOrLimit you can see the comment on executeTradeWithDynamicSlippage()

• Executes a trade that uses a dynamic slippage amount relative to the current oracle price. Link here

Taking a look at the SingleSidedLPVaultBase.sol Interface.
https://github.com/sherlock-audit/2023-10-notional/blob/7aadd254da5f645a7e1b718e7f9128f845e10f02/leveraged-vaults/interfaces/notional/ISingleSidedLPStrategyVault.sol#L13C1-L15C42

/// @notice Parameters for trades
struct TradeParams {
    /// @notice DEX ID
    uint16 dexId;
    /// @notice Trade type (i.e. Single/Batch)
    TradeType tradeType;
@audit read the comment below.
    /// @notice For dynamic trades, this field specifies the slippage percentage relative to
    /// the oracle price. For static trades, this field specifies the slippage limit amount.
@>    uint256 oracleSlippagePercentOrLimit;
    /// @notice DEX specific data
    bytes exchangeData;
}

The slippage limit is calculated based on the oracle price as of then, and in a situation where the oracleSlippagePercentOrLimit is calculated to be type(uint32).max, _getLimitAmount() will return zero as the trade.limit

[nevillehuang]

My apologies, what I meant was there will be no instance that oracleSlippagePercentOrLimit will be equal to uint32, given it is represented as a percentage and the maximum is 1e8 (100%). I have consulted @jeffywu to see if this is intended functionality.

[nevillehuang]

Yes, because it is users choice as to whether or not they want slippage to be disabled, and this is not forced upon them at all, so this would constitute users responsibility to input the correct slippage they desire.

Sure I'm happy to hear what sponsor has to say about this, but I'm quite certain of my stance here.

[Czar102]

Result:
Invalid
Unique

I fully agree with the Lead Judge here.

[nevillehuang]

Out of Scope, issue does not link to how an inscope contract can be affected by finding found in an out of scope contract, and it is not the judges responsibility to help watson do so

[sherlock-admin2]

Escalate

concerning

Out of Scope, issue does not link to how an inscope contract can be affected by finding found in an out of scope contract, and it is not the judges responsibility to help watson do so

Based on impact shown.

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

This library is clearly used under the hood whenever performing trades through out every vault.

used when executing dynamic trades in StrategyUtils(which is in scope) where _getLimitAmount() of tradingUtils is call by getLimitAmount of trading module which is used to get the trading limit amount during dynamic trades executeTradeWithDynamicSlippage() that is called through out StrategyUtils that is used by the vaults, links here here here

This issue is a valid med atleast, as all the exact in trades are prone to extreme slippage attacks during the conditions stated in the primary report

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[shealtielanz]

Something to note :
https://x.com/pashovkrum/status/1732351431884112339?s=46

The severity of this issue should be High, as it allows for loss funds to the max amount when ever this kind of trades are performed.

[nevillehuang]

While the contracts in scope uses these functions, your original submission still lack description of how it will impact contracts in scope.

I suggest you follow the contract flow (it is tedious) and point out to me where exactly a 0 slippage could take place that is not a user input.

[nevillehuang]

@shealtielanz

That is representing a percent not an absolute amount for slippage limit as seen in calculations in _getLimitAmount()here. Since SLIPPAGE_LIMIT_PRECISION here is represented by 1e8 that is 100%, there will be no instance that oracleSlippagePercentOrLimit will exceed uint32 during a dynamic trade.

[shealtielanz]

it doesn't need to exceed uint32 to return zero

   } else {
            // type(uint32).max means no slippage limit
            if (slippageLimit == type(uint32).max) {
                return 0;
            }

the code you referenced will only be excuted for exact out trades and not exact in trades.

[shealtielanz]

I believe this is an edge case and that was the reason it was handled by the developers here however it will lead to issues as stated in the main report.

[nevillehuang]

Since this is a user input percentage and not hardcoded anywhere else in the protocol, I don't see any issue in this finding, other than the fact that the user do not have the optionality to disable slippage during a dynamic trade
(this is the intended functionality I am referring to).

[shealtielanz]

This is literally setting the min amount to get from the DEX as zero when it is type(uint32).max and you’re saying you don’t see any issues, this allows for slippage, front running and sandwich attacks where the trade can return up to the minimum of zero tokens for a trade with the DEX

please I request @jeffywu addresses this matter before resolving this escalation

[shealtielanz]

Hey @nevillehuang
I see your point clearly, however users should never be allowed to be exposed to this kind of extreme slippage, as it allows for loss of funds and due to how MEV is rampant all the protocols I’ve seen never allow for zero as the min amount out from a trade, as it is widely advised against in any scenario.
And also to say if the user inputs type(uint32).max as the slippage params, It wouldn’t be to accept 0 min amount out from the trade, hence a bug.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• shealtielanz: rejected

[shealtielanz]

I do not agree with this you have done @Czar102
We were waiting on the sponsors review of the issue.