Maia DAO Ecosystem - ByteBandits's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L199 https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L342

Vulnerability details

Impact:

Due to a chain of bugs the endIncentive functionality can be bricked and no one else except the owner of the position can call restake.

Proof-Of-Concept:

This report explains 2 Consequences due to passing wrong argument inside the _unstakeToken() as part of the restake flow. Anyone can call restakeToken if the block time is after the end time of the incentive , but this would be invalid as we pass "true" as the isNotRestake argument inside restake here https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L342 while it should be false. Due to this , this statement https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L374 would fail(revert) for everyone except the owner of the position , therefore owner might lose staking rewards if there exists an automatic system which calls restake on the position NFT.

Second Bug:

The endIncentive functionality is used to end an incentive when the end time is reached and ensures there are no current stakes in the incentive. The refund amount is transferred to the minter as part of the flow. We can think of an argument that if "some" user never unstakes his NFT from the incentive ,this statement https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L199 would always revert as there is always a stake left. That argument can be tackled by the fact "anyone can restake that user into a new incentive since that one has ended that is not an issue" , but as we explained above restaking for other users is impossible and therefore endIncentive will be bricked in this case(user never unstakes his position) and the refund would bever be transferred to the minter.

Recommendation:

Pass the isNotRestake argument as "false" for restakeToken

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosStrategyVanilla.sol#L138-L150

Vulnerability details

Impact

Deposit function is at risk of a MEV attack due to no protection from slippage and setting block.timestamp as the deadline

Proof of Concept

Consider the following function

        if (_liquidity > 0) {
            uint128 liquidityDifference;
            (liquidityDifference, amount0, amount1) = nonfungiblePositionManager.increaseLiquidity(
                INonfungiblePositionManager.IncreaseLiquidityParams({
                    tokenId: _tokenId,
                    amount0Desired: balance0,
                    amount1Desired: balance1,
                    amount0Min: 0,
                    amount1Min: 0,
                    deadline: block.timestamp
                })
            );

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/TalosStrategyVanilla.sol#L138-L150

Due to the lack of slippage protection and deadline being set to block.timestamp, there is a possibility that a user's deposit function can be delayed indefinitely and front ran. This is especially problematic if a user submits a gas fee that is low.

One scenario where this can be dangerous:

Let's say that Bob, calls the deposit function. His transaction can be delayed for a very long time by block proposers until they can get a favorable price to front run Bob.

Another scenario:

Bob calls the function deposit with a low gas fee. His transaction is delayed indefinitely until gas fees are low enough to be front ran

Tools Used

Manual Review

Recommended Mitigation Steps

Add slippage protection and do not use block.timestamp as the deadline. Instead use deadline.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L24-L27

Vulnerability details

Impact:

_votingDelay is compared with MIN_VOTING_DELAY and MAX_VOTING_DELAY but these approximations are done using approximations from L1 , leading to wrong calculations to bound the votingDelay(1 week in block.number in L1 will be way smaller in L2 like optimism/Arbitrum)

Proof-Of-Concept:

The values for MIN_VOTING_DELAY and MAX_VOTING_DELAY are hardcoded here https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L24-L27 , these values are in blocks and and are calculated using the approximations from the mainnet(L1) where the lenth of each block is way more than compared to L2 , therefore the values for MIN/MAX_VOTING_DELAY will be way less than 1week and 2week respectively making the check here for voting delay https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L72 useless.

Recommendation:

Use ARB's helpers instead to calculate the correct values.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L193-L199

Vulnerability details

Impact

Branch Strategies lose yield due to a wrong implementation of the time limit in BranchPort.sol. This results in missed yield for branch strategies, less capital utilization of the platform, and ultimately a loss of additional revenue for the protocol's users.

Proof of Concept

The _checkTimeLimit function in BranchPort.sol controls whether amounts used by a branch strategy do cumulatively not exceed the daily limit which is set for the particular strategy. It is only called from the manage function in the same contract (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L161).

The current implementation of _checkTimeLimit looks like this:

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

I.) The current implementation does the following

1. The first time a strategy manages some amount and _checkTimeLimit is called the 24h window is started (strategyDailyLimitRemaining[msg.sender][_token] is initialized to the daily limit amount and lastManaged[msg.sender][_token] is set to block.timestamp)

2. On a second call to use more of the daily limit (if the amount used in 1. is not the full daily amount, which is not enforced), it will set lastManaged[msg.sender][_token] again to block.timestamp. This pushes the time when the daily budget will be reset (strategyDailyLimitRemaining[msg.sender][_token] = strategyDailyLimitAmount[msg.sender][_token]) again 24 hours into the future.

Consequences of the current implementation

• Due to the setting of the lastManaged[msg.sender][_token] on every call the daily budget misses its purpose as a budget reset after 24h is not guaranteed.

• In the worst but likely case, a call is made by the strategy just before the current 24h time window passes to use the remaining amount. This will delay a reset of the daily limit by the maximum possible time. In consequence, a strategy misses 1 full amount of the daily budget.

• The aforementioned results in a loss of yield for the strategy (assuming the strategy generates a yield), less capital utilization of the platform, and ultimately a loss of additional revenue for the protocol's users.

• Assuming there are multiple strategies in the protocol, the negative effect is multiplied.

II.) The implementation that was probably intended

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

• Here the reset of the daily budget is made after a 24h time window as expected.

• What is lost, is the information "when the last time a strategy called the function" as lastManaged[msg.sender][_token] now only stores the block timestamp the last time the daily budget was reset and not when the last time the function was called. If this should still be tracked consider an additional state variable (e.g. lastDailyBudgetReset[msg.sender][_token]).

Tools Used

Manual review

Recommended Mitigation Steps

Implement the logic as shown under "II.) The implementation that was probably intended".

Please also, consider the following comments:

• To get the maximum amount out of their daily budget a strategy must make a call to the manage() function exactly every 24h hours after the first time calling it. Otherwise, there are time frames where amounts could be retrieved but are not. That would have the strategy missing out on investments and therefore potential yield. E.g. 2nd call happens 36h (instead of 24h) after the initial call => 12 hours (1/2 of a daily budget) remains unused.

• The amount also needs to be fully used within the 24h timeframe since the daily limit is overwriting and not cumulating (using strategyDailyLimitRemaining[msg.sender][_token] = strategyDailyLimitAmount[msg.sender][_token] and not strategyDailyLimitRemaining[msg.sender][_token] += strategyDailyLimitAmount[msg.sender][_token])

• An alternative to the aforementioned could be to calculate the amount to grant to a strategy after an initial/last grant like the following: (time since last grant of fresh daily limit / 24h) * daily limit. This would have the effect that a strategy could use their granted limits without missing amounts due to suboptimal timing. It would also spare the strategy the necessary call every 24h which would save some gas and remove the need for setting up automation for each strategy (e.g. using Chainlink keepers). The strategy could never spend more than the cumulative daily budgets. But it may lead to sudden usage of a large amount of accumulated budget which may not be intended.

Assessed type

Timing

L-01 Incorrect Natspec

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L111-L130

L-02 No need to push an optimizer in the constructor as there is no way to delete an optimizer

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/factories/OptimizerFactory.sol#L24-L28

L-03 Outdated Docs in increaseConversionRate in IERC4626PartnerManager.sol

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/interfaces/IERC4626PartnerManager.sol#L64

L-04 Last 2 variables in DeployArbitrumBranchBridgeAgent.sol deploy function are interchanged

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#LL44C2-L45C36

L-05 Interface functions do not exist on the Optimizer contract

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/interfaces/IOptimizerFactory.sol#L16-L21

L-06 block.number will not be a reliable source of timing on Optimism

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/VirtualAccount.sol#L46

L-07 Inconsistent usage of variable naming for mint and burn functions (amount vs. value)

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IERC20hTokenBranch.sol#L22C4-L28 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IERC20hTokenRoot.sol#L47-L55

L-08 Misleading naming of requiresCoreRouter() modifier in ERC20hTokenRootFactory.sol as modifier checks for more than just the core router

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/factories/ERC20hTokenRootFactory.sol#L75

L-09 Mixed usage of uint24 and uint256 for chain ids. See examples below:

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IRootBridgeAgent.sol#L378 (uint24) https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IERC20hTokenRoot.sol#L47 (uint256)

L-10 Function "getLocalTokenFromUnder" should be named "getLocalTokenFromUnderlying"

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IRootPort.sol#L69

L-11 Unnecessary single-line comment of multiple lines of already existing multi-line comment

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/strategies/TalosStrategySimple.sol#L26-L28

L-12 Broken sentence in the documentation of ArbitrumCoreBranchRouter.sol

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumCoreBranchRouter.sol#L21-L23 (some words may be missing in the "Branch Chain is not available" part)

L-13 Typo in multiple files where it should be "not enough budget" instead of "no enough budget"

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L131-L132 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IRootBridgeAgent.sol#L98 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IRootBridgeAgent.sol#L101 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L1230-L1231 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1320-L1321 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IBranchBridgeAgent.sol#L101 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IBranchBridgeAgent.sol#L104

L-14 Typo in Natspec of IVirtualAccount.sol (should be "performed calls" instead of "perfomed call)

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/interfaces/IVirtualAccount.sol#L15

L-15 @notice in Natspec of "createBridgeAgent" function of ArbitrumBranchBridgeAgentFactory is incorrect (should be: "Creates a new bridge agent for root chain." and maybe "root bridge agent" instead of just "bridge agent" as 2 lines below "root bridge agent" is used)

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/factories/ArbitrumBranchBridgeAgentFactory.sol#L75

L-16 Missing zero address check for "_coreRootBridgeAgent" in "initialize" function in ArbitrumBranchBridgeAgentFactory.sol

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/factories/ArbitrumBranchBridgeAgentFactory.sol#L55 Compare with the "initialize" function in BranchBridgeAgentFactory.sol which does this check: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/factories/BranchBridgeAgentFactory.sol#L84

L-17 Unused "requiresPort" modifier in ERC20hTokenBranchFactory.sol

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/factories/ERC20hTokenBranchFactory.sol#L81

L-18 Typo of writing "length" as "lenght" in multiple contracts

https://github.com/search?q=repo%3Acode-423n4%2F2023-05-maia%20lenght&type=code

L-19 Asymmetry in implementation of "bridgeOut" and "bridgeOutMultiple" in BranchPort.sol

Both functions do the same just "bridgeOutMultiple" does it for multiple local addresses. The functions process the 2 if statements in their function body in a different order. This e.g. makes it harder to compare the functions for correctness. Compare https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L248-L256 vs. https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L268-L278

L-20 Risk of accidentally toggling on toggled off items in BranchPort.sol

There are sets of 2 functions in that contract where one function adds an item and another function toggles the item state. As add function does not check whether the item was already added, it could accidentally be added and toggled on again. A side effect of this is that the arrays that track the history of added items will also hold duplicate items. The following combinations of functions were identified: addStrategyToken + toggleStrategyToken, addPortStrategy + togglePortStrategy, addBridgeAgentFactory + toggleBridgeAgentFactory, addBridgeAgent + toggleBridgeAgent. Example of toggling on again by adding: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L291

L-21 NapSpec of increaseConversionRate() function in IERC4626PartnerManager.sol is wrong

In https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/interfaces/IERC4626PartnerManager.sol#L64 it talks about "address(partnerVault)" but in the implementation in uses "address(this)".

L-22 Same NatSpec and implementation for "maxDeposit" and "maxMint" functions in ERC4626PartnerManager

https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L159-L160 https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/tokens/ERC4626PartnerManager.sol#L159-L160

L-23 Inconsistent ways used to increase settlementNonce in DeployRootBridgeAgent.sol and depositNonce in BranchBridgeAgent.sol

In both files nonces are either increased by in place calling ++ or via a dedicated "_getAndIncrement*" function. https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L278 vs. https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L633, https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol vs. https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol.

L-24 UlyssesPoolDeployer.sol should use post-increment instead of pre-increment for poolId and tokenId

These state variabless are commented as "next poolId" and "next tokenId". Due to the pre-increment, they are not used as such. It is also the only occurrence in the protocol where increments are used like this. https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/factories/UlyssesFactory.sol#L85, https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/factories/UlyssesFactory.sol#L141

L-25 Multiple contracts either use Ownable but don't need to use it or import it without using it

1. Contracts that don't need to use it (no onlyOwner modifier present): BribesFactory.sol, FlywheelGaugeRewards.sol, TalosBaseStrategyFactory.sol, ERC20hTokenBranchFactory.sol; 2) Contracts that import Ownable but don't use it: UniswapV3GaugeFactory.sol, UtilityManager.sol, bHermesBoost.sol, bHermesGauges.sol, bHermesVotes.sol, vMaia.sol, PartnerManagerFactory.sol, FlywheelCoreInstant.sol, FlywheelCore.sol, FlywheelInstantRewards.sol, TalosStrategyVanilla.sol, ArbitrumBranchPort.sol, CoreBranchRouter.sol, RootBridgeAgent.sol, RootBridgeAgent.sol, RootBridgeAgentFactory.sol, ERC20hTokenRoot.sol

L-26 Missing comments in GovernorBravoDelegate* contracts on a subtlety which is actually the only protection against flash loans

A proposal pending state is judged by "block.number <= proposal.startBlock" which on the first look may be incorrect since the "active" state is also inclusive of the endBlock. See: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/governance/GovernorBravoDelegateMaia.sol#L304. But the startBlock is excluded intentionally so the proposal starts 1 block later than the block when the user balance is passed. See: https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L367. This should be documented.

L-27 Unused MIN_RESERVE_RATION constant in BranchPort.sol

In https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L92 MIN_RESERVE_RATION is defined but it is never used in that contract. It may have been missed to be used for a second check in the "addStrategyToken" function: https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L331.

L-28 Use Existing Modifiers Instead

Here the checks https://github.com/code-423n4/2023-05-maia/blob/main/src/hermes/bHermes.sol#L144 are already done inside the modifiers at L69 , L77, L85 . Use that insetad for better code consistency.

L-29 Have a check for forfeightMultiple that none of the calls would revrt

We should have a check in this function https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/PartnerUtilityManager.sol#L52 that none of the forfeit calls would revert (ensure userClaimedX >= amount) , this can save gas too as if the first two forfeit calls succeeded but the last one reverted that would waste gas.

L-30 better to have a check amount > userClaimedWeight

better to have a check amount > userClaimedWeight at https://github.com/code-423n4/2023-05-maia/blob/main/src/maia/PartnerUtilityManager.sol#L71 with an error string so the user knows the call reverted due to insufficient claimed weight. Same for L78 and L87

L-31 reward Variable can't be negative

The check here https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L138 is useless since reward is a uint256 which can't be negative , a check for zero value should be enough.

L-32 Unnecessary check at MultiRewardsDepot contract

The check at https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/MultiRewardsDepot.sol#L48 can be done without the OR operator too i.e if (_isAsset[asset] ) should be enough.

L-33 Inconsitencies between function implementations

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L104 This function checks the 0 liquidity case with == while this function https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L129 checks the 0 liquidity at L139 with >

L-34 Misleading comment on PartnerFactory creating vMaia contracts

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/maia/vMaia.sol#L39-L50 the factory does not actually create vMaia contracts, it adds them to the registry

L-35 Large number of interface functions do not exist in the target contract

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/interfaces/ITalosBaseStrategy.sol#L37-L98 Most of the highlighted functions in the interface are variables that do not exist in the main contract

L-36 No point in using castvotebySig over castvote

https://github.com/code-423n4/2023-05-maia/blob/main/src/governance/GovernorBravoDelegateMaia.sol#L344

L-37 CEI pattern to prevent re-entrancy violated

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L409-L414

L-38 Claim reward is buggy

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L262 All rewards are transferred if 0 is passed

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

Maia

Talos

Ulysses

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

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