Maia DAO Ecosystem - Evo's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L836 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1066 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1032 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L811

Vulnerability details

Impact

User underpay for the remote call execution gas, meaning Incorrect minExecCost that being deposited at _replenishGas call inside _payExecutionGas function.

Proof of Concept

Multi chain contracts - anycall v7 lines https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L265

https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L167

https://github.com/anyswap/multichain-smart-contracts/blob/645d0053d22ed63005b9414b5610879094932304/contracts/anycall/v7/AnycallV7Upgradeable.sol#L276

ulysses-omnichain contract lines https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L811

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L851

The user is paying incorrect minimum execution cost for Anycall Mutlichain L820, the value of minExecCost is calculated incorrectly. AnycallV7 protocol considers a premium fee _feeData.premium on top of the TX gas price which is not considered here.

let's get into the flow from the start, the anyExec call that being called by the executer L265 when an anycall request comes from a source chain includes chargeDestFee modifier

    function anyExec(
        address _to,
        bytes calldata _data,
        string calldata _appID,
        RequestContext calldata _ctx,
        bytes calldata _extdata
    )
        external
        virtual
        lock
        whenNotPaused
        chargeDestFee(_to, _ctx.flags)
        onlyMPC
    {
        IAnycallConfig(config).checkExec(_appID, _ctx.from, _to);

now, chargeDestFee modifier will call chargeFeeOnDestChain function as well at L167

/// @dev Charge an account for execution costs on this chain
/// @param _from The account to charge for execution costs
    modifier chargeDestFee(address _from, uint256 _flags) {
        if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
            uint256 _prevGasLeft = gasleft();
            _;
            IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
        } else {
            _;
        }
    }

as you see L198-L210, inside chargeFeeOnDestChain function is including_feeData.premium for the execution cost totalCost.

function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft)
        external
        onlyAnycallContract
    {
        if (!_isSet(mode, FREE_MODE)) {
            uint256 gasUsed = _prevGasLeft + EXECUTION_OVERHEAD - gasleft();
            uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);
            uint256 budget = executionBudget[_from];
            require(budget > totalCost, "no enough budget");
            executionBudget[_from] = budget - totalCost;
            _feeData.accruedFees += uint128(totalCost);
        }
    }

The conclusion: minExecCost calculation doesn't include _feeData.premium at L811 according to multichain AnycallV7 protocol.

You should include _feeData.premium as well in minExecCost same as L204

uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium);

Note: This also applicable on: _payFallbackGas() in RootBridgeAgent at L836 _payFallbackGas() in BranchBridgeAgent at L1066 _payExecutionGas in BranchBridgeAgent at L1032

Tools Used

Manual Review

Recommended Mitigation Steps

add _feeData.premium to minExecCost at _payExecutionGas function L811

you need to get _feeData.premium first from AnycallV7Config by premium() function at L286-L288

uint256 minExecCost = (tx.gasprice  + _feeData.premium) * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft()));

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1227-L1307

Vulnerability details

Impact

_payFallbackGas gas is not being paid for selectors 0x07 and 0x08 which causes a loss for protocol's execution gas budget. In case Execution budget is not enough then anyFallback will fail.

Proof of Concept

In _payFallbackGas() gas should always be paid in case selector (flag) is sent as 0x07 or 0x08 by calldata in anyFallback() L1227-L1307

    function anyFallback(bytes calldata data)
        external
        virtual
        requiresExecutor
        returns (bool success, bytes memory result)
    {
        //Get Initial Gas Checkpoint
        uint256 initialGas = gasleft();


        //Save Flag
        bytes1 flag = data[0];


        //Save memory for Deposit Nonce
        uint32 _depositNonce;


        /// DEPOSIT FLAG: 0, 1, 2
        if ((flag == 0x00) || (flag == 0x01) || (flag == 0x02)) {
            //Check nonce calldata slice.
            _depositNonce = uint32(bytes4(data[PARAMS_START:PARAMS_TKN_START]));


            //Make tokens available to depositor.
            _clearDeposit(_depositNonce);


            emit LogCalloutFail(flag, data, rootChainId);


            //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
            _payFallbackGas(_depositNonce, initialGas);


            return (true, "");


            /// DEPOSIT FLAG: 3
        } else if (flag == 0x03) {
            _depositNonce = uint32(bytes4(data[PARAMS_START + PARAMS_START:PARAMS_TKN_START + PARAMS_START]));


            //Make tokens available to depositor.
            _clearDeposit(_depositNonce);


            emit LogCalloutFail(flag, data, rootChainId);


            //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
            _payFallbackGas(_depositNonce, initialGas);


            return (true, "");


            /// DEPOSIT FLAG: 4, 5
        } else if ((flag == 0x04) || (flag == 0x05)) {
            //Save nonce
            _depositNonce = uint32(bytes4(data[PARAMS_START_SIGNED:PARAMS_START_SIGNED + PARAMS_TKN_START]));


            //Make tokens available to depositor.
            _clearDeposit(_depositNonce);


            emit LogCalloutFail(flag, data, rootChainId);


            //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
            _payFallbackGas for (_depositNonce, initialGas);


            return (true, "");


            /// DEPOSIT FLAG: 6
        } else if (flag == 0x06) {
            //Save nonce
            _depositNonce = uint32(
                bytes4(data[PARAMS_START_SIGNED + PARAMS_START:PARAMS_START_SIGNED + PARAMS_TKN_START + PARAMS_START])
            );


            //Make tokens available to depositor.
            _clearDeposit(_depositNonce);


            emit LogCalloutFail(flag, data, rootChainId);


            //Deduct gas costs from deposit and replenish this bridge agent's execution budget.
            _payFallbackGas(_depositNonce, initialGas);


            return (true, "");


            //Unrecognized Function Selector
        } else {
            return (false, "unknown selector");
        }
    }

The code above shows how no consideration to 0x07 and 0x08 selectors while 0x07 is retrySettlement() and 0x08 is retrieveDeposit() both are calling _sendRetrieveOrRetry() which is performing _createGasDeposit() and _performCall() that means a request to the Root Omni-chain. L418-L439

    function retrySettlement(uint32 _settlementNonce, uint128 _gasToBoostSettlement)
        external
        payable
        lock
        requiresFallbackGas
    {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(
            bytes1(0x07), depositNonce++, _settlementNonce, msg.value.toUint128(), _gasToBoostSettlement
        );
        //Update State and Perform Call
        _sendRetrieveOrRetry(packedData);
    }


    /// @inheritdoc IBranchBridgeAgent
    function retrieveDeposit(uint32 _depositNonce) external payable lock requiresFallbackGas {
        //Encode Data for cross-chain call.
        bytes memory packedData = abi.encodePacked(bytes1(0x08), _depositNonce, msg.value.toUint128(), uint128(0));


        //Update State and Perform Call
        _sendRetrieveOrRetry(packedData);
    }

Basically the User will not pay gas for anyFallback() when it is triggered. This happens when he tried to send a request by calling retrySettlement() or retrieveDeposit() and the request fails. It will cause a loss for protocol's execution gas budget.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider call _payFallbackGas() for 0x07 and 0x08 flags in anyFallback() function

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L411

Vulnerability details

Impact

loss of funds for the User as protocol doesn't add the correct depositedGas for him by adding the previous value of it when he tries retryDeposit() to top up gas.

Proof of Concept

Incase anyExecute call fails (forceRevert is done) at RootBridgeAgent (which a call to the Root Omnichain Environment retrying a failed deposit that hasn't been executed yet) a User is able to call retryDeposit() to top up gas and send the request again to the Root Chain which is the protocol purpose by offering this method to give another chance to the user to retry his request.

Now let's say a User tried to perform performCallOutAndBridge() and failed on root chain, now the gas that will be deposited is gasToBridgeOut which being calculated as wrapped msg.value L521-L527

        if (!isRemote && gasToBridgeOut > 0) wrappedNativeToken.deposit{value: msg.value}();

        //Check Fallback Gas
        _requiresFallbackGas(gasToBridgeOut);

        //Perform Call
        _callOutAndBridge(_depositor, _params, _dParams, gasToBridgeOut, _remoteExecutionGas);

As you see _callOutAndBridge() is being called to deposit the gas which will call _depositAndCall() L695-L697

        _depositAndCall(
            _depositor, packedData, _dParams.hToken, _dParams.token, _dParams.amount, _dParams.deposit, _gasToBridgeOut
        );

_createDepositSingle() will be called as well and Deposit Gas to Port L915 and L929-L931

        IPort(localPortAddress).bridgeOut(_user, _hToken, _token, _amount, _deposit);


        //Deposit Gas to Port
        _depositGas(_gasToBridgeOut);

    function _depositGas(uint128 _gasToBridgeOut) internal virtual {
        address(wrappedNativeToken).safeTransfer(localPortAddress, _gasToBridgeOut);
    }

The issue is happening when retryDeposit() doesn't calculate the correct depositedGas by adding the old deposited Gas to the new one inside retryDeposit() function.

retryDeposit() is overriding the deposited Gas value instead of adding to it L411 :

   //Update Deposited Gas
    getDeposit[_depositNonce].depositedGas = msg.value.toUint128();

The protocol should consider the old deposited value that the user has done when the request failed on the root chain otherwise it will result a loss of funds to the user and not being counted into new depositedGas value.

Tools Used

Manual Review

Recommended Mitigation Steps

Change L411 to:

   //Update Deposited Gas
    getDeposit[_depositNonce].depositedGas += msg.value.toUint128();

so instead of overriding the value basically you are adding to the old saved value (the previous deposited gas)

Assessed type

Other

Analysis *

Any comments for the judge to contextualize your findings: My findings in this project are relying on big part of questions to the Sponsor since not enough description for example: Omni-chain operations flows of the concept, my Audit findings mostly in 3 scopes Maia, Hermes, Omni-chain. so I think that judges always get back to the sponsor for tiny details in the concept because it has different scopes as to know some cases how the protocol should work as intended, for example specific constant values or conditions that fixed in the protocol involved in math validations or amounts range.

Approach taken in evaluating the codebase: Basically I used the same notes that I did written while auditing the code so I read them again and see what I was focusing on, because evaluating a codebase it should take different time than audit as they have different process so I had to rely on my audit skills while not really have experience in evaluating a codebase project and such. I would say I was exploring the code after submit a finding and that helped me more to understand how the code is readable for example and a way to confirm my submitted issues. this codebase is big to be honest and it takes a while just evaluate few scopes in it, lets say Omni-chain concept that is using Multichain contracts cross chain router protocol it got me in time and exploring for while for their docs also how the implementation is done in the project (which 80% understood to me not fully) so am not sure if this approach did help me to create a clear image about each scope in the project as concept design, flow.

Architecture recommendations: (only a feedback) Using cross-chain protocol aim to enable interoperability between different blockchain networks, which allowing the transfer of assets and data across these networks, while using arbitrum chain is really a good choice because It aims to enhance scalability and reduce transaction fees on the Ethereum network and supporting optimistic rollup. but it is totally different story when it comes the project concept design and implementation. Maia Omni-chain is deeply customized with the sponsor perspective we are talking here for example about sending and receiving messages between two different chains which it could be achieved in different ways but with taking care of the chain transaction gas fees and user gas fees of the cross chain messaging concept, it is a sensitive area to implement in the project. my experience was great while observing the implementation of gas flow in the concept but I still see different implementation possible or maybe close to the current one with small modifications. It was hard in the biggening to understand the process flow for example between BranchBridgeAgent and RootBridgeAgent ending with RootBridgeAgentExecutor but what helped me actually is sponsor answers to my questions, I have no recommendations atm while this is a feedback for my experience during exploring the concept design and code structure.

Codebase quality analysis:

• comments in the code are missing few things like: -- some state variables has no explanations -- some core functions needs some or more description
• the functions order in the contract can get better for example: external function first then internal come after while view function at the bottom of the contract.. (my perspective).
• functions blocks are too much and they can be limited into few.
• unsupported functions in the protocol are not clear.
• functions names somehow are repeated and make some confusion.
• some comments are just copied into different part of code place while it's not correct or not apply to the code explanation. (above could have examples to add better clarity to the points but you can ask me if you are interested).

Centralization risks I would mention Arbitrum chain that is relying on centralized sequencer, it uses currently a centralized sequencer operated by Offchain Labs. It has the ability to control the ordering of transactions which could lead to the sequencer front-running transactions and profiting at the user's expense.

Time spent:

125 hours