Vagner - depositFromNotional function is payable, which means that it should accept Ether, but in reality will revert 100% when msg.value > 0

[sherlock-admin2]

Vagner

high

depositFromNotional function is payable, which means that it should accept Ether, but in reality will revert 100% when msg.value > 0

Summary

The function depositFromNotional used in BaseStrategyVault.sol is payable, which means that it should accept Ether, but in reality it will revert every time when msg.value is > than 0 in any of existing strategy.

Vulnerability Detail

depositFromNotional is a function used in BaseStrategyVault.sol for every strategy, to deposit from notional to a specific strategy. As you can see this function has the payable keyword
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L166-L173
which means that it is expected to be used along with msg.value being > than 0. This function would call _depositFromNotional which is different on any strategy used, but let's take the most simple case, since all of them will be the same in the end, the case of CrossCurrencyVault.sol. In CrossCurrencyVault.sol , _depositFromNotional would later call _executeTrade
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyVault.sol#L184
which would use the TradeHandler library as can be seen here
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/vaults/common/BaseStrategyVault.sol#L124
If we look into the TradeHandler library, _executeTrade would delegatecall into the implementation of TradingModule.sol to executeTrade function, as can be seen here
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradeHandler.sol#L41-L42
If we look into the executeTrade function in TradingModule.sol we can see that this function does not have the payable, keyword, which mean that it will not accept msg.value > than 0
https://github.com/sherlock-audit/2023-10-notional/blob/main/leveraged-vaults/contracts/trading/TradingModule.sol#L169-L193
The big and important thing to know about delegate call is that, msg.sender and msg.value will always be kept when you delegate call, so the problem that arise here is the fact that , the calls made to depositFromNotional with msg.value > 0 , would always revert when it gets to this delegatecall, in every strategy, since the function that it delegates to , doesn't have the payable keyword, and since msg.value is always kept trough the delegate calls, the call would just revert. This would be the case for all the strategies since they all uses _executeTrade or _executeTradeWithDynamicSlippage in a way or another, so every payable function that would use any of these 2 functions from the TradeHandler.sol library would revert all the time, if msg.value is > 0.

Impact

Impact is a high one, since strategy using pools that need to interact with Ether would be useless and the whole functionality would be broken.

Code Snippet

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

Tool used

Manual Review

Recommendation

There are multiple solution to this, the easiest one is to make the function executeTrade in TradingModule.sol payable, so the delegate call would not revert when msg.value is greater than 0, or if you don't intend to use ether with depositFromNotional, remove the payable keyword. It is important to take special care in those delegate calls since they are used multiple times in the codebase, and could mess up functionalities when msg.value is intended to be used.

[jeffywu]

Trade Handler is executed in a delegate call context, I don't really follow this issue.

[VagnerAndrei26]

Escalate
I want to escalate this issue and explain it a bit better so it can be understood. The core of this issue stands at the base of how delegatecall works. The big difference between normal call and delegatecall is , as I said , msg.sender and msg.value are kept within the call. Here is the source code of EVM

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

basically what this means is that, anytime when you have a function that is payable, and implicitly has msg.value > 0 , that msg.value will always be passed in a delegatecall. In the case of normal call you can choose if you want to pass some value by using { value: }, after calling the function in a contract, but for delegatecall you can't choose, so what will happen is that, any time you do a delegatecall inside a function that has payable and where msg.value is > 0, that value is always passed in that delegatecall and if the function called does not have the payable keyword, the transaction will always revert, the same as when you are trying to do a simple call to a contract without having a receive function or payable keyword on a function. So in our case, because _executeTrade or _executeTradeWithDynamicSlippage does not have the payable keyword, any call on depositFromNotional with msg.value > 0 , would revert all the time, making the vaults in the cases where ETH is used useless.
Also issue #8 is not a duplicate of this, since it would fall more to this duplicate rule of sherlock which states

since the report identifies the core issue, which is lacking the payable keyword but doesn't correctly identifies the reason behind that being an issue or the impact.

[sherlock-admin2]

Escalate
I want to escalate this issue and explain it a bit better so it can be understood. The core of this issue stands at the base of how delegatecall works. The big difference between normal call and delegatecall is , as I said , msg.sender and msg.value are kept within the call. Here is the source code of EVM

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

basically what this means is that, anytime when you have a function that is payable, and implicitly has msg.value > 0 , that msg.value will always be passed in a delegatecall. In the case of normal call you can choose if you want to pass some value by using { value: }, after calling the function in a contract, but for delegatecall you can't choose, so what will happen is that, any time you do a delegatecall inside a function that has payable and where msg.value is > 0, that value is always passed in that delegatecall and if the function called does not have the payable keyword, the transaction will always revert, the same as when you are trying to do a simple call to a contract without having a receive function or payable keyword on a function. So in our case, because _executeTrade or _executeTradeWithDynamicSlippage does not have the payable keyword, any call on depositFromNotional with msg.value > 0 , would revert all the time, making the vaults in the cases where ETH is used useless.
Also issue #8 is not a duplicate of this, since it would fall more to this duplicate rule of sherlock which states

since the report identifies the core issue, which is lacking the payable keyword but doesn't correctly identifies the reason behind that being an issue or the impact.

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.

[AuditorPraise]

Escalate I want to escalate this issue and explain it a bit better so it can be understood. The core of this issue stands at the base of how delegatecall works. The big difference between normal call and delegatecall is , as I said , msg.sender and msg.value are kept within the call. Here is the source code of EVM

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

basically what this means is that, anytime when you have a function that is payable, and implicitly has msg.value > 0 , that msg.value will always be passed in a delegatecall. In the case of normal call you can choose if you want to pass some value by using { value: }, after calling the function in a contract, but for delegatecall you can't choose, so what will happen is that, any time you do a delegatecall inside a function that has payable and where msg.value is > 0, that value is always passed in that delegatecall and if the function called does not have the payable keyword, the transaction will always revert, the same as when you are trying to do a simple call to a contract without having a receive function or payable keyword on a function. So in our case, because _executeTrade or _executeTradeWithDynamicSlippage does not have the payable keyword, any call on depositFromNotional with msg.value > 0 , would revert all the time, making the vaults in the cases where ETH is used useless. Also issue #8 is not a duplicate of this, since it would fall more to this duplicate rule of sherlock which states since the report identifies the core issue, which is lacking the payable keyword but doesn't correctly identifies the reason behind that being an issue or the impact.

LoL, how is #8 a low? Because i didn't use the same exact words you used when describing your issue?
#8 's 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.

Its 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()

And its impact: vaults will be unable to execute trades on external exchanges via the trading module whenever ETH is the sell Token`

How are they different?
#8 's impact is clearly described and i don't see how it's different from your described impact because they're both implying the same thing
Anyways thanks for escalating sir

[VagnerAndrei26]

Escalate I want to escalate this issue and explain it a bit better so it can be understood. The core of this issue stands at the base of how delegatecall works. The big difference between normal call and delegatecall is , as I said , msg.sender and msg.value are kept within the call. Here is the source code of EVM

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

basically what this means is that, anytime when you have a function that is payable, and implicitly has msg.value > 0 , that msg.value will always be passed in a delegatecall. In the case of normal call you can choose if you want to pass some value by using { value: }, after calling the function in a contract, but for delegatecall you can't choose, so what will happen is that, any time you do a delegatecall inside a function that has payable and where msg.value is > 0, that value is always passed in that delegatecall and if the function called does not have the payable keyword, the transaction will always revert, the same as when you are trying to do a simple call to a contract without having a receive function or payable keyword on a function. So in our case, because _executeTrade or _executeTradeWithDynamicSlippage does not have the payable keyword, any call on depositFromNotional with msg.value > 0 , would revert all the time, making the vaults in the cases where ETH is used useless. Also issue #8 is not a duplicate of this, since it would fall more to this duplicate rule of sherlock which states since the report identifies the core issue, which is lacking the payable keyword but doesn't correctly identifies the reason behind that being an issue or the impact.

LoL, how is #6 a low? Because i didn't use the same exact words you used when describing your issue? #6 's 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.

Its 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()

And its impact: vaults will be unable to execute trades on external exchanges via the trading module whenever ETH is the sell Token`

How are they different? #6 's impact is clearly described and i don't see how it's different from your described impact because they're both implying the same thing Anyways thanks for escalating sir

Hey, I didn't say that it was a low personally, I said that it is not a duplicate of this, and should be considered as the duplicate rule stated, if it is kept as a duplicate. The big difference is that the report failed to find the real problem of this whole issue, the delegatecall. The main important aspect of this issue is the fact that those functions, that were also specified #8, are used in a delegatecall and not in a normal call, if it were done in a normal call, this would have not be a problem since you can select if you want to transfer msg.value with the call or not. Also the report speaks about redeemFromNotional and reinvestReward having the same issue, but that is not correct since they are not payable, so no msg.value is expected to be used when calling those, the only real function affected by this issue is depositFromNotional. Also in the report it is also talking about TradingModule not being able to receive ether which is not a problem again, since the main usage of TradingModule is to be used more as an implementation, where calls could be delegated to. That's the reasons behind my arguments, the only thing which the report accurate is the fact the those function lacks the payable keyword, but fails to explain the real problem behind, or why is that a problem in the current code.

[AuditorPraise]

Escalate I want to escalate this issue and explain it a bit better so it can be understood. The core of this issue stands at the base of how delegatecall works. The big difference between normal call and delegatecall is , as I said , msg.sender and msg.value are kept within the call. Here is the source code of EVM

File: core\vm\contract.go
134: func (c *Contract) AsDelegate() *Contract {
135: 	// NOTE: caller must, at all times be a contract. It should never happen
136: 	// that caller is something other than a Contract.
137: 	parent := c.caller.(*Contract)
138: 	c.CallerAddress = parent.CallerAddress
139: 	c.value = parent.value
140: 
141: 	return c
142: }

basically what this means is that, anytime when you have a function that is payable, and implicitly has msg.value > 0 , that msg.value will always be passed in a delegatecall. In the case of normal call you can choose if you want to pass some value by using { value: }, after calling the function in a contract, but for delegatecall you can't choose, so what will happen is that, any time you do a delegatecall inside a function that has payable and where msg.value is > 0, that value is always passed in that delegatecall and if the function called does not have the payable keyword, the transaction will always revert, the same as when you are trying to do a simple call to a contract without having a receive function or payable keyword on a function. So in our case, because _executeTrade or _executeTradeWithDynamicSlippage does not have the payable keyword, any call on depositFromNotional with msg.value > 0 , would revert all the time, making the vaults in the cases where ETH is used useless. Also issue #8 is not a duplicate of this, since it would fall more to this duplicate rule of sherlock which states since the report identifies the core issue, which is lacking the payable keyword but doesn't correctly identifies the reason behind that being an issue or the impact.

LoL, how is #6 a low? Because i didn't use the same exact words you used when describing your issue? #6 's 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.
Its 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()
And its impact: vaults will be unable to execute trades on external exchanges via the trading module whenever ETH is the sell Token`
How are they different? #6 's impact is clearly described and i don't see how it's different from your described impact because they're both implying the same thing Anyways thanks for escalating sir

Hey, I didn't say that it was a low personally, I said that it is not a duplicate of this, and should be considered as the duplicate rule stated, if it is kept as a duplicate. The big difference is that the report failed to find the real problem of this whole issue, the delegatecall. The main important aspect of this issue is the fact that those functions, that were also specified #8, are used in a delegatecall and not in a normal call, if it were done in a normal call, this would have not be a problem since you can select if you want to transfer msg.value with the call or not. Also the report speaks about redeemFromNotional and reinvestReward having the same issue, but that is not correct since they are not payable, so no msg.value is expected to be used when calling those, the only real function affected by this issue is depositFromNotional. Also in the report it is also talking about TradingModule not being able to receive ether which is not a problem again, since the main usage of TradingModule is to be used more as an implementation, where calls could be delegated to. That's the reasons behind my arguments, the only thing which the report accurate is the fact the those function lacks the payable keyword, but fails to explain the real problem behind, or why is that a problem in the current code.

The delegateCall is not the main issue bro. The lack of a payable keyword on the tradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage() is the main issue.
Because pools that need to interact with Ether would be useless due to the missing payable keyword on those external functions that are supposed to interact with ETH(i.e, tradingModule.executeTrade() and TradingModule.executeTradeWithDynamicSlippage()) and the whole functionality would be broken.

Msg.value will always be > 0 whenever the token being transferred is ETH

[nevillehuang]

Hi @VagnerAndrei26 @AuditorPraise can any of you provide me a coded test file proving the issue? Imo if it is 100% going to revert, should be easy enough to proof it, appreciate your assistance.

[VagnerAndrei26]

Hi @VagnerAndrei26 @AuditorPraise can any of you provide me a coded test file proving the issue? Imo if it is 100% going to revert, should be easy enough to proof it, appreciate your assistance.

Yeah I can, I can show a simpler implementation of 2 contracts doing some delegatecalls, I am 100% sure on this issue, because I got paid for it multiple times already and it got confirmed in other contest too. Will provide the simple example later today, after I get home.

[AuditorPraise]

Hello bro @nevillehuang, i just wrote a quick implementation of 2 contracts doing some delegatecalls on remix.
just copy and paste on remix

// SPDX-License-Identifier: GPL-3.0

pragma solidity =0.8.18;

/**
 * @hypothesis
 * @dev just for research.
 */
import "hardhat/console.sol";

contract ContractA {
    receive() external payable {}
    address public contractBAddress;

    event EtherTransferred(address indexed from, address indexed to, uint256 amount);

    constructor(address _contractBAddress) {
        contractBAddress = _contractBAddress;
    }

    function delegateCallToContractB(uint256 amount) payable external {
        // Use delegatecall to execute the transferEther function in ContractB
        (bool success, ) = payable(contractBAddress).delegatecall(
            abi.encodeWithSignature("transferEther(address,uint256)", msg.sender, amount)
        );

        require(success, "Delegate call to ContractB failed");

        console.log("trf was successful");
    }
}

contract ContractB {
    receive() external payable {}

    function transferEther(address recipient, uint256 amount) 
    external {
        //@audit-issue the delegate call from contract A fails without the payable keyword here

       
    }
}

So this is supposed to show the issue we are talking about.
when contractB.transferEther()'s function has the payable keyword the delegate call from contract A succeeds BUT when its not there it reverts.

The code above should revert, then add the payable keyword to contractB.transferEther() you'll see that contractA.delegateCallToContractB() succeeds.

Here's an article that may be of help with sending Ether on remix How do you send Ether as a function to a contract using Remix?

[shealtielanz]

The 2 reports are duplicates IMO, as the root issues is stated in both of them, and their mitigations solves the issues well.

[nevillehuang]

@jeffywu Is there a foundry test revolving this issue that shows it will not revert if u pass in a msg.value? I am really surprised if this is true that a test would not have caught this.

Since the PoC provided is not protocol specific, I will have to double check before making any final comment.

[VagnerAndrei26]

@jeffywu Is there a foundry test revolving this issue that shows it will not revert if u pass in a msg.value? I am really surprised if this is true that a test would not have caught this.

Since the PoC provided is not protocol specific, I will have to double check before making any final comment.

The example provided from @AuditorPraise is good, it is pretty similar to what I wanted to present. The bases of this issue is simple, if you have a delegatecall inside a payable function, the function which you delegatecall to needs to have payable also, otherwise it reverts anytime when msg.value > 0.

[Czar102]

Awaiting a protocol-specific PoC @VagnerAndrei26 and a comment from @jeffywu.

[VagnerAndrei26]

Awaiting a protocol-specific PoC @VagnerAndrei26 and a comment from @jeffywu.

Hey @Czar102 it was kinda hard to do it, because of the complex codebase and test, but here is the protocol specific POC.
What I did was modifying this specific test https://github.com/sherlock-audit/2023-10-notional-VagnerAndrei26/blob/ebaf16143f4d71d84affb69258bcf00c375628d3/leveraged-vaults/tests/BaseAcceptanceTest.sol#L173-L190
by setting the isETH to true firstly, and giving the ETH to the NOTIONAL address instead of the vault itself. After that I was calling the depositFromNotional with a specific value, which reverts all the time when it gets to executeTrade. The modifications would look like this

    function enterVaultBypass(
        address account,
        uint256 depositAmount,
        uint256 maturity,
        bytes memory data
    ) internal virtual returns (uint256 vaultShares) {
        vm.prank(address(NOTIONAL));
        deal(address(NOTIONAL), depositAmount);
        vaultShares = vault.depositFromNotional{value : depositAmount}(account, depositAmount, maturity, data);
        totalVaultShares[maturity] += vaultShares;
        totalVaultSharesAllMaturities += vaultShares;
    }

and here is the trace logs, as you can see it revert at executeTrade, as I expected and explained in the report

Running 1 test for tests/CrossCurrency/testCrossCurrencyVault.t.sol:TestCrossCurrency_ETH_WSTETH
[FAIL. Reason: TradeFailed(); counterexample: calldata=0xde6f757a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 args=[0, 0]] test_EnterVault(uint256,uint256) (runs: 0, μ: 0, ~: 0)
Logs:
  Bound Result 0
  Bound Result 1000000000000000

Traces:
  [66058] TestCrossCurrency_ETH_WSTETH::test_EnterVault(0, 0)
    ├─ [0] VM::addr(97821013814920132828181473597319662187411378784436296118592727588725089953873 [9.782e76]) [staticcall]
    │   └─ ← account: [0x359e534d4C79745c3c0A0BC80d80cfAe9e82699e]
    ├─ [0] VM::label(account: [0x359e534d4C79745c3c0A0BC80d80cfAe9e82699e], "account")
    │   └─ ← ()
    ├─ [0] console::log("Bound Result", 0) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log("Bound Result", 1000000000000000 [1e15]) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::prank(0x1344A36A1B56144C3Bc62E7757377D288fDE0369)
    │   └─ ← ()
    ├─ [0] VM::deal(0x1344A36A1B56144C3Bc62E7757377D288fDE0369, 1000000000000000 [1e15])
    │   └─ ← ()
    ├─ [26272] nBeaconProxy::depositFromNotional{value: 1000000000000000}(account: [0x359e534d4C79745c3c0A0BC80d80cfAe9e82699e], 1000000000000000 [1e15], 1099511627775 [1.099e12], 0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000200000000000000000000000006eb2dc694eb516b16dc9fbc678c60052bbdd7d80)
    │   ├─ [2308] nUpgradeableBeacon::implementation() [staticcall]
    │   │   └─ ← CrossCurrencyVault: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]
    │   ├─ [16104] CrossCurrencyVault::depositFromNotional(account: [0x359e534d4C79745c3c0A0BC80d80cfAe9e82699e], 1000000000000000 [1e15], 1099511627775 [1.099e12], 0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000200000000000000000000000006eb2dc694eb516b16dc9fbc678c60052bbdd7d80) [delegatecall]
    │   │   ├─ [2335] 0xBf6B9c5608D520469d8c4BD1E24F850497AF0Bb8::getImplementation() [staticcall]
    │   │   │   └─ ← 0x0000000000000000000000002de2b1eecf5bab0add9147ebbb999395238d30a5
    │   │   ├─ [213] 0x2De2B1Eecf5bab0adD9147ebBb999395238d30a5::executeTrade(7, (0, 0x0000000000000000000000000000000000000000, 0x5979D7b546E38E414F7E9822514be443A4800529, 1000000000000000 [1e15], 0, 1699104713 [1.699e9], 0x0000000000000000000000006eb2dc694eb516b16dc9fbc678c60052bbdd7d80)) [delegatecall]
    │   │   │   └─ ← EvmError: Revert
    │   │   └─ ← TradeFailed()
    │   └─ ← TradeFailed()
    └─ ← TradeFailed()

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 712.96ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/CrossCurrency/testCrossCurrencyVault.t.sol:TestCrossCurrency_ETH_WSTETH
[FAIL. Reason: TradeFailed(); counterexample: calldata=0xde6f757a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 args=[0, 0]] test_EnterVault(uint256,uint256) (runs: 0, μ: 0, ~: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

and here is the case where the ETH is transferred to the vault itself, and then depositFromNotional is called

    function enterVaultBypass(
        address account,
        uint256 depositAmount,
        uint256 maturity,
        bytes memory data
    ) internal virtual returns (uint256 vaultShares) {
        vm.prank(address(NOTIONAL));
        deal(address(vault), depositAmount);
        vaultShares = vault.depositFromNotional(account, depositAmount, maturity, data);
        totalVaultShares[maturity] += vaultShares;
        totalVaultSharesAllMaturities += vaultShares;
    }

and here is the trace call, where the call succeded

Running 1 test for tests/CrossCurrency/testCrossCurrencyVault.t.sol:TestCrossCurrency_ETH_WSTETH
[PASS] test_EnterVault(uint256,uint256) (runs: 256, μ: 843050, ~: 982549)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.51s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The reason that they didn't find that in the test case is because they were doing the tests in the second way, by transferring the ETH to the Vault with deal instead setting a value to the call itself, that would be the correct way to test a payable function.

[Czar102]

Interesting, thank you @VagnerAndrei26.

What is the loss of funds in this scenario?

[VagnerAndrei26]

Interesting, thank you @VagnerAndrei26.

What is the loss of funds in this scenario?

This does not fall under loss of funds here, but more towards the contract rendered useless, since the main functionality, the deposit one, would not work, especially for the pools that have ETH, like the one I provided in the test file.

[jeffywu]

@VagnerAndrei26 appreciate the follow up on this issue, I agree it is indeed valid. Good find. I was able to reproduce this below and put a fix in:
notional-finance/leveraged-vaults#80

[Czar102]

The reason that they didn't find that in the test case is because they were doing the tests in the second way, by transferring the ETH to the Vault with deal instead setting a value to the call itself, that would be the correct way to test a payable function.

It seems to me that the same is possible using another way. Also, there is no loss of funds, only loss of functionality. This is a really good find, but per Sherlock rules, I don't think I can make it a medium. Planning to leave it a low severity issue. Appreciate the escalation.

[nevillehuang]

Hi @Czar102 based on this comment by @VagnerAndrei26 I think this qualifies for a medium on grounds on a permanent DoS on a desired functionality no?

This does not fall under loss of funds here, but more towards the contract rendered useless, since the main functionality, the deposit one, would not work, especially for the pools that have ETH, like the one I provided in the test file.

What is the possible other way you are referring to?

[VagnerAndrei26]

The reason that they didn't find that in the test case is because they were doing the tests in the second way, by transferring the ETH to the Vault with deal instead setting a value to the call itself, that would be the correct way to test a payable function.

It seems to me that the same is possible using another way. Also, there is no loss of funds, only loss of functionality. This is a really good find, but per Sherlock rules, I don't think I can make it a medium. Planning to leave it a low severity issue. Appreciate the escalation.

Hey @Czar102 , there is not really other ways of doing it. The only way of doing it is passing msg.value since it is a function that can only be called from Notional main contract, not by users anytime. They were doing the test in other ways which was not correct, but they fixed the tests also with the fix provided above. Also most of the time, when a contract is rendered useless because of an important functionality, it was considered a medium most of the time on sherlock since it can be based on this rule

since the protocol needs to redeploys most of the contract that were rendered useless. Also it was clearly their intention of working with those pools, and interacting with ether, but because of an important bug it is not working. Personally I can't consider it fair for this to be low/informational, since rendering a contract useless and forcing a redeployments with increased costs to the protocol is not something that should be considered low.
Here is one example of a recent contest where, because of an issue inside, the contract was rendering it useless to work with multiple important pools from Curve, which is a similar issue with this one
sherlock-audit/2023-07-blueberry-judging#105

[AuditorPraise]

The reason that they didn't find that in the test case is because they were doing the tests in the second way, by transferring the ETH to the Vault with deal instead setting a value to the call itself, that would be the correct way to test a payable function.

It seems to me that the same is possible using another way. Also, there is no loss of funds, only loss of functionality. This is a really good find, but per Sherlock rules, I don't think I can make it a medium. Planning to leave it a low severity issue. Appreciate the escalation.

Hello boss @Czar102,

There's no other way to send ETH other than having msg.value > 0. IMO i think this qualifies as a MED due to permanent Dos on a desired functionality.

[Czar102]

Hi, that's right, sorry for my misunderstanding and the confusion because of it. I see how the test didn't fully check the functionality. It's a great find.
I think it's a valid medium based on the fact that core functionality is not working at all.

Planning to accept the escalation and make the issue a valid medium.

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• VagnerAndrei26: accepted

[MLON33]

Fix: notional-finance/leveraged-vaults#80

[xiaoming9090]

Fixed in PR 80