search

code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

Proposal can be executed unlimited number of times #723

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L114-L130

Vulnerability details

There is no executed flag for the proposals, so one can be executed an arbitrary number of times.

This will have critical impact, for example a proposal to send out 10 ETH can be run 10 times, transferring 100 ETH. Apart from the malicious Owner case, this also can be a result of an operational mistake, for example repeatedly executing the same proposal over some time if off-chain records that it was already executed were omitted.

As the functions in questions are permissioned, setting the severity to be medium.

Proof of Concept

On queue() the proposal is marked for execution by setting the timestamps value for it:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L114-L130

    /// @notice Schedules a proposal for execution
    /// @param _proposalId The proposal id
    function queue(bytes32 _proposalId) external onlyOwner returns (uint256 eta) {
        // Ensure the proposal was not already queued
        if (isQueued(_proposalId)) revert PROPOSAL_ALREADY_QUEUED();

        // Cannot realistically overflow
        unchecked {
            // Compute the timestamp that the proposal will be valid to execute
            eta = block.timestamp + settings.delay;
        }

        // Store the timestamp
        timestamps[_proposalId] = eta;

        emit TransactionScheduled(_proposalId, eta);
    }

There is no marking for the executed proposal, the timestamps[proposalId] is just deleted, so queue() can be run again right after execute():

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L141-L172

    function execute(
        address[] calldata _targets,
        uint256[] calldata _values,
        bytes[] calldata _calldatas,
        bytes32 _descriptionHash
    ) external payable onlyOwner {
        // Get the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, _descriptionHash);

        // Ensure the proposal is ready to execute
        if (!isReady(proposalId)) revert EXECUTION_NOT_READY(proposalId);

        // Remove the proposal from the queue
        delete timestamps[proposalId];

        // Cache the number of targets
        uint256 numTargets = _targets.length;

        // Cannot realistically overflow
        unchecked {
            // For each target:
            for (uint256 i = 0; i < numTargets; ++i) {
                // Execute the transaction
                (bool success, ) = _targets[i].call{ value: _values[i] }(_calldatas[i]);

                // Ensure the transaction succeeded
                if (!success) revert EXECUTION_FAILED(i);
            }
        }

        emit TransactionExecuted(proposalId, _targets, _values, _calldatas);
    }

This queue() -> wait settings.delay -> execute() sequence can be run an arbitrary number of times.

Recommended Mitigation Steps

Consider introducing the flag, marking the executed proposals.

tbtstl commented 1 year ago

I believe this should be a High Risk finding

GalloDaSballo commented 1 year ago

The Warden has shown how, through a faulty assumption, anyone can execute a proposal multiple times.

Because no other indicator beside the timestamp, is checked for Executing and Queuing, a malicious attacker could repeatedly perform the same operation, impact can very likely lead to maximum loss (assume a swap that can be reliably repeated for front-running, or an allowance that may create opportunity for transfers)

Because of the elevated impact, I believe High Severity to be appropriate

jgeary commented 1 year ago

It is not possible to run into this via an operational mistake.

The malicious Owner case is the only case where this is possible, and to be explicit, this means governance would have to vote to transfer ownership of Treasury to an EOA or a contract that we didn't write.

Treasury.queue can only be called by Governor.queue, which validates that the proposal has state Succeeded. Similarly, Treasury.execute can only be called by Governor.execute, which sets the proposal state to Executed. So it is not possible to queue a proposal that has already been executed, and I have confirmed this with the following test (drop it into Gov.t.sol to run it):

    function test_ProposalVoteQueueExecutionQueueVulnerability() public {
        // propose
        mintVoter1();
        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
        bytes32 descriptionHash = keccak256(bytes("test"));
        vm.warp(1 days);
        vm.prank(voter1);
        governor.propose(targets, values, calldatas, "test");
        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        // vote
        vm.warp(block.timestamp + governor.votingDelay());
        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        // queue
        vm.warp(block.timestamp + governor.votingPeriod());
        vm.prank(voter1);
        governor.queue(proposalId);

        // execute
        vm.warp(block.timestamp + 2 days);
        governor.execute(targets, values, calldatas, descriptionHash);
        assertEq(auction.paused(), true);

        // queue again (fails)
        vm.prank(voter1);
        governor.queue(proposalId);
    }

The above test fails with the output [FAIL. Reason: PROPOSAL_UNSUCCESSFUL()].

That error comes from this line of Governor, which checks the state of the proposal. You can confirm that Governor.state is returning Executed in this case by replacing this line with revert PROPOSAL_DOES_NOT_EXIST(); and rerunning the test. The test will now fail with the output [FAIL. Reason: PROPOSAL_DOES_NOT_EXIST()] instead of the original output.

If there is a way to re-queue an already executed proposal, please provide a test that demonstrates it. Otherwise you can close the issue. Thank you!

GalloDaSballo commented 1 year ago

It is not possible to run into this via an operational mistake.

The malicious Owner case is the only case where this is possible, and to be explicit, this means governance would have to vote to transfer ownership of Treasury to an EOA or a contract that we didn't write.

Treasury.queue can only be called by Governor.queue, which validates that the proposal has state Succeeded. Similarly, Treasury.execute can only be called by Governor.execute, which sets the proposal state to Executed. So it is not possible to queue a proposal that has already been executed, and I have confirmed this with the following test (drop it into Gov.t.sol to run it):

    function test_ProposalVoteQueueExecutionQueueVulnerability() public {
        // propose
        mintVoter1();
        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
        bytes32 descriptionHash = keccak256(bytes("test"));
        vm.warp(1 days);
        vm.prank(voter1);
        governor.propose(targets, values, calldatas, "test");
        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        // vote
        vm.warp(block.timestamp + governor.votingDelay());
        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        // queue
        vm.warp(block.timestamp + governor.votingPeriod());
        vm.prank(voter1);
        governor.queue(proposalId);

        // execute
        vm.warp(block.timestamp + 2 days);
        governor.execute(targets, values, calldatas, descriptionHash);
        assertEq(auction.paused(), true);

        // queue again (fails)
        vm.prank(voter1);
        governor.queue(proposalId);
    }

The above test fails with the output [FAIL. Reason: PROPOSAL_UNSUCCESSFUL()].

That error comes from this line of Governor, which checks the state of the proposal. You can confirm that Governor.state is returning Executed in this case by replacing this line with revert PROPOSAL_DOES_NOT_EXIST(); and rerunning the test. The test will now fail with the output [FAIL. Reason: PROPOSAL_DOES_NOT_EXIST()] instead of the original output.

If there is a way to re-queue an already executed proposal, please provide a test that demonstrates it. Otherwise you can close the issue. Thank you!

Thank you for the well thought out reply, I'll definitely need to re-evaluate this report as well as a couple others relating to governance

GalloDaSballo commented 1 year ago

I'd like to apologize for jumping to conclusions too early.

Ultimately the POC offered by the Sponsor shows how, while a lack of validation is present in the Treasury contract, the normal setup, having Governor.queue used to queue the Proposal for execution will counteract the specific vulnerability shown by the Warden.

An argument can be made, that governance could delegate the Treasury to an EOA, however, that's not "normal behavior", not just that, handling ownership to an EOA for any contract would violate the in-scope expected settings for most contracts, and as such this report shouldn't warrant a Medium Severity.

I believe some of the observations made by the Warden are still correct, however I'll close this one as invalid in favour of similar observations made in: https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/510