code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L346

Vulnerability details

Impact

In NounsDAOLogicV1 and NounsDAOLogicV2, anyone can create proposal with the same actions as other proposal. In that case, if attacker calls cancel() on his proposal, then other proposal with the same action cannot be executed.

Proof of Concept

Function cancel() call cancelTransaction() on timelock

function cancel(uint256 proposalId) external {
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');

        Proposal storage proposal = _proposals[proposalId];
        require(
            msg.sender == proposal.proposer ||
                nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,
            'NounsDAO::cancel: proposer above threshold'
        );

        proposal.canceled = true;
        for (uint256 i = 0; i < proposal.targets.length; i++) {
            timelock.cancelTransaction(
                proposal.targets[i],
                proposal.values[i],
                proposal.signatures[i],
                proposal.calldatas[i],
                proposal.eta
            );
        }

        emit ProposalCanceled(proposalId);
    }

In cancelTransaction(), the hash of action is used to mark the action, Line 137

queuedTransactions[txHash] = false;

When other proposal is execute(), if its action is marked as false on timelock then executeTransaction() will reverted, resulting in execute() function revert. Line 152

require(queuedTransactions[txHash], "NounsDAOExecutor::executeTransaction: Transaction hasn't been queued.");

Tools Used

Manual Review

Recommended Mitigation Steps

Add proposal id in params when queueing and executing on timelock

csanuragjain commented 2 years ago

I think this will not work since eta will be different in both proposals (if you queue a proposal, eta changes and hash is calculated based on this new eta) Also if both proposals are queued simultaneously to get same eta then also it will fail since queue does not allow duplicate transactions

Shungy commented 2 years ago

Also if both proposals are queued simultaneously to get same eta then also it will fail since queue does not allow duplicate transactions

Right. This line would revert https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L313

Maybe the inability to add two proposals that have the same tx can be a low risk issue. But this submission fails to explore that.

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