Incorrect source address decoding in RootBridgeAgent and BranchBridgeAgent's _requiresEndpoint breaks LayerZero communication

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1212 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L943

Vulnerability details

When bridge agent contracts communicate through the LayerZero endpoint, the source contract encodes the _destination parameter of the ILayerZeroEndpoint.send call by concatenating the destination address (first) and the source address (second):

   // BranchBridgeAgent.sol:142
   rootBridgeAgentPath = abi.encodePacked(_rootBridgeAgentAddress, address(this)); 

   // BranchBridgeAgent.sol:770
   ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            rootChainId,
            rootBridgeAgentPath,
            _payload,
            payable(_refundee),
            address(0),
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
        );

    // RootBridgeAgent.sol:1182
    getBranchBridgeAgentPath[_branchChainId] = abi.encodePacked(_newBranchBridgeAgent, address(this));

    // RootBridgeAgent.sol:823
    ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
        _dstChainId,
        getBranchBridgeAgentPath[_dstChainId],
        _payload,
        _refundee,
        address(0),
        abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
    );

When the message is received on the destination chain, the destination agent validates that the sending address is correct after decoding it from the last 20 bytes of the _srcAddress parameter in the ILayerZeroReceiver.lzReceive call:

    // RootBridgeAgent.sol:1212
    if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
        revert LayerZeroUnauthorizedCaller();
    }

    // BranchBridgeAgent.sol:943
    if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();

This byte selection is however incorrect, because when calls pass by actual LayerZero logic the _srcAddress in lzReceive is not equal to the _destination passed to send.

In a real-world scenario, by looking up the last bytes of _srcAddress, the destination agent will always extract its own address instead of the remote source contract's and the validation this address maps to a valid sender on the source chain will consequently always fail.

Impact

Since the faulty logic is a single entry point for communication between chains, it is also a single point of failure, so this vulnerability effectively shuts down the whole branch-to-root and root-to-branch inter-chain communication.

An example high-severity impact scenario is: if after the contracts are deployed any tokens are added and bridged (i.e. out of a branch chain), these will remain permanently locked in the source chain's BranchPort as this vulnerability does not prevent the source operations from completing. The tokens will not be recoverable because:

• the bridged messages will revert in the destination (root) chain, failing to mint any hToken there that could later be bridged back to rescue the original assets
• the retrieve/redeem/retry logic in place for unlocking funds after remote errors would not be a viable way out because it depends on the faulty cross-chain channel too

Proof of Concept

This vulnerability can be verified by instantiating a BranchBridgeAgent and a RootBridgeAgent contract and connecting them via LayerZero's mock endpoint.

This mock, much like the productive endpoint, inverts the order of the bytes in _destination and _srcAddress (relevant code below), effectively breaking the assumption that these are equal and enabling the reproduction of the issue:

    // LzEndpointMock.sol:113
    function send(uint16 _chainId, bytes memory _path, bytes calldata _payload, address payable _refundAddress, address _zroPaymentAddress, bytes memory _adapterParams) external payable override sendNonReentrant {
        require(_path.length == 40, "LayerZeroMock: incorrect remote address size"); // only support evm chains

        address dstAddr;
        assembly {
            dstAddr := mload(add(_path, 20))
        }

        // LzEndpointMock:148
        bytes memory srcUaAddress = abi.encodePacked(msg.sender, dstAddr); // cast this address to bytes
        bytes memory payload = _payload;
        LZEndpointMock(lzEndpoint).receivePayload(mockChainId, srcUaAddress, dstAddr, nonce, extraGas, payload);
    }

A full runnable foundry test showing the failure in an integrated scenario is below:

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {BranchBridgeAgent} from "src/BranchBridgeAgent.sol";
import {RootBridgeAgent} from "src/RootBridgeAgent.sol";
import {GasParams} from "src/interfaces/BridgeAgentStructs.sol";

// as taken from here:
// https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/mocks/LZEndpointMock.sol
import {LZEndpointMock} from "./ulysses-omnichain/mocks/LZEndpointMock.sol";

contract SourceAddrPoc is Test {
    function testIncorrectValidation() public {
        uint16 rootChainId = 9;
        uint16 branchChainId = 10;

        // simulate a real L0 bridge in between
        LZEndpointMock rootLzEndpoint = new LZEndpointMock(rootChainId);
        LZEndpointMock branchLzEndpoint = new LZEndpointMock(branchChainId);

        RootBridgeAgent rba = new RootBridgeAgent(
            rootChainId,             // localChainId
            address(rootLzEndpoint), // _lzEndpointAddress
            address(this),           // _localPortAddress
            address(this)            // _localRouterAddress
        );

        BranchBridgeAgent bba = new BranchBridgeAgent(
            rootChainId,               // _rootChainId
            branchChainId,             // _localChainId
            address(rba),              // _rootBridgeAgentAddress
            address(branchLzEndpoint), // _lzEndpointAddress
            address(this),             // _localRouterAddress
            address(this)              // _localPortAddress
        );

        // BranchBridgeAgent knows already the address of RootBridgeAgent from its construction,
        // here we tell RootBridgeAgent where BranchBridgeAgent is
        rba.syncBranchBridgeAgent(address(bba), branchChainId);

        rootLzEndpoint.setDestLzEndpoint(address(bba), address(branchLzEndpoint));
        branchLzEndpoint.setDestLzEndpoint(address(rba), address(rootLzEndpoint));

        // communication root -> branch is broken (incorrect src address validation in branch)
        // this triggeres a silently ignored "LayerZeroUnauthorizedCaller()" revert that can
        // be logged with -vvvv verbose testing:
        //
        //  ├─ [1393] BranchBridgeAgent::lzReceiveNonBlocking(...)
        //  │   └─ ← "LayerZeroUnauthorizedCaller()"
        //  └─ ← ()
        rba.callOut{value: 22001375000000000}(
            payable(address(this)), // _refundee
            address(this), // _recipient
            branchChainId, // _dstChainId,
            "", // _params
            GasParams(1_000_000, 0) // _gParams
        );

        // communication branch -> root is broken too (incorrect src address validation in root)
        // here, too, we have the same silently ignored error:
        //
        //    │   │   │   │   ├─ [3789] RootBridgeAgent::lzReceiveNonBlocking(..)
        //    │   │   │   │   │   └─ ← "LayerZeroUnauthorizedCaller()"
        //    │   │   │   │   └─ ← ()
        bba.callOut{value: 22001155000000000}(
            payable(address(this)), // _refundee
            "", // _params
            GasParams(1_000_000, 0) // _gParams
        );
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

The following changes fix the inter-chain integration:

    // RootBridgeAgent.sol:1204
    modifier requiresEndpoint(address _endpoint, uint16 _srcChain, bytes calldata _srcAddress) virtual {
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();

        if (_endpoint != getBranchBridgeAgent[localChainId]) {
            if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

-            if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
+            if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[:PARAMS_ADDRESS_SIZE])))) {
                revert LayerZeroUnauthorizedCaller();
            }
        }
        _;
    }

    // BranchBridgeAgent.sol:936
    function _requiresEndpoint(address _endpoint, bytes calldata _srcAddress) internal view virtual {
        //Verify Endpoint
        if (msg.sender != address(this)) revert LayerZeroUnauthorizedEndpoint();
        if (_endpoint != lzEndpointAddress) revert LayerZeroUnauthorizedEndpoint();

        //Verify Remote Caller
        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
-        if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();
+        if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[:20])))) revert LayerZeroUnauthorizedCaller();
    }

It is also recommended to add tests for agents in an integration scenario that leverages the LzEndpointMock.sol contract provided by the LayerZero team, who use it for their own testing.

Assessed type

en/de-code

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #439

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

From the sponsor:

Although, losing assets seems possible in paper, it is certain this would ever happen in production since the initial set up of the system namely the addition of new chains or any tokens would all fail, the only functioning branch for deposits would be the Arbitrum Branch that circumvents these checks and would not have any issues withdrawing assets. There would not be any branches in remote networks since any messages setting up the connection new branches and the root chain would fail due to the validation issue being described.

[c4-sponsor]

0xLightt (sponsor) confirmed

[0xBugsy]

Addressed at https://github.com/Maia-DAO/2023-09-maia-remediations/commit/4d2825cd4e73a69ad33bccaf2ceb07443f127beb