Olympus DAO contest - cryptphi's results

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();
        }

Lines of code

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

Vulnerability details

Impact

The OlympusGovernance.executeProposal() function makes an external call to kernel contract before updating the state variable activeProposal. This does not follow the CEI pattern and allows the function to be possibly be re-entered to execute the proposal multiple times before the proposal is deactivated.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

A non-reentrant modifier or mutex may be necessary. Alternatively, the check-effect-interact pattern should be implemented.

1. Missing zero address check The following are missing checks for zero address.

Occurrences Kernel.executeAction() - is missing zero address validation, allowing the executor and/or admin address to be able to be set to address(0) Kernel.grantRole() - Address(0) can be granted role due to no check for zero address input

2. Single step critical actionsThe executeAction() function carries out some critical actions which should be broken down into two step calls. e.g change of admin and change of executor

3. Missing event and emit Policy.setActiveStatus()

4. Return values ignored Kernel._reconfigurePolicies(Keycode) ignores return value by dependents[i].configureDependencies()

5. CEI pattern not followed The following functions do not follow the Check-Effect-Interact pattern

Occurrences Kernel.activatePolicy() - calls policy.configureDependencies() before updating state variables Kernel.pruneFromDependents() - calls policy.configureDependencies() before updating state variables OlympusTreasury.repayLoan() in TRSRY.sol - external call token_.safeTransferFrom() before state variables reserveDebt and totalDebt updates. Operator.initialize() - external calls before state variables updates.Operator.operate() - external calls before state variables updates