Olympus DAO contest - carlitox477's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L265 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L278-L288

Vulnerability details

Impact

Given that the activeProposal change is done before the for loop, if this function is call through one kernel.executeAction(instruction,target) we can call the same instructions (in the same order) again and again, which may or may not affect funds (depending on the instructions).

Proof of Concept

For instance, if we install a new module, and this module has a vulnerability (even intentional), the next steps can by trigger:

1. Call executeAction
2. This allow us to call kernel.executeAction in the for loop
3. executAction allow us to call _installModule
4. _installModule allow us to call newModule_.Init
5. By init we can call now executeProposal again (suppose that the init function interact with a previous vulnerable proxy contract to scam voters to vote in favour of this proposal as if it was a contract which is ok, and before calling executeProposal we change the implementation to allow this attack),

Tools Used

Static Analysis

Recommended Mitigation Steps

Use nonReentrant modifier or move the line activeProposal = ActivatedProposal(0, 0); before the for loop.

Policy

[Non-critical] getModuleAddress does not respect internal/private naming convention

It should change it name to _getModuleAddress

[Non-critical] _ohmEthPriceFeed, _reserveEthPriceFeed, decimals and _scaleFactor do not respect constant/immutable naming convention

They should change their names to _OHM_ETH_PRICE_FEED, _RESERVE_ETH_PRICE_FEED, DECIMALS and _SCALE_FACTOR to respect naming convention.

OlympusRange

[Non-critical] ohm and reserve do not respect constant/immutable naming convention

They should change their names to OHM and RESERVE to respect naming convention.

OlympusGovernance

[Non-critical] INSTR and VOTES do not respect non constant/immutable naming convention

They should change their names to instr and votes to respect convention.

KernerlUtils

ensureValidKeycode: i++

Change i++ for ++i. POC

ensureValidRole: i++

Change i++ for ++i. POC

Kernel

_activatePolicy: Math operation can be avoided Double access to activePolices can be avoided

It would be better to change this for this:

getPolicyIndex[policy_] = activePolicies.length;
activePolicies.push(policy_);

_activatePolicy: Math operation and double access to activePolices can be avoided

It would be better to change this for this:

getDependentIndex[keycode][policy_] = moduleDependents[keycode].length;
moduleDependents[keycode].push(policy_);

grantRole and revokeRole should be external

They can only be called by the admin, an external address. This will save gas. POC

OlympusInstructions

getInstruction should be external

This will save gas. It is not called by other function inside the contract POC

OlympusRange

updateCapacity: _range is modified and accesed multiples times

Copy it to a memory variable to lessen storage access.

function updateCapacity(bool high_, uint256 capacity_) external permissioned {
    Range __range=_range;
    if (high_) {
        // Update capacity
        __range.high.capacity = capacity_;

        // If the new capacity is below the threshold, deactivate the wall if they are currently active
        if (capacity_ < __range.high.threshold && __range.high.active) {
            // Set wall to inactive
            __range.high.active = false;
            __range.high.lastActive = uint48(block.timestamp);

            emit WallDown(true, block.timestamp, capacity_);
        }
    } else {
        // Update capacity
        __range.low.capacity = capacity_;

        // If the new capacity is below the threshold, deactivate the wall if they are currently active
        if (capacity_ < __range.low.threshold && __range.low.active) {
            // Set wall to inactive
            __range.low.active = false;
            __range.low.lastActive = uint48(block.timestamp);

            emit WallDown(false, block.timestamp, capacity_);
        }
    }
    _range=__range;
}

updatePrices: _range is modified and accessed multiples times

Copy it to a memory variable to lessen storage access.

function updatePrices(uint256 movingAverage_) external permissioned {
    Range memory __range = range;
    // Cache the spreads
    uint256 wallSpread = __range.wall.spread;
    uint256 cushionSpread = __range.cushion.spread;

    // Calculate new wall and cushion values from moving average and spread
    __range.wall.low.price = (movingAverage_ * (FACTOR_SCALE - wallSpread)) / FACTOR_SCALE;
    __range.wall.high.price = (movingAverage_ * (FACTOR_SCALE + wallSpread)) / FACTOR_SCALE;

    __range.cushion.low.price = (movingAverage_ * (FACTOR_SCALE - cushionSpread)) / FACTOR_SCALE;
    __range.cushion.high.price =
        (movingAverage_ * (FACTOR_SCALE + cushionSpread)) /
        FACTOR_SCALE;

    emit PricesChanged(
        __range.wall.low.price,
        __range.cushion.low.price,
        __range.cushion.high.price,
        __range.wall.high.price
    );
    _range = __range;
}

updateMarket should be external

This will save gas. It is not called by other function inside the contract POC

OlympusTreasury

repayLoan: can save gas if we check amount_ != 0

If amount_ == 0 is nonsense to continue the function execution. Adding require(amount_ != 0, 'amount_ can't be 0') can save gas for a wrong execution.

reapyLoan: unnecessary calculations

Given that safeTransferFrom revert in case of an error, it seems nonsense the calculation of received. Meaning we can simplify our function to:

function repayLoan(ERC20 token_, uint256 amount_) external nonReentrant {
    if (reserveDebt[token_][msg.sender] == 0) revert TRSRY_NoDebtOutstanding();

    // Deposit from caller first (to handle nonstandard token transfers)
    token_.safeTransferFrom(msg.sender, address(this), amount_);

    // Subtract debt from caller
    reserveDebt[token_][msg.sender] -= amount_;
    totalDebt[token_] -= amount_;

    emit DebtRepaid(token_, msg.sender, amount_);
}

OlimpusGovernance

endorseProposal: reduce totalEndorsementsForProposal[proposalId_] access and operation

We can grup the substraction and addition to avoid double memory access.

// undo any previous endorsement the user made on these instructions
uint256 previousEndorsement = userEndorsementsForProposal[proposalId_][msg.sender];

// reapply user endorsements with most up-to-date votes
userEndorsementsForProposal[proposalId_][msg.sender] = userVotes;
totalEndorsementsForProposal[proposalId_] += (userVotes - previousEndorsement);

vote: activeProposal is accessed multiple times

We can cache the state variable to avoid multiple storage access

vote: userVotesForProposal[activeProposal.proposalId][msg.sender] is accessed multiple times

We can cache the state variable to avoid multiple storage access

executeProposal: activeProposal is accessed multiple times

We can cache the state variable to avoid multiple storage access

function executeProposal() external {
    ActivatedProposal _activeProposal=activeProposal
    uint256 netVotes = yesVotesForProposal[_activeProposal.proposalId] -
        noVotesForProposal[_activeProposal.proposalId];
    if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
        revert NotEnoughVotesToExecute();
    }

    if (block.timestamp < _activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
        revert ExecutionTimelockStillActive();
    }

    Instruction[] memory instructions = INSTR.getInstructions(_activeProposal.proposalId);

    for (uint256 step; step < instructions.length; ) {
        kernel.executeAction(instructions[step].action, instructions[step].target);
        unchecked {
            ++step;
        }
    }

    emit ProposalExecuted(_activeProposal.proposalId);

    // deactivate the active proposal
    activeProposal = ActivatedProposal(0, 0);
}