GREY-HAWK-REACH - Functionality of multicall is seriously limited due to usage of msg.value and missing payable modifiers

[sherlock-admin2]

GREY-HAWK-REACH

medium

Functionality of multicall is seriously limited due to usage of msg.value and missing payable modifiers

Summary

Missing payable modifier in LMPVaultRouterBase#redeem and #withdraw, alongside with using msg.value over address(this).balance in #_processEthIn, significantly restrict the utility of multicall.

Vulnerability Detail

LMPVaultRouterBase inherits Multicall implementation forked from Uniswap V3. Uni's multicall is payable and it arises an obvious issue of "msg.value in a loop", which in Uniswap's contracts is resolved by:

1. Using address(this).balance (instead of msg.value) for native-token operations.
2. Adding payable to every function that is expected to be multicalled - to prevent these functions from reverting on the require(msg.value == 0) check that every non-payable function has under the hood.

https://github.com/Uniswap/v3-periphery/tree/main/contracts

Uniswap/v3-periphery#52

Impact

Tokemak's implementation lacks both of the aforementioned mitigations:

1. LMPVaultRouterBase#withdraw and #redeem do not use payable modifier.
2. LMPVaultRouterBase#_processEthIn relies on msg.value.

With msg.value > 0, attempts to multicall non-payable withdraw/redeem together with payable deposit/mint will revert. Multicalls that have more than one deposit or mint in total will also revert.

Examples:

Multicall A (msg.value == 1e18):

1. deposit - succeeds.
2. withdraw - reverts because the function is not payable and msg.value > 0.

Multicall B (msg.value == 1e18):

1. mint - succeeds.

2. deposit - reverts in line 120, because msg.value has already been used by the mint:

       weth9.deposit{ value: msg.value }();
   

Code Snippet

LMPVaultRouterBase.sol#L73-L79
LMPVaultRouterBase.sol#L93-L99
LMPVaultRouterBase.sol#L111-L122

Tool used

Manual Review

Recommendation

1. Add payable modifier to LMPVaultRouterBase#redeem and #withdraw.

2. Change msg.value to address(this).balance in _processEthIn:

    function _processEthIn(ILMPVault vault) internal {
        // if any eth sent, wrap it first
-       if (msg.value > 0) {
+       if (address(this).balance > 0) {
            // if asset is not weth, revert
            if (address(vault.asset()) != address(weth9)) {
                revert InvalidAsset();
            }

            // wrap eth
-           weth9.deposit{ value: msg.value }();
+           weth9.deposit{ value: address(this).balance }();
        }
    }

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

low, user can't use the multicall but no fund are at risk