Olas - joaovwfreire's results

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol#L331-L334 https://github.com/code-423n4/2023-12-autonolas/blob/main/tokenomics/contracts/Depository.sol#L445-L463

Vulnerability details

Impact

The getBonds view function does a loop which amount of iterations is equal to bondCounter -1. The issue is whenever a user makes a deposit, bondCounter increases.

As more deposits happen within the contract, it will be impossible to call getBonds from other contracts as the loop will consume all gas available.

Proof of Concept

Loop inside getBonds:

uint256 numBonds = bondCounter;
        bool[] memory positions = new bool[](numBonds);
        // Record the bond number if it belongs to the account address and was not yet redeemed
        for (uint256 i = 0; i < numBonds; ++i) {
            // Check if the bond belongs to the account
            // If not and the address is zero, the bond was redeemed or never existed
            if (mapUserBonds[i].account == account) {
                // Check if requested bond is not matured but owned by the account address
                if (!matured ||
                    // Or if the requested bond is matured, i.e., the bond maturity timestamp passed
                    block.timestamp >= mapUserBonds[i].maturity)
                {
                    positions[i] = true;
                    ++numAccountBonds;
                    // The payout is always bigger than zero if the bond exists
                    payout += mapUserBonds[i].payout;
                }
            }
        }

bondCounter being increased at the deposit external permissionless function :

// Create and add a new bond, update the bond counter
        bondId = bondCounter;
        mapUserBonds[bondId] = Bond(msg.sender, uint96(payout), uint32(maturity), uint32(productId));
        bondCounter = uint32(bondId + 1);

Tools Used

Manual review

Recommended Mitigation Steps

Attempt to index such data off-chain. If the feature is extremely needed, index it at a mapping that maps an array composed of bond ids to the bond owner address.

Assessed type

Loop

Summary

Protocol Overview

Quick briefing

Autonolas Protocol facilitates autonomous on-chain services through the use of smart contracts to orchestrating providers and receivers. These contracts are central to coordinating software contributions from developers within the blockchain ecosystem.

1. Foundational Concepts: Autonolas is developed with a focus on autonomy. It aims to provide solutions for blockchain-based autonomous services, addressing the current needs and functionalities required in this domain.

2. Technical Architecture: The protocol employs a Multi-Agent Systems Architecture to ensure secure and efficient consensus mechanisms. This architecture supports crypto-native agent services and is integral to the on-chain operations of the protocol. Key components include Agent Components, Canonical Agents, Agent Instances, and Services.

3. Tokenomics: The OLAS token plays a crucial role in the Autonolas ecosystem. It is used to balance economic factors and encourage participation. The protocol's economic model includes bonding and discount factors, vital for its long-term economic health.

4. Use Cases: Autonolas has applications in several areas, particularly in DAO operations and protocol infrastructure. It supports functions such as treasury management, yield optimization, and trading strategy execution, underlining its utility in decentralized operations.

5. Governance: The governance system of Autonolas is designed for transparency and community involvement. It features a governance module, specific contracts like veOLAS and GovernorOLAS, and mechanisms including timelock (with incentives for longer-term locks) and a community multisig. These elements collectively enable a decentralized governance process.

Sources

The protocol team has provided extensive high quality documentation both at the audit repo and at the docs webpage. Alongisde some content found on Youtube, this is enough contextualization to begin the security research on the codebase. I have opted not to build diagrams as the team already provided high-quality overviews:

The Whitepaper is an 80-page document that covers the audit scope and much more about Autonolas.

These are the sources utilized for this security research outside of what is contained at the repository:

Methods

The security research was done on three steps: context-gathering, codebase-evaluation and report-generation.

Context-gathering: involved reading all documentation available and reviewing all sources provided at the protocol overview. The main intention during this phase is to develop a mental-model of flows and reach clarity on the protocol's concepts. This helps with the research as threat-modeling can be adjusted to the protocol and its target audience operational risks.

Codebase-evaluation: consisted of 3 steps. Automated diagramming, line-by-line review and semantical analysis.

1. Automated diagramming: with the utilization of [Solidity Visual Developer by tintinweb](Solidity Visual Developer - Visual Studio Marketplace) I have overviewed the contracts before diving into the code. This helps pointing the areas of most expected complexity and they are left to be checked at last. At the end of this stage, I have a list of the functions to analyze in order. Example diagram of the Depository.sol contract: GuardCM.sol contract:
2. Line-by-line review: consisted of analyzing the functions at the order determined at the previous stage. This thorough approach from simple to complex functions enables the understanding of complexity behemoths, such as the [checkpoint function at the Tokenomics contract](2023-12-autonolas/tokenomics/contracts/Tokenomics.sol at main · code-423n4/2023-12-autonolas (github.com)).
3. Semantical analysis: consisted of first mirroring methods that did opposite things (deposit/withdraw, join/leave, sell/buy) to find asymmetries; and secondly a mid-level overview of state-variable-assignments and external calls to see if they made sense based on the documentation-specified features.

Report-generation: involved gathering the observations made on the previous phases and pre-sorting them into the following categories: "ask-the-sponsor", "easy-proof-of-concept", "hard-proof-of-concept", "false-positive". After the pre-sort, questions are asked and the reports are written. Finally the analysis report is written and the research is wrapped up.

Codebase

Tokenomics

extra comments The checkpoint function could be broken down into several smaller functions that can help the developers see more clearly whether a state is being properly transitioned or not.

I've taken this function and commented how it could be refactored into separate smaller functions that could be called in the following sequence:

1. getImplementationAddress
2. checkDonationBlock
3. calculateEpochTime
4. calculateIncentives
5. calculateInflation
6. adjustEffectiveBond
7. updateNextEpochTokenomics
8. adjustMaxBond
9. calculateAndUpdateIDF
10. calculateOwnerTopUps
11. rebalanceTreasuryAndSettleEpoch

By separating the state transitions into 11 smaller blocks, the developers are less likely to commit errors.

Governance

• supportsInterface implementation was missing on GuardCM contract - makes Safe:setGuard revert.

Registries

I have opted not to include the lockbox analysis as it is not a domain (solang) I have good understanding of.

Risks

A-01 - Centralization

There are several functions that are owner-determined and could be used to induce end-user losses. This is an expected side-effect of managed-protocols. The product's interactions are permissionless provided the protocol's core functionalities are set up.

Recommended Mitigation Steps

The protocol has plans to implement Community-Owned multisigs and the contracts deployer is a Timelock governed by the DAO.

A-02 The blacklist mechanics are easily circumvented

Description

The DonatorBlacklist contract inhibits addresses blacklisted by the protocol from donating ETH. This is easily circumvented by transferring the funds to a second address then donating.

There is no clear definition on the parties that should be blacklisted, so it is difficult to suggest mitigation alternatives. Consider defining the blacklist persona further while keeping in mind some of the following elements: OFAC sanctioned addresses Mixer services-related addresses Public-exploit addresses Business-determined addresses

A-03 - Inline Assembly Usage for Data Decoding

Description

The usage of nline assembly bypasses many of Solidity's safety mechanisms. This can lead to issues in data handling, increased difficulty in debugging, and potential exploitation by malicious actors if any vulnerabilities are present. Moreover, the reduced readability and increased complexity make future maintenance and updates more challenging and error-prone.

The following example is taken from the HomeMediator contract:

// solhint-disable-next-line no-inline-assembly
assembly {
    // First 20 bytes is the address (160 bits)
    i := add(i, 20)
    
    target := mload(add(data, i))
    // Offset the data by 12 bytes of value (96 bits)
    i := add(i, 12)
    value := mload(add(data, i))
    // Offset the data by 4 bytes of payload length (32 bits)
    i := add(i, 4)
    payloadLength := mload(add(data, i))
}

Recommended Mitigation Steps

Consider utilizing abi.decode for complex data-handling operations.

A-04 - RegistriesManager:create can be front-run and enable contribution theft

Impact

The RegistriesManager create function enables any developer to create/register a new component or agent to an unit owner of the caller's choice. The issue with that is the mempool is public and malicious parties could opt to front-run the transaction and steal credit for the component/agent contribution by calling the same transaction but with it's own address as the unit owner.

Proof of Concept

As the dependencies data are stored off-chain, the ability of permissionlessly calling [create](2023-12-autonolas/registries/contracts/RegistriesManager.sol at main · code-423n4/2023-12-autonolas (github.com)) with any parameters should suffice as proof of concept.

function create(
        IRegistry.UnitType unitType,
        address unitOwner,
        bytes32 unitHash,
        uint32[] memory dependencies
    ) external returns (uint256 unitId)

    {

        // Check if the creation is paused
        if (paused) {
            revert Paused();
        }

        if (unitType == IRegistry.UnitType.Component) {
            unitId = IRegistry(componentRegistry).create(unitOwner, unitHash, dependencies);
        } else {
            unitId = IRegistry(agentRegistry).create(unitOwner, unitHash, dependencies);
        }
    }

Recommended Mitigation Steps

The issue is contribution theft and cannot be mitigated by in-contract mechanics. Utilize a relay such as flashbots for component/agent creation. Extensively warn developers to properly place licensing and credits at their dependencies and components before pushing a transaction to create the component/agent. Consider the possibility of disputes arising in the future.

A-05 - GuardCM's behaviour under the paused state induces mistakes

Impact

The checkTransaction function at the GuardCM contract checks if it has authorized arguments and is important to protect the Gnosis Safe community multisig. The function works by reverting in case any unwanted parameter is passed. The issue, however, lies at the first if condition (that guards the whole checks ran at the function):

if (paused == 1) {

This means if the contract GuardCM contract is paused (defined by paused == 2), checkTransaction does not check anything and will allow the conditions that were expected to cause reversion not to revert.

This is confusing and can induce mistakes. The "pause" keyword may lead one to believe the whole multisig is paused and the Guard would revert every single call.

Proof of Concept

Place the following code snippet at the GuardCM.js test, after the "Pause the guard" test:

it("Pause the guard and it doesn't revert if to argument is the multisig's address", async function () {
            // Try to pause the guard not by the timelock or the multisig
            await expect(
                guard.pause()
            ).to.be.revertedWithCustomError(guard, "ManagerOnly");

            // Pause the guard by the timelock
            const payload = guard.interface.encodeFunctionData("pause", []);
            await timelock.execute(guard.address, payload);
            expect(await guard.paused()).to.equal(2);
            
            // put random txData to checkTransaction
            // put the multisig address as the to argument.
            let txData = await timelock.interface.encodeFunctionData("schedule", [l1BridgeMediators[0], 0, gnosisPayload,
                Bytes32Zero, Bytes32Zero, 0]);
            await guard.checkTransaction(multisig.address, 0, txData, 0, 0, 0, 0, AddressZero, AddressZero, "0x", AddressZero);
            
        });

Run the test with:

npx hardhat test test/GuardCM.js --grep "Pause the guard and it doesn't revert if to argument is the multisig's address"

Notice how the GuardCM contract doesn't revert.

Recommended Mitigation Steps

Make sure to revert if the paused variable is equal 2 during checkTransaction calls. If the intention is to pause the Safe's calls from being guarded, consider setting it's guard to the zero address during pause periods then setting it back to Guard implementations when unpausing.

Time spent

55 hours

Acknowledgements

Thanks for taking the time to read this report. Thanks to the Autonolas team for the opportunity and specifically to Kupermind for patiently answering all my questions.

Time spent:

55 hours