Olympus DAO contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L216-L221 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Governance.sol#L302-L304

Vulnerability details

Impact

Currently, if users vote for the active proposal, the VOTES are transferred to the contract so that users can't vote or endorse other proposals while the voted proposal is active.

And the active proposal can be replaced only when the proposal is executed successfully or another proposal is activated after GRACE_PERIOD.

But activateProposal() requires at least 20% endorsements here, so it might be impossible to activate a new proposal forever if the current active proposal involves more than 80% of total votes.

Proof of Concept

The below scenario would be possible.

1. Proposal 1 was submitted and activated successfully.
2. Let's assume 81% of the total votes voted for this proposal. Yes = 47%, No = 34%
3. This proposal can't be executed for this requirement because 47% - 34% = 13% < 33%.
4. Currently the contract contains more than 81% of total votes and users have at most 19% in total.
5. Also users can't reclaim their votes among 81% while Proposal 1 is active.
6. So even if a user who has 1% votes submits a new proposal, it's impossible to activate because of this require().
7. So it's impossible to delete Proposal 1 from an active proposal and there won't be other active proposal forever.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we should add one more constant like EXECUTION_EXPIRE = 2 weeks so that voters can reclaim their votes after this period even if the proposal is active.

I am not sure we can use the current GRACE_PERIOD for that purpose.

So reclaimVotes() should be modified like below.

function reclaimVotes(uint256 proposalId_) external {
    uint256 userVotes = userVotesForProposal[proposalId_][msg.sender];

    if (userVotes == 0) {
        revert CannotReclaimZeroVotes();
    }

    if (proposalId_ == activeProposal.proposalId) {
        if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_EXPIRE) //+++++++++++++++++++++++++++++++++
        {
            revert CannotReclaimTokensForActiveVote();
        }
    }

    if (tokenClaimsForProposal[proposalId_][msg.sender] == true) {
        revert VotingTokensAlreadyReclaimed();
    }

    tokenClaimsForProposal[proposalId_][msg.sender] = true;

    VOTES.transferFrom(address(this), msg.sender, userVotes);
}

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/RANGE.sol#L80 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/RANGE.sol#L244-L250 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/RANGE.sol#L264 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L134 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L518

Vulnerability details

Impact

Inconsistant parameter requirements between constructor and Set() functions in RANGE.sol and Operator.sol.

The contracts might work unexpectedly when the params are set improperly using constructor().

Proof of Concept

• In RANGE.sol, setSpreads() and setThresholdFactor() has some requirements but constructor() doesn't check at all.

File: 2022-08-olympus\src\modules\RANGE.sol
242:     function setSpreads(uint256 cushionSpread_, uint256 wallSpread_) external permissioned {
243:         // Confirm spreads are within allowed values
244:         if (
245:             wallSpread_ > 10000 ||
246:             wallSpread_ < 100 ||
247:             cushionSpread_ > 10000 ||
248:             cushionSpread_ < 100 ||
249:             cushionSpread_ > wallSpread_
250:         ) revert RANGE_InvalidParams();
251: 
252:         // Set spreads
253:         _range.wall.spread = wallSpread_;
254:         _range.cushion.spread = cushionSpread_;
255: 
256:         emit SpreadsChanged(wallSpread_, cushionSpread_);
257:     }

File: 2022-08-olympus\src\modules\RANGE.sol
263:     function setThresholdFactor(uint256 thresholdFactor_) external permissioned {
264:         if (thresholdFactor_ > 10000 || thresholdFactor_ < 100) revert RANGE_InvalidParams();
265:         thresholdFactor = thresholdFactor_;
266: 
267:         emit ThresholdFactorChanged(thresholdFactor_);
268:     }
269: 

• In Operator.sol, setCushionFactor() checks the requirement but constructor() doesn't check it.

File: 2022-08-olympus\src\policies\Operator.sol
516:     function setCushionFactor(uint32 cushionFactor_) external onlyRole("operator_policy") {
517:         /// Confirm factor is within allowed values
518:         if (cushionFactor_ > 10000 || cushionFactor_ < 100) revert Operator_InvalidParams();
519: 
520:         /// Set factor
521:         _config.cushionFactor = cushionFactor_;
522: 
523:         emit CushionFactorChanged(cushionFactor_);
524:     }
525: 

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend adding same validation for the parameters between constructor() and Set() functions.

Low Risk Issues

Wrong Comments

Change a-z only => "a-z", "_" only

• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/utils/KernelUtils.sol#L61

Non-critical Issues

Open TODOs

• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L657
• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/TreasuryCustodian.sol#L51-L52
• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/TreasuryCustodian.sol#L56

Immutable addresses should be 0-checked

• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/MINTR.sol#L16
• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/RANGE.sol#L102-L103
• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L121
• https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L123