_gasSwapIn() and _gasSwapOut() could end up with partial swaps due to insufficient slippage protection

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L684-L695
https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L727-L734

Vulnerability details

_gasSwapIn() and _gasSwapOut() are lacking a check to verify that the swap amount received is greater than a specified minimum.

Relying on the sqrtPriceLimitX96 parameter for swap() as a slippage protection is not sufficient. That is because UniswapV3Pool will not revert even when sqrtPriceLimitX96 is hit, which means that it will continue with a partial swap. (see UniswapV3Pool.sol#L641)

Impact

When a partial swap occur for _gasSwapIn() and _gasSwapOut(), the remaining gas token will be stuck in RootBridgeAgent while the crosschain execution will fail with insufficient gas.

Detailed Explanation

Both _gasSwapIn() and _gasSwapOut() only use sqrtPriceLimitX96 as a measure for slippage protection. That will result in partial swaps when sqrtPriceLimitX96 is hit and the input token amount int256(_amount) is not entirely used up.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L684-L695

        try IUniswapV3Pool(poolAddress).swap(
            address(this),
            zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: gasTokenGlobalAddress}))
        ) returns (int256 amount0, int256 amount1) {
            return uint256(zeroForOneOnInflow ? amount1 : amount0);
        } catch (bytes memory) {
            _forceRevert();
            return 0;
        }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L727-L734

        //Swap imbalanced token as long as we haven't used the entire amountSpecified and haven't reached the price limit
        (int256 amount0, int256 amount1) = IUniswapV3Pool(poolAddress).swap(
            address(this),
            !zeroForOneOnInflow,
            int256(_amount),
            sqrtPriceLimitX96,
            abi.encode(SwapCallbackData({tokenIn: address(wrappedNativeToken)}))
        );

Recommended Mitigation Steps

Check that that the swap amount received is greater than the minimum based on the price limit. Refer to UniswapV3's SwapRouter.sol
https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L128

Assessed type

Uniswap

[c4-judge]

trust1995 marked the issue as unsatisfactory:
Insufficient proof

[peakbolt]

Please see below for further clarifications on this issue.

Point 1 - Explanation of issue - partial swap in UniswapV3Pool.swap()

I would like to elaborate on the issue of insufficient slippage protection for _gasSwapIn() and _gasSwapOut().

It is a known issue that directly interacting with UniswapV3Pool (instead of via SwapRouter) and only using sqrtPriceLimitX96 as a slippage protection is insufficient.

That is the reason why UniswapV3 has a minimum amountOut check in their SwapRouter to prevent partial swap (see https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L128).

This issue is reported in sherlock-audit/2023-04-blueberry-judging#132. It is also explained in detail in https://uniswapv3book.com/docs/milestone_3/slippage-protection/#slippage-protection-in-swaps.

Key point is when sqrtPriceLimitX96 is hit, the while loop in UniswapV3Pool.sol#L641 will not trigger a revert and will proceed with next execution.

Point 2 - Explanation of the impact

Regarding the impact of partial swap, we can look at the following scenario, which shows that it will lock up un-utilized gas token and cause cross-chain execution to fail. I have added a POC at the end below that demonstrates the impact.

1. Perform a callOutSignedAndBridge() from Arbitrum Branch to FTM Branch (via root chain).
2. The ArbtriumBranchBridgeAgent will first perform a callout to RootBridgeAgent on the same chain.
3. That will get RootBridgeAgent to do a callOutAndBridge() to FTM, which triggers a _manageGasOut() call to swap WETH to WFTM.
4. The swap is then performed within _gasSwapOut() via a UniswapV3Pool.swap().
5. When a partial swap occur for the UniswapV3Pool.swap(), it triggers uniswapV3SwapCallback(), which will pay the pool only the amount of swapped WETH. That means the un-utilized WETH is left locked in RootBridgeAgent contract. Note that even fixing the bug in sweep() will also not allow us to withdraw the WETH.
6. Due to the partial swap, RootBridgeAgent will only bridge a portion of the WFTM to FTM BranchBridgeAgent.
7. That means in FTM BranchBridgeAgent, _payExecutionGas() will revert using _forceRevert() if there is insufficient WFTM (due to partial swap) to pay for the execution on FTM branch.

Point 3 - Likelihood to hit price limit and encounter partial swap

One could argue that if the pool liquidity is high and priceImpactPercentage is set properly, partial swap will not occur.

3.1 Pool Liquidity

However, if we check out the current UniswapV3 pools on Arbitrum (https://info.uniswap.org/#/arbitrum/pools), we can see that there are currently no WETH pools for WFTM, WMATIC, WBNB, WAVAX. (dev mentioned that Branch contracts will be deployed at Polygon, BSC, Metis and possibly Avalanche and Fantom)

That means during launch, liquidity might not be very high and the team will have to fund the pools initially.

3.2 Price impact

Note that the sqrtPriceLimitX96 is determined by the pre-set priceImpactPercentage for each configured gas pool in the RootPort.
To make it economically feasible for users, priceImpactPercentage is likely to be set as low as possible.

With a low and pre-set value for priceImpactPercentage, combined with a likelihood of low pool liquidity, it is pausible for users to encounter a partial gas swap when it hit the price limit. Furthermore, the users are not able to adjust the slippage as priceImpactPercentage is controlled by contract owner.

Based on the above reasoning, I would recommend to apply the mitigation (minimum amountOut check) to protect the users and eliminate the possibility of the issue at the code level and not via deployment or configuration.

POC

1. Add a revert to _forceRevert() to simulate a revert by anyCall in https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1323-L1329

    function _forceRevert() internal virtual {
        IAnycallConfig anycallConfig = IAnycallConfig(IAnycallProxy(localAnyCallAddress).config());
        uint256 executionBudget = anycallConfig.executionBudget(address(this));

        // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
        if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}

        //@audit - add this to simulate a revert by anyCall
        revert();
    }

2. Add a line to reduce amountSpecified for out going gas swap to simulate partial swap by UniswapV3 in https://github.com/code-423n4/2023-05-maia/blob/main/test/ulysses-omnichain/RootTest.t.sol#L1887-L1916

    function swap(address, bool zeroForOne, int256 amountSpecified, uint160, bytes calldata data)
        external
        returns (int256 amount0, int256 amount1)
    {
        SwapCallbackData memory _data = abi.decode(data, (SwapCallbackData));

        address tokenOut =
            (_data.tokenIn == arbitrumWrappedNativeTokenAddress ? globalGasToken : arbitrumWrappedNativeTokenAddress);

        //@audit simulate partial swap for outgoing Root to FTM chain remote execution. 4e9 is just used to match the remoteExeutionGas amount
        if(amountSpecified == 4e9) amountSpecified = amountSpecified / 2;
        
        if (tokenOut == arbitrumWrappedNativeTokenAddress) {
            deal(address(this), uint256(amountSpecified));
            WETH(arbitrumWrappedNativeTokenAddress).deposit{value: uint256(amountSpecified)}();
            MockERC20(arbitrumWrappedNativeTokenAddress).transfer(msg.sender, uint256(amountSpecified));
        } else {
            deal({token: tokenOut, to: msg.sender, give: uint256(amountSpecified)});
        }

        if (zeroForOne) {
            amount1 = amountSpecified;
        } else {
            amount0 = amountSpecified;
        }

        IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amount0, amount1, data);
    }

3. Add and run the following test case in RootTest.t.sol

    function testPeakboltCrossChainPartialGasSwap() public {
        //Set up
        testAddLocalTokenArbitrum();

        //Prepare data
        bytes memory packedData;

        {
            Multicall2.Call[] memory calls = new Multicall2.Call[](1);

            //Mock action
            calls[0] = Multicall2.Call({target: 0x0000000000000000000000000000000000000000, callData: ""});

            //Output Params
            OutputParams memory outputParams = OutputParams(address(this), newAvaxAssetGlobalAddress, 150 ether, 0);

            //RLP Encode Calldata Call with no gas to bridge out and we top up.
            bytes memory data = abi.encode(calls, outputParams, ftmChainId);

            //Pack FuncId
            packedData = abi.encodePacked(bytes1(0x02), data);
        }

        address _user = address(this);

        //Get some gas.
        hevm.deal(_user, 1 ether);
        hevm.deal(address(ftmPort), 1 ether);

        //assure there is enough balance for mock action
        hevm.prank(address(rootPort));
        ERC20hTokenRoot(newAvaxAssetGlobalAddress).mint(address(rootPort), 50 ether, rootChainId);
        hevm.prank(address(avaxPort));
        ERC20hTokenBranch(avaxMockAssethToken).mint(_user, 50 ether);

        //Mint Underlying Token.
        avaxMockAssetToken.mint(_user, 100 ether);

        //Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(avaxMockAssethToken),
            token: address(avaxMockAssetToken),
            amount: 150 ether,
            deposit: 100 ether,
            toChain: ftmChainId
        });
        //Call Deposit function
        avaxMockAssetToken.approve(address(avaxPort), 100 ether);
        ERC20hTokenRoot(avaxMockAssethToken).approve(address(avaxPort), 50 ether);

   
        console2.log("BALANCE BEFORE:");
        console2.log("User avaxMockAssetToken Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
        console2.log("User avaxMockAssethToken Balance:", MockERC20(avaxMockAssethToken).balanceOf(_user));
        console2.log("User newAvaxAssetLocalToken (FTM chain) Balance:", MockERC20(newAvaxAssetLocalToken).balanceOf(_user));

        console2.log("********************* avaxMulticallBridgeAgent.callOutSignedAndBridge *********************");    
        //remoteExecutionGas is for RootBridge.anyFallback() or branchBridgeAgent.anyExecute()
        uint128 remoteExecutionGas = 4e9; // gas for Root (Arbitrum) to FTM chain remote execution
        uint128 depositedGas = 1e11; //total gas amount for both Root and Branch agents
        avaxMulticallBridgeAgent.callOutSignedAndBridge{value: depositedGas }(packedData, depositInput, remoteExecutionGas);

        console2.log("BALANCE AFTER:");
        console2.log("User avaxMockAssetToken (AVAX) Balance:", MockERC20(avaxMockAssetToken).balanceOf(_user));
        console2.log("User avaxMockAssethToken (AVAX) Balance:", MockERC20(avaxMockAssethToken).balanceOf(_user));
        console2.log("User newAvaxAssetLocalToken (FTM chain) Balance:", MockERC20(newAvaxAssetLocalToken).balanceOf(_user));

        require(MockERC20(newAvaxAssetLocalToken).balanceOf(_user) == 150 ether);
    }