0x52 - Malicious user can use an excessively large _toAddress in OFTCore#sendFrom to break layerZero communication

[github-actions[bot]]

0x52

high

Malicious user can use an excessively large _toAddress in OFTCore#sendFrom to break layerZero communication

Summary

By default layerZero implements a blocking behavior, that is, that each message must be processed and succeed in the order that it was sent. In order to circumvent this behavior the receiver must implement their own try-catch pattern. If the try-catch pattern in the receiving app ever fails then it will revert to its blocking behavior. The _toAddress input to OFTCore#sendFrom is calldata of any arbitrary length. An attacker can abuse this and submit a send request with an excessively large _toAddress to break communication between network with different gas limits.

Vulnerability Detail

function sendFrom(address _from, uint16 _dstChainId, bytes calldata _toAddress, uint _amount, address payable _refundAddress, address _zroPaymentAddress, bytes calldata _adapterParams) public payable virtual override {
    _send(_from, _dstChainId, _toAddress, _amount, _refundAddress, _zroPaymentAddress, _adapterParams);
}

The _toAddress input to OFTCore#sendFrom is a bytes calldata of any arbitrary size. This can be used as follows to break communication between chains that have different block gas limits.

Example: Let's say that an attacker wishes to permanently block the channel Arbitrum -> Optimism. Arbitrum has a massive gas block limit, much higher than Optimism's 20M block gas limit. The attacker would call sendFrom on the Arbitrum chain with the Optimism chain as the destination. For the _toAddress input they would use an absolutely massive amount of bytes. This would be packed into the payload which would be called on Optimism. Since Arbitrum has a huge gas limit the transaction would send from the Arbitrum side but it would be so big that the transaction could never succeed on the Optimism side due to gas constraints. Since that nonce can never succeed the communication channel will be permanently blocked at the Optimism endpoint, bypassing the nonblocking behavior implemented in the OFT design and reverting to the default blocking behavior of layerZero.

Users can still send messages and burn their tokens from Arbitrum -> Optimism but the messages can never be received. This could be done between any two chain in which one has a higher block gas limit. This would cause massive loss of funds and completely cripple the entire protocol.

Impact

Massive loss of user funds and protocol completely crippled

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/external/layer-zero/token/oft/OFTCore.sol#L31-L33

Tool used

Manual Review

Recommendation

Limit the length of _toAddress to some amount (i.e. 256 bytes) as of right now EVM uses 20 bytes address and Sol/Aptos use 32 bytes address, so for right now it could be limited to 32 bytes.

    function sendFrom(address _from, uint16 _dstChainId, bytes calldata _toAddress, uint _amount, address payable _refundAddress, address _zroPaymentAddress, bytes calldata _adapterParams) public payable virtual override {
+       require(_toAddress.length <= maxAddressLength);       
        _send(_from, _dstChainId, _toAddress, _amount, _refundAddress, _zroPaymentAddress, _adapterParams);
    }

[WarTech9]

We are using a non blocking receiver with both UXP and UXD tokens inheriting from the OFTV2Core base contract. Thus when a message is received nonBlockingLzReceive function is called which does not block the channel between 2 chains if an error occurs.

Please provide more details if you feel otherwise.

[IAm0x52]

Correct you do use a non-blocking but using a huge _toAddress will cause the function to revert before the non-blocking error handling.

function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
    // assert and increment the nonce. no message shuffling
    require(_nonce == ++inboundNonce[_srcChainId][_srcAddress], "LayerZero: wrong nonce");

    LibraryConfig storage uaConfig = uaConfigLookup[_dstAddress];

    // authentication to prevent cross-version message validation
    // protects against a malicious library from passing arbitrary data
    if (uaConfig.receiveVersion == DEFAULT_VERSION) {
        require(defaultReceiveLibraryAddress == msg.sender, "LayerZero: invalid default library");
    } else {
        require(uaConfig.receiveLibraryAddress == msg.sender, "LayerZero: invalid library");
    }

    // block if any message blocking
    StoredPayload storage sp = storedPayload[_srcChainId][_srcAddress];
    require(sp.payloadHash == bytes32(0), "LayerZero: in message blocking");

    try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
        // success, do nothing, end of the message delivery
    } catch (bytes memory reason) {
        // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
        storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
        emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
    }
}

Above is the code run on the endpoint. It will use the try statement to call the lzReceive on the non-blocking app. Due to the huge amount of calldata the try function will immediately revert because it will run out of gas. This bypasses all the logic of the app and causes the checkpoint to store the payload, blocking the channel

[hrishibhat]

@WarTech9

[WarTech9]

The message is only stored if the catch block is executed. In your example, if a huge amount of data is passed in the receive function would run out of gas before that gets executed. Secondly, the transaction would revert within the catch block itself if the payload is too huge as a result of a huge _dstAddress being passed in, thus, message would not be stored and channel would not be blocked.

[WarTech9]

There could be some value in adding input size validation at source as recommended, since the size could be arbitrary. But this issue is medium/low risk

[IAm0x52]

// assert and increment the nonce. no message shuffling    
require(_nonce == ++inboundNonce[_srcChainId][_srcAddress], "LayerZero: wrong nonce");

This line in endpoint requires that the nonce is sequential so if the message is too big to be executed at all then it will also block the channel even without being stored. As an example if the message with nonce == 1 cannot be executed at all due to gas limits then the channel would also be blocked because trying to execute nonce == 2 would always revert at that statement.

function send(uint16 _dstChainId, bytes calldata _destination, bytes calldata _payload, address payable _refundAddress, address _zroPaymentAddress, bytes calldata _adapterParams) external payable override sendNonReentrant {
    LibraryConfig storage uaConfig = uaConfigLookup[msg.sender];
    uint64 nonce = ++outboundNonce[_dstChainId][msg.sender];
    _getSendLibrary(uaConfig).send{value: msg.value}(msg.sender, nonce, _dstChainId, _destination, _payload, _refundAddress, _zroPaymentAddress, _adapterParams);
}

The nonce of the message is set on the sending end of the endpoint so it's impossible to work around a nonce that can't execute.

Secondly, the transaction would revert within the catch block itself if the payload is too huge as a result of a huge _dstAddress being passed in, thus, message would not be stored and channel would not be blocked.

This is not true either because the _dstAddress is not the same as _toAddress passed into the OFTCore#sendFrom. _toAddress is packed into the payload of the message and the dstAddress is set by _lzSend:

function _lzSend(uint16 _dstChainId, bytes memory _payload, address payable _refundAddress, address _zroPaymentAddress, bytes memory _adapterParams, uint _nativeFee) internal virtual {
    bytes memory trustedRemote = trustedRemoteLookup[_dstChainId];
    require(trustedRemote.length != 0, "LzApp: destination chain is not a trusted source");
    lzEndpoint.send{value: _nativeFee}(_dstChainId, trustedRemote //@audit _dstAddress, _payload, _refundAddress, _zroPaymentAddress, _adapterParams);
}

The destination address would be the address of the receiving contract (normal sized). Additionally when the message is stored, it only stores the hash of the message so it doesn't require a huge amount of gas to store.

[hrishibhat]

Considering this a valid high issue based on the above comments.

[0xIryna]

LayerZero team is here. Thank you for digging into this! We have a max payload size (10000) set in our RelayerV2 contract which prevents the described situation (the contract isn’t verified, but you can fork a chain and test). If a payload exceeds the specified limit the transaction will revert on the source. If a client application is configured to use a non-default Relayer it must set the payload limit itself. Generally, we believe it’s a protocol’s responsibility to enforce the max payload size.

[berndartmueller]

Escalate for 10 USDC

Consider the response from the LayerZero team above to reconsider the severity and validity of this issue. If UXD uses the RelayerV2 contract provided by LayerZero, this is a non-issue.

[sherlock-admin]

Escalate for 10 USDC

Consider the response from the LayerZero team above to reconsider the severity and validity of this issue. If UXD uses the RelayerV2 contract provided by LayerZero, this is a non-issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[rvierdiiev]

Escalate for 15 usdc. Increased amount just to make sure this will be rechecked.

[sherlock-admin]

Escalate for 15 usdc. Increased amount just to make sure this will be rechecked.

You've created a valid escalation for 15 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

Comment from Layer Zero shows that there is a valid risk from payload which is now fixed: https://github.com/LayerZero-Labs/LayerZero/pull/24/commits The relayer did not seem have a check for max payload size previously.

Additionally as suggested by the LZ

we believe it’s a protocol’s responsibility to enforce the max payload size.

[sherlock-admin]

Escalation rejected

Comment from Layer Zero shows that there is a valid risk from payload which is now fixed: https://github.com/LayerZero-Labs/LayerZero/pull/24/commits The relayer did not seem have a check for max payload size previously.

Additionally as suggested by the LZ

we believe it’s a protocol’s responsibility to enforce the max payload size.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[IAm0x52]

LayerZero has fixed this issue with a new RelayerV2 contract