Maia DAO - Ulysses - 0xsagetony's results

Low Risk and Non-Critical Issues

1. The _settlementNonce was not checked if it was valid in ArbitrumBranchBridgeAgent.sol/_performFallbackCall().

   • File: ArbitrumBranchBridgeAgent.sol
   • Function: _performFallbackCall()

2. There was no tracking of the amount going in or out of the root chain in ArbitrumBranchBridgeAgent.sol/withdrawFromPort().

   • File: ArbitrumBranchBridgeAgent.sol
   • Function: withdrawFromPort()

3. There is no check on _recipient and the _localAddress address which might lead to token loss in ArbitrumBranchPort.sol/_bridgeIn().

   • File: ArbitrumBranchPort.sol
   • Function: _bridgeIn()

4. There was no check on the localchainId validity in the BranchBridgeAgent constructor.

   • File: BranchBridgeAgent.sol
   • Constructor: BranchBridgeAgent()

5. depositNounce used for core system operations is defined as uint32 in BranchBridgeAgent/callOutSystem(). If it exceeds 4,294,967,295 it could lead to a DOS attack.

   • File: BranchBridgeAgent.sol
   • Function: callOutSystem()

6. The refundee address was not checked in BranchBridgeAgent/callOutAndBridge.

   • File: BranchBridgeAgent.sol
   • Function: callOutAndBridge()

7. The dParams.htokens.length should be checked before encoding in BranchBridgeAgent/callOutAndBridgeMultiple. If the length is more than 255 then it will be downcasted and lead to the loss of value.

   • File: BranchBridgeAgent.sol
   • Function: callOutAndBridgeMultiple()

8. The recipient and router address were not checked in BranchBridgeAgentExecutor/executeWithSettlementMultiple().

   • File: BranchBridgeAgentExecutor.sol
   • Function: executeWithSettlementMultiple()

9. There was no address check in BranchPort/withdraw.

   • File: BranchPort.sol
   • Function: withdraw()

10. The addresses were not checked in CoreRootRouter/initialize() and CoreRootRouter/removeBranchBridgeAgent().

    • File: CoreRootRouter.sol
    • Function: initialize()
    • Function: removeBranchBridgeAgent()

11. lzendpointaddress was not checked in the constructor of the RootBridgeAgentFactory contract.

    • File: RootBridgeAgentFactory.sol
    • Constructor: RootBridgeAgentFactory()

12. No address check on RootBridgeAgentExecutor/executeSystemRequest().

    • File: RootBridgeAgentExecutor.sol
    • Function: executeSystemRequest()

13. Inconsistent use of require and revert statements. One of the statements should be used across the board. Revert statements are the recommended approach to save gas, in initialize(), initializeCore(), setCoreRouter() of RootPort.sol.

    • File: RootPort.sol
    • Function: initialize()
    • Function: initializeCore()

14. Missing events for core system function in BranchPort/addBridgeAgent().

    • File: BranchPort.sol

15. Errors should be prefixed with the contract name for better error handling. E.g:

error BranchPort__AlreadyAddedBridgeAgent instead of error AlreadyAddedBridgeAgent().

• File: BranchPort.sol

Please address these issues to improve the security and robustness of your contract.

Any comments for the judge to contextualize your findings

Context was added where it was relevant to aid the judges.

Approach taken in evaluating the protocol

Since this audit was focused on the Ulysses protocol, here are the steps taken to complete this audit.

1. Went through the Ulysses docs to understand the protocol.
   • There are 4 major components:
     1. Ports: Root and Branch ports.
     2. Routers: Root and Branch routers.
     3. Bridge Agents: Root and Branch Bridge Agents.
     4. Virtualized Accounts and Liquidity.
2. Drew two diagrams based on this understanding. [1],[2]
3. Started to read the codebase high-level, starting from root and branch ports.
4. With the docs in mind, I started to read each contract matches their supposed functionalities, functions calls (depending on how they are chained) do what they are supposed to do.
5. When in doubt or confused, I consult the docs or ask in the discord channel dedicated for the audit. The sponsor and other wardens were kind enough to answer my questions.
6. Next, I evaluate the contracts's logic, following the chained internal functions.
7. Over the course of the above processes, I take notes about observations and possible attack vectors/paths.
8. Last but not least, I review the notes and examine observations/attack vectors with more depth.

Observations

• Ulysses codebase has some of the cleanest codes out of the box.
• More often than not, Integration with Layerzero follows best practices.
• Inconsistencies is some part of the codebase e.g usage of reverts and require simultaneously plus several missing checks for addresses passed to core functions.

Architecture recommendations

• The current design uses _amount - _deposit in several parts of the codebase for calculation of the htokens that a user wants to bridge in/out or use in a transaction. This is counter-intuitive in some cases. For instance: If a user has htoken in source chain but only wants to use underlying tokens for a specific transaction, they would have no means to do so. A boolean can be passed as well to indicate if the user wants to use htokens or not before even doing the calculation of htokens for the tx.

• Minor but, error handling is good but it could be better if it includes the contract where the error originated from. e.g error BranchPort__InvalidInputArrays(); instead of generic error InvalidInputArrays();

Systemic and/or Centralization risks

The system is designed to be as decentralized as possible. However, ownership is renounced is some areas of the codebase.

Time spent:

80 hours.

Time spent:

80 hours