The transmission of messages to Layer Zero does not conform to Best Practices.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L770-L777
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L161-L173
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L578-L584

Vulnerability details

Impact

Interactions with Layer Zero contracts require specific considerations to prevent message stalls during transmission and to address potential issues in the protocol's operation in the future.

Proof of Concept

Here is a list of Best Practices for Layer Zero:

https://layerzero.gitbook.io/docs/evm-guides/best-practice

https://layerzero.gitbook.io/docs/evm-guides/layerzero-integration-checklist

Failure to adhere to these requirements may lead to disruptions in the protocol's functionality.

1. While sending a message, a user can specify their preferred gas amount for transaction execution. However, the msg.value parameter is not verified before sending, resulting in transaction failure in the LZ contract if the user sends an insufficient amount of native tokens.

https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/UltraLightNodeV2.sol#L150

Similar medium case:

https://solodit.xyz/issues/m-07-redemptionsender-should-estimate-fees-to-prevent-failed-transactions-code4rena-velodrome-finance-velodrome-finance-git

2. The lzReceive function does not store information about failed messages.

   function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
   (bool success,) = address(this).excessivelySafeCall(
       gasleft(),
       150,
       abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
   );
   
   if (!success) if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();
   }
   

If the message execution fails at the destination, try-catch, and store it for future retry. From LayerZero's perspective, the message has been delivered. It is much cheaper and easier for your programs to recover from the last state at the destination chain.

https://layerzero.gitbook.io/docs/evm-guides/error-messages/fix-a-storedpayload

In order to deliver this message, once it is stored, you must call a transaction on the Endpoint.sol called retryPayload
You'll need 3 things:
source Chain ID of the chain the message was sent from
source UserApplication address
UA payload

In the current implementation, the user can't retry to execute his message if it gets stuck for some reason like a lack of gas. Only function retryDeposit from BranchBridgeAgent.sol has similar functionality but messages without tokens will be lost.

Link to a similar case where the user can't retry the execution of his message in case of miscalculation of gas values:

https://solodit.xyz/issues/h-03-layerzeromodule-miscalculates-gas-risking-loss-of-assets-code4rena-holograph-holograph-contest-git_

3. Hardcoded values:

Do not hardcode LayerZero chain Ids. Use admin-restricted setters instead.
Do not hardcode address zero (address(0)) as zroPaymentAddress when estimating fees and sending messages. Pass it as a parameter instead.
Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.
Do not hardcode zero bytes (bytes(0)) as adapterParamers. Pass them as a parameter instead.

RootBridgeAgent.sol:

40: uint16 public immutable localChainId;

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

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

4. The maximum payload size is limited to 10000, and there is no check to ensure the message payload is less than 10000.

https://github.com/LayerZero-Labs/LayerZero/blob/48c21c3921931798184367fc02d3a8132b041942/contracts/RelayerV2.sol#L139-L143

The lack of this check was already discussed here:

https://solodit.xyz/issues/h-2-malicious-user-can-use-an-excessively-large-_toaddress-in-oftcoresendfrom-to-break-layerzero-communication-sherlock-uxd-uxd-protocol-git

Tools Used

vs

Recommended Mitigation Steps

1. Verify that msg.value is greater than the return value of the getFeeEstimate() function. Additionally, consider another discovery where sponsors have opted to introduce a mechanism for computing gas expenses on a per-target chain basis. This approach is intended to prevent transactions from becoming stuck.

https://solodit.xyz/issues/h-03-layerzeromodule-miscalculates-gas-risking-loss-of-assets-code4rena-holograph-holograph-contest-git_

2. Consider implementing a function to store failed messages and retry their execution, similar to the example provided.

https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/NonblockingLzApp.sol

function _storeFailedMessage(uint16 _srcChainId, bytes memory _srcAddress, uint64 _nonce, bytes memory _payload, bytes memory _reason) internal virtual {
    failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
    emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
}

3. Consider making hardcoded values that participate in message exchanges with Layer Zero configurable.

4. Add a require statement to check the size of the payload before processing.

Assessed type

Context

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #412

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[viktorcortess]

Hello, Alcueca.

The first point and the first mitigation step are similar to issue 399

Regarding issue 399:

Root causes:
Issue A. User can specify any gas params against best-practice guidelines
Issue B. The protocol implements a non-blocking implementation pattern for lzReceive and doesn't handle the "in-blocking scenario"
Issue C. The protocol doesn't implement the ILayerZeroUserApplicationConfig functions as recommended

I decided to write a comprehensive submission because the issue with the gas value was described in the LZ Best Practices (399 points to the same page) and in similar contests (I added links to them in the report). Also, similar issues, such as hardcoded values of chainId, are present in the code from the contest.

Since these are well-known and "classic" issues, I decided not to flood the contest with separate reports but to consolidate them into one.

I've just noticed your old comment about bulking issues: code-423n4/org#8 (comment) So here I had a similar idea to bulk several issues in one because all these issues are described on the same page with best practices.

[alcueca]

Your report reads like the other QA reports of lack of adherence to LZ's best practices. #399 points out how a DoS can be constructed thanks to one of those failures.

[viktorcortess]

Recommended Mitigation Steps

1. Verify that msg.value is greater than the return value of the getFeeEstimate() function. Additionally, consider another discovery where sponsors have opted to introduce a mechanism for computing gas expenses on a per-target chain basis. This approach is intended to prevent transactions from becoming stuck.

My first recommendation reads like a duplicate of the issue #785 which is a duplicate of the main issue, but without a big story that lays behind this problem.

Also all of these LZ issues are well explained and known in the best-practices that's why I decided not to spam the contest with big separate reports.

If I added some additional problems to this msg.value checking doesn't mean that it's invalid or QA.