KungFuPanda - Malicious actors can temporarily DoS user's access to the ArrakisPublicVaultRouter contract's permit-based functions by frontrunning and calling the UniPermit2 contract with the signatures of those users

[sherlock-admin2]

KungFuPanda

medium

Malicious actors can temporarily DoS user's access to the ArrakisPublicVaultRouter contract's permit-based functions by frontrunning and calling the UniPermit2 contract with the signatures of those users

Summary

As the Trust Security team originally disclosured, the use of the EIP-2612 pattern is quite tricky, because if there're no proper try {} & catch {} blocks implemented, the user's access to the permit-based functions can be easily DoS'ed. Please refer to this talententedly-written article: https://www.trust-security.xyz/post/permission-denied.

Vulnerability Detail

There're 4 publicly exposed permit-based functions in the ArrakisPublicVaultRouter that are using the permit2 contract.

All of these can be DoS'ed by malicious front-running.

The root function call that causes the DoS is inside an internal method, so I'll provide you the full callstack for reference:

1. Either one of these internal functions is called (through these public functions, the internal problematic functions are called)... →

1a) a multi-step swap & add liquidity swapAndAddLiquidity call that can be DoS'ed: ↓

    /// @notice swapAndAddLiquidityPermit2 transfer tokens to and calls RouterSwapExecutor
    /// @param params_ SwapAndAddPermit2Data struct containing data for swap
    /// @return amount0 amount of token0 transferred from msg.sender to mint `mintAmount`
    /// @return amount1 amount of token1 transferred from msg.sender to mint `mintAmount`
    /// @return sharesReceived amount of public vault tokens transferred to `receiver`
    /// @return amount0Diff token0 balance difference post swap
    /// @return amount1Diff token1 balance difference post swap
    function swapAndAddLiquidityPermit2(
        SwapAndAddPermit2Data memory params_
    )
        external
        payable
        nonReentrant
        whenNotPaused
        onlyPublicVault(params_.swapAndAddData.addData.vault)
        returns (
            uint256 amount0,
            uint256 amount1,
            uint256 sharesReceived,
            uint256 amount0Diff,
            uint256 amount1Diff
        )
    {
        if (
            params_.swapAndAddData.addData.amount0Max == 0
                && params_.swapAndAddData.addData.amount1Max == 0
        ) {
            revert EmptyMaxAmounts();
        }

        address token0 = IArrakisMetaVault(
            params_.swapAndAddData.addData.vault
        ).token0();
        address token1 = IArrakisMetaVault(
            params_.swapAndAddData.addData.vault
        ).token1();

@@        _permit2SwapAndAddLengthOneOrTwo(params_, token0, token1); // permit2 call originates from here

        (amount0, amount1, sharesReceived, amount0Diff, amount1Diff) =
        _swapAndAddLiquiditySendBackLeftOver(
            params_.swapAndAddData, token0, token1
        );
    }

1b) An addLiquidity call can be DoS'ed, too: ↓

    /// @notice addLiquidityPermit2 adds liquidity to public vault of interest (mints LP tokens)
    /// @param params_ AddLiquidityPermit2Data struct containing data for adding liquidity
    /// @return amount0 amount of token0 transferred from msg.sender to mint `mintAmount`
    /// @return amount1 amount of token1 transferred from msg.sender to mint `mintAmount`
    /// @return sharesReceived amount of public vault tokens transferred to `receiver`
    function addLiquidityPermit2(
        AddLiquidityPermit2Data memory params_
    )
        external
        payable
        nonReentrant
        whenNotPaused
        onlyPublicVault(params_.addData.vault)
        returns (
            uint256 amount0,
            uint256 amount1,
            uint256 sharesReceived
        )
    {
        // #region checks.
        if (
            params_.addData.amount0Max == 0
                && params_.addData.amount1Max == 0
        ) {
            revert EmptyMaxAmounts();
        }

        (sharesReceived, amount0, amount1) = _getMintAmounts(
            params_.addData.vault,
            params_.addData.amount0Max,
            params_.addData.amount1Max
        );

        if (sharesReceived == 0) revert NothingToMint();

        if (
            amount0 < params_.addData.amount0Min
                || amount1 < params_.addData.amount1Min
                || sharesReceived < params_.addData.amountSharesMin
        ) revert BelowMinAmounts();

        address token0 =
            IArrakisMetaVault(params_.addData.vault).token0();
        address token1 =
            IArrakisMetaVault(params_.addData.vault).token1();

        // #endregion checks.

>>        _permit2AddLengthOneOrTwo(
>>            params_, token0, token1, amount0, amount1
>>        );

        _addLiquidity(
            params_.addData.vault,
            amount0,
            amount1,
            sharesReceived,
            params_.addData.receiver,
            token0,
            token1
        );
    }

1c) A removeAndAddLiquidity permit2-based function can be DoS'ed as well: ↓

    /// @notice removeLiquidityPermit2 removes liquidity from vault and burns LP tokens
    /// @param params_ RemoveLiquidityPermit2Data struct containing data for withdrawals
    /// @return amount0 actual amount of token0 transferred to receiver for burning `burnAmount`
    /// @return amount1 actual amount of token1 transferred to receiver for burning `burnAmount`
    function removeLiquidityPermit2(
        RemoveLiquidityPermit2Data memory params_
    )
        external
        nonReentrant
        whenNotPaused
        onlyPublicVault(params_.removeData.vault)
        returns (uint256 amount0, uint256 amount1)
    {
        if (params_.removeData.burnAmount == 0) {
            revert NothingToBurn();
        }

        SignatureTransferDetails memory transferDetails =
        SignatureTransferDetails({
            to: address(this),
            requestedAmount: params_.removeData.burnAmount
        });
>>        permit2.permitTransferFrom(
>>            params_.permit,
>>            transferDetails,
>>            msg.sender,
>>            params_.signature
>>        );

        (amount0, amount1) = _removeLiquidity(params_.removeData);
    }

1d) *Even the wrapAndAddLiquidity[Permit2] permit2-based function can be DoS'eda at the malicious actor's discretion: ↓*

    /// @notice wrapAndAddLiquidityPermit2 wrap eth and adds liquidity to public vault of interest (mints LP tokens)
    /// @param params_ AddLiquidityPermit2Data struct containing data for adding liquidity
    /// @return amount0 amount of token0 transferred from msg.sender to mint `mintAmount`
    /// @return amount1 amount of token1 transferred from msg.sender to mint `mintAmount`
    /// @return sharesReceived amount of public vault tokens transferred to `receiver`
    function wrapAndAddLiquidityPermit2(
        AddLiquidityPermit2Data memory params_
    )
        external
        payable
        nonReentrant
        whenNotPaused
        onlyPublicVault(params_.addData.vault)
        returns (
            uint256 amount0,
            uint256 amount1,
            uint256 sharesReceived
        )
    {
        if (msg.value == 0) {
            revert MsgValueZero();
        }

        // #region wrap eth.

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

        // #endregion wrap eth.
        // #region checks.
        if (
            params_.addData.amount0Max == 0
                && params_.addData.amount1Max == 0
        ) {
            revert EmptyMaxAmounts();
        }

        (sharesReceived, amount0, amount1) = _getMintAmounts(
            params_.addData.vault,
            params_.addData.amount0Max,
            params_.addData.amount1Max
        );

        if (sharesReceived == 0) revert NothingToMint();

        if (
            amount0 < params_.addData.amount0Min
                || amount1 < params_.addData.amount1Min
                || sharesReceived < params_.addData.amountSharesMin
        ) revert BelowMinAmounts();

        address token0 =
            IArrakisMetaVault(params_.addData.vault).token0();
        address token1 =
            IArrakisMetaVault(params_.addData.vault).token1();

        if (token0 == nativeToken || token1 == nativeToken) {
            revert NativeTokenNotSupported();
        }
        if (token0 != address(weth) && token1 != address(weth)) {
            revert NoWethToken();
        }

        // #endregion checks.

>>        _permit2AddLengthOne(
>>            params_, token0, token1, amount0, amount1
>>        );

        _addLiquidity(
            params_.addData.vault,
            amount0,
            amount1,
            sharesReceived,
            params_.addData.receiver,
            token0,
            token1
        );

        if (token0 == address(weth) && msg.value > amount0) {
            weth.withdraw(msg.value - amount0);
            payable(msg.sender).sendValue(msg.value - amount0);
        } else if (token1 == address(weth) && msg.value > amount1) {
            weth.withdraw(msg.value - amount1);
            payable(msg.sender).sendValue(msg.value - amount1);
        }
    }

1e) Finally, the wrapAndSwapAndAddLiquidityPermit2 function is DoSable too! ↓

    /// @notice wrapAndSwapAndAddLiquidityPermit2 wrap eth and transfer tokens to and calls RouterSwapExecutor
    /// @param params_ SwapAndAddPermit2Data struct containing data for swap
    /// @return amount0 amount of token0 transferred from msg.sender to mint `mintAmount`
    /// @return amount1 amount of token1 transferred from msg.sender to mint `mintAmount`
    /// @return sharesReceived amount of public vault tokens transferred to `receiver`
    /// @return amount0Diff token0 balance difference post swap
    /// @return amount1Diff token1 balance difference post swap
    function wrapAndSwapAndAddLiquidityPermit2(
        SwapAndAddPermit2Data memory params_
    )
        external
        payable
        nonReentrant
        whenNotPaused
        onlyPublicVault(params_.swapAndAddData.addData.vault)
        returns (
            uint256 amount0,
            uint256 amount1,
            uint256 sharesReceived,
            uint256 amount0Diff,
            uint256 amount1Diff
        )
    {
        if (msg.value == 0) {
            revert MsgValueZero();
        }

        // #region wrap eth.

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

        // #endregion wrap eth.
        if (
            params_.swapAndAddData.addData.amount0Max == 0
                && params_.swapAndAddData.addData.amount1Max == 0
        ) {
            revert EmptyMaxAmounts();
        }

        address token0 = IArrakisMetaVault(
            params_.swapAndAddData.addData.vault
        ).token0();
        address token1 = IArrakisMetaVault(
            params_.swapAndAddData.addData.vault
        ).token1();

        if (token0 == nativeToken || token1 == nativeToken) {
            revert NativeTokenNotSupported();
        }
        if (token0 != address(weth) && token1 != address(weth)) {
            revert NoWethToken();
        }

        if (
            token0 == address(weth)
                && params_.swapAndAddData.addData.amount0Max != msg.value
        ) {
            revert MsgValueDTMaxAmount();
        }
        if (
            token1 == address(weth)
                && params_.swapAndAddData.addData.amount1Max != msg.value
        ) {
            revert MsgValueDTMaxAmount();
        }

>>        _permit2SwapAndAddLengthOne(params_, token0, token1);

        (
            ,
            ,
            amount0,
            amount1,
            sharesReceived,
            amount0Diff,
            amount1Diff
        ) = _swapAndAddLiquidity(
            params_.swapAndAddData, token0, token1
        );

        /// @dev hack to get rid of stack too depth
        uint256 amount0Use = (
            params_.swapAndAddData.swapData.zeroForOne
        )
            ? params_.swapAndAddData.addData.amount0Max - amount0Diff
            : params_.swapAndAddData.addData.amount0Max + amount0Diff;
        uint256 amount1Use = (
            params_.swapAndAddData.swapData.zeroForOne
        )
            ? params_.swapAndAddData.addData.amount1Max + amount1Diff
            : params_.swapAndAddData.addData.amount1Max - amount1Diff;

        if (amount0Use > amount0) {
            if (token0 == address(weth)) {
                weth.withdraw(amount0Use - amount0);
                payable(msg.sender).sendValue(amount0Use - amount0);
            } else {
                uint256 balance =
                    IERC20(token0).balanceOf(address(this));
                IERC20(token0).safeTransfer(msg.sender, balance);
            }
        }

        if (amount1Use > amount1) {
            if (token1 == address(weth)) {
                weth.withdraw(amount1Use - amount1);
                payable(msg.sender).sendValue(amount1Use - amount1);
            } else {
                uint256 balance =
                    IERC20(token1).balanceOf(address(this));
                IERC20(token1).safeTransfer(msg.sender, balance);
            }
        }
    }

2. That is because they either directly execute the permit2 call checked call, or call either of these internal methods:

2a. ↓

    function _permit2SwapAndAddLengthOne(
        SwapAndAddPermit2Data memory params_,
        address token0_,
        address token1_
    ) internal {
        uint256 permittedLength = params_.permit.permitted.length;
        if (permittedLength != 1) {
            revert LengthMismatch();
        }

        if (params_.permit.permitted[0].token == address(weth)) {
            revert Permit2WethNotAuthorized();
        }

>>        _permit2SwapAndAdd(permittedLength, params_, token0_, token1_);
    }

OR 2b) ↓

function _permit2SwapAndAddLengthOneOrTwo(
        SwapAndAddPermit2Data memory params_,
        address token0_,
        address token1_
    ) internal {
        uint256 permittedLength = params_.permit.permitted.length;
        if (permittedLength != 2 && permittedLength != 1) {
            revert LengthMismatch();
        }

>>        _permit2SwapAndAdd(permittedLength, params_, token0_, token1_);
    }

2c) ↓

    function _permit2SwapAndAdd(
        uint256 permittedLength_,
        SwapAndAddPermit2Data memory params_,
        address token0_,
        address token1_
    ) internal {
        SignatureTransferDetails[] memory transfers =
            new SignatureTransferDetails[](permittedLength_);

        for (uint256 i; i < permittedLength_; i++) {
            TokenPermissions memory tokenPermission =
                params_.permit.permitted[i];

            if (tokenPermission.token == token0_) {
                transfers[i] = SignatureTransferDetails({
                    to: address(this),
                    requestedAmount: params_
                        .swapAndAddData
                        .addData
                        .amount0Max
                });
            }
            if (tokenPermission.token == token1_) {
                transfers[i] = SignatureTransferDetails({
                    to: address(this),
                    requestedAmount: params_
                        .swapAndAddData
                        .addData
                        .amount1Max
                });
            }
        }

>>        permit2.permitTransferFrom(
>>            params_.permit, transfers, msg.sender, params_.signature
>>        );  
    }

Impact

This will permanently DoS the usage of the signed message (the signature) for particular users (anyone can become a victim), because front-running is easy, especially on L2's, where transaction's gas prices are cheap.

Code Snippet

• https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L413-L418
• https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1117-L1119
• https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/ArrakisPublicVaultRouter.sol#L1185-L1187

Tool used

Manual review.

Recommendation

Consider implementing the following fix: wrapping all permit and permitTransferFrom function calls with a try {} catch {} exception handler block. For instance, like that:

1185: - permit2.permitTransferFrom(
1186: -           params_.permit, transfers, msg.sender, params_.signature
1187: -       );

1185: + try { permit2.permitTransferFrom(
1186: + params_.permit, transfers, msg.sender, params_.signature
1187: + );
1188: + }
1189: + catch { }

And like that:

413: -        permit2.permitTransferFrom(
414: -            params_.permit,
415: -            transferDetails,
416: -            msg.sender,
417: -            params_.signature
418: -        );

413: +        try { permit2.permitTransferFrom(
414: +            params_.permit,
415: +            transferDetails,
416: +            msg.sender,
417: +            params_.signature
418: +        ); } catch { }

Finally, wrap it up here, too:

1117: -        permit2.permitTransferFrom(
1118:              params_.permit, transfers, msg.sender, params_.signature
1119: -        );

1117: +        try { permit2.permitTransferFrom(
1118:              params_.permit, transfers, msg.sender, params_.signature
1119: +        ); } catch { }