Maia DAO Ecosystem - kodyvim's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340

Vulnerability details

Impact

denormalizeDecimals is meant to denormalizes an input from 18 decimal places to it's native decimals but it's implementation wrongly does the opposite. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388

/**
     * @notice Internal function that denormalizes an input from 18 decimal places.
     * @param _amount amount of tokens
     * @param _decimals number of decimal places
     */
    function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
    }

while normalizeDecimals is meant to normalizes an input to 18 decimal places but it's implementation does the opposite. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340

/**
     * @notice Internal function that normalizes an input to 18 decimal places.
     * @param _amount amount of tokens
     * @param _decimals number of decimal places
     */
    function _normalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * (10 ** _decimals) / 1 ether;
    }

This creates discrepancies within contracts using these functions which could lead to lost of funds to users or the protocol. for instance _denormalizeDecimals is used in BranchPort.sol to bridge out funds.

function bridgeOut(
        address _depositor,
        address _localAddress,
        address _underlyingAddress,
        uint256 _amount,
        uint256 _deposit
    ) external virtual requiresBridgeAgent {
        if (_amount - _deposit > 0) {
            _localAddress.safeTransferFrom(_depositor, address(this), _amount - _deposit);
            ERC20hTokenBranch(_localAddress).burn(_amount - _deposit);
        }
        if (_deposit > 0) {
            _underlyingAddress.safeTransferFrom(
                _depositor, address(this), _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())//@audit
            );
        }
    }

but instead of _denormalizing the decimal to the underlying assets decimal it rather normalizes it to 18 decimals which implies that if the underlying asset decimal is less than 18 it would be overstated and more tokens would be bridge out to the user than intended.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L388 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340

Tools Used

Manual Review

Recommended Mitigation Steps

Correct the implementation of _denormalizeDecimals and normalizeDecimals by swapping both implementation.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L442 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootPort.sol#L282 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L460

Vulnerability details

Impact

From the contest description:

Ulysses AMM (Ulysses Pools and Ulysses Tokens) only supports tokens with 18 decimals, but Ulysses Omnichain accounting supports tokens with any decimals and converts them to 18 decimals.

But some instances in Ulysses Omnichain fails to convert/normalize such tokens. Within Ulysses Omnichain calculations are done with tokens which might have different precision/decimals, this could lead to issues like transferring fewer or more tokens to users or reverts in some cases. for instances: https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L442

function _updateStateOnBridgeOut(
        address _sender,
        address _globalAddress,
        address _localAddress,
        address _underlyingAddress,
        uint256 _amount,
        uint256 _deposit,
        uint24 _toChain
    ) internal {
        if (_amount - _deposit > 0) {//@audit _deposit should be normalized first
            //Move output hTokens from Root to Branch
            if (_localAddress == address(0)) revert UnrecognizedLocalAddress();
            _globalAddress.safeTransferFrom(_sender, localPortAddress, _amount - _deposit);//@audit _deposit should be normalized 
        }

        if (_deposit > 0) {
            //Verify there is enough balance to clear native tokens if needed
            if (_underlyingAddress == address(0)) revert UnrecognizedUnderlyingAddress();
            if (IERC20hTokenRoot(_globalAddress).getTokenBalance(_toChain) < _deposit) {//@audit should be normalized first
                revert InsufficientBalanceForSettlement();
            }
            IPort(localPortAddress).burn(_sender, _globalAddress, _deposit, _toChain);
        }
    }

This function takes two value input amount and _deposit from the Natspec comment amount is from the htoken which is known to be 18 decimals but _deposit can be any underlying token with different precision, these difference could cause multiple issues within these contracts.

Proof of Concept

other instances of same issue. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L249 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L275 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L277 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L276 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L123 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L124 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L144 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchPort.sol#L146 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L623 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L979 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L451 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L454 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L460 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootPort.sol#L282 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootPort.sol#L283

Tools Used

Manual Review

Recommended Mitigation Steps

Precision of both token should be consistent before performing operations on them. normalize the _deposit parameter before usage.

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1263 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L823

Vulnerability details

Impact

DAO attempts to sweep accumulatedFees paid in transactions but fails to unwrap before sweeping. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1259

function sweep() external {
        if (msg.sender != daoAddress) revert NotDao();
        uint256 _accumulatedFees = accumulatedFees - 1;
        accumulatedFees = 1;
        SafeTransferLib.safeTransferETH(daoAddress, _accumulatedFees);
    }

gas fees are wrapped to gasToken when paid by users then any excess deposited gas fees are recorded in the accumulatedFees but when trying to sweep this excess, it fails to unwrap this amount before attempting to sweep. This implies that when trying to sweep the amount _accumulatedFees, the contracts does not have that amount rather if the ether held in the contracts is upto the value _accumulatedFees then sweep would be successful else it reverts. all _accumulatedFees would be stuck in the wrappedNativeToken contract.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1263 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L823

Tools Used

Manual Review

Recommended Mitigation Steps

Unwrap _accumulatedFees before transferring eth to DAO

Assessed type

ETH-Transfer

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/gauges/BaseV2Gauge.sol#L144 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/MultiRewardsDepot.sol#L57

Vulnerability details

Impact

Flywheel could be gamed to still accrue rewards on the subsequent epochs even when removed or inactive.

Proof of Concept

In BaseV2Gauge.sol#addBribeFlywheel when adding bribeflywheel the function calls multiRewardsDepot.addAsset(flyWheelRewards, bribeFlywheel.rewardToken()) https://github.com/code-423n4/2023-05-maia/blob/main/src/gauges/BaseV2Gauge.sol#L128

function addBribeFlywheel(FlywheelCore bribeFlywheel) external onlyOwner {
        /// @dev Can't add existing flywheel (active or not)
        if (added[bribeFlywheel]) revert FlywheelAlreadyAdded();

        address flyWheelRewards = address(bribeFlywheel.flywheelRewards());
        FlywheelBribeRewards(flyWheelRewards).setRewardsDepot(multiRewardsDepot);

        multiRewardsDepot.addAsset(flyWheelRewards, bribeFlywheel.rewardToken());//@audit <= added here
        bribeFlywheels.push(bribeFlywheel);
        isActive[bribeFlywheel] = true;
        added[bribeFlywheel] = true;

        emit AddedBribeFlywheel(bribeFlywheel);
    }

This adds the bribeFlywheel as a rewardContract in MultiRewardsDepot to enable the flywheel accrue rewards from the depot https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/MultiRewardsDepot.sol#L47

function addAsset(address rewardsContract, address asset) external onlyOwner {
        if (_isAsset[asset] || _isRewardsContract[rewardsContract]) revert ErrorAddingAsset();
        _isAsset[asset] = true;
        _isRewardsContract[rewardsContract] = true;
        _assets[rewardsContract] = asset;

        emit AssetAdded(rewardsContract, asset);
    }

but when removing bribeflywheel the removeBribeFlywheel function does not call MultiRewardsDepot.sol#removeAsset this allows users to call accrue from the flywheel with the strategy and accrue rewards even when the flywheel is removed in subsequent epochs. https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/MultiRewardsDepot.sol#L57

function removeAsset(address rewardsContract) external onlyOwner {
        if (!_isRewardsContract[rewardsContract]) revert ErrorRemovingAsset();

        emit AssetRemoved(rewardsContract, _assets[rewardsContract]);

        delete _isAsset[_assets[rewardsContract]];
        delete _isRewardsContract[rewardsContract];
        delete _assets[rewardsContract];
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/gauges/BaseV2Gauge.sol#L144

function removeBribeFlywheel(FlywheelCore bribeFlywheel) external onlyOwner {//@audit <== not removed here
        /// @dev Can only remove active flywheels
        if (!isActive[bribeFlywheel]) revert FlywheelNotActive();

        /// @dev This is permanent; can't be re-added
        delete isActive[bribeFlywheel];

        emit RemoveBribeFlywheel(bribeFlywheel);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add multiRewardsDepot.removeAsset(flyWheelRewards) to BaseV2Gauge.sol#removeBribeFlywheel and set BaseV2Gauge.sol#removeBribeFlywheel behind a timelock before the next epoch to allow users who haven't claimed to claim before removal.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1219 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L850

Vulnerability details

Impact

RootBridgeAgent.sol#depositGasAnycallConfig can be use to topup gas to contract. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1219

function depositGasAnycallConfig() external payable {
        //Deposit Gas
        _replenishGas(msg.value);//@audit
    }

but due to how _replenishGas is implemented in rootBridgeAgent.sol. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L850

function _replenishGas(uint256 _executionGasSpent) internal {
        //Unwrap Gas
        wrappedNativeToken.withdraw(_executionGasSpent);//@audit
        IAnycallConfig(IAnycallProxy(localAnyCallAddress).config()).deposit{value: _executionGasSpent}(address(this));
    }

it wrongly withdraw from WrappedNativeToken before deposit, but this value is sent directly following the call to depositGasAnycallConfig and shouldn't be unwrapped first. The unwrapped eth would be unaccounted for and remains within the contract.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1219 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L850

Tools Used

Manuel Review

Recommended Mitigation Steps

depositGasAnycallConfig should not withdraw from WrappedNativeToken, you can implement _replenishGas as in BranchBridgeAgent.sol and unwrap tokens seperately before calling _replenishGas.

Assessed type

Context

QA report

_payFallbackGas doesn't refund excess or account for excess.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1061 https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L831 Due to gas price fluctuation there might be excess depositedGas remaining within _payFallbackGas this excess could be accumulated or returned to the user. since the checks in _requiresFallbackGas allows for excess.

function _requiresFallbackGas(uint256 _depositedGas) internal view virtual {
        if (_depositedGas <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGas();
    }

and also the check in _payFallbackGas specifies that the depositedGas should be greater than minExecCost, there could be excess within these transaction. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1069

//Check if sufficient balance
        if (minExecCost > getDeposit[_depositNonce].depositedGas) {
            _forceRevert();
            return;
        }

        //Update user deposit reverts if not enough gas => user must boost deposit with gas
        getDeposit[_depositNonce].depositedGas -= minExecCost.toUint128();

Recommendation:

Excess could be refunded or accounted for.

Missing checkdeviation on init could expose initializer to unlimited slippage.

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L102 checkDeviation modifier checks if price has not moved a lot recently. most functions on talosBaseStrategy.sol has this modifier but this is missing on the init function. this could expose initializer to unlimited slippage.

Recommendation:

Add checkDeviation modifier to init

Redundant functionality could be removed

This functions has access control but they can never be called since no handle is provided within required contracts. https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchPort.sol#L301 setCoreRouter is never called from coreRouter https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L137 setBooster is never called from owner contract

Recommendation

if the functionality are not required the should be removed

operation can be refactored.

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L1197C9-L1207C10

if (flag == 0x00) {
            _settlementNonce = uint32(bytes4(data[PARAMS_START_SIGNED:25]));
            _reopenSettlemment(_settlementNonce);

            /// SETTLEMENT FLAG: 1 (single asset settlement)
        } else if (flag == 0x01) {
            _settlementNonce = uint32(bytes4(data[PARAMS_START_SIGNED:25]));
            _reopenSettlemment(_settlementNonce);

            /// SETTLEMENT FLAG: 2 (multiple asset settlement)
        }

can be refactored to

if (flag  == 0x00 || flag == 0x01) {
	    _settlementNonce = uint32(bytes4(data[PARAMS_START_SIGNED:25]));
            _reopenSettlemment(_settlementNonce);
}

Maia DAO Ecosystem Analysis

Comment & Approach

Initial Assessment:

• Started by skimming through the overall codebase, looking for commonly found keywords or similar protocol implementations.
• Noted a similar bug in the governance contracts, based on previous audits of nounsdao.
• Ran and checked the test files.

Deep Dive into Rewards Portion:

• Decided to focus on the Rewards portion of the protocol.
• Noticed discrepancies between adding and removing reward contracts.
• Reached out to the sponsor to understand the decision-making process.
• Learned that the implementation faced a dilemma between bricking unclaimed rewards and users potentially gaming the "bribeflywheel" in the next epoch.

Investigation of Ulysses Omnichain:

While comparing similar functionalities within the protocol, discovered that the normalize and denormalize functions had their logic interchanged. Identified inconsistencies between tokens with potential different precision within Ulysses Omnichain. Noticed instances where arithmetic operations and comparisons were performed between tokens with different precision. Explored the workflow, specifically examining if users receive refunds when they overpay for gas while transacting across the omnichain. Found that gas fees were refunded in BranchBridgeAgent.sol. Observed that in RootBridgeAgent.sol, excess gas could be swept by the DAO, but the sweep function did not unwrap the ETH before attempting to sweep, resulting in accumulated fees being stuck in the wrapper contract. Noted inconsistencies in the implementations of RootBridgeAgent.sol and BranchBridgeAgent.sol, causing issues with wrapping and unwrapping ETH.

• QA Report Compilation.

Codebase quality analysis

Centralization risks

No centralization risks were found as critical functionalities are accessed through on-chain governance. Additionally, there is an Emergency Multisig admin with veto powers. This admin has the authority to cancel malicious proposals but does not have the ability to override the governance voting process.

Mechanism review

• DAO Governance: The Maia DAO ecosystem is governed by its community of token holders who participate in the decision-making process. Token holders can propose and vote on changes, upgrades, or other initiatives within the ecosystem. Governance decisions are typically made using voting mechanisms, where token holders can cast their votes proportional to the number of tokens they hold.

• Investment Strategies: Maia DAO offers a range of investment strategies to its participants. These strategies are designed to provide exposure to different assets and investment opportunities, such as cryptocurrencies, tokens, or other digital assets. The specific investment strategies offered by Maia DAO can vary over time based on market conditions and community proposals.

• Tokenomics: The Maia DAO ecosystem has its native token, which typically plays a central role in the governance and economic aspects of the ecosystem. The token may have various utilities, such as voting rights, staking, rewards, or fee sharing. The tokenomics of Maia DAO can incentivize active participation and align the interests of the ecosystem's participants.

• Staking and Rewards: Maia DAO may offer staking mechanisms, allowing token holders to lock their tokens for a certain period to support network security or participate in governance. In return for staking, token holders may receive rewards, such as additional tokens or a share of the fees generated within the ecosystem.

• Cross-Chain Interoperability : The Maia DAO ecosystem using anycall v7 as messaging layer means that an asset deposited from a specific chain, is recognized as a different asset from the "same" asset but from a different chain.

• Smart Contracts and Automation: Smart contracts play a crucial role in the Maia DAO ecosystem by automating various processes, such as voting, fund management, and reward distribution. These smart contracts are typically executed on the blockchain, providing transparency, security, and immutability.

Systemic risks

• Smart contract vulnerabilities: Smart contracts form the backbone of the Maia DAO ecosystem. If there are vulnerabilities in the code, it could be exploited by malicious actors, resulting in financial losses or disruption of the ecosystem.

• Blockchain Platform: Arbitrum may face issues with data availability and security. Arbitrum’s challenge period can be up to one week, which can lead to slower finality and higher latency. but this can be a trade-off between gas fees and data availability.

• Governance flaws: DAOs rely on decentralized governance mechanisms for decision-making. However, if the governance model is susceptible to manipulation, it could lead to biased decision-making, centralization of power, or governance gridlock.

• Regulatory challenges: The regulatory landscape surrounding cryptocurrencies and DAOs is still evolving. Maia DAO could face regulatory challenges or uncertain legal frameworks in certain jurisdictions, which may impact its operations and growth.

• Economic instability: The Maia DAO ecosystem may be influenced by broader economic factors, such as market volatility, inflation, or financial crises. These external factors can impact the value and stability of the assets held within the ecosystem.

• Market manipulation: As with any financial ecosystem, there is a risk of market manipulation within the Maia DAO ecosystem. Manipulative practices, such as pump-and-dump schemes or insider trading, can distort prices and harm the overall integrity of the ecosystem.

• Sybil attacks: Sybil attacks involve individuals or entities creating multiple identities to gain undue influence or control over a decentralized system. If the Maia DAO ecosystem is susceptible to such attacks, it could lead to unfair decision-making or manipulation of the voting process.

Time spent:

185 hours