code-423n4 / 2022-08-olympus-findings

5 stars 4 forks source link

Proposals can be submitted and executed sucessfully when VOTES totalySupply is 0. #393

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L265-L289 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L240-L262 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L205-L236 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180-L201 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L159-L176 https://github.com/code-423n4/2022-08-olympus/blob/main/src/modules/VOTES.sol#L1

Vulnerability details

Impact

When all contracts have been deployed and/or initialized, the OlympusVotes contract does not mint an initial token supply. This would allow users to be able to submit proposals, then vote and execute proposals if there has been no token supply (totalSupply = 0) after 1 week of proposal activation.

Proof of Concept

  1. Alice all contracts have been deployed and initialized but admin is yet to issue Votes to users.
  2. Alice submits a proposal and endorses it.
  3. After 1 week, there is still no VOTES toke mint, so still 0 totalSupply.
  4. Alice, activates Proposal, calls votes and executeProposal()
  5. Proposal is executed successfully since the functions will not revert.

Tools Used

Manual review

Recommended Mitigation Steps

An initial VOTES token supply should be minted. and to accommodate the initial supply, some changes to the if-statement

if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
            revert NotEnoughVotesToExecute();
        }
fullyallocated commented 2 years ago

Technically correct, but the production version will use a different version of the token that has an initial supply. We can consider adding in a minimum token threshold though for proposal execution

fullyallocated commented 2 years ago

Duplicate of #392