code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L285-L292 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L323-L330 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L346-L356 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L374-L382 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOInterfaces.sol#L286

Vulnerability details

Impact

The functions queue(), execute(), cancel() and veto() contain unbounded loops, which can cause transactions to consume more gas than the block limit (run out of gas) and revert. Since these functions are critical for the proposals flow, this could impact the availability of the protocol.

Proof of Concept

The loops inside the functions mentioned above are iterating through proposals.targets.length and targets is a dynamic-size array of addresses.

File: contracts/governance/NounsDAOLogicV2.sol
function queue(uint256 proposalId) external {
    require(
        state(proposalId) == ProposalState.Succeeded,
        'NounsDAO::queue: proposal can only be queued if it is succeeded'
    );
    Proposal storage proposal = _proposals[proposalId];
    uint256 eta = block.timestamp + timelock.delay();
    for (uint256 i = 0; i < proposal.targets.length; i++) {

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L285-L292

File: contracts/governance/NounsDAOLogicV2.sol
function execute(uint256 proposalId) external {
    require(
        state(proposalId) == ProposalState.Queued,
        'NounsDAO::execute: proposal can only be executed if it is queued'
    );
    Proposal storage proposal = _proposals[proposalId];
    proposal.executed = true;
    for (uint256 i = 0; i < proposal.targets.length; i++) {

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L323-L330

File: contracts/governance/NounsDAOLogicV2.sol
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++) {

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

File: contracts/governance/NounsDAOLogicV2.sol
function veto(uint256 proposalId) external {
    require(vetoer != address(0), 'NounsDAO::veto: veto power burned');
    require(msg.sender == vetoer, 'NounsDAO::veto: only vetoer');
    require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');

    Proposal storage proposal = _proposals[proposalId];

    proposal.vetoed = true;
    for (uint256 i = 0; i < proposal.targets.length; i++) {

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L374-L382

File: contracts/governance/NounsDAOInterfaces.sol#L286
286: address[] targets;

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOInterfaces.sol#L286

Through a user mismake or through an attack vector, a proposal targets grows too large and interating over all of it's items consumes an amount gas larger than what's available. (Note: An user mistake would be more likelly than an attack vector, because the attacker would only harm his own proposal. However, the possibility of an attack vector should not ignored).
The operations queue, execute, cancel and veto will not be avaiable for such propotal.

Tools Used

Manual review

Recommended Mitigation Steps

One way to solve this issue is to limit the amount of targets that can be added of each proposal.

Another solution would be add a slice functionality into the functions queue(), execute(), cancel() and veto(), to enable iterations on only a subsection of targets, instead of the entire array. This could be done by adding a startIndex and endIndex as function arguments.

catchup99 commented 2 years ago

targets.length is limited to proposalMaxOperations when proposing the proposal. https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L208 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L91-L92 So I think the number of targets; hence the boundary of the for loop, is already limited.

davidbrai commented 2 years ago

agree with @catchup99 don't see a scenario where these functions would run out of gas

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