Maia DAO - Ulysses - LokiThe5th's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreBranchRouter.sol#L62-L78

Vulnerability details

Impact

The communication channel between a branch chain and a destination chain can be blocked by exploiting the ability to specify arbitrary gas parameters for the destination chain function calls. As Ulysses implements a non-blocking pattern, forcing the destination chain calls to revert creates an undesirable "in blocking" state.

In addition, as the requisite functions to clear the blocked channel were not implemented, the channel will remain blocked until a successful manual call to the blocked endpoint's retryPayload (with appropriate gas parameters).

As this vulnerability is created by the confluence of multiple underlying issues these have been split into three Root Causes for clarity.

Root causes: Issue A. User can specify any gas params against best-practice guidelines
Issue B. The protocol implements a non-blocking implementation pattern for lzReceive and doesn't handle the "in-blocking scenario" Issue C. The protocol doesn't implement the ILayerZeroUserApplicationConfig functions as recommended

It should also be noted that this can be exploited to cause economic damage to the protocol. In it's current state the manual call to Endpoint::retryPayload() is the only way to clear the channel. If the griefer were to initiate a high payload size call on a low-cost branch like Polygon zkEVM or Optimism, then the team will need to pay the gas fees to process the griefing-payload at the higher gas cost on Arbitrum.

Due to the ability to consistently (and at a low cost) block communication between the root chain and branch chain (which can only be unblocked through a manual intervention) the issue has been evaluated to high.

Proof of Concept

Bob deploys an ERC20 to the local chain, for example, Polygon. Bob now adds the token via the CoreBranchRouter by calling addLocalToken. Importantly, Bob sets the gasParams as gasLimit: 1 (Issue A). This should not be allowed, as Layer Zero explicitly advises:

And the UA should enforce the gas estimate on-chain at the source chain to prevent users from inputting too low the value for gas.

The branch hToken is created and these "poisoned" gas parameters are now encoded and sent to the local BridgeAgent for a system callout.

        //Send Cross-Chain request (System Response/Request)
        IBridgeAgent(localBridgeAgentAddress).callOutSystem{value: msg.value}(payable(msg.sender), payload, _gParams);
    

In the local BaseBridgeAgent the callOutSystem function is called. This passes on the poisoned _gParams to _performCall.

In _performCall the poisoned params are passed through to the local chain LayerZeroEndpoint via send:

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

In the Endpoint contract the outboundNonce is incremented, and the _getSendLibrary(uaConfig).send() is called.

The call now passes through to the nodes and relayers and this finally passes the call through to the try/catch block in the receivePayload() function of the Endpoint contract on Arbitrum.

Here the Endpoint contract must make a call to the destination address via alzReceve call. Such calls to lzReceive is should not fail in the Ulysses protocol (i.e. it is supposed to be non-blocking). We can see this in the lzReceive code implemented in the RootBridgeAgent.sol contract here. It should never revert a call which originates from the Endpoint contract (Issue B).

Crucially, the _gasLimit used here for the lzReceive call is the _gasLimit that was provided by Bob. But because Bob has specified a gasLimit of 1 he forces the call from the Endpoint contract to RootBridgeAgent::lzReceive() to revert due to an out-of-gas error. This causes the Endpoint contract to store the payload. This blocks any subsequent cross-chain messages for that chain which will revert with LayerZero: in message blocking.

The message channel is now blocked. The channel can only be unblocked through a manual call to Endpoint::retryPayload(); crucially, the Maia protocol team will need to bear the costs of this transaction.

Layer Zero provides a "get-out-of-jail-card" for these cases through its forceResumeReceive functionality. Unfortunately, because of Issue C, the protocol doesn't implement forceResumeReceive and thus has no other way to clear the channel without bearing the execution cost. This results in blocking the channel and communication loss.

Coded PoC

To accurately demonstrate the proof of concept we will use the below code, testProofOfConcept, and paste it into the file RootForkTest.t.sol in the contest repo. This provides us with a fork test environment which uses the actual Endpoint contract from Layer Zero.

The test uses the example of using CoreBranchRouter::addLocalToken() to demonstate Issue A, where a user-supplied gasParams can lead to an "OutOfGas" revert on the destination chain call to RootBridgeAgent:lzReceive. It then demonstrates Issue B by showing how subsequent messages to the same chain fail (even when done with valid gas parameters) due to the blocking nature of Endpoint. This, combined with issue C, where there is no implementation of forceResumeReceive(), creates a situation where a channel between the source chain and the root chain can be blocked permanently.

Instructions:

1. Setup the contest repo as described in the contest readme (note that an Infura key and setting up the .env file is required).
2. Copy the below code block into the RootForktest.t.sol in the test/ulysses-omnichain directory
3. Run the test with: forge test --match-test testProofOfConcept -vvv (-vvv is necessary to show the errors for OutOfGas and LayerZero: in message blocking)

    function testProofOfConcept() public {
        
        //Switch Chain and Execute Incoming Packets
        switchToLzChain(avaxChainId);

        vm.deal(address(this), 10 ether);

        avaxCoreRouter.addLocalToken{value: 10 ether}(address(avaxMockAssetToken), GasParams(1, 0));

        //Switch Chain and Execute Incoming Packets

        /*  We expect the call to `RootBridgeAgent` to fail with an out of gas error,
            but this won't be caught here due to the `switchToLzChain` abstraction.
        */
        switchToLzChain(rootChainId);

        console2.log("RootBridgeAgent: ", address(arbitrumCoreBranchBridgeAgent).balance);
        avaxMockAssethToken = rootPort.getLocalTokenFromUnderlying(address(avaxMockAssetToken), avaxChainId);

        newAvaxAssetGlobalAddress = rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId);

        // We can now assert that it failed in the root chain
        require(
            rootPort.getGlobalTokenFromLocal(avaxMockAssethToken, avaxChainId) == address(0),
            "Token should be added"
        );
        require(
            rootPort.getLocalTokenFromGlobal(newAvaxAssetGlobalAddress, avaxChainId) == address(0),
            "Token should be added"
        );
        require(
            rootPort.getUnderlyingTokenFromLocal(avaxMockAssethToken, avaxChainId) == address(0),
            "Token should be added"
        );
        switchToLzChain(avaxChainId);

        vm.deal(address(this), 10 ether);

        MockERC20 validToken = new MockERC20("Valid Token", "VALID", 18);

        avaxCoreRouter.addLocalToken{value: 10 ether}(address(validToken), GasParams(2_000_000, 0));

        /*  Here we confirm that we the channel is blocked
            We can see in the stack trace in the console that the call on the Root chain
            was reverted with "LayerZero: in message blocking"
        */
        switchToLzChain(rootChainId);

    }

Tools Used

Manual code review. Foundry.

Recommended Mitigation Steps

As this submission demonstrates a high severity impact stemming from multiple root causes, the recommendations will be provided for each.

Issue A

Layer Zero acknowledges that a cross-chain call can use more or less gas than the standard 200k. For this reason it allows the passing of custom gas parameters. This overrides the default amount of gas used. By allowing users to directly set these custom gas parameters (without validation) it opens the Ulysses implementation up to vulnerabilities related to cross-chain gas inequalities.

Consider adding input validation within the BridgeAgents before a cross-chain call is commenced that ensures the gasLimit supplied is sufficient for the lzReceive call on the root chain. This can be expanded by calculating sufficient minimums for the various functions which are implemented (e.g. adding a token, bridging). An alternative would be to deny the user the ability to modify these params downward. The BranchBridgeAgent::getFeeEstimate() is already implemented, but never used in the contracts - this would be perfect for input validation.

Issue B The current implementation is designed to never allow failures from the Layer Zero Endpoint as it implements a non-blocking pattern. Due to Issue A the lzReceive call from Endpoint can be forced to fail. This blocks the message channel, violating the intended non-blocking pattern (and giving rise to this issue).

Consider inheriting from the Layer Zero non-blocking app example.

Issue C

It is highly recommended to implement the ILayerZeroApplicationConfig as it provides access to forceResumeReceive in the case of a channel blockage and allows the protocol to resume communication between these two chains. Most importantly, it will allow the team to resume messaging at a fraction of what it might cost to call retryPayload.

Assessed type

DoS

Low-01: Incorrectly labeled input paramaters may become confusing

In the RootPort contract the natspec, the named input parameters and the input types do not correspond. The chainId input variable is actually of an address type, and the localAddress/globalAddress is of the uint256 type. These were probably meant to be reversed.

    /// @notice Mapping with all global hTokens deployed in the system.
    mapping(address token => bool isGlobalToken) public isGlobalAddress;

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

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

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

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

Recommendation

The fix would be to simply rename the input parameters correctly (i.e. just swap chainId and localAddress around).

---

Low-02: It may become economically feasible to coerce depositNonce to overflow

On chains where gas costs are hugely reduced, it may become feasible for an attacker to increment the depositNonce such that it reaches the max of the uint32 type it uses.

Forcing this bricks the target BranchBridgeAgent as it can no longer accept any deposits or call out to the endpoint contracts.

This has been evaluated to low, as it does not cause losses and is not profitable at current gas costs and market conditions. The attack can impact the availability of the protocol. In addition, while the denial of service impact is high, it must be considered that there is little profit motive for the attacker. It does however still present a possible edge case in a sector where the price of assets (and thus the cost to execute an attack) can be volatile and where new technologies are increasingly lowering gas costs.

Although it is low, a PoC is provided to demonstrate the concept.

PoC

The below code demonstrates that an attacker can force the nonce overflow.

Assumptions:

• The chain must have an extremely low gas cost (either through design or market conditions)

The below code block can be placed in the RootFork.t.sol file in the test/ulysses-omnichain directory. Please take care to set up your local .env with an Infura key.

It can be run with forge test --match-test testCallOutNonce -vvv

   function testCallOutNonce() public {
        //Switch to avax
        switchToLzChainWithoutExecutePendingOrPacketUpdate(avaxChainId);

        // Give the attacker some ETH
        vm.deal(address(this), 1000000 ether);
        // Here we simply check the attacker's balance
        uint256 startingBalance = address(this).balance;
        console2.log("Attacker starting balance: ", startingBalance);

        /*  An attacker would now execute the below from an attacking contract
            There are some contraints which will dictate profitability:
            1) the local chain block gas limit  
            2) the cost of gas

            For some layer 2 chains the gas cost can become extremely small
            Even if the block gas limit is reached, it would simply mean 
            that the attacker would need to call `callOut` multiple times from
            the attacking contract, incrementing the nonce by some high amount
            each time. 

            In our example we are incrementing the nonce by some 100_000 in each call.
            The max value held by the `depositNonce` is 4,294,967,295
            
            This is obviously a value that is not expected to be reached during the normal
            course of business, but it is quite achievable by a griefer intending to inflict
            damage on the protocol on a low-tx cost chain.
        */
        for (uint256 i; i < 100_000; i++) {
            avaxMulticallBridgeAgent.callOut{value: 0.2 ether}(payable(address(this)), "", GasParams(2, 1));
            console2.log(avaxMulticallBridgeAgent.depositNonce());
        }

        // Here we simply check the attacker's balance
        uint256 endingBalance = address(this).balance;
        // The cost of the attack: 5 281 735 169 326 183 500 000
        // Not profitable at all in current market conditions,
        console2.log("Cost of attack (fees): ", startingBalance - endingBalance);
        // We can check the nonce here.
        console2.log(avaxMulticallBridgeAgent.depositNonce());
    }

From the PoC we can see it will take multiple attempts over some period of time (and some economic resources) to attack successfully. This is impactical in the current environment, but may change in the future.

Recommendations

Increase the size to uint256, or prevent empty calls that can be exploited to brick the branchBridgeAgent in this way.

---

Low-03: There is no validation for the payload size of cross-chain messages

For functions like addLocalToken or the callOut variants (see here and here for examples) where the payload can be an arbitrary bytes object, there is no implemented validation that the payload size is within the 10_000 limit required by the layer zero RelayerV2.

This may waste gas for users, as execution must continue up to the RelayerV2 before execution will be reverted. It is always better to revert as early as possible.

Recommendations

The best would be to determine the correct fees with the already implemented BranchBridgeAgent::getFeeEstimate() before the cross-chain call is initiated (because it will automatically check the payload size). The getFeeEstimate is not used anywhere in the protocol, but would prevent multiple issues such as this one.

Ulysses Protocol from Maia DAO

Description

The contest focus was on the Ulysses protocol contracts from Maia DAO.

The protocol uses Layer Zero as an inter-blockchain communication layer. This allows virtualization of liquidity which is spread across multiple chains.

Approach

The focus for this reviewer was the contract integration with the Layer Zero contracts to allow multichain communication.

Manual review of the code and the provided protocol documents was done. Approximately 4 hours per day over the course of 8 days were spent reviewing the code.

Architectural recommendations

The protocol consists of permissioned and non-permissioned components. The components of the protocol are spread in a hub-and-spoke pattern, with Arbitrum being the root chain and all the other chains being branch chains.

Users can permissionlessly create hTokens (which are representations of underlying tokens). These can then be moved around various blockchains.

There is a commendable effort to use as little centralization as possible.

Qualitative Analysis

Overall the code reviewed was of a great quality with good test coverage.

Centralization Risks

The protocol is designed in such a way that there is little direct risk to user funds as a consequence of admin privilige abuse.

It should be noted, however, that the Layer Zero communication protocol presents a singular point of failure. A failure in this regard may be, for example, Layer Zero pausing their contracts due to an exploit. It could also be due to a failure in any of the message channels.

Systemic Risks

An interesting risk to consider is that of local vs remote execution costs. As the Ulysses protocol operates across chains it may encounter novel attack patterns which exploit the difference in gas costs between chains (an example of this was made in one of the reviewer's QA submissions). In short: for some chains like zkEVMs with extremely low gas costs, a downward shift in market conditions might make it feasible to grief the protocol by incrementing the depositNonce in the BranchBridgeAgent contract via the callOut method. While economically unviable in the current market conditions, should execution cost fall far enough it may become a viable attack vector.

This means regular monitoring of the current gas and execution environment across relevant chains would be recommended. It is also crucial to enforce the correct gas parameters at the source chain in all functions that pass gasParams; at the moment these parameters can be set arbitrarily, allowing an attack path that could block message channels at will.

Time Spent

30 hours.

Time spent:

30 hours