QA Report

[code423n4]

[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.