code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

For the Governance.sol contract, the functions activateProposal() and executeProposal() can be called by anyone.

Proof of Concept

An malicious user could monitor the protocal DAO and activate or execute a proposal in a time not intended by the proposal submitter, since any address can call activateProposal() or executeProposal() and no input address validation is implemented on these functions.

File: src/policies/Governance.sol
function activateProposal(uint256 proposalId_) external {
    ProposalMetadata memory proposal = getProposalMetadata[proposalId_];

    if (msg.sender != proposal.submitter) {
        revert NotAuthorizedToActivateProposal();
    }

    if (block.timestamp > proposal.submissionTimestamp + ACTIVATION_DEADLINE) {
        revert SubmittedProposalHasExpired();
    }

    if (
        (totalEndorsementsForProposal[proposalId_] * 100) <
        VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
    ) {
        revert NotEnoughEndorsementsToActivateProposal();
    }

    if (proposalHasBeenActivated[proposalId_] == true) {
        revert ProposalAlreadyActivated();
    }

    if (block.timestamp < activeProposal.activationTimestamp + GRACE_PERIOD) {
        revert ActiveProposalNotExpired();
    }

    activeProposal = ActivatedProposal(proposalId_, block.timestamp);

    proposalHasBeenActivated[proposalId_] = true;

    emit ProposalActivated(proposalId_, block.timestamp);
}

File: src/policies/Governance.sol
function executeProposal() external {
    uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
        noVotesForProposal[activeProposal.proposalId];
    if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
        revert NotEnoughVotesToExecute();
    }

    if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) {
        revert ExecutionTimelockStillActive();
    }

    Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId);

    for (uint256 step; step < instructions.length; ) {
        kernel.executeAction(instructions[step].action, instructions[step].target);
        unchecked {
            ++step;
        }
    }

    emit ProposalExecuted(activeProposal.proposalId);

    // deactivate the active proposal
    activeProposal = ActivatedProposal(0, 0);
}

Tools Used

Manual review

Recommened Mitigation Steps

Ensure activateProposal() and executeProposal() are being called by the proposal submitter. Alternatively, if the idea is to open the execution of a proposal by other parties, ensure it's getting called by trusted contract addresses or trusted EOAs.

fullyallocated commented 2 years ago

activateProposal() can only be called by the original submitter, it is expected for anyone to execute the proposal since the only security check is governance consensus

0xean commented 1 year ago

closing as invalid

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