sherlock-audit / 2024-03-arrakis-judging

2 stars 2 forks source link

0xlookman - 0xlookman - `ArrakisPublicVaultRouter.sol::wrapAndSwapAndAddLiquidity` most likely to revert hence denying users this service. #63

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

0xlookman

medium

0xlookman - ArrakisPublicVaultRouter.sol::wrapAndSwapAndAddLiquidity most likely to revert hence denying users this service.


0xlookman

Medium

ArrakisPublicVaultRouter.sol::wrapAndSwapAndAddLiquidity most likely to revert hence denying users this service.


Summary

The function ArrakisPublicVaultRouter.sol::wrapAndSwapAndAddLiquidity requires that the non weth token's params_.addData.amountMax to be equal to msg.value which is most likely to be false hence making the function to revert.

Vulnerability Detail

Users of public vaults are supposed to be able wrap eth, swap and add Liquidity directly using the ArrakisPublicVaultRouter.sol::wrapAndSwapAndAddLiquidity function.

But there is a check in this function which would make it impossible and it is most likely to revert.

if (token0 != address(weth)) {
            if (params_.addData.amount0Max > 0) {
                IERC20(token0).safeTransferFrom(
                    msg.sender,
                    address(this),
                    params_.addData.amount0Max
                );
            }
        } else if (params_.addData.amount0Max != msg.value) {
            revert MsgValueDTMaxAmount();
        }
        if (token1 != address(weth)) {
            if (params_.addData.amount1Max > 0) {
                IERC20(token1).safeTransferFrom(
                    msg.sender,
                    address(this),
                    params_.addData.amount1Max
                );
            }
        } else if (params_.addData.amount1Max != msg.value) {
            revert MsgValueDTMaxAmount();
        }

Since the other token is not weth and is of a different value with a different number of decimals and supply, it is more likely that its params_.addData.amountMax is of a different quantity to the msg.value.

This makes the above check having high chances of reverting with a MsgValueDTMaxAmount() error.

Impact

Denial of service where users will not be able to wrap eth, swap and add Liquidity directly using this function.

Code Snippet

https://github.com/ArrakisFinance/arrakis-modular/blob/395fa728f6a2fc39ff30f55585ca1d191f56d7e3/src/ArrakisPublicVaultRouter.sol#L571

Tool used

Manual Review

Recommendation

Replace the above code of the function with this one:-

if (
            token0 == address(weth)
                && params_.swapAndAddData.addData.amount0Max != msg.value
        ) {
            revert MsgValueDTMaxAmount();
        }
        if (
            token1 == address(weth)
                && params_.swapAndAddData.addData.amount1Max != msg.value
        ) {
            revert MsgValueDTMaxAmount();
        }
Zinda-Lukman commented 3 months ago

Equating two tokens of different values is most likely to be false.

A user cannot estimate what the token 1 will be with the value of another token 2.

Hence this function is most likely to revert leading to a denial of service.

@sherlock-admin2