Maia DAO - Ulysses - 0xbrett8571's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/e6201f9b07110ba15d3aa39355884bfada00bb34/src/ArbitrumBranchPort.sol#L73-L95 https://github.com/code-423n4/2023-09-maia/blob/e6201f9b07110ba15d3aa39355884bfada00bb34/src/ArbitrumBranchPort.sol#L86-L87 https://github.com/code-423n4/2023-09-maia/blob/e6201f9b07110ba15d3aa39355884bfada00bb34/src/ArbitrumBranchPort.sol#L92 https://github.com/code-423n4/2023-09-maia/blob/e6201f9b07110ba15d3aa39355884bfada00bb34/src/ArbitrumBranchPort.sol#L94

Vulnerability details

Impact

If _globalAddress is an invalid or non-existent token on the Root Port, the function will fail silently without providing any feedback. This can lead to confusion for users and make it difficult to identify the root cause of the issue.

Proof of Concept

In the withdrawFromPort function, the code assumes that any address provided as _globalAddress is a valid global token on the Root Port. However, it doesn't perform any validation to ensure that the provided address corresponds to an actual token.

The function withdrawFromPort lacks input validation for the _globalAddress parameter. Here's a breakdown of the relevant parts:

   function withdrawFromPort(address _depositor, address _recipient, address _globalAddress, uint256 _amount)
       external
       override
       lock
       requiresBridgeAgent

The function withdrawFromPort takes four parameters:

• _depositor: Address of the depositor.
• _recipient: Address of the recipient.
• _globalAddress: Address representing the global token.
• _amount: Amount to be withdrawn.

Local Variables:

   address _rootPortAddress = rootPortAddress;
   address _underlyingAddress = IRootPort(_rootPortAddress).getUnderlyingTokenFromLocal(_globalAddress, localChainId);

• _rootPortAddress: It stores the value of rootPortAddress which represents the address of the Root Port.
• _underlyingAddress: It fetches the underlying token address based on the provided _globalAddress.

Function Execution:

   IRootPort(_rootPortAddress).burnFromLocalBranch(_depositor, _globalAddress, _amount);
   _underlyingAddress.safeTransfer(_recipient, _amount);

• These lines execute the burn operation on the Root Port and the safe transfer of tokens.

Missing Input Validation:

We can see there is no explicit validation for the _globalAddress parameter. This means that the function assumes the provided _globalAddress is valid and does not check if it's a legitimate global token on the Root Port.

This can lead to potential issues if _globalAddress is an invalid or non-existent token on the Root Port. The code does not provide any feedback or error handling in such cases.

Consider this scenario

Suppose a user interacts with the withdrawFromPort function, providing an _globalAddress that corresponds to a token that doesn't exist on the Root Port.

Here's the scenario:

The user calls the withdrawFromPort function with the following parameters:

• _depositor: 0xUserA
• _recipient: 0xUserB
• _globalAddress: 0xInvalidToken (an address that doesn't represent any valid token on the Root Port)
• _amount: 100

   withdrawFromPort(0xUserA, 0xUserB, 0xInvalidToken, 100);

Execution:

The function starts executing. It doesn't perform any validation on the provided _globalAddress.

   address _underlyingAddress = IRootPort(_rootPortAddress).getUnderlyingTokenFromLocal(_globalAddress, localChainId);

• In this line, _underlyingAddress will receive address(0) because 0xInvalidToken doesn't exist on the Root Port.

   IRootPort(_rootPortAddress).burnFromLocalBranch(_depositor, _globalAddress, _amount);

• This line attempts to burn tokens from 0xUserA on the Root Port using _globalAddress, which is 0xInvalidToken. Since 0xInvalidToken is not a valid token, this operation will fail, and no tokens will be burnt.

   _underlyingAddress.safeTransfer(_recipient, _amount);

• Since _underlyingAddress is address(0), this line attempts to transfer tokens from address 0x0 to 0xUserB, which is not possible.

As a result of the scenario:

• The user's attempt to withdraw tokens fails silently without providing any feedback.
• No tokens are burnt from 0xUserA.
• No tokens are transferred to 0xUserB.

Without input validation for _globalAddress, the function assumes that any address provided is a valid global token on the Root Port. In reality, if an invalid address is used, the function will fail silently, potentially causing confusion for users and making it harder to identify the root cause of the issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Adding input validation for _globalAddress in the withdrawFromPort function can help prevent potential issues caused by providing an incorrect or invalid address. This validation should verify that the provided address corresponds to a valid token on the Root Port before proceeding with the withdrawal process.

Assessed type

Invalid Validation

Overview:

• Maia DAO has created a new cross-chain bridge system called Ulysses to connect chains and allow asset transfers between chains.

• The contest involves reviewing and analyzing the core smart contracts for security issues.

• There are 3 main smart contracts:

  • BranchBridgeAgent - manages bridged asset deposits and processes cross-chain requests
  • BranchBridgeAgentExecutor - executes settlement logic for cross-chain transfers
  • Port - holds bridged assets and releases them

Key Details:

• The BranchBridgeAgent acts as the endpoint for cross-chain messages using LayerZero. It processes deposits, executes cross-chain settlements, and coordinates settlements.

• The BranchBridgeAgentExecutor contains the logic for settling assets and transferring to recipients on destination chains. It is called by the BranchBridgeAgent.

• The Port holds bridged assets on each chain until they are ready to be released to recipients. It coordinates deposits and withdrawals.

• The contracts allow users to deposit assets, bridge them across chains, and later withdraw them on the destination chain.

• The contest requires analyzing the contracts for issues like reentrancy, access controls, withdrawal limits, gas limits, numerical errors, and more.

• The goal is to identify any security issues in the core bridge contracts before they are deployed. Rewards are available for different severity levels of issues.

This contest involves a security review of Maia DAO's new Ulysses cross-chain bridge system to identify any vulnerabilities before launch. The focus is on the core BranchBridgeAgent, BranchBridgeAgentExecutor and Port contracts.

Ulysses is a novel cross-chain bridge system that utilizes Arbitrum rollups to enable fast, low-cost asset transfers between chains. The core value proposition is virtualizing liquidity across chains to improve capital efficiency.

The system consists of Branch Bridge Agent contracts on each chain that coordinate settlements, a Branch Bridge Agent Executor that contains settlement logic, and Port contracts that hold bridged assets. This architecture decentralizes operations across chains while still enabling rapid settlements.

Analysis Approach

My analysis evaluated the Ulysses architecture, key mechanisms, risk areas and code quality:

• Reviewed the system design and architecture patterns used

• Analyzed core mechanisms like deposits, settlements, withdrawals, fallbacks

• Assessed centralization risks related to roles and privileges

• Evaluated code quality, comments, natspec documentation

• Identified systemic risks from economic incentives, governance, failure modes

Architecture

• Well-designed architecture that partitions responsibilities across different contracts based on roles

• Bridge Agent acts as endpoint for cross-chain messages and coordinates settlements

• Executor contains core settlement logic, minimizing need to deploy new contracts when changing logic

• Ports act as custodians, optimizing custody layouts per chain

• Arbitrum provides speed while still retaining security of Ethereum

• Additional chains can be easily integrated by deploying new Agents pointing to the core Executor

Recommendations

• Feature to allow monitoring deposits in Ports via events or externaltooling

• Consider allowing custom Executors per chain pair for advanced workflows

• Add more comprehensive natspec documentation for public interfaces

Mechanism Analysis

The core mechanisms for deposits, transfers, and settlements were analyzed:

• Deposit mechanism is secure - assets locked in Ports until bridged

• Cross-chain execution uses proper nonces and gas limits to prevent replays

• Fallback design protects users if settlements fail, allowing for retries

• Withdrawals release assets from Ports to recipients upon finalization

• Settlements limit asset discrepancies via atomic operations

• Mechanisms provide correctness guarantees even during network failures

• Sufficient validation on methods to prevent input errors

• Edge cases considered around settlement finality, fallback expiration, and more

Risk Analysis

• Minimal centralization risks - agents are decentralized and executor logic is modular

• No elevated privileges or special roles in the core contracts

• Economic incentives align - senders pay fees while fallbacks reimburse gas

• Failure modes considered - allows for retries and redemption during outages

• Key personnel risk somewhat mitigated given open source nature

• Potential liquidation risks if collateral seized during settlement is insufficient

Code Quality

• Code is well-structured and modular with logical separations

• Follows best practices like Checks-Effects-Interactions pattern

• Extensive natspec documentation on core interfaces

• Detailed comments explain the rationale behind core mechanisms

• Proper validation of inputs and return values from calls

• Lack of comprehensive test coverage is the main deficiency

Overall, Ulysses demonstrates strong architecture, mechanisms, and code quality, with some areas for improvement around tests and docs. The design minimizes centralization risks. Core risks stem from reimbursement ratios and settlement finality guarantees.

Time spent:

30 hours