Maia DAO - Ulysses - Limbooo's results

Lines of code

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

Vulnerability details

Description

The VirtualAccount contract allows users to manage assets and perform interactions remotely while maintaining an encapsulated user balance. The contract includes a payableCall function that aggregates and executes a batch of calls to external contracts. Unfortunately, this function lacks proper access control checks, allowing any external user or contract to invoke it without authorization. However, it's important to note that the function does enforce proper validation of msg.value to prevent unauthorized Ether withdrawals.

The lack of access control in the payableCall function of the VirtualAccount contract poses a high-security risk, as it allows unauthorized users to perform actions that may result in financial losses and unintended behavior. To address this vulnerability, it is crucial to implement proper access control checks to ensure that only authorized entities can use the function. Additionally, applying the existing requiresApprovedCaller modifier to payableCall would provide a consistent access control mechanism across all contract functions.

Impact

This vulnerability poses a significant security risk as it allows unauthorized users to call the payableCall function, potentially resulting in the following adverse consequences:

1. Unauthorized Token Withdrawals: Attackers can exploit the lack of access control to call payableCall and withdraw ERC20 tokens or ERC721 tokens from the VirtualAccount without proper authorization.

2. Potential Financial Loss: If the function performs actions that involve transferring tokens to external contracts, an unauthorized caller could drain the contract of assets, leading to a financial loss for the VirtualAccount and its users.

3. Attacks Over Cross-Chain: Some of contracts trusted VirtualAccount to preform some cross-chain procedures like this validation in RootBridgeAgent, where it can be used to extend some different attacks that would potentiality leads to a financial loss on other chains.

Proof of Concept

Below is a simplified demonstrating how an unauthorized user can exploit the lack of access control in the payableCall function to withdraw ERC20 tokens from the VirtualAccount:

// Contract representing an unauthorized user
contract UnauthorizedUser {
    function exploitPayableCall(IVirtualAccount virtualAccount, address token, uint256 amount) external {
        // Define a malicious PayableCall
        IVirtualAccount.PayableCall[] memory maliciousCall = new IVirtualAccount.PayableCall[](1);
        maliciousCall[0] = IVirtualAccount.PayableCall({
            target: address(token),
            callData: abi.encodeWithSignature("transfer(address, uint256)", msg.sender, amount),
            value: 0 // No value sent
        });

        // Execute the malicious PayableCall
        (bool success, ) = address(virtualAccount).call{value: 0}(abi.encodeWithSignature("payableCall(PayableCall[])", maliciousCall));

        // Check if the call was successful (it should not be)
        require(!success, "Exploit failed");
    }
}

The UnauthorizedUser contract attempts to execute a malicious PayableCall to the payableCall function of the VirtualAccount. Since there are no access control checks in place, the unauthorized user can attempt this exploit easily for all VirtualAccounts in the system.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

To address this vulnerability and enhance the security of the VirtualAccount contract, it is strongly recommended to implement proper access control checks within the payableCall function. Access control should ensure that only authorized users or contracts can execute this function. Consider the following steps:

1. Implement a modifier or access control checks within the payableCall function to verify that the caller is authorized to use the function.

2. Define a list of authorized users or contracts that are allowed to call payableCall.

3. Restrict access to the function based on the ownership of the VirtualAccount or any other designated access control mechanism.

Additionally, it's important to note that the VirtualAccount contract already implements access control checks using the requiresApprovedCaller modifier in other interaction functions, such as withdrawNative, withdrawERC20, and call. Therefore, it is recommended to apply the same access control pattern to the payableCall function to maintain consistency and ensure that unauthorized users cannot exploit this vulnerability.

Assessed type

Access Control

Lines of code

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 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L776

Vulnerability details

Description

Many function in the Maia Protocol's contracts allow users to send a cross-chain messages with specified gas parameters without any limitation. Where they eventually will be sent through these functions:

• RootBridgeAgent._performCall
• RootBridgeAgent._performRetrySettlementCall
• BranchBridgeAgent._performCall

A potential vulnerability has been identified in multiple functions that using _perform* functions, due to the lack of proper gas validation and enforcement. This vulnerability allows any caller to specify GasParams (used to build LayerZero's Relayer adapterParams) for the cross-chain call, including a potentially low gas value. Without proper gas validation, this could lead to scenarios where cross-chain calls fail due to insufficient gas, potentially causing denial-of-service (DoS) situations or blockage of the message pathway.

External Vulnerable Functions

1. BranchBridgeAgent contract

   • callOutAndBridge
   • callOutAndBridgeMultiple
   • callOutSigned
   • callOutSignedAndBridge
   • callOutSignedAndBridgeMultiple
   • retryDeposit
   • retrieveDeposit
   • retrySettlement - 2 level cross-chain messages

2. BaseBranchRouter

   • callOut

3. RootBridgeAgent contract

   • retrySettlement
   • retrieveSettlement

4. CoreBranchRouter

   • addGlobalToken - 3 level cross-chain messages
   • addLocalToken

5. CoreRootRouter

   • addBranchToBridgeAgent - 2 level cross-chain messages
   • toggleBranchBridgeAgentFactory
   • removeBranchBridgeAgent

...

Some of these functions controlled by a trusted entities but this issue could happens unintentionally, and It's important to note that multi level cross-chain calls is allowed! while this vulnerability can be occurs in very complex chain of cross-chain calls.

Impact

In scenarios where a LayerZero cross-chain call fails due to low gas, it could potentially block the message pathway, preventing the normal flow of messages channel between two chains.

Proof of Concept

Explanation:

When the gas limit specified in the try block (inside LayerZero Endpoint) extremely low and cannot cover the gas requirements of the internal calls within lzReceive, It's important to note that while excessivelySafeCall can help prevent reverts due to out-of-gas conditions caused by low-level calls, it cannot overcome extremely low gas limits set at the transaction level. To ensure the successful execution of desired operations, the gas limit should be appropriately set.

Additionally, If the gas limit provided in the try block is insufficient to even enter the lzReceive function, the transaction will revert before the internal call inside lzReceive reaches excessivelySafeCall.

Therefore, if an attacker sets an extremely low gas limit in the try block, and that limit is inadequate to enter the lzReceive function, the error of out-of-gas will be caught before the internal call inside lzReceive reaches the excessivelySafeCall, leading to a failed transaction.

Attack scenario

1. Attacker's Intent:

   • An attacker seeks to disrupt the Maia Protocol's message pathway or cause a DoS situation.

2. Low Gas Specification:

   • The attacker calls one of the functions that allow controlling adapterParams, specifying an exceptionally low gas value, for instance, 30,000 gas units.

3. Cross-Chain Call:

   • The function initiates a cross-chain call with the specified gas value.

4. Failure to Execute:

   • As a result of the low gas value provided, the cross-chain call fails to execute due to insufficient gas to cover the internal calls within lzReceive, lead to storing the payload in StoredPayload.

5. Impact:

   • The Maia Protocol experiences a DoS situation, and the channel message pathway may be blocked as the protocol is using NonBlockingLzApp architecture.

Test Case

This test will verify the vulnerability by simulating two scenarios:

1. Low Gas Attack: It will test the scenario where an attacker specifies an extremely low gas value, causing a cross-chain call to fail due to insufficient gas. This test aims to demonstrate how this vulnerability can lead to a denial-of-service (DoS) situation or blockage of the message pathway.

2. Normal Gas: It will test the scenario with an appropriate gas value to ensure that the cross-chain call executes successfully, preventing a DoS situation. This test serves as a control to show the expected behavior when using the correct gas parameters.

These test cases aim to demonstrate the impact of gas limitations on the Maia Protocol's message pathway and highlight the importance of proper gas validation and enforcement.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "forge-std/Test.sol";

import {ExcessivelySafeCall} from "lib/ExcessivelySafeCall.sol";


contract EndpointMock {

    struct StoredPayload {
        uint64 payloadLength;
        address dstAddress;
        bytes32 payloadHash;
    }

    // storedPayload = [srcChainId][srcAddress]
    mapping(uint16 => mapping(bytes => StoredPayload)) public storedPayload;

    event PayloadStored(uint16 srcChainId, bytes srcAddress, address dstAddress, uint64 nonce, bytes payload, bytes reason);

    function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external {
        try LzReceiverMock(_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);
        }
    }
}

contract LzReceiverMock {
    using ExcessivelySafeCall for address;
    uint256 nonce;

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

    function lzReceiveNonBlocking(
        address _endpoint,
        uint16 _srcChainId,
        bytes calldata _srcAddress,
        bytes calldata _payload
    ) public {
        require(_srcChainId == 0, "reverting test");
        _endpoint;
        _srcAddress;
        _payload;
        nonce++;
    }

}
contract LowGasTest is Test {
    EndpointMock endpointMock;
    LzReceiverMock lzReceiver;

    event PayloadStored(uint16 srcChainId, bytes srcAddress, address dstAddress, uint64 nonce, bytes payload, bytes reason);


    constructor() {
        endpointMock = new EndpointMock();
        lzReceiver = new LzReceiverMock();
    }

    function test_PoC_LowGasStoreThePayload() public {
        bytes memory _payload = bytes("dummy");
        bytes memory _srcAddress = bytes("_srcAddress");


        vm.expectEmit(true, true, false, true);
        emit PayloadStored(0, _srcAddress, address(lzReceiver), 0, _payload, bytes(""));
        endpointMock.receivePayload(0, _srcAddress, address(lzReceiver), 0, 10, _payload); // Call lzReceiver with low gas

        (, address payloadSrcAddress,) = endpointMock.storedPayload(0, _srcAddress);
        assertEq(payloadSrcAddress, address(lzReceiver));
    }

    function test_PoC_RightGasDoseNotStorePayloadWhenReverts() public {
        // chainId = 1 to make the lzReceive reverts
        endpointMock.receivePayload(1, bytes("_srcAddress"), address(lzReceiver), 0, 10000, bytes("dummy")); 
    }
}

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

consider Implementing gas validation and enforcement mechanisms within the function. Ensure that the gas provided by the caller is above a minimum threshold, which should cover the worst-case gas consumption scenario for the cross-chain call. it should hit (bool success,) = address(this).excessivelySafeCall(... when receiving cross-chain messages.

According to (LayerZero Integration Checklist - LayerZero Docs):

Make sure to test the amount of gas required for the execution on the destination. Use custom adapter parameters and specify minimum destination gas for each cross-chain path when the default amount of gas (200,000) is not enough. This requires whoever calls the send function to provide the adapter params with a destination gas >= amount set in the minDstGasLookup for that chain. So that your users don't run into failed messages on the destination. It makes it a smoother end-to-end experience for all.

Also this snippet can be used to implement the check from LayerZero implementation of LzApp

function _checkGasLimit(uint16 _dstChainId, uint16 _type, bytes memory _adapterParams, uint _extraGas) internal view virtual {
    uint providedGasLimit = _getGasLimit(_adapterParams);
    uint minGasLimit = minDstGasLookup[_dstChainId][_type] + _extraGas;
    require(minGasLimit > 0, "LzApp: minGasLimit not set");
    require(providedGasLimit >= minGasLimit, "LzApp: gas limit is too low");
}

function _getGasLimit(bytes memory _adapterParams) internal pure virtual returns (uint gasLimit) {
    require(_adapterParams.length >= 34, "LzApp: invalid adapterParams");
    assembly {
        gasLimit := mload(add(_adapterParams, 34))
    }
}

It would be easier to implement the check right before the send call, Also it will prevent the multi level cross-chain calls and break any call that will end up in StoredPayload.

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

Overview

We discovered a remote address verification issue due to inappropriate extraction of the remote address from the source chain. This issue affects the Maia protocol's interaction with LayerZero and could lead to cross-chain message delivery failures.

Background

LayerZero Trusted Remotes

LayerZero defines a Trusted Remote as a 40-byte object used to identify another contract from which a user application contract (UA) will receive messages. The Trusted Remote consists of the concatenated bytes of the REMOTE contract address and the LOCAL contract address. This is the same as the one named bytes calldata _srcAddress in lzReceive parameters.

Address Slicing

Array slices in Solidity are views on a contiguous portion of an array, represented as x[start:end]. Both start and end are optional, defaulting to 0 and the length of the array, respectively. Array slices do not have a type name and are implicitly convertible to arrays of their underlying type. Index access within a slice is relative to the start of the slice.

Issue Description

The source address verification issue arises when extracting the source contract's address from the _srcAddress parameter within the Maia protocol's LayerZero-related contracts. This parameter represents the Trusted Remote of the source User Application contract (UA), but the verification process assumes the last 20 bytes of _srcAddress correspond to the source contract's address. However, this assumption is not accurate, potentially leading to incorrect address extraction. according to LayerZero contracts, this is how the node is encoding the _srcAddress before calling the lzReceive on the destination contract:

bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress);

Verification in Root Bridge Agent

In the Root Bridge Agent contract lzReceive, which is responsible for receiving messages on the destination chain. The _srcAddress parameter is passed to this function and is expected to represent the source UA's Trusted Remote. However, the verification process in requiresEndpoint takes the last 20 bytes _srcAddress[20:] as the source contract's address, assuming that the first 20 bytes are the local contract's address. (PARAMS_ADDRESS_SIZE is equal to 20)

if (_srcAddress.length != 40) revert LayerZeroUnauthorizedCaller();

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

However, this verification process may result in extracting the local contract's address rather than the source contract's address. Also, the same happens in the Branch Bridge Agent.

Impact

The impact of this issue is primarily related to cross-chain message delivery. When the source address verification is incorrect, it can result in the failure of cross-chain message delivery. This means that messages may not be successfully transmitted from the source UA to the destination UA, leading to message loss or disruption in communication between blockchains.

Tools Used

• Manual analyzing
• LayerZero documentation

Recommended Mitigation Steps

Consider correcting desire address extraction. Modify the source address extraction process to accurately identify the source contract's address. Utilize address slicing to extract the correct remote address from _srcAddress, specifically the first 20 bytes.

Assessed type

Invalid Validation