Certain functions in the Router don't validate if the msg.value == amount when dealing with native token

Summary

Even though the Valantis modules don't work with native ETH at the moment, the public vault router is equipped to handle native ETH for future modules who will support it. There are multiple instances in the router in which no validation is made whether the msg.value provided by the caller corresponds to the amount0 / amount1.

This can present a problem during addition of liquidity as well as swaps since native tokens are provided with push mechanics (i.e. they're provided in the msg.value) where ERC20 tokens can be taken out with pull mechanics (safeTransferFrom), and if the owner doesn't have enough ERC20 tokens or hasn't given sufficient approval, these can revert, but there's no way to control this with native tokens.

Vulnerability Detail

As we can see there are already present instances in which this check is performed to make sure that enough msg.value is provided:


   if (
            token0 == nativeToken && params_.addData.amount0Max > 0
                && msg.value != params_.addData.amount0Max
        ) {
            revert NotEnoughNativeTokenSent();
        }

Although, besides this, we have no such checks in addLiquidity, wrapAndAddLiquidity, as well as addLiquidityPermit2, swapAndAddLiquidityPermit2, there might be other instances of this as well.

Let's take _addLiquidity for example, since no validation is performed whether the msg.value is equal to either amount0 or amount1:


 if (token0_ != nativeToken) {
            IERC20(token0_).safeIncreaseAllowance(module, amount0_);
            balance0 = IERC20(token0_).balanceOf(address(this));
        } else {
            valueToSend = amount0_;
            balance0 = address(this).balance;
        }
        if (token1_ != nativeToken) {
            IERC20(token1_).safeIncreaseAllowance(module, amount1_);
            balance1 = IERC20(token1_).balanceOf(address(this));
        } else {
            valueToSend = amount1_;
            balance1 = address(this).balance;
        }

        IArrakisMetaVaultPublic(vault_).mint{value: valueToSend}(
            shares_, receiver_
        );

And further in the mint function we can see that the share is based on the user-inputted amounts (calculated in the previous step based on the amount0/1 function arguments), as well as the valueToSend which is the amount0 is sent to to the mint function, if the contract holds more native token than what we've provided, it never validates whether the user supplied the msg.value but rather forwards the amount that user specified to the Public vault contract for minting shares.

Impact

Malicious users can mint more shares than they are entitled when adding liquidity when one of the pairs is the native token, this can happen if the router contract has more native token than we've supplied (i.e. has enough to cover the user-inputted amount)

Code Snippet

https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L869-L901

Tool used

Manual Review

Recommendation

Perform enough validations to make sure that msg.value equals to the amount0/1 which the user inputted.

sherlock-audit / 2024-03-arrakis-judging

iamandreiski - Certain functions in the Router don't validate if the msg.value == amount when dealing with native token #41