An proposal with a large amount of instructions can run out of gas and become unavailable.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Unbounded loops with external calls can run out of gas. It's possible for a proposal to not be executed if it has a large amount of instructions, causing a DoS on the contract availability.

Proof of Concept

In the executeProposal() function, the action for each instruction will be executed on a unbounded loop.

File: src/policies/Governance.sol
function executeProposal() external {
    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);
}

Recommended Mitigation Steps

Add a slice functionality into the function executeProposal(), to also enable iterations on a subsection of instructions, instead of the entire array.

This would ensure that even if a proposal has a large amount of instructions, the actions can still be executed in batches.

This could be done by adding a startIndex and endIndex as function arguments.

[0xLienid]

duplicate of #258

[0xean]

downgrading to QA - marking as dupe of #499 users QA report