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

24 stars 13 forks source link

Analysis #868

Open CloudEllie opened 1 year ago

CloudEllie commented 1 year ago

Description

Hermes

Hermes is a protocol built around its main token bHermes, which is earned from staking Hermes token. bHermes is then split into 3 utility tokens, each with a separate use:

BHermes Gauge: Vote on gauges and receive a proportion of the gauges’ revenue

BHermes Boost: Earn boosted rewards by providing liquidity to a gauge

BHermes Votes: Vote on governance proposals such as adding/removing gauges, bribes, partners, etc.

Hermes also introduces a gauge system, where the users can stake their UniV3 position NFTs and earn rewards. Moreover, these rewards can be boosted (using the BoostAggregator contract) up to a maximum of 2.5x. The new version also introduces ERC4626 deposit-only vaults where the burn rate is increased allowing users to vote once.

Maia

Maia DAO is the cornerstone of the Maia protocol. It is an aggregator for Talos and Hermes. Users can earn rewards by staking their $Maia(native) into vMaia(ERC4626 compliant token), leveraging their vMaia to participate in Maia governance, and earning bribes like in Talos (rewards from vault strategy revenue).

The utilities earned by the user are weight and governance but not boost, this is because Maia’s treasury hosts a boost aggregator with Talos Positions to enable further accumulation of hermes.

Talos

Talos builds upon the gauge system by introducing strategies. These strategies rebalance and rerange a Uniswap V3 position portfolio based on settings set in an optimizer contract. These strategies come in 2 forms. Vanilla and staked. The Vanilla Strategy is a strategy where Uniswap V3 positions are left in pools to collect liquidity fees. The Staked Strategy,u in contrast, takes Uniswap V3 positions and stakes them into Uniswap V3 gauges where users can earn Hermes emissions instead of using the Uniswap V3 staker implementation.

UniV3 ensures that positions are staked in tick ranges with the highest liquidity to ensure the highest rewards, the need for rebalance/rerange occurs when the position deviates from the tick spacing (deviation) defined.

An important component of the Talos system is flywheel contracts. These contracts manage the token incentive distribution from Talos strategies. This helps to protect against exploits and ensure the safe and fair distribution of strategy rewards.

Ulysses

Ulysses is an ominchain protocol inspired by layer0 that provides capital efficiency across multiple chains. It attempts to solve the bridging trilemma problem involved with the growing demand to move tokens across multiple chains. Ulysses allows users to provide liquidity and earn fees across chains as well as allows users to seamlessly move assets across chains. It accomplishes this with a branch port system where each chain has a branch router and a branch port that communicates with the root router and the root port on the root chain (Arbitrum). The interaction between branch chains and the root chain is facilitated by a BridgeAgent that exists on each chain. The underlying cross-chain communication protocol is not Maia's own implementation. Instead, Ulysses uses Multichain anyCall v7 under the hood.

The root chain keeps a virtual account of users that manages their balances across chains. This allows users to leverage their virtual account to participate in many activities on the root chain.

When inspecting Ulysses some noteworthy patterns were discovered:

Pattern to prevent frontrunning contract initialization

In other contests, it was frequently discussed whether frontrunning an initialize() call on a freshly deployed contract was noteworthy. And if so the severity of this was usually in question. Because usually, the project could just redeploy that contract. Reviewing the Ulysses contracts a pattern was noted that spares this discussion:

  1. The constructor is called and sets the owner
  2. The initialize() function can only be called by the owner due to its “onlyOwner” modifier
  3. Within the initialize() function the ownership is renounced and the contract becomes permissionless

Mappings are named like functions

Mappings in Ulysses are named like functions and therefore accessing a value for a key feels like a “get” function call. This is the first code base inspected where this naming convention was found. Here is an example: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L122

Approach

We divided the audit into 4 parts. Hermes, Maia, Talos and Ulysses. We started with Hermes, then Maia, then Talos, and then Ulysses. This is because the first 3 are interconnected particularly due to Maia being an aggregator for both Talos and Hermes.

Architecture

Hermes

Hermes

Maia

Maia

Talos

Talos

Ulysses

Ulysses-AMM

Ulysses-AMM

Codebase Quality

Overall, we believe that the codebase quality for Maia DAO is very good. Codebase is very mature and has clearly undergone extensive testing. We notice the employment of various standards, including ERC20, ERC721, and ERC2646. We also noticed that some sections of the codebase take inspiration from protocols such as popsicle finance and layer 0. Details are explained below

Codebase Quality Categories Comments
Unit Testing Codebase is extensively well-tested using the Foundry framework. The inclusion of fuzz tests was great to see. It is worth noting that some of the test files are too large and it would be beneficial to separate them for better readability.
Code Comments Comments in general were solid but there are many sections in the codebase where the comments were either outdated or incorrect.
Documentation The documentation overall is fantastic and does a great job of explaining the ecosystem. It would be helpful, however, if the docs explained how the ecosystem works from a basic contract level so that it is easier to digest for developers looking to integrate into the Maia ecosystem
Organization Codebase is very mature and well organized with clear distinctions between the 4 protocols, their respective factories, interfaces, etc
Static Analysis Codebase makes great use of Slither which is an industry standard tool for static analysis

Systemic/Centralization Risks

Global

  1. It is important to note that since factories in the ecosystem are permissionless by design, a user can create contracts with poisonous characteristics or just create an infinite amount of contracts, creating potential vulnerabilities such as DOS attacks
  2. This can also result in risks related to social engineering such as phishing attacks.

Hermes

  1. It is possible that the V3 staker is vulnerable to a Sybil-type attack, where a user can create multiple Uniswap LP NFTs to stake and earn Hermes emission at the expense of everyone else.

Maia

  1. Whales with huge pockets can bribe voters and essentially control the ecosystem.
  2. The cost of accepting a proposal is at a fixed cost of ETH. If the price of ETH grows, this will be a problem for future proposals.

Talos

The ecosystem of Talos carries some noteworthy systemic risks:

  1. There is a possibility that a Uniswap pool can be removed, disabling several functions.
  2. Having unprotected functions in the Talos Manager contract could open the door to future vulnerabilities
  3. On the surface, it doesn’t make sense for a user to not use the Talos Strategy Vanilla over Staked as a user should be able to collect fees while also receiving Hermes emissions at the same time.

Ulysses

  1. There is a possibility of unexpected results if an L2 chain goes down
  2. Each chain has some differences from one another, and it may be possible that some chains do not synchronize well with others due to differences in block timing. This may be a problem when managing funds across chains.
  3. Maia should be aware of possible cross chain MEV attacks between Arbitrum and another chain. If there is a large discrepancy in the price of an asset between chains, then assets within a chain port could be drained via swaps.

Recommendations

  1. While the documentation for the codebase is excellent, the contract flow is not particularly clear as a user can enter the ecosystem in various ways. This may be in part due to the permissionless nature of the ecosystem. It is recommended that the docs are updated to describe important contracts and their functions.
  2. There are many areas where the documentation is either incorrect or simply does not match the code. It is recommended to keep Natspecs up to date to avoid confusion.
  3. Throughout the codebase, there is repeated use of functions with the same or even similar names. It is recommended to avoid this practice as it makes the code very confusing and may have unintended consequences by calling the wrong function.
  4. Many contracts have unused imports, unused variables, or even inherit from a contract whose functionality is in the end not used (Ownable). A thorough scan of the codebase should be done to clean this up.
  5. Too much inheritance and nested functions can make certain functions difficult to read and audit. Consider reducing this if possible as this can make the code more readable and reveal potential bugs. e.g. Is there really a need to have TalosBaseSimple.sol when the functions could have been written in TalosBaseStrategy or TalosStrategyStaked?
  6. Since the codebase relies on 3rd party integrations like Uniswap, it is essential to ensure that these 3rd parties are secure, in short, the risks of integrating from a third party should be noted.
  7. We have noticed that there are a lot of interface functions that do not exist in the actual contract. It is recommended to have these unneeded functions remo
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

Very well structured report!

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed