Maia DAO - Ulysses - Sathish9098's results

QA FINDINGS

[L-1] The _checkTimeLimit function reverts unexpected way if amount exceeds dailyLimit

IMPACT

The _checkTimeLimit function reverts unexpectedly when amount exceeds dailyLimit indicates that there might be a problem or an unexpected behavior in the _checkTimeLimit function when the value of the amount surpasses the defined dailyLimit. When assigning values to strategyDailyLimitRemaining[msg.sender][_token] state variable. If the amount exceeds the dailyLimit the result is negative value. So the over all function may be reverted

POC

FILE: 2023-09-maia/src/BranchPort.sol

 function _checkTimeLimit(address _token, uint256 _amount) internal {
        uint256 dailyLimit = strategyDailyLimitRemaining[msg.sender][_token];
        if (block.timestamp - lastManaged[msg.sender][_token] >= 1 days) {
            dailyLimit = strategyDailyLimitAmount[msg.sender][_token];
            unchecked {
                lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
            }
        }
        strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;
    }

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

[L-2] _minimumReserves() Function Returns 0 for Non-StrategyTokens

Impact

_minimumReserves() always returns return value as zero if the given token is not a StrategyToken

POC

FILE: 2023-09-maia/src/BranchPort.sol

468: function _minimumReserves(uint256 _strategyTokenDebt, uint256 _currBalance, address _token)
469:        internal
470:        view
471:        returns (uint256)
472:    {
473:        return ((_currBalance + _strategyTokenDebt) * getMinimumTokenReserveRatio[_token]) / DIVISIONER;
474:    }

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

Recommended Mitigation

Add if condition to check getMinimumTokenReserveRatio[_token] . If the value is 0 then directly return 0

+ if(getMinimumTokenReserveRatio[_token] ==0){
+ return 0;
+ }

[L-3] Overflow could occur over time as the depositNonce is incremented with each function call

Impact

Functions like callOutSystem(),callOut(),callOutSigned() all functions incrementing the depositNonce for every function call.

The depositNonce used for encoding data in the callOutSystem(),callOut(),callOutSigned() function may eventually overflow, given that it's of type uint32.

An overflow in the depositNonce could lead to unexpected behavior and potentially disrupt the protocol's functionality

POC

FILE: 2023-09-maia/src/BranchBridgeAgent.sol

180: bytes memory payload = abi.encodePacked(bytes1(0x00), depositNonce++, _params);

220: bytes memory payload = abi.encodePacked(bytes1(0x01), depositNonce++, _params);

269: bytes memory payload = abi.encodePacked(bytes1(0x04), msg.sender, depositNonce++, _params);

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

Recommended Mitigation

Use uint256 for depositNonce variable

[L-4] updatePortStrategy() not check the isPortStrategy[_portStrategy][_token] is allowed are not

Impact

updatePortStrategy function allows updating the strategyDailyLimitAmount even if isPortStrategy[_portStrategy][_token] is set to false, which might not align with the intended behavior of the protocol.

POC

FILE: 2023-09-maia/src/BranchPort.sol

 function togglePortStrategy(address _portStrategy, address _token) external override requiresCoreRouter {
        isPortStrategy[_portStrategy][_token] = !isPortStrategy[_portStrategy][_token];

        emit PortStrategyToggled(_portStrategy, _token);
    }

function updatePortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
        external
        override
        requiresCoreRouter
    {
        strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;

        emit PortStrategyUpdated(_portStrategy, _token, _dailyManagementLimit);
    }

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

Recommended Mitigation

Add check to ensure isPortStrategy[_portStrategy][_token] is not false

function updatePortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
        external
        override
        requiresCoreRouter
    {
        // Check if the strategy is allowed for the given token
+        require(isPortStrategy[_portStrategy][_token], "Strategy not allowed for the token");

        strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;

        emit PortStrategyUpdated(_portStrategy, _token, _dailyManagementLimit);
    }

[L-5] Avoid double casting

Impact

Consider refactoring the following code, as double casting may introduce unexpected truncations and/or rounding issues.

Furthermore, double type casting can make the code less readable and harder to maintain, increasing the likelihood of errors and misunderstandings during development and debugging.

FILE: 2023-09-maia/src/RootBridgeAgentExecutor.sol

125:  PARAMS_END_OFFSET + uint256(uint8(bytes1(_payload[PARAMS_START]))) * PARAMS_TKN_SET_SIZE_MULTIPLE

213:  + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE //@audit double casting

222:  + uint256(uint8(bytes1(_payload[PARAMS_START_SIGNED]))) * PARAMS_TKN_SET_SIZE_MULTIPLE 

281: for (uint256 i = 0; i < uint256(uint8(numOfAssets));) { 

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

[L-6] Lack of sanity checks in place when assigning values to critical variables, such as immutable variables

Impact

It is important to perform sanity checks when assigning values to unit-to-critical variables, such as immutable variables and new deployments. This is because errors in these variables can have serious consequences, such as leading to the loss of funds or the disruption of critical services.

FILE: 2023-09-maia/src/ArbitrumBranchPort.sol

constructor(uint16 _localChainId, address _rootPortAddress, address _owner) BranchPort(_owner) {
        require(_rootPortAddress != address(0), "Root Port Address cannot be 0");

        localChainId = _localChainId;
        rootPortAddress = _rootPortAddress;
    }

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

FILE: 2023-09-maia/src/MulticallRootRouter.sol

constructor(uint256 _localChainId, address _localPortAddress, address _multicallAddress) {
        require(_localPortAddress != address(0), "Local Port Address cannot be 0");
        require(_multicallAddress != address(0), "Multicall Address cannot be 0");

        localChainId = _localChainId;
        localPortAddress = _localPortAddress;
        multicallAddress = _multicallAddress;
        _initializeOwner(msg.sender);
    }

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

FILE: 2023-09-maia/src/ArbitrumBranchBridgeAgent.sol

constructor(
        uint16 _localChainId,
        address _rootBridgeAgentAddress,
        address _localRouterAddress,
        address _localPortAddress
    )
        BranchBridgeAgent(
            _localChainId,
            _localChainId,
            _rootBridgeAgentAddress,
            address(0),
            _localRouterAddress,
            _localPortAddress
        )
    {}

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/ArbitrumBranchBridgeAgent.sol#L43-L57

Recommended Mitigation

Add necessary checks like > 0 ,> MIN , < MAX

require(_localChainId > 0, Wrong chainid) ;

[L-7] The function name "_checkTimeLimit" conflicts with its implementation

The name "_checkTimeLimit" implies that the function is primarily responsible for checking a time limit but doesn't convey that it also updates state values

POC

FILE: 2023-09-maia/src/BranchPort.sol

function _checkTimeLimit(address _token, uint256 _amount) internal {
        uint256 dailyLimit = strategyDailyLimitRemaining[msg.sender][_token];
        if (block.timestamp - lastManaged[msg.sender][_token] >= 1 days) {
            dailyLimit = strategyDailyLimitAmount[msg.sender][_token];
            unchecked {
                lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
            }
        }
        strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;
    }

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

Recommended Mitigation

Modify the function name

+ function _updateAndCheckTimeLimit(address _token, uint256 _amount) internal {

[L-8] Division/multiplication in unchecked block is unsafe

The additions/multiplications may silently overflow because they're in unchecked blocks with no preceding value checks, which may lead to unexpected results

FILE: 2023-09-maia/src/BranchPort.sol

489: unchecked {
490:                lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
491:            }

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

[L-9] Loss of precision

In the expression lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;, there is a possibility of precision loss due to integer division.

The expression block.timestamp / 1 days performs integer division, which means it truncates the fractional part of the result. Since block.timestamp is a timestamp in seconds, dividing it by 1 days (which is also in seconds) will give you the number of full days that have passed since the timestamp.

However, when you multiply this result by 1 days again, it effectively sets the timestamp to the start of the day (i.e., midnight) on the day of the original timestamp. Any fractional seconds from the original block.timestamp are discarded, leading to precision loss.

FILE: 2023-09-maia/src/BranchPort.sol

489: unchecked {
490:                lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
491:            }

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

[L-10] Missing disallowlist for ERC721

Lately, there has been a rise in cases where NFTs are being stolen. These stolen NFTs are then added to platforms, allowing them to be easily converted into liquidity.

Some popular marketplaces, such as Opensea, have already taken steps to combat this issue by introducing a disallowlist feature. This means that if an NFT is reported as stolen, it won't be listed or sold on their platform.

This may increase the project centralization, but it can help solve this problem, if this issue is a concern.

File: src/VirtualAccount.sol

import {ERC721} from "solmate/tokens/ERC721.sol";

https://github.com/code-423n4/2023-09-maia/blob/6c21f6b739252367c5fe770fed3ff76eb3ca2c86/src/VirtualAccount.sol#L7

[L-11] Using zero as a parameter

Taking zero as a valid argument without checks can lead to severe security issues in some cases.

If using a zero argument is mandatory, consider using descriptive constants or an enum instead of passing zero directly on function calls, as that might be error-prone, to fully describe the caller's intention.

FILE: 2023-09-maia/src/RootBridgeAgent.sol

837: IBranchBridgeAgent(callee).lzReceive(0, "", 0, _payload); //@audit 0 as parameter

933: IBranchBridgeAgent(callee).lzReceive(0, "", 0, payload); //@audit 0 as parameter

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

[L-12] Enum values should be used instead of constant array indexes

Create a commented enum value to use instead of constant array indexes, this makes the code far easier to understand.

FILE: 2023-09-maia/src/BranchBridgeAgent.sol

  deposit.hTokens[0],
                    deposit.tokens[0],
                    deposit.amounts[0],
                    deposit.deposits[0],

376:  deposit.hTokens[0],
377:                    deposit.tokens[0],
378:                    deposit.amounts[0],
379:                    deposit.deposits[0],

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

[L-13] Vulnerable versions of packages are being used

This project is using specific package versions which are vulnerable to the specific CVEs listed below. Consider switching to more recent versions of these packages that don't have these vulnerabilities.

```IERC1155Receiveruses the vulnerable versionv4.5.0``

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.5.0) (token/ERC1155/IERC1155Receiver.sol)

pragma solidity ^0.8.0;

import "../../utils/introspection/IERC165.sol";

/**
 * @dev _Available since v3.1._
 */
interface IERC1155Receiver is IERC165 {

``
``IERC721Receiver `` contract uses ``v4.6.0``

// SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v4.6.0) (token/ERC721/IERC721Receiver.sol)

pragma solidity ^0.8.0;

/**

• @title ERC721 token receiver interface
• @dev Interface for any contract that wants to support safeTransfers
• from ERC721 asset contracts. */ interface IERC721Receiver {

### Recommended Mitigation
Use at least ``OpenZeppelin v4.9.2``

##

## [L-14] Governance functions should be controlled by time locks

Governance functions (such as upgrading contracts, setting critical parameters) should be controlled using time locks to introduce a delay between a proposal and its execution. This gives users time to exit before a potentially dangerous or malicious operation is applied.

```solidity

File: src/BranchPort.sol

331      function setCoreRouter(address _newCoreRouter) external override requiresCoreRouter {
332          require(coreBranchRouterAddress != address(0), "CoreRouter address is zero");
333          require(_newCoreRouter != address(0), "New CoreRouter address is zero");
334          coreBranchRouterAddress = _newCoreRouter;
335:     }

362      function addStrategyToken(address _token, uint256 _minimumReservesRatio) external override requiresCoreRouter {
363          if (_minimumReservesRatio >= DIVISIONER || _minimumReservesRatio < MIN_RESERVE_RATIO) {
364              revert InvalidMinimumReservesRatio();
365          }
366  
367          strategyTokens.push(_token);
368          getMinimumTokenReserveRatio[_token] = _minimumReservesRatio;
369          isStrategyToken[_token] = true;
370  
371          emit StrategyTokenAdded(_token, _minimumReservesRatio);
372:     }

382      function addPortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
383          external
384          override
385          requiresCoreRouter
386      {
387          if (!isStrategyToken[_token]) revert UnrecognizedStrategyToken();
388          portStrategies.push(_portStrategy);
389          strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;
390          isPortStrategy[_portStrategy][_token] = true;
391  
392          emit PortStrategyAdded(_portStrategy, _token, _dailyManagementLimit);
393:     }

403      function updatePortStrategy(address _portStrategy, address _token, uint256 _dailyManagementLimit)
404          external
405          override
406          requiresCoreRouter
407      {
408          strategyDailyLimitAmount[_portStrategy][_token] = _dailyManagementLimit;
409  
410          emit PortStrategyUpdated(_portStrategy, _token, _dailyManagementLimit);
411:     }

414      function setCoreBranchRouter(address _coreBranchRouter, address _coreBranchBridgeAgent)
415          external
416          override
417          requiresCoreRouter
418      {
419          coreBranchRouterAddress = _coreBranchRouter;
420          isBridgeAgent[_coreBranchBridgeAgent] = true;
421          bridgeAgents.push(_coreBranchBridgeAgent);
422  
423          emit CoreBranchSet(_coreBranchRouter, _coreBranchBridgeAgent);
424:     }

https://github.com/code-423n4/2023-09-maia/blob/6c21f6b739252367c5fe770fed3ff76eb3ca2c86/src/BranchPort.sol#L331-L335

FILE: File: src/RootPort.sol

259      function setLocalAddress(address _globalAddress, address _localAddress, uint256 _srcChainId)
260          external
261          override
262          requiresCoreRootRouter
263      {
264          if (_localAddress == address(0)) revert InvalidLocalAddress();
265  
266          getGlobalTokenFromLocal[_localAddress][_srcChainId] = _globalAddress;
267          getLocalTokenFromGlobal[_globalAddress][_srcChainId] = _localAddress;
268  
269          emit GlobalTokenAdded(_localAddress, _globalAddress, _srcChainId);
270:     }

382      function addBridgeAgent(address _manager, address _bridgeAgent) external override requiresBridgeAgentFactory {
383          if (isBridgeAgent[_bridgeAgent]) revert AlreadyAddedBridgeAgent();
384  
385          bridgeAgents.push(_bridgeAgent);
386          getBridgeAgentManager[_bridgeAgent] = _manager;
387          isBridgeAgent[_bridgeAgent] = true;
388  
389          emit BridgeAgentAdded(_bridgeAgent, _manager);
390:     }

483      function addEcosystemToken(address _ecoTokenGlobalAddress) external override onlyOwner {
484          // Check if token already added
485          if (isGlobalAddress[_ecoTokenGlobalAddress]) revert AlreadyAddedEcosystemToken();
486  
487          // Check if token is already a underlying token in current chain
488          if (getUnderlyingTokenFromLocal[_ecoTokenGlobalAddress][localChainId] != address(0)) {
489              revert AlreadyAddedEcosystemToken();
490          }
491  
492          // Check if token is already a local branch token in current chain
493          if (getLocalTokenFromUnderlying[_ecoTokenGlobalAddress][localChainId] != address(0)) {
494              revert AlreadyAddedEcosystemToken();
495          }
496  
497          // Update State
498          // 1. Add new global token to global address mapping
499          isGlobalAddress[_ecoTokenGlobalAddress] = true;
500          // 2. Add new branch local token address to global token mapping
501          getGlobalTokenFromLocal[_ecoTokenGlobalAddress][localChainId] = _ecoTokenGlobalAddress;
502          // 3. Add new global token to branch local token address mapping
503          getLocalTokenFromGlobal[_ecoTokenGlobalAddress][localChainId] = _ecoTokenGlobalAddress;
504  
505          emit EcosystemTokenAdded(_ecoTokenGlobalAddress);
506:     }

509      function setCoreRootRouter(address _coreRootRouter, address _coreRootBridgeAgent) external override onlyOwner {
510          if (_coreRootRouter == address(0)) revert InvalidCoreRootRouter();
511          if (_coreRootBridgeAgent == address(0)) revert InvalidCoreRootBridgeAgent();
512  
513          coreRootRouterAddress = _coreRootRouter;
514          coreRootBridgeAgentAddress = _coreRootBridgeAgent;
515          getBridgeAgentManager[_coreRootBridgeAgent] = owner();
516  
517          emit CoreRootSet(_coreRootRouter, _coreRootBridgeAgent);
518:     }

https://github.com/code-423n4/2023-09-maia/blob/6c21f6b739252367c5fe770fed3ff76eb3ca2c86/src/RootPort.sol#L259-L270

GAS OPTIMIZATIONS

Findings

• [G-1] State variables can be packed into fewer storage slots - Saves 10000 GAS , 5 SLOT
  • [G-1.1] _unlocked , bridgeAgentExecutorAddress can be packed same SLOT : Saves 2000 GAS , 1 SLOT
  • [G-1.2] settlementNonce and _unlocked can be packed same SLOT : Saves 2000 GAS , 1 SLOT
  • [G-1.3] coreBranchRouterAddress , _unlocked can be packed with same SLOT : Saves 2000 GAS , 1 SLOT
  • [G-1.4] depositNonce and _unlocked should be packed same SLOT : Saves 2000 GAS , 1 SLOT
  • [G-1.5] localPortAddress and _unlocked can be packed with same SLOT : Saves 2000 GAS , 1 SLOT
• [G-2] Refactor _minimumReserves() function to be more gas-efficient- Saves 800 GAS
• [G-3] Storage variables should be cached in stack variables rather than re-reading them from storage- Saves 1200 GAS , 12 SLOD
  • [G-3.1] deposit.hTokens.length,deposit.owner,executionState[nonce] should be cached : Saves 900 GAS ,9 SLOD
  • [G-3.2] settlementReference.owner should be cached : Saves 300 GAS , 3 SLOD
• [G-4]] Remove redundant cache for strategyDailyLimitRemaining[msg.sender][_token] to save gas - Saves 113 GAS
• [G-5] Cache the state variables outside the loop - Saves 100 GAS
• [G-6] <x> += <y> or <x> -= <y> costs more gas than <x> = <x> + <y> or <x> = <x> - <y> for state variables - Saves 300 GAS
• [G-7] Avoid initializing default values for state and parameters to save gas - Saves 65 GAS
• [G-8] Using immutable directly is more gas-efficient than caching - 100 GAS
• [G-9] Do not cache variables that are only used once - Saves 390 GAS
  • [G-9.1] abi.encode and abi.encodePacked are unnecessarily cached : Saves 350 GAS , 10 Instances
  • [G-9.2] Unnecessary Cache : Saves 40 GAS
• [G-10] Use assembly to perform efficient back-to-back calls - Saves 4116 GAS
• [G-11] Use assembly for loops

[G-1] State variables can be packed into fewer storage slots

Saves 10000 GAS , 5 SLOTs

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper

[G-1.1] _unlocked , bridgeAgentExecutorAddress can be packed same SLOT : Saves 2000 GAS , 1 SLOT

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

_unlocked variable only stores the values 1 and 2 . So this can be declared as uint96 instead of uint256 to save one storage slot .

FILE: 2023-09-maia/src/MulticallRootRouter.sol

73:   /// @notice Bridge Agent to manage communications and cross-chain assets.
74:    address payable public bridgeAgentAddress;
75:
76:    /// @notice Bridge Agent Executor Address.
77:    address public bridgeAgentExecutorAddress;
78:
79:    /// @notice Re-entrancy lock modifier state.
+ 80:    uint96 internal _unlocked = 1;
- 80:    uint256 internal _unlocked = 1;

[G-1.2] settlementNonce and _unlocked can be packed same SLOT : Saves 2000 GAS , 1 SLOT

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

_unlocked variable only stores the values 1 and 2 . So this can be declared as uint96 instead of uint256 to save one storage slot .

FILE: 2023-09-maia/src/RootBridgeAgent.sol

  /// @notice Deposit nonce used for identifying transaction.
-    uint32 public settlementNonce;

    /// @notice Mapping from Settlement nonce to Settlement Struct.
    mapping(uint256 nonce => Settlement settlementInfo) public getSettlement;

    /*///////////////////////////////////////////////////////////////
                            EXECUTOR STATE
    //////////////////////////////////////////////////////////////*/

    /// @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.
+    uint32 public settlementNonce;
-    uint256 internal _unlocked = 1;
+    uint96 internal _unlocked = 1;

[G-1.3] coreBranchRouterAddress , _unlocked can be packed with same SLOT : Saves 2000 GAS , 1 SLOT

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

_unlocked variable only stores the values 1 and 2 . So this can be declared as uint96 instead of uint256 to save one storage slot .

Some of the lines trimmed for better undertandings

FILE: Breadcrumbs2023-09-maia/src/BranchPort.sol

/// @notice Local Core Branch Router Address.
25:     address public coreBranchRouterAddress;
+ 91:     uint96 internal _unlocked = 1;
/// @notice Mapping returns the amount of a Strategy Token a given Port Strategy can manage.
    mapping(address strategy => mapping(address token => uint256 dailyLimitRemaining)) public
        strategyDailyLimitRemaining;

    /*///////////////////////////////////////////////////////////////
                            REENTRANCY STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Reentrancy lock guard state.
- 91:     uint256 internal _unlocked = 1;

[G-1.4] depositNonce and _unlocked should be packed same SLOT : Saves 2000 GAS , 1 SLOT

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

_unlocked variable only stores the values 1 and 2 . So this can be declared as uint96 instead of uint256 to save one storage slot .

FILE: 2023-09-maia/src/BranchBridgeAgent.sol

 /// @notice Deposit nonce used for identifying the transaction.
- 84:    uint32 public depositNonce;

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

    /*///////////////////////////////////////////////////////////////
                        SETTLEMENT EXECUTION STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice If true, the bridge agent has already served a request with this nonce from a given chain.
94:    mapping(uint256 settlementNonce => uint256 state) public executionState;

    /*///////////////////////////////////////////////////////////////
                           REENTRANCY STATE
    //////////////////////////////////////////////////////////////*/

    /// @notice Re-entrancy lock modifier state.
+ 84:    uint32 public depositNonce;
- 101:    uint256 internal _unlocked = 1;
+ 101:    uint96 internal _unlocked = 1;

[G-1.5] localPortAddress and _unlocked can be packed with same SLOT : Saves 2000 GAS , 1 SLOT

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

_unlocked variable only stores the values 1 and 2 . So this can be declared as uint96 instead of uint256 to save one storage slot .

FILE: Breadcrumbs2023-09-maia/src/BaseBranchRouter.sol

 /// @inheritdoc IBranchRouter
- 33:    address public localPortAddress;

    /// @inheritdoc IBranchRouter
36:    address public override localBridgeAgentAddress;

    /// @inheritdoc IBranchRouter
39:    address public override bridgeAgentExecutorAddress;

    /// @notice Re-entrancy lock modifier state.
+ 33:    address public localPortAddress;
- 42:    uint256 internal _unlocked = 1;
+ 42:    uint96 internal _unlocked = 1;

[G-2] Refactor _minimumReserves() function to be more gas-efficient

Saves 800 GAS as per tests . Gas increases as per complexity

The case where the getMinimumTokenReserveRatio[_token] is not checked can be optimized to save gas. If the getMinimumTokenReserveRatio[_token] is 0, then the expression (_currBalance + _strategyTokenDebt) * getMinimumTokenReserveRatio[_token]) / DIVISIONER will always return 0.

To avoid this unnecessary computation, we can check the value of getMinimumTokenReserveRatio[_token] before performing the calculation. If the getMinimumTokenReserveRatio[_token] is 0, then we can simply return 0

FILE: 2023-09-maia/src/BranchPort.sol

468: function _minimumReserves(uint256 _strategyTokenDebt, uint256 _currBalance, address _token)
468:        internal
469:        view
470:        returns (uint256)
+      if(getMinimumTokenReserveRatio[_token] == 0){
+       return 0;
+       }

471:    {
472:        return ((_currBalance + _strategyTokenDebt) * getMinimumTokenReserveRatio[_token]) / DIVISIONER;
473:    }

[G-3] Storage variables should be cached in stack variables rather than re-reading them from storage

Saves 1200 GAS , 12 SLOD

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

[G-3.1] deposit.hTokens.length,deposit.owner,executionState[nonce] should be cached : Saves 900 GAS ,9 SLOD

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

FILE: 2023-09-maia/src/BranchBridgeAgent.sol

 //Encode Data for cross-chain call.
        bytes memory payload;
+  uint256 length_ = deposit.hTokens.length ;
-        if (uint8(deposit.hTokens.length) == 1) {
+        if (uint8(length_) == 1) {
            if (_isSigned) {
                //Pack new Data
                payload = abi.encodePacked(
                    _hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
                    msg.sender,
                    _depositNonce,
                    deposit.hTokens[0],
                    deposit.tokens[0],
                    deposit.amounts[0],
                    deposit.deposits[0],
                    _params
                );
            } else {
                payload = abi.encodePacked(
                    bytes1(0x02),
                    _depositNonce,
                    deposit.hTokens[0],
                    deposit.tokens[0],
                    deposit.amounts[0],
                    deposit.deposits[0],
                    _params
                );
            }
-        } else if (uint8(deposit.hTokens.length) > 1) {
+        } else if (uint8(length_) > 1) {
            if (_isSigned) {
                //Pack new Data
                payload = abi.encodePacked(
                    _hasFallbackToggled ? bytes1(0x86) : bytes1(0x06),
                    msg.sender,
-                    uint8(deposit.hTokens.length),
+                    uint8(length_),
                    _depositNonce,
                    deposit.hTokens,
                    deposit.tokens,
                    deposit.amounts,
                    deposit.deposits,
                    _params
                );
            } else {
                payload = abi.encodePacked(
                    bytes1(0x03),
-                    uint8(deposit.hTokens.length),
+                    uint8(length_),
                    _depositNonce,
                    deposit.hTokens,
                    deposit.tokens,
                    deposit.amounts,
                    deposit.deposits,
                    _params
                );
            }
        }


439: if (deposit.status == STATUS_SUCCESS) revert DepositRedeemUnavailable();
+ address _owner = deposit.owner ;
- 440 if (deposit.owner == address(0)) revert DepositRedeemUnavailable();
+ 440 if (_owner  == address(0)) revert DepositRedeemUnavailable();
- 441:        if (deposit.owner != msg.sender) revert NotDepositOwner();
+ 441:        if (_owner != msg.sender) revert NotDepositOwner();


+   uint256 nonce_ = executionState[nonce] ;
// DEPOSIT FLAG: 0 (No settlement)
        if (flag == 0x00) {
            // Get Settlement Nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

            //Check if tx has already been executed
-            if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();
+            if (nonce_ != STATUS_READY) revert AlreadyExecutedTransaction();

            //Try to execute the remote request
            //Flag 0 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeNoSettlement(localRouterAddress, _payload)
            _execute(
                nonce,
                abi.encodeWithSelector(
                    BranchBridgeAgentExecutor.executeNoSettlement.selector, localRouterAddress, _payload
                )
            );

            // DEPOSIT FLAG: 1 (Single Asset Settlement)
        } else if (flag == 0x01) {
            // Parse recipient
            address payable recipient = payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))));

            // Parse Settlement Nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

            //Check if tx has already been executed
-            if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();
+            if (nonce_ != STATUS_READY) revert AlreadyExecutedTransaction();

            //Try to execute the remote request
            //Flag 1 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlement(recipient, localRouterAddress, _payload)
            _execute(
                _payload[0] == 0x81,
                nonce,
                recipient,
                abi.encodeWithSelector(
                    BranchBridgeAgentExecutor.executeWithSettlement.selector, recipient, localRouterAddress, _payload
                )
            );

            // DEPOSIT FLAG: 2 (Multiple Settlement)
        } else if (flag == 0x02) {
            // Parse recipient
            address payable recipient = payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))));

            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[22:26]));

            //Check if tx has already been executed
-            if (executionState[nonce] != STATUS_READY) revert AlreadyExecutedTransaction();
+            if (_nonce != STATUS_READY) revert AlreadyExecutedTransaction();

            //Try to execute remote request
            // Flag 2 - BranchBridgeAgentExecutor(bridgeAgentExecutorAddress).executeWithSettlementMultiple(recipient, localRouterAddress, _payload)
            _execute(
                _payload[0] == 0x82,
                nonce,
                recipient,
                abi.encodeWithSelector(
                    BranchBridgeAgentExecutor.executeWithSettlementMultiple.selector,
                    recipient,
                    localRouterAddress,
                    _payload
                )
            );

            //DEPOSIT FLAG: 3 (Retrieve Settlement)
        } else if (flag == 0x03) {
            // Parse recipient
            address payable recipient = payable(address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))));

            //Get nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));



  //Check if settlement is in retrieve mode

-            if (executionState[nonce] == STATUS_DONE) {
+            if (nonce_ == STATUS_DONE) {
                revert AlreadyExecutedTransaction();
            } else {
                //Set settlement to retrieve mode, if not already set.
-                if (executionState[nonce] == STATUS_READY) executionState[nonce] = STATUS_RETRIEVE;
+                if (nonce_ == STATUS_READY) executionState[nonce] = STATUS_RETRIEVE;

[G-3.2] settlementReference.owner should be cached : Saves 300 GAS , 3 SLOD

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

FILE: 2023-09-maia/src/RootBridgeAgent.sol

241: Settlement storage settlementReference = getSettlement[_settlementNonce];
+    address owner_ =  settlementReference.owner ;
        // Check if Settlement hasn't been redeemed.
-        if (settlementReference.owner == address(0)) revert SettlementRetryUnavailable();
+        if (owner_ == address(0)) revert SettlementRetryUnavailable();

        // Check if caller is Settlement owner
-        if (msg.sender != settlementReference.owner) {
+        if (msg.sender != owner_) {
-            if (msg.sender != address(IPort(localPortAddress).getUserAccount(settlementReference.owner))) {
+            if (msg.sender != address(IPort(localPortAddress).getUserAccount(owner_))) {
                revert NotSettlementOwner();
            }
        }

        // Update Settlement Status
        settlementReference.status = STATUS_SUCCESS;

        // Perform Settlement Retry
        _performRetrySettlementCall(
            _hasFallbackToggled,
            settlementReference.hTokens,
            settlementReference.tokens,
            settlementReference.amounts,
            settlementReference.deposits,
            _params,
            _settlementNonce,
-            payable(settlementReference.owner),
+            payable(owner_),
            _recipient,
            settlementReference.dstChainId,
            _gParams,
            msg.value
        );

[G-4] Remove redundant cache for strategyDailyLimitRemaining[msg.sender][_token] to save gas

Saves 113 Gas

The dailyLimit value is not utilized within the _checkTimeLimit IF block. The cache strategyDailyLimitRemaining[msg.sender][_token] is redundant.

FILE: Breadcrumbs2023-09-maia/src/BranchPort.sol

 function _checkTimeLimit(address _token, uint256 _amount) internal {
        uint256 dailyLimit = strategyDailyLimitRemaining[msg.sender][_token];
        if (block.timestamp - lastManaged[msg.sender][_token] >= 1 days) {
-            dailyLimit = strategyDailyLimitAmount[msg.sender][_token];
            unchecked {
                lastManaged[msg.sender][_token] = (block.timestamp / 1 days) * 1 days;
            }
        }
        strategyDailyLimitRemaining[msg.sender][_token] = dailyLimit - _amount;
    }

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

[G-5] Cache the state variables outside the loop

Saves 100 GAS for each iterations

FILE: 2023-09-maia/src/RootBridgeAgent.sol

+  uint24 _dstChainId = settlement.dstChainId;
317: // Clear Global hTokens To Recipient on Root Chain cancelling Settlement to Branch
        for (uint256 i = 0; i < settlement.hTokens.length;) {
            // Save to memory
            address _hToken = settlement.hTokens[i];

            // Check if asset
            if (_hToken != address(0)) {
                // Save to memory
-                uint24 _dstChainId = settlement.dstChainId;

                // Move hTokens from Branch to Root + Mint Sufficient hTokens to match new port deposit
                IPort(localPortAddress).bridgeToRoot(
                    msg.sender,
                    IPort(localPortAddress).getGlobalTokenFromLocal(_hToken, _dstChainId),
                    settlement.amounts[i],
                    settlement.deposits[i],
                    _dstChainId
                );
            }

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

Saves 300 GAS

Using the addition operator instead of plus-equals saves 113 gas

FILE: 2023-09-maia/src/BranchPort.sol

157: getPortStrategyTokenDebt[msg.sender][_token] += _amount;

169: getPortStrategyTokenDebt[msg.sender][_token] -= _amount;

172: getStrategyTokenDebt[_token] -= _amount;

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

[G-7] Avoid initializing default values for state and parameters to save gas

Ethereum smart contracts and gas efficiency, initializing default values can indeed result in unnecessary gas costs. Gas is a limited resource on the Ethereum network, and minimizing gas consumption is crucial to keeping transaction costs low.

FILE: 2023-09-maia/src/MulticallRootRouter.sol

278: for (uint256 i = 0; i < outputParams.outputTokens.length;) {

367: for (uint256 i = 0; i < outputParams.outputTokens.length;) {

455: for (uint256 i = 0; i < outputParams.outputTokens.length;) {

557: for (uint256 i = 0; i < outputTokens.length;) {

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

FILE: 2023-09-maia/src/RootBridgeAgentExecutor.sol

281: for (uint256 i = 0; i < uint256(uint8(numOfAssets));) {

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

[G-8] Using immutable directly is more gas-efficient than caching

Using immutable directly is more gas efficient than caching. This is because caching requires additional gas to store and retrieve the cached data. Saves 15 Gas per instance

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

FILE: 2023-09-maia/src/ArbitrumBranchPort.sol

56: // Save root port address to memory
-        address _rootPortAddress = rootPortAddress;

        // Get global token address from root port
-        address _globalToken = IRootPort(_rootPortAddress).getLocalTokenFromUnderlying(_underlyingAddress, 
+        address _globalToken = IRootPort(rootPortAddress).getLocalTokenFromUnderlying(_underlyingAddress, localChainId);

        // Check if the global token exists
        if (_globalToken == address(0)) revert UnknownGlobalToken();

        // Deposit Assets to Port
        _underlyingAddress.safeTransferFrom(_depositor, address(this), _deposit);

        // Request Minting of Global Token
-        IRootPort(_rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);
+        IRootPort(rootPortAddress).mintToLocalBranch(_recipient, _globalToken, _deposit);



- 80:  address _rootPortAddress = rootPortAddress;

        // Check if the global token exists
-        if (!IRootPort(_rootPortAddress).isGlobalToken(_globalAddress, localChainId)) revert UnknownGlobalToken();
+        if (!IRootPort(rootPortAddress).isGlobalToken(_globalAddress, localChainId)) revert UnknownGlobalToken();

        // Get the underlying token address from the root port
        address _underlyingAddress =
-            IRootPort(_rootPortAddress).getUnderlyingTokenFromLocal(_globalAddress, localChainId);
+            IRootPort(rootPortAddress).getUnderlyingTokenFromLocal(_globalAddress, localChainId);

        // Check if the underlying token exists
        if (_underlyingAddress == address(0)) revert UnknownUnderlyingToken();

-        IRootPort(_rootPortAddress).burnFromLocalBranch(_depositor, _globalAddress, _amount);
+        IRootPort(rootPortAddress).burnFromLocalBranch(_depositor, _globalAddress, _amount);

        _underlyingAddress.safeTransfer(_recipient, _amount);

[G-9] Do not cache variables that are only used once

If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend . Saves 35 GAS

[G-9.1] abi.encode and abi.encodePacked are unnecessarily cache : Saves 350 GAS , 10 Instances

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

FILE: 2023-09-maia/src/CoreRootRouter.sol

167:  // Encode CallData
-        bytes memory params = abi.encode(_branchBridgeAgentFactory);

        // Pack funcId into data
-        bytes memory payload = abi.encodePacked(bytes1(0x03), params);

        //Add new global token to branch chain
        IBridgeAgent(bridgeAgentAddress).callOut{value: msg.value}(
-            payable(_refundee), _refundee, _dstChainId, payload, _gParams
+         payable(_refundee), _refundee, _dstChainId, abi.encodePacked(bytes1(0x03), abi.encode(_branchBridgeAgentFactory)), _gParams
        );

 
192: //Encode CallData
-        bytes memory params = abi.encode(_branchBridgeAgent);

        // Pack funcId into data
-        bytes memory payload = abi.encodePacked(bytes1(0x04), params);

        //Add new global token to branch chain
        IBridgeAgent(bridgeAgentAddress).callOut{value: msg.value}(
-            payable(_refundee), _refundee, _dstChainId, payload, _gParams
+            payable(_refundee), _refundee, _dstChainId, abi.encodePacked(bytes1(0x04), abi.encode(_branchBridgeAgent)), _gParams
        );

219: // Encode CallData
-        bytes memory params = abi.encode(_underlyingToken, _minimumReservesRatio);

        // Pack funcId into data
-        bytes memory payload = abi.encodePacked(bytes1(0x05), params);

        //Add new global token to branch chain
        IBridgeAgent(bridgeAgentAddress).callOut{value: msg.value}(
-            payable(_refundee), _refundee, _dstChainId, payload, _gParams
+            payable(_refundee), _refundee, _dstChainId, abi.encodePacked(bytes1(0x05), abi.encode(_underlyingToken, _minimumReservesRatio)), _gParams
        );

250: // Encode CallData
-        bytes memory params = abi.encode(_portStrategy, _underlyingToken, _dailyManagementLimit, _isUpdateDailyLimit);

        // Pack funcId into data
-        bytes memory payload = abi.encodePacked(bytes1(0x06), params);

        //Add new global token to branch chain
        IBridgeAgent(bridgeAgentAddress).callOut{value: msg.value}(
-            payable(_refundee), _refundee, _dstChainId, payload, _gParams
+            payable(_refundee), _refundee, _dstChainId, abi.encodePacked(bytes1(0x06), abi.encode(_portStrategy, _underlyingToken, _dailyManagementLimit, _isUpdateDailyLimit)), _gParams
        );

280: // Encode CallData
-        bytes memory params = abi.encode(_coreBranchRouter, _coreBranchBridgeAgent);

        // Pack funcId into data
-        bytes memory payload = abi.encodePacked(bytes1(0x07), params);

        //Add new global token to branch chain
        IBridgeAgent(bridgeAgentAddress).callOut{value: msg.value}(
-            payable(_refundee), _refundee, _dstChainId, payload, _gParams
+            payable(_refundee), _refundee, _dstChainId, abi.encodePacked(bytes1(0x07), abi.encode(_coreBranchRouter, _coreBranchBridgeAgent)), _gParams
        );

[G-9.2] Unnecessary cache : Saves 40 GAS

FILE: 2023-09-maia/src/RootPort.sol

- 195: address globalAddress = getGlobalTokenFromLocal[_localAddress][_srcChainId];
- 196:        return getLocalTokenFromGlobal[globalAddress][_dstChainId];
+ 196:        return getLocalTokenFromGlobal[getGlobalTokenFromLocal[_localAddress][_srcChainId]][_dstChainId];

- 206: address localAddress = getLocalTokenFromGlobal[_globalAddress][_srcChainId];
- 207:        return getUnderlyingTokenFromLocal[localAddress][_srcChainId];
+ 207:        return getUnderlyingTokenFromLocal[getLocalTokenFromGlobal[_globalAddress][_srcChainId]][_srcChainId];

[G-10] Use assembly to perform efficient back-to-back calls

If a similar external call is performed back-to-back, we can use assembly to reuse any function signatures and function parameters that stay the same. In addition, we can also reuse the same memory space for each function call (scratch space + free memory pointer + zero slot), which can potentially allow us to avoid memory expansion costs.

FILE: 2023-09-maia/src/CoreRootRouter.sol

426: ERC20(_globalAddress).name(),
427: ERC20(_globalAddress).symbol(),
428: ERC20(_globalAddress).decimals(),

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

FILE: 2023-09-maia/src/ArbitrumCoreBranchRouter.sol

56: string.concat("Arbitrum Ulysses ", ERC20(_underlyingAddress).name()),
57:            string.concat("arb-u", ERC20(_underlyingAddress).symbol()),
58:            ERC20(_underlyingAddress).decimals()

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

FILE: 2023-09-maia/src/BaseBranchRouter.sol

63: localPortAddress = IBridgeAgent(_localBridgeAgentAddress).localPortAddress();
64: bridgeAgentExecutorAddress = IBridgeAgent(_localBridgeAgentAddress).bridgeAgentExecutorAddress();

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

FILE: 2023-09-maia/src/factories/ERC20hTokenBranchFactory.sol

66: ERC20(_wrappedNativeTokenAddress).name(),
67: ERC20(_wrappedNativeTokenAddress).symbol(),
68: ERC20(_wrappedNativeTokenAddress).decimals(),

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

[G-11] Use assembly for loops

Saves minimum 1400 GAS

Assembly for loops can save gas in Solidity by avoiding the overhead of stack operations. When using a high-level language like Solidity, the compiler generates assembly code that includes stack operations to store and retrieve variables. This can lead to high gas costs, especially if the for loop is iterating over a large number of elements.

To avoid this overhead, you can use an assembly for loop instead. An assembly for loop allows you to explicitly manage the stack, which can lead to significant gas savings.

As per sample tests in Remix IDE this saves around 350 Gas per iterations. The gas savings may high based on the complexity

FILE: 2023-09-maia/src/BaseBranchRouter.sol

- for (uint256 i = 0; i < _hTokens.length;) {
-            _transferAndApproveToken(_hTokens[i], _tokens[i], _amounts[i], _deposits[i]);
-
-            unchecked {
-                ++i;
-            }

+ assembly {
+  // Initialize the counter variable
+  let i := 0
+
+  // Get the length of the _hTokens array
+  let hTokensLength := mload(_hTokens)
+
+  // Iterate over the _hTokens array
+  for {} i lt hTokensLength {} {
+    // Load the element at index i from the _hTokens array
+    let hToken := mload(add(_hTokens, mul(i, 32)))
+
+    // Load the element at index i from the _tokens array
+    let token := mload(add(_tokens, mul(i, 32)))
+
+    // Load the element at index i from the _amounts array
+    let amount := mload(add(_amounts, mul(i, 32)))
+
+    // Load the element at index i from the _deposits array
+    let deposit := mload(add(_deposits, mul(i, 32)))
+
+    // Call the _transferAndApproveToken function
+    calldatacopy(0, hToken, 32)
+    calldatacopy(32, token, 32)
+    calldatacopy(64, amount, 32)
+    calldatacopy(96, deposit, 32)
+    staticcall(gas, _transferAndApproveToken.address, 0, 128, 0, 0)
+
+    // Increment the counter
+    i := add(i, 1)
+  }
+ }

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

FILE: 2023-09-maia/src/MulticallRootRouter.sol

 for (uint256 i = 0; i < outputParams.outputTokens.length;) {
                IVirtualAccount(userAccount).withdrawERC20(outputParams.outputTokens[i], outputParams.amountsOut[i]);

                unchecked {
                    ++i;
                }
            }

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

FILE: 2023-09-maia/src/VirtualAccount.sol

70: for (uint256 i = 0; i < length;) {
            bool success;
            Call calldata _call = calls[i];

            if (isContract(_call.target)) (success, returnData[i]) = _call.target.call(_call.callData);

            if (!success) revert CallFailed();

            unchecked {
                ++i;
            }

90: for (uint256 i = 0; i < length;) {
            _call = calls[i];
            uint256 val = _call.value;
            // Humanity will be a Type V Kardashev Civilization before this overflows - andreas
            // ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
            unchecked {
                valAccumulator += val;
            }

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

Maia DAO - Ulyss Analysis

Overview

Ulysses Protocol is a decentralized and permissionless liquidity protocol that aims to promote capital efficiency in the face of an increasingly fragmented liquidity landscape in DeFi. It enables liquidity providers to deploy their assets from one chain and earn revenue from activity in an array of chains, all while mitigating the negative effects of current market solutions. Additionally, it offers a way for DeFi protocols to lower their operational and managing costs by incentivizing liquidity for a single unified liquidity token instead of managing incentives for pools scattered across different AMMs and chains

High-level overview of how Ulysses Protocol works

• Liquidity providers deposit their assets into a Ulysses liquidity pool on a single chain
• Ulysses cross-chain messaging is used to communicate with liquidity pools on other chains
• Liquidity providers earn yield from across multiple chains, even though their assets are only deposited on a single chain.
• DeFi protocols can use Ulysses Unified Liquidity Tokens to access liquidity from across multiple chains without having to manage incentives for pools scattered across different AMMs and chains.

Approach taken in evaluating the codebase

• Preliminary analysis: I read the contest Readme.md file and took the following notes:

• The Maia DAO - Ulysses learnings,

  • Inheritance
  • Test coverage is 69%
  • Uses L2, Multi-Chain, Side-Chain, ERC-20 Token, Timelock function
  • Using Layerzero Messaging layer

Areas to focus

• Double spending of deposit and settlement nonces / assets (Bridge Agents and Bridge Agent Executors).
• Griefing of user deposits and settlements (Bridge Agents).
• Bricking of Bridge Agent and subsequent Omnichain dApps that rely on it.
• Circumventing Bridge Agent's encoding rules to manipulate remote chain state.

Main invariants

1. The total balance of any given Virtualized Liquidity Token should never be greater than the amount of Underlying Tokens deposited in the asset's origin chain Branch Port.
2. A Deposit / Settlement can never be redeemable and retryable at the same time.

• High-level overview : I analyzed the overall codebase in one iteration to get a high-level understanding of the code structure and functionality.

• Documentation review : I studied the documentation to understand the purpose of each contract, its functionality, and how it is connected with other contracts.

• Literature review : I read old audits and known findings, as well as the bot races findings.

• Testing setup : I set up my testing environment and ran the tests to ensure that all tests passed. I used Foundry to test the Maia DAO - Ulysses , and I used forge comments for testing.

  • forge build
  • forge test

• Detailed analysis : I started with the detailed analysis of the code base, line by line. I took the necessary notes to ask some questions to the sponsors.

Architecture recommendations

Concerns Regarding Excessive Centralization in Ports, Bridges, and Routers

Regarding the centralization of ports, bridges, and routers in the protocol's architecture is valid. When these components are centralized, it introduces a single point of failure and potential vulnerabilities that can lead to unexpected behavior across the entire protocol.

Here are some considerations related to centralization in the context of ports, bridges, and routers

Single Point of Failure: Centralized components can become a single point of failure. If any issues or disruptions occur with these components, it can impact the overall functionality of the protocol, potentially causing downtime or operational disruptions.

Security Risks: Centralized components may present security risks. Malicious actors might target these components to exploit vulnerabilities, compromise the protocol's security, or steal assets.

Lack of Redundancy: Without decentralization and redundancy, there's limited backup or failover mechanisms in place. If a centralized component fails, there may be no backup to maintain the protocol's operations.

Mitigation

• Consider decentralizing critical components like ports, bridges, and routers to distribute control and reduce the risk of single points of failure
• Establish contingency plans and redundancy mechanisms to ensure the protocol can continue functioning even if one component encounters issues

Use a more efficient data structure to store the Port data and branch data

The current data structure used by the Ports and bridges is a simple mapping . This data structure can be inefficient for certain operations, such as querying the total balance of all assets in a Port and bridges. One way to improve the efficiency of the Ports and bridges is to use a more sophisticated data structure, such as a Merkle tree. This would allow the Ports and bridges to quickly query the total balance of all assets in a Port and brances, as well as the balance of any individual asset

Implement a load balancing mechanism to distribute traffic across multiple Ports

The current architecture of the Ports and bridges is centralized, with all traffic being routed through a single Port and single bridge. This can be a bottleneck, especially if the Port and bridges is under a lot of load. One way to improve the scalability of the Ports is to implement a load balancing mechanism. This would distribute traffic across multiple Ports, which would help to improve performance and reliability

Again the single Root Bridge Agent can be a single point of failure in a cross-chain bridge system. If the Root Bridge Agent is compromised or unavailable, the entire system can be impacted. This is a serious security risk, as it could allow an attacker to steal funds or disrupt the operation of the system.

To mitigate the risk of a single Root Bridge Agent being a single point of failure

Implement a monitoring system

Monitor the Root Bridge Agents to detect and respond to any problems. This could be achieved using a combination of tools and techniques, such as intrusion detection systems (IDSs) and security information and event management (SIEM) systems.

Implement a disaster recovery plan

Have a disaster recovery plan in place to minimize the impact of any outages or disruptions. This plan should include steps such as backing up the Root Bridge Agent data and deploying the Root Bridge Agents to a secondary environment in the event of an outage.

Use a queuing system

Queueing systems are useful for handling spikes in traffic because they can buffer requests and process them in a timely manner. This is especially important for Bridge Agents, as they need to be able to handle a large number of concurrent requests from users.

One way to implement a queuing system for Bridge Agents is to use a message broker such as RabbitMQ or Kafka. Message brokers are designed to handle large volumes of messages and can provide high performance and reliability.

RabbitMQ - RabbitMQ is a popular message broker that is often used in DeFi applications. It is a highly scalable and reliable queuing system that can handle large volumes of messages.

Kafka: Kafka is another popular message broker that is often used in DeFi applications. It is a distributed queuing system that is well-suited for processing high volumes of streaming data.

Possible risks using LayerZero messaging layer

• Complexity: The LayerZero messaging layer is a complex system, and it can be difficult to understand and use. This can make it challenging for developers to build applications on top of LayerZero.

• Security: The LayerZero messaging layer is still under development, and there is some concern about its security. For example, there have been some reports of attacks on LayerZero-based applications.

• Centralization: The LayerZero messaging layer is currently centralized, meaning that it is controlled by a single team. This centralization could pose a risk if the team were to be compromised or decide to abandon the project.

• Cost: The LayerZero messaging layer can be expensive to use. This is because it requires a lot of computing power to process messages.

Codebase quality analysis

• You have done a good job providing comments for various parts of your contract. However, it's important to ensure that the comments are clear and provide sufficient information for someone reading the code. Make sure to explain the purpose and functionality of the contract's functions and variables.
• Conventionally, modifiers are often placed before the function declaration.This makes the code more readable as it's clear which modifiers apply to a function.
• Instead of using raw numbers like 1e4 and 3e3, consider defining named constants to make the code more readable and maintainable
• Consider using require for input validation instead of if statements. This provides better readability and ensures that the function reverts if the condition is not met
• Organize your functions in a logical order within the contract
• Emit events with descriptive names and include relevant information. Events are essential for tracking contract interactions and debugging
• Be cautious when using address(this) as it can have security implications. Make sure that only trusted contracts can call your functions with this address
• The contract uses multiple modifiers in some functions, leading to a complex modifier chain

Centralization risks

The following functions in the BranchPort.sol and RootPort.sol files are related to the single point of failure

• initialize(): This function is used to initialize the BranchPort or RootPort contract. It takes the addresses of the CoreBranchRouter and BridgeAgentFactory contracts as arguments.
• renounceOwnership() : This function is used to renounce ownership of the BranchPort or RootPort contract.
• toggleBridgeAgent() and addBridgeAgentFactory(): These functions are used to toggle or add BridgeAgent contracts.
• toggleBridgeAgentFactory(): This function is used to toggle a BridgeAgentFactory contract.
• addNewChain(): This function is used to add a new chain to the protocol.
• addEcosystemToken(): This function is used to add a new ecosystem token to the protocol.
• setCoreBranchRouter(): This function is used to set the CoreBranchRouter contract address.
• setCoreRootRouter(): This function is used to set the CoreRootRouter contract address.
• syncNewCoreBranchRouter(): This function is used to sync a new CoreBranchRouter contract address.

All of these functions are restricted to the only owner of the BranchPort or RootPort contract. This means that the only owner has complete control over the contract and can make any changes they want.

If the only owner is compromised, the attacker could use these functions to steal funds, disrupt the operation of the system, or manipulate the price of assets.

Possible Mitigation Steps

• multi-signature wallet Multi-signature wallet to control the only owner account. This would require multiple people to sign off on transactions, making it more difficult for an attacker to gain control of the account.
• Decentralized governance system Decentralized governance system to give users a say in the management of the protocol. This would make it more difficult for the only owner to make decisions that could harm the protocol
• Use role based owners By using role-based owners, you can reduce the risk of a compromised only owner impacting your protocol. If an attacker were to gain control of one of the roles, they would only be able to make changes related to that role

Mechanism review

The omnichain architecture employs several key mechanisms:

Ports: Ports serve as liquidity repositories and vaults for specific tokens, facilitating deposit and withdrawal actions without protocol fees. Branch Ports within each chain and the Root Port in the Root Chain manage token pools.

Virtualized Liquidity: This mechanism enables assets to remain locked in their respective chains while providing liquidity across multiple chains. It enhances capital efficiency and enables participation in various pools and protocols.

Bridge Agents: Bridge Agents act as intermediaries, ensuring seamless communication between chains. Branch and Root Bridge Agents mediate interactions and manage settlements between different chains.

Routers: Branch and Root Routers facilitate cross-chain interactions. Branch Routers handle user-facing entry and exit points within the omnichain system, while Root Routers connect to the Root Chain and interact with various applications, enabling customizable actions before, during, and after cross-chain interactions.

These mechanisms work together to optimize liquidity provision, reduce operational costs, and enable composability across the ecosystem.

Systemic risks

Consensus Mechanism Compatibility

Different blockchains may use different consensus mechanisms (e.g., Proof of Work, Proof of Stake). Ensuring compatibility and security when transferring assets between chains with different consensus mechanisms is a challenge.

Scaling Issues

Scaling an omnichain architecture to handle a large number of chains, routers, and users can be a significant challenge. Ensuring that the system remains performant under heavy loads is crucial.

Token Standards

Different blockchains may use different token standards (e.g., ERC-20, BEP-20). Handling tokens with different standards and ensuring compatibility can be complex.

Cross-Chain Liquidity

Managing liquidity across multiple chains to support withdrawals and deposits efficiently can be complex, especially during periods of high demand.

Interoperability risks in LayerZero messaging layer

The LayerZero messaging layer is still relatively new, and it is not yet interoperable with all blockchain networks

Scalability risks in LayerZero messaging layer

It is not yet clear how well the LayerZero messaging layer will scale to handle large volumes of traffic

Smart contract vulnerabilities

Smart contracts are the foundation of the Ulysses Protocol, and any vulnerabilities in these contracts could be exploited by attackers to steal funds or disrupt the operation of the protocol.

Regulatory risks

The Ulysses Protocol is a decentralized protocol, but it is still subject to regulation. If there are changes in regulation that impact the Ulysses Protocol, it could impact the adoption and use of the protocol.

Concentration risks

Virtualized Liquidity systems are often centralized, with a small number of actors controlling the system. This concentration of power could lead to abuse or failure.

Cross-Chain Slippage

Variations in prices and liquidity across different chains can lead to slippage for users conducting cross-chain swaps. Managing and minimizing slippage is a challenge.

Network Congestion

In periods of high demand, network congestion can occur, leading to delays and higher transaction costs. Have strategies in place to manage and optimize transactions during such periods.

Resource Scaling Risks

Be prepared to scale Bridge Agents resources, such as computing power and bandwidth, as the platform grows to accommodate increased demand.

Systemic Risks Based On CodeBase Analysis

2023-09-maia/src/RootPort.sol

• The contract has multiple initialization functions (initialize and initializeCore) that should be called in a specific order. If these functions are not called correctly or in the wrong order, it could lead to improper contract setup.
• Be cautious about potential reentrancy vulnerabilities when interacting with external contracts, especially in functions like bridgeToRoot() and bridgeToRootFromLocalBranch() , bridgeToLocalBranchFromRoot() .
• The contract uses several mappings, which can be expensive in terms of gas costs and storage. Ensure that the storage costs are considered and that there is no excessive storage usage that could impact scalability
• Unable to remove BridgeAgent once added to array. We can toggle the isBridgeAgent[_bridgeAgent] value but not possible to remove completely
• The contract uses an Ownable pattern to manage ownership and access control. If the owner account is compromised or misuses their power, it could have systemic consequences.
• The contract allows adding and toggling Bridge Agents. If malicious or untrusted Bridge Agents are added, they could manipulate the contract's behavior and impact the entire system's security and stability
• The contract synchronizes with other chains and agents. If synchronization fails or is tampered with, it could disrupt the entire system's functionality

2023-09-maia/src/BranchBridgeAgent.sol

• The _performCall function is called from multiple external functions without proper checks to prevent reentrancy. If the called contract performs an external call back into this contract, it could re-enter the same function before the previous call completes
• The contract handles deposits with increasing nonces, but it doesn't track or limit the maximum nonce, which could lead to a growing storage size over time. Functions related to deposit handling, including callOutAndBridge, callOutAndBridgeMultiple, retryDeposit, retrieveDeposit, and redeemDeposit.
• The contract uses a mapping to track if a given settlement nonce has been executed. If this mapping grows without bounds, it could lead to higher gas costs and potential denial-of-service attacks . The executionState mapping used in functions related to settlement execution, such as retrySettlement.
• The contract includes a fallback function (receive()), but it's not clear what purpose this function serves
• The contract relies heavily on data serialization and deserialization for cross-chain calls, and errors in this process could lead to incorrect or failed transactions
• The getFeeEstimate function estimates gas fees based on external contract calls. Depending on external contracts for gas estimation could lead to incorrect fee calculations and failed transactions
• The contract includes functionality to toggle fallback behavior based on a boolean flag (_hasFallbackToggled). Depending on how this flag is set, it could potentially introduce unexpected behavior or vulnerabilities.

Implementation Suggestions

Considering upgradeable contracts and proxies for future updates

By incorporating upgradeable contracts and proxies, you can position Ulysses Protocol as a dynamic and robust DeFi platform, capable of evolving to meet the changing needs of the DeFi community while maintaining trust and security.

Test Coverage

The test coverage analysis for the codebase reveals an overall coverage percentage of 55.05%. While this represents some test coverage, it's essential to acknowledge that achieving higher coverage levels is generally advisable for ensuring the reliability and robustness of the code

$ forge coverage
[... snip ...]

 | % Lines                                                                    | % Statements      | % Branches        | % Funcs         |
|-----------------------------------------------------------------------------|-------------------|-------------------|------------------|------------------|
| src/ArbitrumBranchBridgeAgent.sol                                           | 88.89% (8/9)      | 72.73% (8/11)     | 50.00% (2/4)     | 71.43% (5/7)     |
| src/ArbitrumBranchPort.sol                                                  | 100.00% (17/17)   | 95.24% (20/21)    | 80.00% (8/10)    | 100.00% (4/4)    |
| src/ArbitrumCoreBranchRouter.sol                                            | 46.67% (14/30)    | 50.00% (19/38)    | 21.43% (3/14)    | 100.00% (3/3)    |
| src/BaseBranchRouter.sol                                                    | 87.50% (21/24)    | 88.00% (22/25)    | 50.00% (3/6)     | 70.00% (7/10)    |
| src/BranchBridgeAgent.sol                                                   | 79.87% (119/149)  | 74.48% (143/192)  | 42.42% (28/66)   | 76.92% (20/26)   |
| src/BranchBridgeAgentExecutor.sol                                           | 84.62% (11/13)    | 88.24% (15/17)    | 0.00% (0/4)      | 100.00% (4/4)    |
| src/BranchPort.sol                                                          | 48.04% (49/102)   | 43.22% (51/118)   | 34.09% (15/44)   | 51.85% (14/27)   |
| src/CoreBranchRouter.sol                                                    | 87.30% (55/63)    | 88.61% (70/79)    | 67.86% (19/28)   | 100.00% (9/9)    |
| src/CoreRootRouter.sol                                                      | 72.86% (51/70)    | 73.68% (70/95)    | 63.89% (23/36)   | 61.11% (11/18)   |
| src/MulticallRootRouter.sol                                                 | 74.44% (67/90)    | 73.74% (73/99)    | 35.71% (10/28)   | 76.92% (10/13)   |
| src/MulticallRootRouterLibZip.sol                                           | 100.00% (1/1)     | 100.00% (1/1)     | 100.00% (0/0)    | 100.00% (1/1)    |
| src/RootBridgeAgent.sol                                                     | 76.50% (166/217)  | 73.15% (188/257)  | 43.65% (55/126)  | 90.91% (20/22)   |
| src/RootBridgeAgentExecutor.sol                                             | 75.00% (27/36)    | 75.51% (37/49)    | 50.00% (4/8)     | 80.00% (8/10)    |
| src/RootPort.sol                                                            | 49.56% (56/113)   | 42.96% (58/135)   | 30.26% (23/76)   | 54.84% (17/31)   |
| src/VirtualAccount.sol                                                      | 40.00% (12/30)    | 40.54% (15/37)    | 30.00% (3/10)    | 33.33% (3/9)     |
| src/factories/ArbitrumBranchBridgeAgentFactory.sol                          | 50.00% (4/8)      | 44.44% (4/9)      | 33.33% (2/6)     | 50.00% (1/2)     |
| src/factories/BranchBridgeAgentFactory.sol                                  | 50.00% (4/8)      | 44.44% (4/9)      | 50.00% (3/6)     | 50.00% (1/2)     |
| src/factories/ERC20hTokenBranchFactory.sol                                  | 25.00% (2/8)      | 22.22% (2/9)      | 0.00% (0/2)      | 33.33% (1/3)     |
| src/factories/ERC20hTokenRootFactory.sol                                    | 50.00% (3/6)      | 50.00% (3/6)      | 0.00% (0/2)      | 66.67% (2/3)     |
| src/factories/RootBridgeAgentFactory.sol                                    | 100.00% (2/2)     | 100.00% (2/2)     | 100.00% (0/0)    | 100.00% (1/1)    |
| src/token/ERC20hTokenBranch.sol                                             | 100.00% (3/3)     | 100.00% (3/3)     | 100.00% (0/0)    | 100.00% (2/2)    |
| src/token/ERC20hTokenRoot.sol                                               | 100.00% (5/5)     | 100.00% (5/5)     | 100.00% (0/0)    | 100.00% (2/2)    |
| test/test-utils/fork/LzForkTest.t.sol                                       | 0.00% (0/132)     | 0.00% (0/148)     | 0.00% (0/21)     | 0.00% (0/30)     |
| test/ulysses-omnichain/MulticallRootRouterZippedTest.t.sol                  | 50.00% (1/2)      | 50.00% (1/2)      | 100.00% (0/0)    | 50.00% (1/2)     |
| test/ulysses-omnichain/RootForkTest.t.sol                                   | 0.00% (0/1)       | 0.00% (0/1)       | 100.00% (0/0)    | 0.00% (0/1)      |
| test/ulysses-omnichain/RootTest.t.sol                                       | 90.62% (29/32)    | 90.91% (30/33)    | 50.00% (3/6)     | 100.00% (3/3)    |
| test/ulysses-omnichain/helpers/ArbitrumBranchBridgeAgentFactoryHelper.t.sol | 0.00% (0/14)      | 0.00% (0/14)      | 0.00% (0/12)     | 0.00% (0/8)      |
| test/ulysses-omnichain/helpers/ArbitrumBranchPortHelper.t.sol               | 0.00% (0/16)      | 0.00% (0/16)      | 0.00% (0/12)     | 0.00% (0/10)     |
| test/ulysses-omnichain/helpers/ArbitrumCoreBranchRouterHelper.t.sol         | 0.00% (0/3)       | 0.00% (0/3)       | 100.00% (0/0)    | 0.00% (0/2)      |
| test/ulysses-omnichain/helpers/BaseBranchRouterHelper.t.sol                 | 0.00% (0/4)       | 0.00% (0/4)       | 0.00% (0/2)      | 0.00% (0/3)      |
| test/ulysses-omnichain/helpers/CoreRootRouterHelper.t.sol                   | 0.00% (0/16)      | 0.00% (0/16)      | 0.00% (0/12)     | 0.00% (0/10)     |
| test/ulysses-omnichain/helpers/ERC20hTokenRootFactoryHelper.t.sol           | 0.00% (0/13)      | 0.00% (0/13)      | 0.00% (0/8)      | 0.00% (0/8)      |
| test/ulysses-omnichain/helpers/MulticallRootRouterHelper.t.sol              | 0.00% (0/16)      | 0.00% (0/16)      | 0.00% (0/12)     | 0.00% (0/10)     |
| test/ulysses-omnichain/helpers/RootBridgeAgentExecutorHelper.t.sol          | 0.00% (0/2)       | 0.00% (0/2)       | 0.00% (0/2)      | 0.00% (0/2)      |
| test/ulysses-omnichain/helpers/RootBridgeAgentFactoryHelper.t.sol           | 0.00% (0/11)      | 0.00% (0/11)      | 0.00% (0/6)      | 0.00% (0/6)      |
| test/ulysses-omnichain/helpers/RootBridgeAgentHelper.t.sol                  | 0.00% (0/11)      | 0.00% (0/11)      | 0.00% (0/10)     | 0.00% (0/6)      |
| test/ulysses-omnichain/helpers/RootForkHelper.t.sol                         | 0.00% (0/19)      | 0.00% (0/19)      | 100.00% (0/0)    | 0.00% (0/3)      |
| test/ulysses-omnichain/helpers/RootPortHelper.t.sol                         | 0.00% (0/21)      | 0.00% (0/21)      | 0.00% (0/18)     | 0.00% (0/14)     |
| test/ulysses-omnichain/mocks/MockRootBridgeAgent.t.sol                      | 100.00% (35/35)   | 100.00% (42/42)   | 100.00% (0/0)    | 100.00% (1/1)    |
| test/ulysses-omnichain/mocks/Multicall2.sol                                 | 25.00% (6/24)     | 31.03% (9/29)     | 16.67% (1/6)     | 8.33% (1/12)     |
| test/ulysses-omnichain/mocks/WETH9.sol                                      | 0.00% (0/19)      | 0.00% (0/19)      | 0.00% (0/8)      | 0.00% (0/6)      |
| Total                                                                       | 55.05% (768/1395) | 54.67% (895/1637) | 33.55% (205/611) | 43.93% (152/346) |

The areas with relatively higher coverage include ArbitrumBranchPort.sol (100%), RootBridgeAgent.sol (76.50%), and BranchBridgeAgent.sol (79.87%). However, there are critical components like VirtualAccount.sol (40.00%) and MulticallRootRouter.sol (74.44%) that have lower coverage percentages, which could be a potential source of vulnerabilities.

it's advisable to work on the following actions to enhance the codebase's quality

Augment Coverage: Identify sections with the lowest coverage percentages and prioritize the creation of comprehensive test suites for these areas. Particular attention should be given to contracts like VirtualAccount.sol to bolster their reliability and security.

Test Edge Cases: Ensure that tests cover edge cases, exceptional scenarios, and corner cases to simulate real-world conditions effectively.

Test Documentation: Maintain thorough documentation for tests, explaining their purpose, coverage, and expected outcomes. This documentation aids in understanding the rationale behind specific test cases.

Solidity Versions

If you are using a floating version of Solidity, such as ^0.8.0, this means that your codebase will be compatible with all versions of Solidity from ^0.8.0 to the latest version. This can be useful if you are not sure which version of Solidity you want to use, or if you need to support multiple versions of Solidity.

Overall, I would recommend using the latest version like 0.8.20 or 0.8.21 of Solidity if possible.

Comments

Comments can be very helpful for both auditors and developers, especially in complex codebases. I also agree that the codebase you are describing is generally clean and strategically straightforward, but there are a few sections that could benefit from additional comments.

Some specific tips for commenting :

• Add comments to explain complex code
• Add comments to document your design decisions
• Add comments to document your design decisions
• Use clear and concise language

Conclusion

Ulysses Protocol is a forward-thinking decentralized finance (DeFi) project with a mission to tackle the growing issue of liquidity fragmentation across various blockchain networks. It introduces innovative concepts like Virtualized and Unified Liquidity Management, which aim to create a unified liquidity token representing assets from different chains, thereby enabling seamless composability across a range of DeFi platforms. By doing so, Ulysses Protocol empowers liquidity providers to deploy their assets across multiple chains, optimizing capital efficiency and revenue generation while mitigating the challenges posed by the current fragmented liquidity landscape. Moreover, the protocol offers a solution for DeFi projects to reduce operational costs by incentivizing liquidity for a single unified token, simplifying management and lowering overhead. With cross-chain capabilities and a focus on community ownership, Ulysses Protocol appears poised to make a significant impact in the DeFi space, provided it can strike the right balance between innovation and security and offer clear education and documentation to users and developers.

Time spent:

30 hours

Time spent:

30 hours