Maia DAO - Ulysses - lsaudit's results

Lines of code

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

Vulnerability details

Impact

Contract does not follow the LayerZero Best Practice, which straightforwardly states, that forceResumeReceive should be implemented.

It is highly recommended User Applications implement the ILayerZeroUserApplicationConfig. Implementing this interface will provide you with forceResumeReceive which, in the worse case can allow the owner/multisig to unblock the queue of messages if something unexpected happens.

In case of any unexpected error - application does not provide any mechanism to unblock the queue of messages.

Proof of Concept

Even though, application implements non-blocking mechanism, it does not inherit from NonblockingLzApp - https://layerzero.gitbook.io/docs/evm-guides/advanced/nonblockinglzapp - which guarantees that the non-blocking mechanism will be working correctly.

This is also mentioned in the LayerZero Integration Checklist:

Use the latest version of solidity-examples package. Do not copy contracts from LayerZero repositories directly to your project.

However, protocol does not use NonblockingLzApp from solidity-examples, but it's implementing own non-blocking mechanism:

File: RootBridgeAgent.sol

 function lzReceiveNonBlocking(

File: BranchBridgeAgent.sol

 function lzReceiveNonBlocking(

Since protocol does not use provided NonblockingLzApp and it uses its own implementation - there's no guarantee, that this mechanism won't misbehave in the future. Whenever that happens, according to LayerZero docs, the default behavior is that when the transaction on the destination fails, the channel between the source and the destination is blocked. Because the protocol does not implement forceResumeReceive, it won't be possible to unblock it.

According to Code4rena Severity Categorization:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Since the availability of the protocol could be impacted (channel cannot be unblocked due to lack of forceResumeReceive) and since it's a hypothetical attack path with stated assumptions (protocol implements own non-blocking mechanism, instead of using NonblockingLzApp - which might not work correctly) - the severity has been evaluated as Medium.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement forceResumeReceive function. Moreover, please keep in mind that forceResumeReceive should be called only by user with higher privilege rights. Since Bridge Agents does not implement functions with admin privileges (e.g. like 'owner' EOA) - solving this issue might require large redeployment of the code-base.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L1212 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L943

Vulnerability details

Impact

LayerZero implements something called Trusted Remotes. According to LayerZero documentation:

A trusted remote is the 40 bytes (for evm-to-evm messaging) that identifies another contract which you will receive messages from within your LayerZero User Application contract. The 40 bytes object is the packed bytes of the  remoteAddress + localAddress

Contract uses its own implementation of Trusted Remotes and implements it as requiresEndpoint modifier. However, the implementation is wrong, since it compares the wrong part of _srcAddress.

This issue had been discussed with the sponsor on the Discord private message. They stated, that this issue - quoting - "would prevent functioning upon deployment of system causing the need of redeployment (...) should qualify as a valid medium since its a sort of DDOS".

Proof of Concept

File: RootBridgeAgent.sol

if (getBranchBridgeAgent[_srcChain] != address(uint160(bytes20(_srcAddress[PARAMS_ADDRESS_SIZE:])))) {

File: BranchBridgeAgent.sol

if (rootBridgeAgentAddress != address(uint160(bytes20(_srcAddress[20:])))) revert LayerZeroUnauthorizedCaller();

As demonstrated above, the implementation compares getBranchBridgeAgent[_srcChain] and rootBridgeAgentAddress with the 2nd part of the _srcAddress, instead of the first part of the _srcAddress.

Tools Used

Manual code review

Recommended Mitigation Steps

Change _srcAddress[PARAMS_ADDRESS_SIZE:] to _srcAddress[:PARAMS_ADDRESS_SIZE] in RootBridgeAgent.sol Change _srcAddress[20:] to _srcAddress[:20] in BranchBridgeAgent.sol.

Assessed type

Access Control

[QA-01] Lack of following LayerZero Integration Checklist by hardcoding zroPaymentAddress

The LayerZero Integration Checklist states:

• Do not hardcode address zero (address(0)) as zroPaymentAddress when estimating fees and sending messages. Pass it as a parameter instead.

The issue affects below instances: File: BranchBridgeAgent.sol

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

File: BranchBridgeAgent.sol

       ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
            rootChainId,
            rootBridgeAgentPath,
            abi.encodePacked(bytes1(0x09), _settlementNonce),
            _refundee,
            address(0),
            ""
        );

File: RootBridgeAgent.sol

            ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
                _dstChainId,
                getBranchBridgeAgentPath[_dstChainId],
                _payload,
                _refundee,
                address(0),
                abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
            );

File: RootBridgeAgent.sol

            ILayerZeroEndpoint(lzEndpointAddress).send{value: _value}(
                _dstChainId,
                getBranchBridgeAgentPath[_dstChainId],
                payload,
                payable(_refundee),
                address(0),
                abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
            );

File: RootBridgeAgent.sol

        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}(
            _dstChainId,
            getBranchBridgeAgentPath[_dstChainId],
            abi.encodePacked(bytes1(0x04), _depositNonce),
            payable(_refundee),
            address(0),
            ""
        );

According to ILayerZeroEndpoint interface, the 5th parameter of send function is _zroPaymentAddress,

File: ILayerZeroEndpoint.sol

    function send(
        uint16 _dstChainId,
        bytes calldata _destination,
        bytes calldata _payload,
        address payable _refundAddress,
        address _zroPaymentAddress,
        bytes calldata _adapterParams
    ) external payable;

which implies that this LayerZero Integration Checklist requirement is not fullfilled, because both RootBridgeAgent.sol and BranchBridgeAgent.sol hardcode _zroPaymentAddress to address(0), instead of passing it as parameter.

[QA-02] Invalid comment describing GasParams

Issue occurs in: File: BridgeAgentStructs.sol

The GasParam is being used for crafting the _adapterParams for ILayerZeroEndpoint.send() call.

However, this comment: gas allocated for remote branch execution. Must be lower than `gasLimit` is incorrect and seems to be a leftover from the previous architecture. Especially, that even LayerZero documentation mentions that the native gas value is bigger than the gasLimit - https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop

[QA-03] Avoid using toggle functions

It's good security practice to avoid toggle functions in the blockchain, because their result may not lead to the expected result - if the first transaction you sent was not correctly submitted (but it's just pending for a very long period of time). This will lead to double-toggling the state (which basically means - setting the initial value again).

Protocol implements multiple of toggle functions:

• File: BranchPort.sol

    function toggleBridgeAgentFactory(address _newBridgeAgentFactory) external override requiresCoreRouter {
        isBridgeAgentFactory[_newBridgeAgentFactory] = !isBridgeAgentFactory[_newBridgeAgentFactory];

        emit BridgeAgentFactoryToggled(_newBridgeAgentFactory);
    }

    /// @inheritdoc IBranchPort
    function toggleBridgeAgent(address _bridgeAgent) external override requiresCoreRouter {
        isBridgeAgent[_bridgeAgent] = !isBridgeAgent[_bridgeAgent];

        emit BridgeAgentToggled(_bridgeAgent);
    }


    /// @inheritdoc IBranchPort
    function toggleStrategyToken(address _token) external override requiresCoreRouter {
        isStrategyToken[_token] = !isStrategyToken[_token];

        emit StrategyTokenToggled(_token);
    }

    function togglePortStrategy(address _portStrategy, address _token) external override requiresCoreRouter {
        isPortStrategy[_portStrategy][_token] = !isPortStrategy[_portStrategy][_token];

        emit PortStrategyToggled(_portStrategy, _token);
    }

• File: RootPort.sol

  /// @inheritdoc IRootPort
    function toggleVirtualAccountApproved(VirtualAccount _userAccount, address _router)
        external
        override
        requiresBridgeAgent
    {
        isRouterApproved[_userAccount][_router] = !isRouterApproved[_userAccount][_router];
    }

     function toggleBridgeAgent(address _bridgeAgent) external override onlyOwner {
        isBridgeAgent[_bridgeAgent] = !isBridgeAgent[_bridgeAgent];

        emit BridgeAgentToggled(_bridgeAgent);
    }

     function toggleBridgeAgentFactory(address _bridgeAgentFactory) external override onlyOwner {
        isBridgeAgentFactory[_bridgeAgentFactory] = !isBridgeAgentFactory[_bridgeAgentFactory];

        emit BridgeAgentFactoryToggled(_bridgeAgentFactory);
    }

Similar findings have been evaluated as low-risk in the previous Code4rena contests:

• https://code4rena.com/reports/2021-06-realitycards#l-24-dangerous-toggle-functions

Instead of creating a toggle-like functions - create two separate functions - first one to enable the state, and the 2nd one - to disable it.

[QA-04] Lack of following LayerZero Integration Checklist by hardcoding useZro

The LayerZero Integration Checklist states:

Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.

In function getFeeEstimate() we're calling ILayerZeroEndpoint(lzEndpointAddress).estimateFees, which hardcodes useZro to false, instead of passing it as parameter:

File: RootBridgeAgent.sol

 (_fee,) = ILayerZeroEndpoint(lzEndpointAddress).estimateFees(
            _dstChainId,
            address(this),
            _payload,
            false,
            abi.encodePacked(uint16(2), _gasLimit, _remoteBranchExecutionGas, getBranchBridgeAgent[_dstChainId])
        );

File: BranchBridgeAgent.sol

        (_fee,) = ILayerZeroEndpoint(lzEndpointAddress).estimateFees(
            rootChainId,
            address(this),
            _payload,
            false,
            abi.encodePacked(uint16(2), _gasLimit, _remoteBranchExecutionGas, rootBridgeAgentAddress)
        );

This seem to intended way of calculating fees - since the protocol does not use ZRO to pay for the messages and does not pose any security threat. Nonetheless, it violates the LayerZero Integration Checklist, thus is had to be mentioned in the QA Report.

[N-01] Risk of delivery messages across chains

Protocol uses LayerZero to transport messages across different chains. In case of LayerZero being compromised, the worst case scenario might include forges of these messages. This may be causing havoc and possibility of funds loss. Even though, using the LayerZero is the conscious choice for the developers - this risk should also be taken into a consideration and mentioned in the report. For the reference, similar issues has been reported as Informational - in other web3 security report.

[G-01] Ordering of require statements can be changed in RootPort.sol

Performing checks inside require statements costs gas. This implies, that checks which are more likely to revert should be execute before checks which are less likely to revert. If first require reverts, then the 2nd one won't be executed (thus there will be no gas paid for executing the 2nd require)

Both initialize and initializeCore can be called only once, thus checking that initialize and initializeCore had already been initialized - is the most likely scenario. This implies, that require(_setup, "Setup ended."); and require(_setupCore, "Core Setup ended.");, should be the first require statement.

The code-base after the fix should look as below:

File: RootPort.sol

 function initialize(address _bridgeAgentFactory, address _coreRootRouter) external onlyOwner {
        require(_setup, "Setup ended.");
        require(_bridgeAgentFactory != address(0), "Bridge Agent Factory cannot be 0 address.");
        require(_coreRootRouter != address(0), "Core Root Router cannot be 0 address.");
        _setup = false;

File: RootPort.sol

  function initializeCore(
        address _coreRootBridgeAgent,
        address _coreLocalBranchBridgeAgent,
        address _localBranchPortAddress
    ) external onlyOwner {
        require(_setupCore, "Core Setup ended.");
        require(_coreRootBridgeAgent != address(0), "Core Root Bridge Agent cannot be 0 address.");
        require(_coreLocalBranchBridgeAgent != address(0), "Core Local Branch Bridge Agent cannot be 0 address.");
        require(_localBranchPortAddress != address(0), "Local Branch Port Address cannot be 0 address.");
        require(isBridgeAgent[_coreRootBridgeAgent], "Core Bridge Agent doesn't exist.");
        

[G-02] Updating Port Strategy Token Debt in BranchPort.sol can be unchecked

File: BranchPort.sol

        uint256 amountToWithdraw = portStrategyTokenDebt < reservesLacking ? portStrategyTokenDebt : reservesLacking;

        // Update Port Strategy Token Debt
        getPortStrategyTokenDebt[_strategy][_token] = portStrategyTokenDebt - amountToWithdraw;

Because of the condition: portStrategyTokenDebt < reservesLacking ? portStrategyTokenDebt : reservesLacking, the expression: getPortStrategyTokenDebt[_strategy][_token] = portStrategyTokenDebt - amountToWithdraw; will never underflow, thus it can be unchecked.

[G-03] Length of arrays are calculated twice

As calculated length of the array is a costly operation - doing it twice is unnecessary. The more efficient approach would be to cache length of _hTokens, _tokens, _amounts, instead of calculating it twice:

File: BranchBridgeAgent.sol

        if (_hTokens.length > MAX_TOKENS_LENGTH) revert InvalidInput();
        if (_hTokens.length != _tokens.length) revert InvalidInput();
        if (_tokens.length != _amounts.length) revert InvalidInput();
        if (_amounts.length != _deposits.length) revert InvalidInput();

The length of _globalAddresses is calculated 4 times. The length of _amounts - twice. The more efficient approach would be to cache the length, instead of calculating it multiple of times.

File: RootBridgeAgent.sol

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);

[G-04] Redundant check in redeemDeposit

In redeemDeposit, we firstly check if deposit.owner is not address(0) and then if deposit.owner is msg.sender. The first check is unessesary. msg.sender cannot be address(0), so deposit.owner == msg.sender implies that deposit.owner != address(0).

File: BranchBridgeAgent.sol

        if (deposit.owner == address(0)) revert DepositRedeemUnavailable();
        if (deposit.owner != msg.sender) revert NotDepositOwner();

Line if (deposit.owner == address(0)) revert DepositRedeemUnavailable() can be removed.

[G-05] Calculation for additional calldata in the payload in executeWithDepositMultiple can be done once

File: RootBridgeAgentExecutor.sol

219:        if (
220:            _payload.length
221:                > PARAMS_END_SIGNED_OFFSET
222:                    + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE
223:        ) {
224:            //Execute remote request
225:            IRouter(_router).executeSignedDepositMultiple{value: msg.value}(
226:                _payload[
227:                    PARAMS_END_SIGNED_OFFSET
228:                        + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE:
229:               ],

PARAMS_END_SIGNED_OFFSET + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE can be calculated once, the result cached in the variable and then used in line 221-222 and 227-228, instead of being calculated twice.

File: RootBridgeAgentExecutor.sol

134:        if (length > PARAMS_END_OFFSET + (numOfAssets * PARAMS_TKN_SET_SIZE_MULTIPLE)) {
135:            //Try to execute remote request
136:            IRouter(_router).executeDepositMultiple{value: msg.value}(
137:                _payload[PARAMS_END_OFFSET + uint256(numOfAssets) * PARAMS_TKN_SET_SIZE_MULTIPLE:], dParams, _srcChainId
138:            );

PARAMS_END_OFFSET + (numOfAssets * PARAMS_TKN_SET_SIZE_MULTIPLE) can be calculated once, the result cached in the variable and then used in line 134 and 137, instead of being calculated twice.

[G-06] Calculations on constants can be pre-calculated

File: RootBridgeAgent.sol

nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED + PARAMS_START:PARAMS_START_SIGNED + PARAMS_TKN_START]));

PARAMS_START_SIGNED + PARAMS_START and PARAMS_START_SIGNED + PARAMS_TKN_START can be calculated before the compilation time.

[G-07] _performRetrySettlementCall in RootBridgeAgent.sol can be optimized for gas usage

There are multiple of issues which should be done to optimize this function:

• Getting destination Branch Bridge Agent and checking if valid destination has been returned should be moved on top

File: RootBridgeAgent.sol

        address callee = getBranchBridgeAgent[_dstChainId];

        // Check if valid destination
        if (callee == address(0)) revert UnrecognizedBridgeAgent();

Those line should be moved on top of the function. If callee returns address(0), we'll be saving gas, due to reverting immediately.

• _hTokens.length should be calculated once and cached

_hTokens.length is being checked in multiple of if's conditions. Cache the _hTokens.length into a variable and use that variable, instead of calculating the length every time.

• else if (line 891) should be changed to else Firstly we're comparing: if (_hTokens.length == 0). If it is - function reverts with SettlementRetryUnavailableUseCallout() Then, another if condition occurs. We're comparing: if (_hTokens.length == 1). If it's not, then, we're comparing: else if (_hTokens.length > 1).

Please notice, that since _hTokens.length is not 0 (we've checked that at line 871) and if it's not 1 (we've checked that at line 877), then _hTokens.length has to be greater than 1. This implies, that else if (_hTokens.length > 1) can be changed to a simple: else.

[G-08] Parsing payload can be better optimized

To understand this issue more clearly, please take a look at function execute in MulticallRootRouter.sol. Firstly, it extracts the first byte (parse funcId): bytes1 funcId = encodedData[0]; and then - it compares that funcId in if-else branches. This is more optimized version than accessing encodedData[0] ins every if-else statement.

To prove that, you can check these two example functions:

  function aaa(bytes calldata a) public {
        uint x;

        uint s = gasleft();
        if (a[0] == 0x00) x = 0;
        else if (a[0] == 0x01) x = 1;
        else if (a[0] == 0x02) x = 2;
        else if (a[0] == 0x03) x = 3;
        else x = 4;
        console.log(s - gasleft());
    }

      function bbb(bytes calldata a) public {
        uint x;

        uint s = gasleft();
        bytes1 z = a[0];
        
        if (z == 0x00) x = 0;
        else if (z == 0x01) x = 1;
        else if (z == 0x02) x = 2;
        else if (z == 0x03) x = 3;
        else x = 4;
        console.log(s - gasleft());
    }

Calling bbb() (237 gas usage) is more optimized than aaa() (382 gas usage)

However, in multiple of instances, first byte of payload is not cached in the variable, but it's being compared in every if-else branch:

• File: ArbitrumCoreBranchRouter.sol

./src/ArbitrumCoreBranchRouter.sol:127:        if (_data[0] == 0x02) {
./src/ArbitrumCoreBranchRouter.sol:146:        } else if (_data[0] == 0x03) {
./src/ArbitrumCoreBranchRouter.sol:152:        } else if (_data[0] == 0x04) {
./src/ArbitrumCoreBranchRouter.sol:157:        } else if (_data[0] == 0x05) {
./src/ArbitrumCoreBranchRouter.sol:162:        } else if (_data[0] == 0x06) {

• File: CoreBranchRouter.sol

./src/CoreBranchRouter.sol:88:        if (_params[0] == 0x01) {
./src/CoreBranchRouter.sol:100:        } else if (_params[0] == 0x02) {
./src/CoreBranchRouter.sol:115:        } else if (_params[0] == 0x03) {
./src/CoreBranchRouter.sol:121:        } else if (_params[0] == 0x04) {
./src/CoreBranchRouter.sol:127:        } else if (_params[0] == 0x05) {
./src/CoreBranchRouter.sol:133:        } else if (_params[0] == 0x06) {
./src/CoreBranchRouter.sol:140:        } else if (_params[0] == 0x07) {

• File: RootBridgeAgent.sol

./src/RootBridgeAgent.sol:444:        if (_payload[0] == 0x00) {
./src/RootBridgeAgent.sol:467:        } else if (_payload[0] == 0x01) {
./src/RootBridgeAgent.sol:490:        } else if (_payload[0] == 0x02) {
./src/RootBridgeAgent.sol:513:        } else if (_payload[0] == 0x03) {
./src/RootBridgeAgent.sol:539:        } else if (_payload[0] == 0x04) {
./src/RootBridgeAgent.sol:577:        } else if (_payload[0] & 0x7F == 0x05) {
./src/RootBridgeAgent.sol:600:                _payload[0] == 0x85,
./src/RootBridgeAgent.sol:617:        } else if (_payload[0] & 0x7F == 0x06) {
./src/RootBridgeAgent.sol:640:                _payload[0] == 0x86,
./src/RootBridgeAgent.sol:657:        } else if (_payload[0] & 0x7F == 0x07) {
./src/RootBridgeAgent.sol:684:                _payload[0] == 0x87,
./src/RootBridgeAgent.sol:699:        } else if (_payload[0] == 0x08) {
./src/RootBridgeAgent.sol:718:        } else if (_payload[0] == 0x09) {

Instead of extracting the first byte every time, do it once, assign the result to the variable, and then use that variable. See how it's done in the previously mentioned MulticallRootRouter.sol.

[G-09] retryDeposit in BranchBridgeAgent.sol should not calculate deposit.hTokens.length twice

uint8(deposit.hTokens.length) is being calculated twice. Firstly, in the first condition, and then, when that condition fails - in the else-if:

File: BranchBridgeAgent.sol

 if (uint8(deposit.hTokens.length) == 1) {

 (...)

   } else if (uint8(deposit.hTokens.length) > 1) {

Instead of calculating the length of deposit.hTokens twice - cache the result in the variable and use that variable in the conditional branches.

[G-10] Do-while loops are cheaper than for loops

Using do-while loops are better for saving gas than for-loops.

Multiple of for-loops can be rewritten to do-while loops.

./src/BaseBranchRouter.sol:        for (uint256 i = 0; i < _hTokens.length;) {
./src/BranchBridgeAgent.sol:        for (uint256 i = 0; i < deposit.tokens.length;) {
./src/BranchBridgeAgent.sol:        for (uint256 i = 0; i < numOfAssets;) {
./src/BranchPort.sol:        for (uint256 i = 0; i < length;) {
./src/BranchPort.sol:        for (uint256 i = 0; i < length;) {
./src/MulticallRootRouter.sol:            for (uint256 i = 0; i < outputParams.outputTokens.length;) {
./src/MulticallRootRouter.sol:            for (uint256 i = 0; i < outputParams.outputTokens.length;) {
./src/MulticallRootRouter.sol:            for (uint256 i = 0; i < outputParams.outputTokens.length;) {
./src/MulticallRootRouter.sol:        for (uint256 i = 0; i < outputTokens.length;) {
./src/RootBridgeAgent.sol:        for (uint256 i = 0; i < settlement.hTokens.length;) {
./src/RootBridgeAgent.sol:        for (uint256 i = 0; i < length;) {
./src/RootBridgeAgent.sol:        for (uint256 i = 0; i < hTokens.length;) {
./src/RootBridgeAgentExecutor.sol:        for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {
./src/VirtualAccount.sol:        for (uint256 i = 0; i < length;) {
./src/VirtualAccount.sol:        for (uint256 i = 0; i < length;) {

[G-11] uint256(uint8(numOfAssets)) should not be looked up in every loop of a for-loop

File: RootBridgeAgentExecutor.sol

for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {

Every iteration of loop calculates uint256(uint8(numOfAssets)). This operation can be cached in local variable instead of looking it up in every for-loop iteration:

uint256 numOfAssetLoop = uint256(uint8(numOfAssets));
for (uint256 i = 0; i < numOfAssetLoop;) {

[G-12] Make 3 event parameters indexed when possible

The most gas efficient is to make up to 3 event parameters indexed. If there are less than 3 parameters, all of them should be indexed.

./src/interfaces/IBranchPort.sol:220:    event DebtCreated(address indexed _strategy, address indexed _token, uint256 _amount);
./src/interfaces/IBranchPort.sol:221:    event DebtRepaid(address indexed _strategy, address indexed _token, uint256 _amount);
./src/interfaces/IRootPort.sol:380:    event VirtualAccountCreated(address indexed user, address account);

Introduction

Since I took part in the previous Maia DAO contest at Code4rena, I'm somehow familiar with the changes which had been made. The developers decided to switch from anyCall to LayerZero. As using LayerZero is pretty new concept which was not observed in the previous Maia DAO code-base – this Analysis Report is focused only on that code related to LayerZero integration.

Approach Taken

The most of the Code4rena contests are being performed in a security review approach. During the given time-frame, the code is being analyzed to identify as many vulnerabilities and security issues as possible. The web3 security review is somehow related to web2 penetration test and/or vulnerability assessment – its goal is to identify as many issues as possible and report them back to the developers. During the contest – I took the same approach, which allowed me to identify multiple of Medium and Low risk issues. However, for this Analysis Report, I’ve decide to take a different approach – an audit. An audit, is a sort of checklist, which allows to verify if previously defined security assumptions are fulfilled. I decided to use two resources which allowed me to prepare such a checklist – and verified the LayerZero integration. Those are:

• LayerZero Integration Checklist
• Smart Contract Security Verification Standard - I4 - Cross-Chain

LayerZero Integration Checklist

• Use the latest version of solidity-examples package. Do not copy contracts from LayerZero repositories directly to your project.

Protocol implements own non-blocking mechanism. It does not copy-paste code directly from LayerZero repository.

• If your project requires token bridging inherit your token from OFT or ONFT. For new tokens use OFT or ONFT, for bridging existing tokens use ProxyOFT or ProxyONFT.

OFT is Omnichain Fungible Token. Tokens implementation do not inherit from OFT. Developers chose their own implementation - with less extra features that don't seem to be desired. Those tokens (ERC20hTokenRoot, ERC20hTokenBranch) implement their own burn() and mint() functions. The rest is inherited from ERC20.

• For bridging only between EVM chains use OFT and for bridging between EVM and non EVM chains (e.g., Aptos) use OFTV2.

Protocol does not use LayerZero's Omnichain Fungible Token implementation.

• Do not hardcode address zero (address(0)) as zroPaymentAddress when estimating fees and sending messages. Pass it as a parameter instead.

In every ILayerZeroEndpoint(lzEndpointAddress).send() call, zroPaymentAddress is hardcoded to address(0). This issue was reported in QA report.

• Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.

useZro is hardcoded to false in getFeeEstimate() function. This issue affects both BranchBridgeAgent.sol and RootBridgeAgent.sol. When user calls getFeeEstimate to get the estimate fee of the message - the call to ILayerZeroEndpoint(lzEndpointAddress).estimateFees is being executed. The parameter useZro in that call is hardcoded to false.

• Do not hardcode zero bytes (bytes(0)) as adapterParamers. Pass them as a parameter instead.

adapterParamers is the last parameter in the ILayerZeroEndpoint(lzEndpointAddress).send. In both BranchBridgeAgent.sol and RootBridgeAgent.sol - in the last parameter of ILayerZeroEndpoint(lzEndpointAddress).send, we pass abi.encodePacked data. Only the _performFallbackCall function passes empty string: "", however, since it's a fallback function - it is expected behavior.

• Make sure to test the amount of gas required for the execution on the destination. Use custom adapter parameters and specify minimum destination gas for each cross-chain path when the default amount of gas (200,000) is not enough. This requires whoever calls the send function to provide the adapter params with a destination gas >= amount set in the minDstGasLookup for that chain. So that your users don't run into failed messages on the destination. It makes it a smoother end-to-end experience for all.

Function uses excessivelySafeCall to pass the amount of gas to be used:

(bool success,) = address(this).excessivelySafeCall(
            gasleft(),
            150,
            abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
        );

It sets that parameter to gasleft(), which implies, that it will be enough gas to execute. However, using gasleft() may have other security consequences, which were reported as separate issue.

• Do not add requires statements that repeat existing checks in the parent contracts. For example, lzReceive function in LzApp contract checks that the message sender is LayerZero endpoint and the scrAddress is a trusted remote, do not perform the same checks in nonblockingLzReceive.

Protocol does not derive from LzApp/NonblockingLzApp, thus it does not repeat any require checks.

• If your contract derives from LzApp, do not call lzEndpoint.send directly, use _lzSend.

Protocol does not derive from LzApp/NonblockingLzApp, thus it uses ILayerZeroEndpoint(lzEndpointAddress).send{value: _value} directly

• For ONFTs that allow minting a range of tokens on each chain, make the variables that specify the range (e.g. startMintId and endMintId) immutable.

Protocol does not use LayerZero's Omnichain Non Fungible Token implementation.

Smart Contract Security Verification Standard - I4 - Cross-Chain

• (I4.4) Dynamic parameters are passed to abi.encodePacked in a way, which does not provoke any collisions. There's either no dynamic type or only one dynamic type passed to abi.encodePacked. The only part of the code where abi.encodePacked encodes dynamic datatypes which are close to each other (thus can lead to collison) are in BranchBridgeAgent.sol, where deposit.hTokens, deposit.tokens, deposit.amounts, deposit.deposits are being abi.encodePacked together.

However, protocol protects from collision in that case, because it verifies the length of those data:

 if (_hTokens.length > MAX_TOKENS_LENGTH) revert InvalidInput();
        if (_hTokens.length != _tokens.length) revert InvalidInput();
        if (_tokens.length != _amounts.length) revert InvalidInput();
        if (_amounts.length != _deposits.length) revert InvalidInput();

Since deposit.hTokens, deposit.tokens, deposit.amounts, deposit.deposits need to have the same length - they cannot create a collision in abi.encodePacked.

Conclusion

LayerZero integration passed most of the requirements from the above checklist. The elements which were not passed do not pose any security risk. This implies that LayerZero has been integrated correctly.

Time spent:

20 hours