code-423n4 / 2023-05-maia-findings

24 stars 13 forks source link

Analysis #840

Open CloudEllie opened 1 year ago

CloudEllie commented 1 year ago

Hermes Analysis

Approach

The process of auditing the Hermes protocol started with a review of the tokens implementation and management contracts. This included a review of the ERC20 Boost, Gauge & Vote tokens, as well as the UtilityManagement contract. The team then proceeded to review the distribution of awards ("emissions") over new epochs contained within the "Flywheel" contracts. The final step involved reviewing the Uniswap Staker Incentive mechanics and the novel additions made by the developers.

Architecture

The architecture of the novel tokens is well-composed, with abstract contracts that are easy to follow. The developers took inspiration from OpenZeppelin's ERC20 Votes contract for the ERC20MultiVotes, and the novel contracts seem to follow the general idea and implementation with a few minor differences. However, the architecture of the rewards distribution part, i.e., the "Flywheel" contracts, was more challenging to grasp. There were some inconveniences, such as having two different contracts (one abstract and one implementation) with the same contract name "FlywheelCore", the naming of contracts was very similar to each other and overall a lack of well-structured documentation in that part of the project. Moreover, user entry points could be better defined. Lastly, the new Uniswap Staker contract follows the main functionality of the original one; however, there are notable differences considering access control.

Overall, the recommendation for the Hermes protocol would be to improve the naming of contracts, design high-level architecture resources such as diagrams, user and contract interaction flows from the perspective of different participants, and expand further on the documentation.

Centralization Risks

There is a substantial centralization risk due to the use of many onlyOwner modifiers. However, for a project of this size, it is hard to completely avoid them. If the developers follow a roadmap that steadily transfers the ownership to the DAO, then the centralization risks would be mitigated, and trust in the ecosystem would improve.

Testability

This is an area where we think the developers did an outstanding job. Although test coverage could be higher and include more complex scenarios, we found implementing PoCs extremely easy and trouble-free. This makes us believe that the testability of Hermes is flexible and would be easy to build on with more tests and end-to-end scenarios.

Omnichain Analysis

Approach

The process of auditing Ulysses-Omnichain started by creating and reviewing the architecture from a high-level perspective, using visualization tools. The audit continued with a thorough review of the contracts, starting with the Branch Chain contracts (Routers, BridgeAgent, BranchPort), followed by a review of the Root Chain contracts (Routers, RootBridgeAgent, RootPort, MulticallRouter), and the Arbitrum specific contracts. The provided test suite was extensively used both to gather more context on the system and to develop PoCs for any identified issues.

Architecture

The Omnichain system aims to implement cross-chain messaging and asset transfer, which is naturally a complex challenge and inherently requires complex code. The architecture is difficult to grasp, however, after thorough review, we conclude that it is of high enough quality to efficiently achieve the underlying task. The developers incorporate Multichain as the off-chain component that will transfer messages across different chains. Multichain is a robust and easy-to-integrate cross-chain messaging framework, and the developers have made a very informed choice here. The implementation of the Ports and BridgeAgents is of high quality. However, the Routers seem to contain functionality that needs to be extended by dApp developers. Moreover, some of the provided Router contracts had functions that weren't implemented (reverting by default), making it difficult to assess whether there could be potential issues arising from that. There are parts of the Root & Branch BridgeAgent’s implementation such as the anyExecute functions that are very long and complex that could benefit from extraction in another contract for better readability. Documentation should also be expanded upon and include high-level architecture resources such as diagrams, user & contract interaction flows from the perspective of different participants. Main area of concern that could contain more severe issues are gas payments.

Centralization Risks

The centralization risks mainly revolve around the deployed factories. To mitigate these risks and improve trust in the ecosystem, it is recommended that the developers follow a roadmap for gradually transferring ownership to the DAO.

Testability

While the test suite has some flaws, such as lengthy and hard-to-follow setup functions, and a lack of complex tests involving nested operations within the anyExecute call, the overall testability of the system is commendable. Once the setup is understood, experimenting, developing tests, and implementing PoCs become easy. The developers have provided a high-quality testing sandbox for implementing, testing, and expanding ideas.

Conclusion

In conclusion, the developers have done an excellent job of implementing an ecosystem that encompasses AMM, Rewards, Voting, Staking, and Cross-chain messaging. Conducting an audit significantly contributes to securing the project, but security in large codebases should be an ongoing process. It is highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the project's security and reliability.

CloudEllie commented 1 year ago

Due to technical difficulties with the submission form, teams were not able to submit Analyses. I have added this Analysis manually; the original can be viewed here.

c4-judge commented 1 year ago

trust1995 marked the issue as grade-a

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

0xBugsy commented 1 year ago

Overall complete and well sustained analysis, some doubled paragraphs but must have slipped due to time constraints

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed