Maia DAO Ecosystem - ihtishamsudo's results

Lines of code

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

Vulnerability details

Impact

Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.

Proof of Concept

Exploit Scenario

interface WETH9 {
    function withdraw(uint256 wad) external;

    function deposit() external payable;

    function balanceOf(address guy) external view returns (uint256 wad);

    function transfer(address dst, uint256 wad) external;
}

WETH9.transfer does not return a boolean. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC20 interface implementation. Alice's contract is unable to interact with Bob's contract.

Tools Used

VS Code/Slither

Recommended Mitigation Steps

Set the appropriate return values and types for the defined ERC20 functions.

Assessed type

ERC20

[L-01] Division Before Multiplication (Instances Missed By The Bot)

Solidity's integer division truncates. Thus, performing division before multiplication can lead to precision loss.

Vulnerable Code

uint256 _newEpoch = (block.timestamp / WEEK) * WEEK;

If WEEK Value Is Somehow Greater Than block.timestamp, Then The Value of uint256 _newEpoch Will Be Zero.

Vulnerable Code Link

BaseV2Guage.sol#L84 BaseV2Guage.sol#L68 FlywheelAcummulatedRewards.sol FlywheelGaugeRewards.sol#60 FlywheelGaugeRewards.sol#79 FlywheelGaugeRewards.sol#114

Tool Used

Manual / Slither

Recommended Mitigation Step

Consider ordering multiplication before division.

Reference

Division Before Multiplication

[L-02] ERC20Boost.decrementGaugeBoost ignores return value by _userGauges[msg.sender].remove(gauge)

The return value of an external call is not stored in a local or state variable. It fails to handle the return value when attempting to remove the gauge associated with the user, which could potentially allow an attacker to bypass gauge removal and manipulate the contract state. Vulnerable Code

_userGauges[msg.sender].remove(gauge);

Vulnerable Code Link

ERCBoost.sol#L178 BaseV2Guage.sol#L115 FlyWheelGuageRewards.sol#L76

Tools Used

Slither

Recommended Mitigation Step

Ensure that all the return values of the function calls are used.

Reference

Unused Return

[L-03] IUniswapV3Staker.tokenIdRewards Shadows IUniswapV3Staker.rewards

Vulnerable Code Link

IUniswapV3Staker.sol#L156 IUniswapV3Staker.sol#L162

Tools Used

Slither

Recommended Mitigation Step

Rename the local variables that shadow another component.

Reference

Local Variable Shadowing

[L-04] UtilityManager.checkWeight does not always execute _; or revertModifier

If a modifier does not execute _ or revert, the execution of the function will return the default value, which can be misleading for the caller. Vulnerable Code

    modifier checkWeight(uint256 amount) virtual;

Vulnerable Code Link

UtitlityManager.sol#L141 UtitlityManager.sol#L145 UtitlityManager.sol#L147 RewardsDepot.sol

Tools Used

Slither

Recommended Mitigation Step

All the paths in a modifier must execute _ or revert.

Reference

Incorrect Modifier

[L-05] Dangerous Usage Of block.timestamp

block.timestamp can be manipulated by miners. Suppose Bob's contract relies on block.timestamp for its randomness. Eve is a miner and manipulates block.timestamp to exploit Bob's contract. Vulnerable Code

            if (cycle - block.timestamp <= incrementFreezeWindow) revert IncrementFreezeError();

Vulnerable Code Link

ERC20Guages.sol#L205 FlywheelAcummulatedRewards.sol#L46

Tools Used

Solidity Visual Developer / Slither

Recommended Mitigation Steps

Avoid relying on block.timestamp

Reference

block.timestamp

[L-06] BaseV2GaugeManager.changeAdmin lacks a zero-check

The lack of a zero address check in the changeAdmin function allows an attacker to set the admin address to the zero address, resulting in unauthorized access and control over the contract. Vulnerable Code

function changeAdmin(address newAdmin) external onlyAdmin {
        admin = newAdmin;

        emit ChangedAdmin(newAdmin);
    }

Vulnerable Code Link

BaseV2GaugeManager.sol#L146

Tools Used

Remix/Slither

Reference

Missing Zero Address Check

[L-07] The initialize Function Is Missing An Event Emission After Assigning The Value To bridgeAgentExecutorAddress.

Missing Emmiting an event makes it difficult to track changes in the contract's state or important updates related to the bridgeAgentExecutorAddress off-chain Vulnerable Code

function initialize(address _localBridgeAgentAddress) external onlyOwner {
        require(_localBridgeAgentAddress != address(0), "Bridge Agent address cannot be 0");
        localBridgeAgentAddress = _localBridgeAgentAddress;
        bridgeAgentExecutorAddress = IBridgeAgent(localBridgeAgentAddress).bridgeAgentExecutorAddress();
        renounceOwnership();
    }

Vulnerable Code Link

BaseBranchRouter.sol#L37

Tools Used

VS Code/ Slither

Recommended Mitigation Steps

Emit an event for critical parameter changes.

Reference

Missing Event Access

[L-08] RootBridgeAgent._gasSwapIn.amount0 Is A Local Variable Never Initialized

Vulnerable Code

returns (int256 amount0, int256 amount1

Vulnerable Code Link

RootBridgeAgent.sol#L690 RootBridgeAgent.sol#L980 RootBridgeAgent.sol#L1036 RootBridgeAgent.sol#L871 RootBridgeAgent.sol#L953

Tools Used

Solidity Visual Developer/Slither

Recommended Mitigation Steps

Initialize all the variables. If a variable is meant to be initialized to zero, explicitly set it to zero to improve code readability.

Reference

Uninitialized Local Variable

[L-09] outputParams_scope_2 Is A Storage Variable Never Initialized

An uninitialized storage variable will act as a reference to the first state variable, and can override a critical variable. Vulnerable Code

(Call[] memory calls, OutputMultipleParams memory outputParams, uint24 toChain)

Vulnerable Code Link

MulticallRootRouter.sol#L457 MulticallRootRouter.sol#L305 MulticallRootRouter.sol#L381

Tools Used

Slither/VS Code

Recommended Mitigations Steps

Initialize all storage variables.

Reference

Uninitialized Storage variables

[N-01] ERC20Boost._burn is never used and should be removed

Vulnerable Code

function _burn(address from, uint256 amount) internal override notAttached(from, amount) {
        super._burn(from, amount);
    }

Vulnerable Code Link ERC20Boost.sol#L302 ERC20Gauges.sol#L379 ERC20Gauges.sol#L485 ERC20Gauges.sol#L383 ERC20MultiVotes.sol#L258 ERC20MultiVotes.sol#L260

Tools Used

Slither

Recommended Mitigation Steps

Remove unused functions.

Reference

Dead Code

[N-02] ERC4626.sol Is Missing Inheritance From IERC4626DepositOnly

Recommended Mitigation Steps

ERC4626 should inherit from IERC4626DepositOnly

[N-03] Avoid Using Low Level Calls

Function call in VirtualAccount.sol Is Using Low Level Call. The use of low-level calls is error-prone. Low-level calls do not check for code existence or call success. Vulnerable Code

 function call(Call[] calldata calls)
        external
        requiresApprovedCaller
        returns (uint256 blockNumber, bytes[] memory returnData)
    {
        ..........
        for (uint256 i = 0; i < calls.length; i++) {
            .........
            (bool success, bytes memory data) = calls[i].target.call(calls[i].callData);
            .........
            }
    }

Vulnerable Code Link

VirtualAccount.sol#L49

Tools Used

Slither

Recommended Mitigation Steps

Avoid low-level calls. Check the call success. If the call is meant for a contract, check for code existence.

Reference

Low Level Calls

Based on my experience doing this audit was pretty good because that's my first audit I've found a bunch of High + med vulns and it indicates that the code needs more improvement. Answering The Questions:

Analysis of the codebase (What's unique? What's using existing patterns?)

Overall, there is a good effort from the dev team of Maia, but it still needs much improvement. What I found unique in Maia is using MVC Architectural patterns very efficiently. It adheres to the MVC architectural pattern, separating concerns between models, views, and controllers & that's a very good thing for the protocol itself. The overall codebase is too huge which makes it difficult to handle all the External Calls from being exploited as I have found most vulnerabilities are from External Calls. So, there must be a good implementation of External Calls. Otherwise, Maia has to face a severe loss whether it's the loss of funds or loss of the system maintenance. One more thing that Maia Protocol needs more audits like this one because this protocol lacks very much a well written & exploit proof code. Bacause, Larger the protocol more the vulns. Overall, codebase will be much better because many wardens are taking care of this protocol and protocol will have the better approach in sense of security of the system after mitigation of this contest.

Architecture Feedback

As A Security Researcher, I will point out multiple things to be considered.

Hermes:

Ensure that the lockers' direct emissions to gauges are implemented securely to prevent any potential manipulation or unauthorized access to revenue. Implement robust authentication and authorization mechanisms to protect locker accounts and their associated funds. Perform thorough testing and auditing of the smart contracts involved in the lock and voting mechanisms to identify and mitigate any potential vulnerabilities. Implement proper access controls to prevent unauthorized modifications to the lockers' liquidity farming rewards and rebases. Regularly monitor and update the system to address any emerging security threats or vulnerabilities. Maia:

Conduct a security assessment of the Partner bHermes Vault to identify any potential vulnerabilities or attack vectors. Ensure that the staking process for Maia and conversion to vMaia is secure and resistant to potential attacks. Implement proper audit trails and monitoring mechanisms to detect and respond to any suspicious activities related to Maia and its associated utilities. Apply best practices for secure handling of user funds and ensure that appropriate security measures are in place to protect the vault. Talos:

Perform a thorough security audit of the decentralized Uniswap V3 Liquidity Management protocol to identify and mitigate any potential security risks. Ensure that the LP creation and management processes are secure and properly validate input parameters to prevent manipulation or unauthorized access. Implement adequate measures to protect LP assets, including secure key management and protection against potential attacks on LP positions. Regularly update and patch the protocol to address any discovered security vulnerabilities or weaknesses. Ulysses:

Conduct a security assessment of the Virtualized Liquidity and Unified Liquidity components to identify any security risks associated with cross-chain asset handling and the stable swap AMM. Ensure that the messaging layer (any call v7) used for virtualized liquidity is properly secured to prevent unauthorized access or tampering with assets. Implement robust security measures to protect the Multi-Asset ERC4626 tokenized vault and prevent unauthorized access or manipulation of the deposited assets. Regularly monitor and update the system to address any security vulnerabilities or emerging threats.

Centralization risks

There is only one spot where I find that there might be a centralization risk which is Governance. It's important to look out for the governance utility because mostly centralized risks are often found in Governance in Web 3 Protocol. So, It is important to evaluate the governance model and assess whether it allows for decentralized decision-making and community participation.

Systemic risks

There are a bunch of Systematic Risks. Many of them you can find out in my Reports and Findings. For Instance, Incorrect Implementation of ERC20 Interface in IWETH9.sol, Benign Reentrancy that can unexpectedly exploit a contract and much more.

Other recommendations

The Codebase is pretty huge, the devs need to be able to implement security guidelines while designing or maintaining the system. It Means the developer should have enough knowledge of security as well. Because a little flaw in such large code base can make trouble for developers. The Developers must be experienced to maintain the Maia in the future. There Lacks a focus on Guages contracts that needs more attention of the developers as well as security researchers.

How much time did you spend?

I didn't count hours but based on the days so I spent roughly 10-12 Days and each day I dedicated 2-3 hours, So, Basically 25-30 hours on this protocol.

Time spent:

25 hours