Olas - 0xA5DF's results

L1 - Guard and owners can't be changed in CM

Gnosis safe guard/owners change are done by an external call made by the Safe to itself. The GuardCM prevents any calls to self. This makes sense as it prevents the CM owners from changing the guard on their own, but there's no alternative way to change the guard or owners when needed.

As mitigation - consider adding mechanism for the governance to approve self calls when needed.

L2 - CM can replace the guard while paused

While the GuardCM is paused the CM can run any function, including the call to self to replace the guard. That means the governance would lose the ability to unpause the guard later on.

L3 - Users might burn their tokens by mistake

The OLAS token allows anybody to burn their tokens, this function might lead to users burning their own tokens by mistake.

OLAS.sol#L117-L120

    function burn(uint256 amount) external {
        _burn(msg.sender, amount);
    }

L4 - Users can exhaust/spam the system by starting the governorCheckProposalId

The governorCheckProposalId is a proposal ID that'll be used to check if the governance is still 'alive'. If the proposal doesn't pass then

• In order to propose you need proposal threshold (5 ETH in testing)
• In order for a vote to pass you need at least quorum for votes
• That means that an attacker can continuously spam the system with proposing this vote, while each time the propose is raised the protocol would have to pass the vote AND pass another proposal to replace the governorCheckProposalId

L5 - CM guard shouldn’t be allowed to call other contracts by default

The guard checks the calls only if they are to the timelock or CM addresses, calls to other addresses are allowed and aren't checked. While I can't point to a practical security risk that seems to stem from that it's best to reject calls to other addresses by default (If there's any other address that the CM needs to call then that should be added to the list of allowed addresses, but the rest should be prohibited).

L6 - There’s no check that the sum of the top up fractions isn’t less than 100

When incentive fractions are changed there's a check that the fractions aren't greater than 100. But there's no check to ensure they're not less than 100. In case that their sum is less than 100 then the tokenomics contract wouldn't be using the full potential of top ups.

Tokenomics.sol#L580-L583

        // Same check for top-up fractions
        if (_maxBondFraction + _topUpComponentFraction + _topUpAgentFraction > 100) {
            revert WrongAmount(_maxBondFraction + _topUpComponentFraction + _topUpAgentFraction, 100);
        }

L7 - treasury doesn't allow to withdraw tokens that were sent directly to the contract

The Treasury allows the owner to withdraw the tokens

Treasury.sol#L366-L369

            }  else {
                // Insufficient amount of LP tokens
                revert LowerThan(tokenAmount, reserves);
            }

L8 - getUserPoint can revert when index is out of bound error

It seems that wveOLAS.getUserPoint() tries to wrap the call to veOLAS and prevent out of bond index error. However, it fails to do so. It checks that userNumPoints > 0 instead of checking that idx < userNumPoints. In case that idx is greater than userNumPoints the check might pass and the call would revert with an index out of bound error.

wveOLAS.sol#L188-L199

    /// @dev Gets the checkpoint structure at number `idx` for `account`.
    /// @notice The out of bound condition is treated by the default code generation check.
    /// @param account User wallet address.
    /// @param idx User point number.
    /// @return uPoint The requested user point.
    function getUserPoint(address account, uint256 idx) public view returns (PointVoting memory uPoint) {
        // Get the number of user points
        uint256 userNumPoints = IVEOLAS(ve).getNumUserPoints(account);
        if (userNumPoints > 0) {
            uPoint = IVEOLAS(ve).getUserPoint(account, idx);
        }
    }

L9 -Multiple tokens might share the same tokenURI()

The tokenURI() is based on the unitHash which isn't unique for the tokens, meaning the tokenURI() isn't unique either. This goes against the ERC721 standard which requires it to be distinct for each asset:

A distinct Uniform Resource Identifier (URI) for a given asset.

Mitigation - keep track of the hashes, and don’t allow the same hash to be created twice. Alternately, find a way to add the unitId to the tokenURI

GenericRegistry.sol#L131-L141

L10 - Treasury.receive() function would revert on simple ETH transfer

When ETH is sent to a contract without any additional gas (e.g. via Solidity's trasnfer() or send()) then the gas limit for the receive() function is 2300 gas. The Treasury.receive() function uses more than that (it sloads 2 storage variable, and then sstores 1), meaning the function would revert in those cases.

(users can still send ETH using call() with sufficient gas, in that case it won't revert)

L11 - getCurrentPriceLP() underestimates the price of the LP

getCurrentPriceLP() calculates the price based only on the OLAS part of the LP token, it doesn't take into account the value of the other token used in the pool.

This results in a price estimation that's low by 50% than the actual price. If the results of this function are fed into the Depository when creating a product then it'd result in a price that's too low and LP holders won't deposit.

GenericBondCalculator.sol#L70-L92

L12 - OLAS price might change after product creation

The LP price of a product is set during its creation and isn't changed afterwards. In reality the price of the LP can change (due to OLAS price change) which will end up in buying LP in a higher price than the actual price (loss for the protocol) or the price being too low for the LPs to sell their LP tokens.

R1 - getCurrentPriceLP() should revert in case that none of the tokens are olas

getCurrentPriceLP() states that there should never be a case where none of the tokens are olas. The function checks that, but it doesn't revert if the check fails. Instead it does nothing (which will return zero as a value). It makes more sense to revert in case of an error rather than return zero.

GenericBondCalculator.sol#L81-L83

            // token0 != olas && token1 != olas, this should never happen
            if (token0 == olas || token1 == olas) {
                // If OLAS is in token0, assign its reserve to reserve1, otherwise the reserve1 is already correct

R2 - ComponentRegistry._getSubComponents() doesn't assert that the UnitType is component

The function ignores UnitType and seems to assume that it'll always be a component (since components can't hold subcomponent of other types). While this is expected to be the case, it might be worth to assert that.

ComponentRegistry.sol#L37-L45

    /// @dev Gets subcomponents of a provided component Id.
    /// @notice For components this means getting the linearized map of components from the local map of subcomponents.
    /// @param componentId Component Id.
    /// @return subComponentIds Set of subcomponents.
    function _getSubComponents(UnitType, uint32 componentId) internal view virtual override
        returns (uint32[] memory subComponentIds)
    {
        subComponentIds = mapSubComponents[uint256(componentId)];
    }

R3 - ComponentRegistry doesn’t implement calculateSubComponents

calculateSubComponents is part of the IRegistry interface but isn’t implemented in ComponentRegistry (there's an internal function with a similar name, but not a public/external)

NC1 - GnosisSafeSameAddressMultisig.create() function name doesn’t reflect what the function does

The function doesn’t seem to create any new instance, but just to verify an existing instance.

[G1] A more efficient way to assert that this is a delegate call

Gas saved: 2.1K per tx

Current check to ensure a delegate-call-only uses an sload which costs ~2.1K per tx. There's a much simpler and cheaper way to do that, see OZ's docs

Current code

        assembly {
            implementation := sload(PROXY_TOKENOMICS)
        }
        // Check if there is any address in the PROXY_TOKENOMICS address slot
        if (implementation == address(0)) {
            revert DelegatecallOnly();
        }

OZ's code:

abstract contract OnlyDelegateCall {
    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
    address private immutable self = address(this);

    function checkDelegateCall() private view {
        require(address(this) != self);
    }

    modifier onlyDelegateCall() {
        checkDelegateCall();
        _;
    }
}

[G2] Token1 isn’t used in some case

Gas saved: 2.4 per tx (sload + call)

In getCurrentPriceLP() token1 isn't needed when token0 == olas. Not reading that can save us an sload to a cold key inside the call + the call itself.

Code

        if (totalSupply > 0) {
            address token0 = pair.token0();
            address token1 = pair.token1();
            uint256 reserve0;
            uint256 reserve1;
            // requires low gas
            (reserve0, reserve1, ) = pair.getReserves();
            // token0 != olas && token1 != olas, this should never happen
            if (token0 == olas || token1 == olas) {
                // If OLAS is in token0, assign its reserve to reserve1, otherwise the reserve1 is already correct
                if (token0 == olas) {
                    reserve1 = reserve0;
                }
                // Calculate the LP price based on reserves and totalSupply ratio multiplied by 1e18
                // Inspired by: https://github.com/curvefi/curve-contract/blob/master/contracts/pool-templates/base/SwapTemplateBase.vy#L262
                priceLP = (reserve1 * 1e18) / totalSupply;
            }
        }

[G3] Use assembly to trim arrays

Gas saved: 10.8K = ~300 units per element * a sum 36 elements on avg

See PoC I tested the PoC with 20 elements, it saved about 6K gas units.

GuardCM

There are a few cases in GuardCM where we trim an array by copying it to a new array. However, this can be done by assembly, given that the old array isn't used anymore in the code.

Estimated avg # of elements: 12 (min is 8 according to the code)

GuardCM.sol#L273-L275

            for (uint256 i = 0; i < payload.length; ++i) {
                payload[i] = data[i + 4];
            }

GuardCM.sol#L291-L294

            bytes memory bridgePayload = new bytes(mediatorPayload.length - SELECTOR_DATA_LENGTH);
            for (uint256 i = 0; i < bridgePayload.length; ++i) {
                bridgePayload[i] = mediatorPayload[i + SELECTOR_DATA_LENGTH];
            }

GuardCM.sol#L317-L320

            bytes memory payload = new bytes(data.length - SELECTOR_DATA_LENGTH);
            for (uint256 i = 0; i < payload.length; ++i) {
                payload[i] = data[i + SELECTOR_DATA_LENGTH];
            }

GuardCM.sol#L339-L342

        bytes memory payload = new bytes(data.length - SELECTOR_DATA_LENGTH);
        for (uint256 i = 0; i < payload.length; ++i) {
            payload[i] = data[i + 4];
        }

GnosisSafeMultisig

This one has one instance Estimated average # of elements: 4

GnosisSafeMultisig.sol#L79-L81

                for (uint256 i = 0; i < payloadLength; ++i) {
                    payload[i] = data[i + DEFAULT_DATA_LENGTH];
                }

UnitRegistry

Here we're not trimming the beginning of the array but the end of it, but the same optimization can be applied. Estimated average # of elements: 20

UnitRegistry.sol#L261-L264

        subComponentIds = new uint32[](counter);
        for (uint32 i = 0; i < counter; ++i) {
            subComponentIds[i] = allComponents[i];
        }

About this instance I'm not 100% sure it's safe since it's adding an element, so I won't count that (see an alternative way to save the gas here in the next section)

UnitRegistry.sol#L95-L104

        if (unitType == UnitType.Component) {
            uint256 numSubComponents = subComponentIds.length;
            uint32[] memory addSubComponentIds = new uint32[](numSubComponents + 1);
            for (uint256 i = 0; i < numSubComponents; ++i) {
                addSubComponentIds[i] = subComponentIds[i];
            }
            // Adding self component Id
            addSubComponentIds[numSubComponents] = uint32(unitId);
            subComponentIds = addSubComponentIds;
        }

[G4] Add the unit ID at _calculateSubComponents()

Gas saved: 6K (300 per element * 20 elements on avg)

Instead of copying the array after it was created by _calculateSubComponents() it'd be much more efficient to refactor the code so that _calculateSubComponents() would add it on it's own when needed.

UnitRegistry.sol#L95-L104

        if (unitType == UnitType.Component) {
            uint256 numSubComponents = subComponentIds.length;
            uint32[] memory addSubComponentIds = new uint32[](numSubComponents + 1);
            for (uint256 i = 0; i < numSubComponents; ++i) {
                addSubComponentIds[i] = subComponentIds[i];
            }
            // Adding self component Id
            addSubComponentIds[numSubComponents] = uint32(unitId);
            subComponentIds = addSubComponentIds;
        }

[G5] Assign only the required field from storage to memory

Gas saved: 2.1K (sload to a cold slot)

This one is similar to G40 in the bot report, but it wasn't caught by it. The Unit struct contains also the unitHash field, but this one isn't used here. Therefore, it'd be cheaper to make unit a storage variable, this way we aren't reading the unitHash field from storage.

UnitRegistry.sol#L163

    function getDependencies(uint256 unitId) external view virtual
        returns (uint256 numDependencies, uint32[] memory dependencies)
    {
        Unit memory unit = mapUnits[unitId];
        return (unit.dependencies.length, unit.dependencies);
    }