code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

After two users' proposals receive enough endorsements, both users will call the following activateProposal function to activate their proposals. One user is incentivized to front-run the other user's activateProposal call by providing a slightly higher gas fee for activating own proposal first without any delay. After a proposal becomes active, the other proposal must wait for the GRACE_PERIOD time before it can be activated because only one proposal can be active at a time. After the front-running, the other user's activateProposal call would revert with gas being wasted while she or he has to wait.

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

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

Proof of Concept

Please append the following test in src\test\policies\Governance.t.sol. This test will pass to demonstrate the described scenario.

    function testScenario_UserActivatesByFrontRunning() public {
        // voter1 submits proposal 1
        Instruction[] memory instructions_1 = new Instruction[](1);
        instructions_1[0] = Instruction(Actions.ActivatePolicy, address(newProposedPolicy));
        vm.prank(voter1);
        governance.submitProposal(instructions_1, "proposalName 1", "This is the proposal URI 1");

        // voter2 submits proposal 2
        Instruction[] memory instructions_2 = new Instruction[](1);
        instructions_2[0] = Instruction(Actions.ActivatePolicy, address(governance));
        vm.prank(voter2);
        governance.submitProposal(instructions_2, "proposalName 2", "This is the proposal URI 2");

        // voter2 and voter4 endorse proposal 2
        vm.prank(voter2);
        governance.endorseProposal(2);
        vm.prank(voter4);
        governance.endorseProposal(2);

        // voter1 and voter3 endorse proposal 1
        vm.prank(voter1);
        governance.endorseProposal(1);
        vm.prank(voter3);
        governance.endorseProposal(1);

        // voter1 wants proposal 1 to be activated first so proposal 1 does not need to wait for the GRACE_PERIOD time.
        // The code lines below simulate that voter1's activateProposal call front-runs voter2's activateProposal call.
        vm.prank(voter1);
        governance.activateProposal(1);

        // after voter1's activateProposal call front-runs voter2's, voter2's activateProposal call reverts
        vm.expectRevert(ActiveProposalNotExpired.selector);
        vm.prank(voter2);
        governance.activateProposal(2);
    }

Tools Used

VSCode

Recommended Mitigation Steps

Please consider supporting activation and voting for multiple proposals at a time. If this is not possible, please consider using flashbots for keeping users' activateProposal call transactions away from the mempool.

fullyallocated commented 2 years ago

Duplicate of #273

0xean commented 2 years ago

I don't think this is a dupe of #273 - but I also don't think its really medium severity either. The DOS attack is a separate problem, this warden is simply highlighting that two approved proposals can "fight" to become the next active proposal. While this is true, I am not sure why alone this is actually an attack vector if all else is functioning correctly in the governance process. The community would still have had to endorse them knowing that a vote on one could delay another.

Downgrading to QA, marking as dupe of wardens QA report #457

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