c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

v2 adapterParams are used to send messages, which means that the relayer will send native token to RootBridgeAgent before RootBridgeAgent.lzReceive is called. However, if an exception occurs inside lzReceiveNonBlocking, lzReceive will not revert (except for ARB branch). In this way, the native token sent to RootBridgeAgent will stay in RootBridgeAgent. Malicious users can steal these native tokens by sending some messages.

Proof of Concept

The messages discussed below are all sent using V2 (Airdrop).

When the cross-chain message reaches RootBridgeAgent, the relayer from layerZero will first send the native token to RootBridgeAgent and then call RootBridgeAgent.lzReceive which internally calls lzReceiveNonBlocking to process various messages.

File: src\RootBridgeAgent.sol
423:     function lzReceive(uint16 _srcChainId, bytes calldata _srcAddress, uint64, bytes calldata _payload) public {
424:         (bool success,) = address(this).excessivelySafeCall(
425:             gasleft(),
426:             150,
427:             abi.encodeWithSelector(this.lzReceiveNonBlocking.selector, msg.sender, _srcChainId, _srcAddress, _payload)
428:         );
429:         
430:->       if (!success) if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();
431:     }

From the above code we can see that if lzReceiveNonBlocking returns false, as long as the sender is not from getBranchBridgeAgent[localChainId](ARB branch), then tx will not revert. This means that the native token previously sent by the relayer is left in this contract (RootBridgeAgent).

In lzReceiveNonBlocking, the functions used to process messages are two _execute functions: 1, 2. The difference between the former and the latter is whether Fallback can be triggered if a failure to process the message occurs.

File: src\RootBridgeAgent.sol
749:     function _execute(uint256 _depositNonce, bytes memory _calldata, uint16 _srcChainId) private {
750:         //Update tx state as executed
751:         executionState[_srcChainId][_depositNonce] = STATUS_DONE;
752: 
753:         //Try to execute the remote request
754:         (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);
755: 
756:         // No fallback is requested revert allowing for retry.
757:->       if (!success) revert ExecutionFailure();
758:     }

If the message is processed by _execute in L749, when bridgeAgentExecutorAddress.call returns false, the value sent is still in the current contract.

File: src\RootBridgeAgent.sol
768:     function _execute(
769:         bool _hasFallbackToggled,
770:         uint32 _depositNonce,
771:         address _refundee,
772:         bytes memory _calldata,
773:         uint16 _srcChainId
774:     ) private {
775:         //Update tx state as executed
776:         executionState[_srcChainId][_depositNonce] = STATUS_DONE;
777: 
778:         //Try to execute the remote request
779:         (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);
780: 
781:         //Update tx state if execution failed
782:         if (!success) {
783:             //Read the fallback flag.
784:             if (_hasFallbackToggled) {
785:                 // Update tx state as retrieve only
786:                 executionState[_srcChainId][_depositNonce] = STATUS_RETRIEVE;
787:                 // Perform the fallback call
788:                 _performFallbackCall(payable(_refundee), _depositNonce, _srcChainId);
789:             } else {
790:                 // No fallback is requested revert allowing for retry.
791:                 revert ExecutionFailure();
792:             }
793:         }
794:     }

_hasFallbackToggled is set when the user sends a message. when bridgeAgentExecutorAddress.call returns false:

If the user sets the fallback flag, _performFallbackCall will be called, where the native token previously sent by the relayer will be taken away. There is no problem.
If the user doesn't set the fallback flag, the value sent is still in the current contract.

Consider the following scenario:

Alice wants to send the USDC of the ftm branch to the mainnet branch via BranchBridgeAgent.callOutSignedAndBridge. The _hasFallbackToggled argument is set to false, that is, fallback will be not triggered. This operation requires two cross-chain messages: ftm->arb and arb->mainnet. _gParams.remoteBranchExecutionGas is set to 1 ether.

When the message reaches RootBridgeAgent, relayer sends 1 ether native token to RootBridgeAgent, then calls RootBridgeAgent.lzReceive. The processing flow is as follows:

RootBridgeAgent.lzReceive
  (bool success,) = lzReceiveNonBlocking
    _execute    //_hasFallbackToggled = false
      (bool success,) = bridgeAgentExecutorAddress.call
      //if success = false
      revert ExecutionFailure()
  //success is false due to revert internally, msg.sender is not from arb, so tx will not revert

This resulted in 1 ether native token being left in the RootBridgeAgent.

Bob notices that RootBridgeAgent has ether and immediately calls via BranchBridgeAgent.callOutSignedAndBridge. The _hasFallbackToggled argument is set to true, that is, fallback will be triggered. The Call encoded in _params parameter intentionally triggers revert.

When the message reaches RootBridgeAgent, the processing flow is as follows:

RootBridgeAgent.lzReceive
  (bool success,) = lzReceiveNonBlocking
    _execute    //_hasFallbackToggled = true
      (bool success,) = bridgeAgentExecutorAddress.call
      //success = false due to intentionally revert
      _performFallbackCall  //L788
        //all native token held by RootBridgeAgent is taken away by this function
        ILayerZeroEndpoint(lzEndpointAddress).send{value: address(this).balance}

Bob get all native token held by RootBridgeAgent because the excess gas will be returned to bob by relayer.

Tools Used

Manual Review

Recommended Mitigation Steps

In lzReceive, if success is false and msg.sender is not an ARB branch, then the balance held by this should be returned to the sender address of the source message.

Assessed type

Context

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #887

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as sufficient quality report

c4-judge commented 1 year ago

alcueca changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

alcueca marked the issue as partial-50

securitygrid commented 1 year ago

I don't think this report is a duplicate of 464. The problem highlighted by this report is: if lzReceiveNonBlocking returns false, as long as the sender is not from getBranchBridgeAgent[localChainId](ARB branch), then tx will not revert. This means that the native token previously sent by the relayer is left in this contract (RootBridgeAgent). The above POV is from Proof of Concept in this report.

It gives two examples to illustrate situations where message processing fails.

It also describes a scenario to steal the native token stuck in RootBridgeAgent. Because layerEndpoint.send will internally check whether msg.value is enough, and if it is too much, it will be returned to _refundee.

This report describes the same problem as #611. However, #611 only describes the problem and does not provide further attack paths.

Please review it, thank you.

securitygrid commented 1 year ago

464 is invalid. It ignores `address(this).excessivelySafeCall` in `RootBridgeAgent.lzReceive`. If the message comes from a non-ArbBranch and `lzReceiveNonBlocking` reverts internally, then `lzReceive` will return successfully. How is it possible: the native token will be sent back to the LayerZeroEndpoint contract and will be left there. This is impossible. The calling process is as follows:

Endpoint.receivePayload
  ILayerZeroReceiver(_dstAddress).lzReceive         //_dstAddress = RootBridgeAgent
    (bool success,) = address(this).excessivelySafeCall     //call this.lzReceiveNonBlocking by low-level call
       excessivelySafeCall
          //if something fails, revert.
          .........
    //so success=false since there is exception inside excessivelySafeCall
    //but msg.sender==Endpoint, so no revert happens.
    //airdrop is stuck in RootBridgeAgent.
    if (!success) if (msg.sender == getBranchBridgeAgent[localChainId]) revert ExecutionFailure();

securitygrid commented 1 year ago

There is more context about LayzerZero: In LayerZero, an messages starts here: https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/RelayerV2.sol#L164-L171

164:     function validateTransactionProofV2(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _blockHash, bytes32 _data, bytes calldata _transactionProof, address payable _to) external payable onlyApproved nonReentrant {
165:->       (bool sent, ) = _to.call{value: msg.value}("");
166:         //require(sent, "Relayer: failed to send ether");
167:         if (!sent) {
168:             emit ValueTransferFailed(_to, msg.value);
169:         }
170:->       uln.validateTransactionProof(_srcChainId, _dstAddress, _gasLimit, _blockHash, _data, _transactionProof);
171:     }

L165, airdrop is sent to _to(adapterParams.nativeForDst=RootBridgeAgent). L170, calls uln.validateTransactionProof which is from https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/UltraLightNodeV2.sol#L76-L114

File: contracts\UltraLightNodeV2.sol
076:     function validateTransactionProof(uint16 _srcChainId, address _dstAddress, uint _gasLimit, bytes32 _lookupHash, bytes32 _blockData, bytes calldata _transactionProof) external override {
......
111:         bytes memory pathData = abi.encodePacked(_packet.srcAddress, _packet.dstAddress);
112:         emit PacketReceived(_packet.srcChainId, _packet.srcAddress, _packet.dstAddress, _packet.nonce, keccak256(_packet.payload));
113: ->      endpoint.receivePayload(_srcChainId, pathData, _dstAddress, _packet.nonce, _gasLimit, _packet.payload);
114:     }

L113, calls endpoint.receivePayload which is from https://github.com/LayerZero-Labs/LayerZero/blob/main/contracts/Endpoint.sol#L100-L125

File: contracts\Endpoint.sol
100:     function receivePayload(uint16 _srcChainId, bytes calldata _srcAddress, address _dstAddress, uint64 _nonce, uint _gasLimit, bytes calldata _payload) external override receiveNonReentrant {
......
118: ->      try ILayerZeroReceiver(_dstAddress).lzReceive{gas: _gasLimit}(_srcChainId, _srcAddress, _nonce, _payload) {
119:             // success, do nothing, end of the message delivery
120:         } catch (bytes memory reason) {
121:             // revert nonce if any uncaught errors/exceptions if the ua chooses the blocking mode
122:             storedPayload[_srcChainId][_srcAddress] = StoredPayload(uint64(_payload.length), _dstAddress, keccak256(_payload));
123:             emit PayloadStored(_srcChainId, _srcAddress, _dstAddress, _nonce, _payload, reason);
124:         }
125:     }

L118, _dstAddress.lzReceive doesn't pass any msg.value.

Note: The airdropped native token is sent to RootBridgeAgent before RootBridgeAgent.lzReceive is called.

Through the above analysis, we can have a clearer understanding of message processing. So, I think this report should be reconsidered. It's correct and better than #611. And this issue should be High.

c4-judge commented 1 year ago

alcueca marked the issue as not a duplicate

c4-judge commented 1 year ago

alcueca marked the issue as primary issue

c4-judge commented 1 year ago

alcueca marked the issue as satisfactory

c4-judge commented 1 year ago

alcueca marked the issue as selected for report

0xBugsy commented 12 months ago

Addressed at https://github.com/Maia-DAO/2023-09-maia-remediations/commit/5ef0d547a9683750cee0859d0eca8f760b45959f

code-423n4 / 2023-09-maia-findings

If RootBridgeAgent.lzReceiveNonBlocking reverts internally, the native token sent by relayer to RootBridgeAgent is left in RootBridgeAgent #518

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type