No deposit cross-chain calls/communication can still originate from a removed branch bridge agent

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Bridge agents are removed/toggled off to stop communication to/from them (confirmed by sponsor - check image below) in case of some situation such as a bug in protocol or in case of an upgrade to a newer version of the protocol (in case LayerZero decides to upgrade/migrate their messaging library)

Admin router contracts are able to disable or toggle off anybody's bridge agents due to any reasons through the removeBranchBridgeAgent() function in CoreRootRouter.sol contract. But although this toggles off the branch bridge agent in the Branch chain's BranchPort, it still allows No Deposit calls to originate from that deactivated branch bridge agent.

Proof of Concept

Here is the whole process:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreRootRouter.sol#L186

1. Admin in CoreRootRouter decides to remove branch bridge agent through removeBranchBridgeAgent() function

Here is the execution path of the call:

Root chain to Endpoint (EP): removeBranchBridgeAgent => callOut => _performCall => send

EP to Branch chain: receivePayload => lzReceive => lzReceiveNonBlocking => _execute => executeNoSettlement (Executor) => executeNoSettlement (Router) => _removeBranchBridgeAgent => toggleBridgeAgent (Port)

The following state change occurs in function toggleBridgeAgent():

• Line 356 - isBridgeAgent for _bridgeAgent is set to false, thus deactivating it.

  File: BranchPort.sol
  355:     function toggleBridgeAgent(address _bridgeAgent) external override requiresCoreRouter {
  356:         isBridgeAgent[_bridgeAgent] = !isBridgeAgent[_bridgeAgent];
  357: 
  358:         emit BridgeAgentToggled(_bridgeAgent);
  359:     }

Note: Over here there is no call made back to the root chain again which means the synced branch bridge agent is still synced in root (as shown here). But even if it is synced in Root chain, there are no checks or even state variables to track activeness (bool isActive) of branch bridge agent on the Root chain end as well. Not related to this POC but good to be aware of.

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

2. Now although bridge agent has been deactivated from BranchPort's state, it can still be used for no deposit calls/communication when calling function callOut(). This is because in the function callOut(), there is no check to see if it the current BranchBridgeAgent (that is deactivated) is active in the BranchPort through the modifier requiresBridgeAgent(). Now let's see how a call can occur through a deactivated branch bridge agent.

3. User calls callOut() to communicate through the branch BridgeAgent (that was deactivated):

File: BaseBranchRouter.sol
83:     function callOut(bytes calldata _params, GasParams calldata _gParams) external payable override lock {
84:         IBridgeAgent(localBridgeAgentAddress).callOut{value: msg.value}(payable(msg.sender), _params, _gParams);
85:     }

4. Here is the execution path of the call from callOut() function in BaseBranchRouter contract:

Branch chain to EP: callOut() => callOut (branch bridge agent - that was deactivated) => _performCall => send

EP to Root chain: receivePayload => lzReceive => lzReceiveNonBlocking => _execute => executeNoDeposit => execute (Router) => logic continues based on custom Router's implementation

There are few important points to note here:

1. Calls (No deposit calls) can occur through both callOut() and callOutSigned()
2. Adding the modifier requiresBridgeAgent() check is critical here communication between user's custom Branch chain to Root chain routers is still active
3. Incoming calls to lzReceive() are not fine from user's custom Root Router since that still allows no deposit router-router communication through the deactivated branch bridge agent.
4. Incoming calls to lzReceive() are fine from admin CoreRootRouter since such calls will be made to manage bridgeAgents, factories, strategy tokens and port strategies in the BranchPort.
5. Outgoing calls from function callOutSystem() are fine since the function is only used to respond back to the root chain when an incoming call from root to branch occurs to manage bridgeAgents, factories, strategy tokens and port strategies.
6. Preventing outgoing no deposit calls are important because user's custom Root Router can communicate with other chains for various purposes such as bridging of tokens from Arbitrum (Root) to another branch chains like Avalanche, Fantom and many more. This is crucial since the call for bridging between Arbitrum and Avalanche originates from a branch bridge agent that is considered inactive.

Tools Used

Manual Review

Recommended Mitigation Steps

The issues here to solve are:

1. Outgoing No deposit calls from callOut() and callOutSigned()
2. Allowing incoming calls from CoreRootRouter but not user's custom Root Router.

Solution for callOut() function (check added on Line 202 - same can be applied for callOutSigned()):

File: BranchBridgeAgent.sol
195:     function callOut(address payable _refundee, bytes calldata _params, GasParams calldata _gParams)
196:         external
197:         payable
198:         override
199:         lock
200:     {
201:         //Perform check to see if this BranchBridgeAgent is active in BranchPort
202:         require(localPortAddress.isBridgeAgent(address(this)), "Unrecognized BridgeAgent!");
203:
204:         //Encode Data for cross-chain call.
205:         bytes memory payload = abi.encodePacked(bytes1(0x01), depositNonce++, _params);
206:         
207:         //Perform Call
208:         _performCall(_refundee, payload, _gParams);
209:     }

Solution for point 2: In user's branch bridge agent, the following check should be added in Func ID 0x00 No deposit if block in the lzReceiveNonBlockingFunction.

Check added on Line 602:

File: BranchBridgeAgent.sol
598:         // DEPOSIT FLAG: 0 (No settlement)
599:         if (flag == 0x00) {
600:
601:             //Perform check to see if this BranchBridgeAgent is active in BranchPort
602:             require(localPortAddress.isBridgeAgent(address(this)), "Unrecognized BridgeAgent!");
603:
604:             // Get Settlement Nonce
605:             nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));
606: 
607:             //Check if tx has already been executed
608:             if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();
609: 
610:             //Try to execute the remote request
611:             //Flag 0 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeNoSettlement(localRouterAddress, _payload)
612:             _execute(
613:                 nonce,
614:                 abi.encodeWithSelector(
615:                     BranchBridgeAgentExecutor.executeNoSettlement.selector, localRouterAddress, _payload
616:                 )
617:             );

In case of the Maia administration contracts, the calls from the root chain CoreRootRouterAddress-CoreRootBridgeagent pair should not be stopped. Therefore, in the administration's corresponding CoreBranchBridgeAgent contract, do not add the above check in order to allow no deposit calls to go through.

This way when the branch bridge agent is inactive, only the Maia Administration CoreRootRouter can continue execution.

Assessed type

Error

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-sponsor]

0xBugsy (sponsor) confirmed

[c4-sponsor]

0xBugsy marked the issue as disagree with severity

[0xBugsy]

Despite only affecting functions that don't use token deposits and disagreeing with severity, this is a great finding and we will address it in our codebase

[alcueca]

There is no loss of funds or denial of service to users. However, part of the protocol (the emergency features) are rendered ineffective. Medium (barely).

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[mcgrathcoutinho]

Hi @alcueca, thank you for taking the time to read this. Here are some points I'd like to point out about the duplicate issue #190:

1. I believe the issue should be marked grade-c or invalid since the warden is mentioning that the lack of synchronization on the RootBridgeAgent is causing the impact (which is calls originating from the removed branch bridge agent). This is incorrect since even if synchronization is done correctly through syncBranchBridgeAgent, the issue of calls originating still exists (as mentioned in the note under point 1 in my report above). In short, bridge agent synchronization on root chain has no relation to this issue. The root cause is the absence of this check require(localPortAddress.isBridgeAgent(address(this)), "Unrecognized BridgeAgent!"); in the branch bridge agent functions itself.
2. It is more like the duplicate issue is mentioning the root cause of #548 and the impact of my issue, which is totally wrong. In my opinion, due to the low-quality report, insufficient proof, and incorrect identification of the root cause, it should be marked as invalid.

Thank you once again for taking the time to read this.

[0xBugsy]

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