Maia DAO - Ulysses - K42's results

Q/A Report for Maia-DAO-Ulysses

• I focused on some of the main contracts I saw for Q/A improvements for during my gas optimizations and analysis reporting for efficiency:

Q/A BaseBranchRouter.sol Contract

Possible Optimization 1: Use IERC20 Interface for Token Approvals

// Current Code:
ERC20(_hToken).approve(_localPortAddress, _amount - _deposit);

// Optimized Code:
IERC20(_hToken).approve(_localPortAddress, _amount - _deposit);

• Rationale: Using the IERC20 interface instead of ERC20 can make the contract more flexible and potentially reduce bytecode size, enhancing the contract's security by adhering to standard interfaces.

• Security Improvements: The use of the IERC20 standard interface simplifies the code and adheres to established best practices, reducing the attack surface.

Possible Optimization 2: Optimize the lock() Modifier

// Current Code:
modifier lock() {
    require(_unlocked == 1);
    _unlocked = 2;
    _;
    _unlocked = 1;
}


// Optimized Code:
modifier lock() {
    uint256 originalState = _unlocked;
    _unlocked = 2;
    _;
    _unlocked = originalState;
}

• Rationale: The optimization ensures that the _unlocked state variable is restored to its original state, making it safer in scenarios where nested calls might occur.
• Security Improvements: The code becomes more robust against re-entrancy attacks by ensuring the original state is restored, reducing the risk of vulnerabilities due to nested or malicious calls.

Q/A for BranchBridgeAgent.sol

Possible Optimization: Use External Calls Last (Checks-Effects-Interactions Pattern)

// Current Code:
depositNonce = _depositNonce + 1;
IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);


// Optimized Code:
IPort(localPortAddress).bridgeOut(msg.sender, _hToken, _token, _amount, _deposit);
depositNonce = _depositNonce + 1;

• Rationale: By adhering to the Checks-Effects-Interactions pattern, the optimized code reduces the risk of re-entrancy attacks.

• Security Improvements: The change increases the contract's security by ensuring that state changes occur after external calls, reducing the risk associated with re-entrancy attacks.

Gas Optimization Report for Maia-DAO-Ulysses by K42

Possible Optimization in ArbitrumBranchPort.sol

Possible Optimization =

• For the IRootPort contract interactions, you could minimize the number of external contract calls by using batch calls.

After Optimization:

IRootPort(_rootPortAddress).batchExecuteMintAndBurn(_recipient, _depositor, _globalToken, _globalAddress, _deposit, _amount);

• Estimated gas saved = Around 20,000 - 40,000 gas due to reducing the number of CALL opcodes.

Possible Optimizations in MulticallRootRouter.sol

Possible Optimization 1 =

• Use bytes4 for funcId instead of bytes1 to directly compare function selectors.

After Optimization:

bytes4 funcId = bytes4(encodedData[0:4]);

// Then update the if statement like so
if (funcId == bytes4(keccak256("execute(bytes,uint16)"))) {
    // ...
}

• Estimated gas saved = ~10-20 gas (Saves one or two bitwise operations)

Possible Optimization 2 =

• Use bitwise operations for _unlocked state variable to toggle its state.

After Optimization:

_unlocked ^= 1;

• Estimated gas saved = ~3-5 gas (Saves one ADD/SUB opcode)

Possible Optimization 3 =

• Use a single bytes array for multiple parameters in execute() and executeSigned() functions.

After Optimization:

function execute(bytes calldata encodedData, uint16) external payable override lock requiresExecutor {

    bytes1 funcId = encodedData[0];
    bytes memory remainingData = _decode(encodedData[1:]);

    if (funcId == 0x01) {
        _multicall(abi.decode(remainingData, (IMulticall.Call[])));
    } else if (funcId == 0x02 || funcId == 0x03) {
        (IMulticall.Call[] memory callData, bytes memory outputData) = abi.decode(remainingData, (IMulticall.Call[], bytes));
        _multicall(callData);
        _handleOutputData(outputData, funcId);
    } else {
        revert UnrecognizedFunctionId();
    }
}

• Estimated gas saved = Around 100-200 gas per call. This reduces the number of abi.decode operations.

Possible Optimizations in CoreRootRouter.sol

Possible Optimization 1 =

• Use bytes4 in this contract also, for funcId on line 304 and 334, instead of bytes1 to directly compare function selectors.

Here is the optimized code snippet:

bytes4 funcId = bytes4(_encodedData[0:4]);

• Estimated gas saved = ~10-20 gas (Saves one or two bitwise operations)

Possible Optimization 2 =

• Use delegatecall for calling IBridgeAgent(bridgeAgentAddress).callOut on line 139, 174, 199, 226, 257, 287 and 437 if the function does not change the state of the contract.

Here is the optimized code:

(address(bridgeAgentAddress).delegatecall(abi.encodeWithSignature("callOut(address,address,uint16,bytes,GasParams)", payable(_refundee), _refundee, _dstChainId, payload, _gParams)));

• Estimated gas saved = ~700-800 gas (DELEGATECALL is cheaper than CALL when state change is not needed).

Possible Optimization 3 =

• Use a single bytes array for multiple parameters in execute() and executeResponse() functions.

Here is the optimized code snippet:

function execute(bytes calldata encodedData, uint16) external payable override requiresExecutor {
    // Assuming you add previous optimization
    bytes4 funcId = bytes4(encodedData[0:4]);
    // Then define array here
    bytes memory remainingData = encodedData[4:];
    // ... (rest of the code)
}

• Estimated gas saved = Around 100-200 gas per call. This reduces the number of abi.decode operations.

Possible Optimization 4 =

• In the executeResponse() function, multiple abi.decode are used. You can decode all variables in a single abi.decode call.

Here is the optimized code snippet:

(address underlyingAddress, address localAddress, string memory name, string memory symbol, uint8 decimals, address globalAddress, address newBranchBridgeAgent, address rootBridgeAgent) = abi.decode(_encodedData[1:], (address, address, string, string, uint8, address, address, address));

• Estimated gas saved = ~20-40 gas due to reducing the number of abi.decode operations, saving gas.

Possible Optimization in CoreBranchRouter.sol

Possible Optimization =

• In the executeNoSettlement() function, you're using abi.decode multiple times for different conditions. You can use a single abi.decode to unpack all variables and then use them in the conditions.

Here is the optimized code snippet:

(bytes1 functionId, bytes memory params) = abi.decode(_params, (bytes1, bytes));
if (functionId == 0x01) {
    // Use params here
} else if (functionId == 0x02) {
    // Use params here
}

• Estimated gas saved = ~20-40 gas per call. This reduces the number of abi.decode operations, saving gas.

Possible Optimization in RootBridgeAgent.sol

Possible Optimization =

• Use bitwise operations for status checks instead of using equality checks for status in lzReceiveNonBlocking() function.

Here is the optimized code snippet:

// Modify here
if (executionState[_srcChainId][nonce] & STATUS_READY == STATUS_READY) {
    // ...
}

• Estimated gas saved = ~10-20 gas.

Possible Optimization in VirtualAccount.sol

Possible Optimization =

• Use unchecked for Incrementing Loop Counter in call() and payableCall() Functions. As the ++i operation in loops is subject to overflow checks by default since Solidity 0.8.x.

Here is the optimized code snippet:

// In the 'call' function
for (uint256 i = 0; i < length;) {
    // ... existing code ...
    unchecked {
        ++i;  // Remove overflow check for loop counter
    }
}

// Similarly in the 'payableCall' function
for (uint256 i = 0; i < length;) {
    // ... existing code ...
    unchecked {
        ++i;  // Remove overflow check for loop counter
    }
}

• Estimated gas saved = ~5-10 gas per iteration, due to avoiding the JUMPI opcode for overflow checks. This is safe, as the loop bounds are well-defined.

Possible Optimization in BranchBridgeAgent.sol

Possible Optimization =

• Use bytes4 for Function Selectors in lzReceive(() function.

Here is the optimized code:

// Current:
abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcAddress, _payload)

// Optimized: 
abi.encodeWithSelector(0x12345678, msg.sender, _srcAddress, _payload)  // Replace 0x12345678 with actual function selector

• Estimated gas saved = ~500-1000 gas due to directly using the function selector instead of computing it at runtime.

Possible Optimization in BaseBranchRouter.sol

Possible Optimization =

• Cache msg.sender in stack variable in the _transferAndApproveToken() function. Extra context for this one; this does partially overlap with [G-05] on the bot report, which suggests caching state variables in stack variables to save gas. However, msg.sender is not a state variable, so this is a distinct optimization.

Here is the optimized code snippet:

// Current:
_hToken.safeTransferFrom(msg.sender, address(this), _amount - _deposit);

// Optimized: 
address sender = msg.sender;
_hToken.safeTransferFrom(sender, address(this), _amount - _deposit);

• Estimated gas saved = ~200-400 gas per usage.

Possible Optimization in RootPort.sol

Possible Optimization =

• Use || and && to short-circuit the require statements in the initialize() function, reducing the number of JUMPI, ISZERO, and DUP executed.

Here is the optimized code snippet:

// Current:
require(_coreRootRouter != address(0), "Core Root Router cannot be 0 address.");
require(_coreRootBridgeAgent != address(0), "Core Root Bridge Agent cannot be 0 address.");

// Optimized:
require(_coreRootRouter != address(0) && _coreRootBridgeAgent != address(0), "Addresses cannot be 0.");

• Estimated gas saved = ~100-200 gas per usage.

Advanced Analysis Report for Maia-Dao-Ulysses

Overview

• During this audit, I focused on gas optimizations; see my gas report for more details. This analysis encapsulates my understanding of the audit scope, focusing on key contracts. The Ulysses Protocol is engineered to optimize liquidity across multiple blockchains, aiming to enhance capital efficiency and user experience, while increasing opportunity of asset utilization in the decentralized finance sector. This analysis revolves around three of the core contracts for efficiency: VirtualAccount.sol, BranchPort.sol, and RootPort.sol, each serving distinct functionalities from asset withdrawal to liquidity management and cross-chain interactions.

Understanding the Ecosystem:

• Virtualized liquidity and Virtual Accounts:

  • Key Contract in Scope: VirtualAccount.sol
  • Technical Insight: Handles asset withdrawals through withdrawNative(), withdrawERC20(), and withdrawERC721() functions.
  • Security: Access control relies on IRootPort(localPortAddress).isRouterApproved(), which could be a point of failure if compromised.

• Ports:

  • Key Contracts in Scope: BranchPort.sol, RootPort.sol, and ArbitrumBranchPort.sol
  • Technical Insight: Manages liquidity and asset transfers.
  • Security: No explicit reentrancy guards are present, posing a potential risk.

• Bridge Agents:

  • Key Contracts in Scope: ArbitrumBranchBridgeAgent.sol, RootBridgeAgentExecutor.sol, RootBridgeAgent.sol, BranchBridgeAgent.sol, BranchBridgeAgentExecutor.sol, RootBridgeAgentFactory.sol, and ArbitrumBranchBridgeAgentFactory.sol
  • Technical Insight: Facilitates cross-chain communication.
  • Security: Requires secure message-passing protocols to prevent MITM attacks.

Codebase Quality Analysis:

• 1. Structures and Libraries:

  • Uses custom modifiers like requiresApprovedCaller and lock for access control and reentrancy protection.

• 2. Security Measures:

  • Access control is primarily done through custom modifiers.
  • Reentrancy protection is attempted but not guaranteed.

• 3. Error Handling:

  • Uses revert statements with custom messages for debugging.

Architecture Recommendations:

• Consider using OpenZeppelin's AccessControl instead of current usage, for a more standardized approach to access control.
• Implement more specific error codes for easier debugging.

Centralization Risks:

• Dependency on IRootPort. for access control in VirtualAccount could lead to unauthorized access if compromised.

Mechanism Review:

• Access control is primarily done through custom modifiers.
• Asset management is handled through various withdraw and manage functions.

Systemic Risks:

• Contract failures due to bugs in the manage function could lead to liquidity issues.
• Access control failures if IRootPort. is compromised.

Areas of Concern:

• The contracts do not appear to be upgradeable.
• The lock modifier's effectiveness is not clear without full context.

Codebase Analysis:

• The contracts are modular but could benefit from using standard libraries for access control and storage.

VirtualAccount Contract:

• Functions: withdrawNative(), withdrawERC20(), and withdrawERC721()
• Purpose: These functions handle asset withdrawals.

Security Concerns:

• Access Control:
  • Modifier: requiresApprovedCaller
  • Dependency: Relies on IRootPort(localPortAddress).isRouterApproved()
  • Risk: If the IRootPort contract is compromised, unauthorized access could occur.
  • Context: The requiresApprovedCaller modifier delegates access control to an external contract ( IRootPort). If that contract is compromised, it could grant approval to malicious addresses.
  • Mitigation: Implement a decentralized access control mechanism that doesn't rely solely on IRootPort.

BranchPort Contract:

Technical Insights:

• Functions: manage
  • Purpose: Manages liquidity within the contract, allowing users to add or remove assets.

Security Concerns:

• Reentrancy Protection:
  • Modifier: lock
  • Risk: Potential for reentrancy attacks if _unlocked is not managed securely.
  • Mitigation: Use a well-vetted reentrancy guard such as OpenZeppelin's ReentrancyGuard.

RootPort Contract:

Security Analysis: Functions and Risks:

• initialize:

  • Risk: No validation for _setup variable, allowing for potential overwrites.
  • Context: Without validation, multiple calls to initialize could overwrite important state variables.
  • Mitigation: Implement a state variable to track initialization status.

• initializeCore:

  • Risk: No validation for _setupCore variable.
  • Context: Similar to initialize, multiple calls could overwrite core settings.
  • Mitigation: Implement a state variable to track core initialization status.

• renounceOwnership:

  • Risk: None. Function is overridden to revert.
  • Context: This is a good practice to prevent accidental renouncement of ownership.

• setAddresses:

  • Risk: Can overwrite existing mappings.
  • Context: Overwriting mappings without checks could lead to unintended consequences.
  • Mitigation: Implement checks to prevent unintended overwrites.

• addBridgeAgent:

  • Risk: Can add an arbitrary _bridgeAgent.
  • Context: Without a whitelist, any address could be added as a bridge agent, potentially compromising the system.
  • Mitigation: Implement a whitelist of approved agents.

• toggleBridgeAgent:

  • Risk: Can toggle the state of any _bridgeAgent.
  • Context: Toggling state without additional checks could enable or disable agents unintentionally.
  • Mitigation: Require multi-signature approval for this action.

• addNewChain:

  • Risk: Can add a new chain with arbitrary parameters.
  • Context: Adding a new chain without checks could introduce insecure or incompatible chains.
  • Mitigation: Require multi-signature approval for adding new chains.

• setCoreRootRouter:

  • Risk: Can change coreRootRouterAddress and coreRootBridgeAgentAddress.
  • Context: Changing these addresses without checks could redirect transactions to malicious contracts.
  • Mitigation: Implement a timelock for sensitive changes.

Recommendations:

• Formal Verification: Use tools like Certora, and extend the test coverage from 69% to 100%.
• Monitoring Tools: Use Tenderly and Defender for real-time monitoring.

Conclusion:

• The Ulysses Protocol is a complex ecosystem with multiple functionalities. The use of custom modifiers for access control and reentrancy protection is noted, but these need to be more rigorously tested. Given the dependency on IRootPort for access control in VirtualAccount.sol, a more standardized and decentralized solution for access control is recommended. Specific recommendations tied to individual functions have been provided for mitigating identified risks. More rigorous testing, 100% coverage using formal verification and invariant testing, and continued future third-party audits are crucial for its long-term success and security.

Time spent:

36 hours