c4-submissions commented 1 year ago

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

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #887

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-judge commented 1 year ago

alcueca changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory

c4-judge commented 1 year ago

alcueca marked the issue as duplicate of #518

code-423n4 / 2023-09-maia-findings

Native coin (ETH) can be stuck in the MulticallRootRouter and MulticallRootRouterLibZip contract #257

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type