sherlock-admin2 commented 4 months ago

auditsbyradev

high

`NounsDAOProposals.sol` - The `GRACE_PERIOD` adjustments can lead to reactivate and execute expired Pproposals

Summary

The NounsDAOProposals.sol contract is a crucial component of the Nouns DAO governance system, enabling token holders to propose, vote on, and execute changes within the ecosystem. It facilitates a structured proposal process, from initiation and voting phases to final execution or cancellation. Each proposal goes through a lifecycle, including states such as Active, Succeeded, Defeated, Queued, Executed, Expired, and Canceled.

Vulnerability Detail

The problem is the inability to cancel proposals that have reached the Expired state. The cancel() function is designed to prevent cancellation of proposals in their final states, including Canceled, Defeated, Expired, Executed, and Vetoed. However, treating Expired as a final state overlooks the potential for the GRACE_PERIOD parameter of the timelock to be adjusted due to upgrades, which could retroactively alter the state of a proposal from Expired back to Queued.

Exploit Scenario: Manipulating Proposal Outcomes via GRACE_PERIOD Adjustment in NounsDAOProposals.sol Contract

Background Kevin is a member of the Nouns DAO and has submitted a high-stakes proposal (Proposal A) to invest a significant amount of the DAO's funds into a new, promising DeFi protocol. The proposal passes and enters the Queued state, awaiting execution. However, due to a sudden downturn in the DeFi protocol's security, it's discovered that the protocol is at risk of being exploited.

Initial State Proposal A is now considered risky, and the majority of the DAO members believe it should not be executed. As the execution date approaches, due to unforeseen circumstances, the proposal fails to get executed and subsequently moves to the Expired state based on the current GRACE_PERIOD setting of the timelock.

The Vulnerability

Eva, a malicious actor with administrative access to the timelock contract or significant influence in the DAO, proposes to extend the GRACE_PERIOD due to a separate, seemingly unrelated issue. This proposal (Proposal B) is quickly passed and executed, extending the GRACE_PERIOD significantly.
Due to the extension of the GRACE_PERIOD, Proposal A's state changes from Expired back to Queued. Most DAO members are unaware of this change or its implications.
Eva then covertly funds the DAO's timelock with enough assets to meet the requirements of Proposal A. Since it's back in the Queued state and the community is unaware, he executes Proposal A, directing a large sum of the DAO's funds to the now-compromised DeFi protocol.
The compromised DeFi protocol is exploited soon after, resulting in the loss of all funds sent there by the Nouns DAO as a result of Proposal A's execution.

Impact

This vulnerability poses a risk of unintended execution of proposals that were previously considered Expired but became executable due to changes in the GRACE_PERIOD. In scenarios where the financial or operational context has changed significantly since the proposal's expiration (e.g., a proposed DeFi investment becoming unviable due to a protocol hack), the inability to cancel such proposals can lead to substantial losses or adverse outcomes for the DAO.

Summarized Impact:

Governance Manipulation
Financial Risks
Legal and Regulatory Concerns
Operational Disruptions (Workflow Interruptions or Policy Inconsistencies)

Code Snippet

NounsDAOProposals.sol#cancel()

/**
 * @notice Cancels a proposal only if sender is the proposer or a signer, or proposer & signers voting power
 * dropped below proposal threshold
 * @param proposalId The id of the proposal to cancel
 */
function cancel(NounsDAOTypes.Storage storage ds, uint256 proposalId) external {
    NounsDAOTypes.ProposalState proposalState = stateInternal(ds, proposalId);
    if (
        proposalState == NounsDAOTypes.ProposalState.Canceled ||
        proposalState == NounsDAOTypes.ProposalState.Defeated ||
        proposalState == NounsDAOTypes.ProposalState.Expired ||
        proposalState == NounsDAOTypes.ProposalState.Executed ||
        proposalState == NounsDAOTypes.ProposalState.Vetoed
    ) {
        revert CantCancelProposalAtFinalState();
    }

    NounsDAOTypes.Proposal storage proposal = ds._proposals[proposalId];
    address proposer = proposal.proposer;
    NounsTokenLike nouns = ds.nouns;

    uint256 votes = nouns.getPriorVotes(proposer, block.number - 1);
    bool msgSenderIsProposer = proposer == msg.sender;
    address[] memory signers = proposal.signers;
    for (uint256 i = 0; i < signers.length; ++i) {
        msgSenderIsProposer = msgSenderIsProposer || msg.sender == signers[i];
        votes += nouns.getPriorVotes(signers[i], block.number - 1);
    }

    require(
        msgSenderIsProposer || votes <= proposal.proposalThreshold,
        'NounsDAO::cancel: proposer above threshold'
    );

    proposal.canceled = true;
    INounsDAOExecutor timelock = getProposalTimelock(ds, proposal);
    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 NounsDAOEventsV3.ProposalCanceled(proposalId);
}

Tool used

Manual Review

Recommendation

Several things can be done here:

Modify the cancel() function to allow for the cancellation of Expired proposals. This change acknowledges that the Expired state may not be final if the GRACE_PERIOD is adjustable.
Implement an explicit expiration time field in the proposal structure. This field should be set during the queueing process based on the current GRACE_PERIOD, ensuring that the proposal expiration is clearly defined and immutable post-queueing.
Implement a mechanism to notify DAO members of changes to the timelock's GRACE_PERIOD, allowing for informed decisions regarding ongoing and future proposals.

sherlock-admin2 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

owners are trusted + if the proposal is secceeded for extending the grace period; how come most member are unaware of it?

radeveth commented 4 months ago

The issue is about how adjustments to the GRACE_PERIOD can inadvertently resurrect and execute proposals previously deemed expired, underpins a significant design oversight in the governance model of the Nouns DAO protocol.

Validation of the Vulnerability

The possibility of GRACE_PERIOD adjustments leading to the reactivation and execution of expired proposals directly challenges the principles of transparent, immutable, and predictable governance. Such a mechanism inadvertently grants undue advantage to those within administrative reach or with in-depth system knowledge, enabling potential manipulation of the DAO's decision-making process.

In the depicted scenario, the unauthorized redirection of substantial DAO funds to a compromised DeFi protocol following an unjust GRACE_PERIOD extension unveils a stark vulnerability. It portrays not just a theoretical loophole but a practical exploit pathway that could lead to significant financial losses and tarnish the DAO's credibility and trust among its members.

The critique that most members being unaware of a successfully executed proposal to extend the GRACE_PERIOD undermines the DAO’s commitment to consensus-driven governance. In reality, the dynamic nature of member engagement and the reliance on digital notification systems mean that critical governance changes can indeed go unnoticed by a significant portion of the membership until it's too late.

While trust in owners or administrators is foundational, the ethos of decentralized governance emphasizes trust in code as the ultimate arbiter. The blockchain ethos advocates for systems designed to minimize trust assumptions and mitigate central points of failure or manipulation, a principle this vulnerability directly contravenes.

Asserting that the majority of DAO members should be aware of a proposal to extend the GRACE_PERIOD overlooks practical engagement challenges within large, decentralized communities. Information asymmetry, varying levels of activity, and engagement, as well as reliance on third-party platforms for updates, can result in significant governance actions slipping past the collective radar of the community.

Additionally, it's important to highlight that a proposal doesn't have to have malicious intent, as illustrated in the scenario where a standard proposal becomes expired due to external factors preventing its execution (such as insufficient contract balance). It's worth noting that adjustments to the GRACE_PERIOD (for instance, through a timelock upgrade) could result in a proposal transitioning from an Expired status back to Queued. This could lead to the unwelcome situation where a proposal, believed by the DAO to be dormant and not executable, suddenly becomes active and is executed without anticipation.

Arabadzhiew commented 4 months ago

Escalate

sherlock-admin2 commented 4 months ago

Escalate

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

WangSecurity commented 4 months ago

But as I understand, GRACE_PERIOD is constant, i.e. cannot be changed. Can you elaborate on how it can be changed?

radeveth commented 4 months ago

But as I understand, GRACE_PERIOD is constant, i.e. cannot be changed. Can you elaborate on how it can be changed?

As I described in my report, it's indeed possible for a proposal to move from an Expired state back to Queued due to a change in the executor's GRACE_PERIOD (GRACE_PERIOD parameter of the timelock to be adjusted due to upgrades). Once the GRACE_PERIOD of the timelock is changed, the state of the proposal may also be changed, so Expired is not the final state. Yes, this us rare event, but low likelihood + high impact is usually considered M, which is an edge case that corresponds to medium risk.

WangSecurity commented 4 months ago

What do you mean by upgrades? How's the timelock contract can be upgraded? You mean by successful proposals? Also, by the way, GRACE_PERIOD is inside the NounsDAOExecutor contract and in NounsDAOExecutorV2 it's extended to 21 days. Also there are lots of variables like malicious admins or majority of governance. Also I don't understand how users can be unaware of the proposal going through and all this factors make me think that this scenario is in fat infeasible honestly.

radeveth commented 4 months ago

Here's a streamlined explanation focused solely on the exploit scenario and the bug's validity:

The exploit scenario depends on the ability to change the GRACE_PERIOD through a governance proposal. If the GRACE_PERIOD is extended, proposals previously marked as Expired could revert to Queued, making them executable again. This can occur without broad community awareness, leading to the unexpected execution of proposals thought to be dead. For instance, a proposal to allocate significant DAO funds, which expires due to not being executed in time, could suddenly become active again and executed following a GRACE_PERIOD extension, potentially redirecting funds under changed circumstances.

This vulnerability is valid because it exploits the DAO's adaptive governance system, designed to evolve through community consensus. Adjusting parameters like the GRACE_PERIOD is a built-in feature for such evolution. However, this flexibility can inadvertently alter the lifecycle of proposals in a way that could compromise the DAO's integrity and the community's trust, especially if it leads to the execution of proposals under conditions different from those under which they were voted on.

Additionally, it's important to highlight that a proposal doesn't have to have malicious intent, as illustrated in the scenario where a standard proposal becomes expired due to external factors preventing its execution (such as insufficient contract balance). It's worth noting that adjustments to the GRACE_PERIOD (for instance, through a timelock upgrade) could result in a proposal transitioning from an Expired status back to Queued. This could lead to the unwelcome situation where a proposal, believed by the DAO to be dormant and not executable, suddenly becomes active and is executed without anticipation.

@WangSecurity

WangSecurity commented 4 months ago

There are a couple of problems that I don't like here:

If the only way to change GRACE_PERIOD is only via upgrading NounsDAOExecutor contract, then yes we have it upgraded from 14 days to 21 days inside NounsDAOExecutorV2. But both NounsDAOExecutor and NounsDAOExecutorV2 are out of scope as well as timelock contract.
Can you send a link to the code where expired proposals can be set back to queued? How it can "suddenly become active again" as you say?

radeveth commented 4 months ago

Hey, @WangSecurity! You raises valid concerns about the mechanism of changing the GRACE_PERIOD and its impact on the lifecycle of governance proposals within the Nouns DAO. However, there is a misunderstanding about the nature of how governance parameters like the GRACE_PERIOD can influence proposal states beyond simple contract upgrades.

Firstly, it's important to recognize that the GRACE_PERIOD is a parameter that can be adjusted not just through contract upgrades but also potentially through governance actions that directly or indirectly affect the execution logic in the timelock or executor contracts.

The critical piece of code to consider here is in the stateInternal function within the NounsDAOProposals.sol, which determines the state of proposals based on various conditions including the GRACE_PERIOD. Here’s a simplified look at the relevant code:

function stateInternal(NounsDAOTypes.Storage storage ds, uint256 proposalId) internal view returns (NounsDAOTypes.ProposalState) {
    NounsDAOTypes.Proposal storage proposal = ds._proposals[proposalId];
    if (block.timestamp >= proposal.eta + getProposalTimelock(ds, proposal).GRACE_PERIOD()) {
        return NounsDAOTypes.ProposalState.Expired;
    } else {
        return NounsDAOTypes.ProposalState.Queued;
    }
}

code link

This function uses getProposalTimelock(ds, proposal).GRACE_PERIOD() to determine whether a proposal is expired or still queued. If the GRACE_PERIOD is extended, this function will recalculate the state of a proposal based on its execution attempt time (eta) and the new GRACE_PERIOD. This means a proposal initially marked as Expired could revert to Queued if the GRACE_PERIOD is increased through a governance decision affecting the timelock's settings.

Regarding the "suddenly become active again" concern, this is precisely the scenario where an Expired proposal, due to an extended GRACE_PERIOD, becomes executable again because its new expiration check (proposal.eta + new GRACE_PERIOD) has not yet been surpassed. This transition is seamless and automated based on the contract’s logic reacting to the updated GRACE_PERIOD.

The integrity of governance relies heavily on the predictability and stability of proposal outcomes based on the conditions under which they were voted. Any change in parameters that can retroactively alter a proposal's state poses a governance risk—potentially leading to decisions being executed under different conditions from those under initial consideration.

This vulnerability rooted in the practical mechanics of how state transitions are managed within the Nouns DAO proposals system. To enhance the protocol security, it is essential to address this adaptability in governance parameters with caution and provide mechanisms to either lock critical parameters post-voting or ensure transparent communication and revalidation of conditions when significant governance parameters are altered.

Have a good one, sir!

WangSecurity commented 4 months ago

The problem is that the function you show has far more checks:

    function stateInternal(
        NounsDAOTypes.Storage storage ds,
        uint256 proposalId
    ) internal view returns (NounsDAOTypes.ProposalState) {
        require(ds.proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
        NounsDAOTypes.Proposal storage proposal = ds._proposals[proposalId];

        if (proposal.vetoed) {
            return NounsDAOTypes.ProposalState.Vetoed;
        } else if (proposal.canceled) {
            return NounsDAOTypes.ProposalState.Canceled;
        } else if (block.number <= proposal.updatePeriodEndBlock) {
            return NounsDAOTypes.ProposalState.Updatable;
        } else if (block.number <= proposal.startBlock) {
            return NounsDAOTypes.ProposalState.Pending;
        } else if (block.number <= proposal.endBlock) {
            return NounsDAOTypes.ProposalState.Active;
        } else if (block.number <= proposal.objectionPeriodEndBlock) {
            return NounsDAOTypes.ProposalState.ObjectionPeriod;
        } else if (isDefeated(ds, proposal)) {
            return NounsDAOTypes.ProposalState.Defeated;
        } else if (proposal.eta == 0) {
            return NounsDAOTypes.ProposalState.Succeeded;
        } else if (proposal.executed) {
            return NounsDAOTypes.ProposalState.Executed;
        } else if (block.timestamp >= proposal.eta + getProposalTimelock(ds, proposal).GRACE_PERIOD()) {
            return NounsDAOTypes.ProposalState.Expired;
        } else {
            return NounsDAOTypes.ProposalState.Queued;
        }
    }

As I understand, before checking the GRACE_PERIOD it will revert cause else if (proposal.canceled) or else if (block.number <= proposal.endBlock). Moreover it can vetoed and it will revert at ther very first clause if (proposal.vetoed)

radeveth commented 4 months ago

While these checks are indeed present and functional, they do not address the core issue raised by the vulnerability related to the GRACE_PERIOD adjustment.

Your argument assumes that the proposal will be either canceled or vetoed and thus won’t reach the check concerning the GRACE_PERIOD. However, the vulnerability specifically concerns scenarios where a proposal naturally expires without these interventions and then gets reactivated due to a GRACE_PERIOD extension. Here’s why your checks don't negate the vulnerability:

Scenario: The vulnerability focuses on proposals that reach an "Expired" state naturally and could be affected by a later adjustment to the GRACE_PERIOD. This adjustment could retroactively push the proposal back into a "Queued" state, which your checks prior to the GRACE_PERIOD evaluation do not address.

Let’s break down the stateInternal function's logic to clarify the sequence that leads to the vulnerability:

    function stateInternal(
        NounsDAOTypes.Storage storage ds,
        uint256 proposalId
    ) internal view returns (NounsDAOTypes.ProposalState) {
        require(ds.proposalCount >= proposalId, 'NounsDAO::state: invalid proposal id');
        NounsDAOTypes.Proposal storage proposal = ds._proposals[proposalId];

        if (proposal.vetoed) {
            return NounsDAOTypes.ProposalState.Vetoed;
        } else if (proposal.canceled) {
            return NounsDAOTypes.ProposalState.Canceled;
        } else if (block.number <= proposal.updatePeriodEndBlock) {
            return NounsDAOTypes.ProposalState.Updatable;
        } else if (block.number <= proposal.startBlock) {
            return NounsDAOTypes.ProposalState.Pending;
        } else if (block.number <= proposal.endBlock) {
            return NounsDAOTypes.ProposalState.Active;
        } else if (block.number <= proposal.objectionPeriodEndBlock) {
            return NounsDAOTypes.ProposalState.ObjectionPeriod;
        } else if (isDefeated(ds, proposal)) {
            return NounsDAOTypes.ProposalState.Defeated;
        } else if (proposal.eta == 0) {
            return NounsDAOTypes.ProposalState.Succeeded;
        } else if (proposal.executed) {
            return NounsDAOTypes.ProposalState.Executed;
        } else if (block.timestamp >= proposal.eta + getProposalTimelock(ds, proposal).GRACE_PERIOD()) {
            return NounsDAOTypes.ProposalState.Expired;
        } else {
            return NounsDAOTypes.ProposalState.Queued;
        }
    }

If a proposal is neither vetoed nor canceled and the block.timestamp calculation falls within the extended GRACE_PERIOD, the proposal, previously marked as Expired, will revert to a Queued state.

Your concerns do not directly address the issue where an Expired proposal could be unintentionally reactivated due to an extended GRACE_PERIOD.

WangSecurity commented 4 months ago

But in that case, can other users cancel or veto the proposal even if it's back from expired to queued?

eladmallel commented 4 months ago

this is a known issue from our previous audit, and therefore we think should remain excluded.

radeveth commented 4 months ago

known issues

cvetanovv commented 4 months ago

According to the Sherlock rules issues that have been reported in previous audits of the protocol are also known issues and are considered invalid.

This is the case here - Reference

I planning to reject the escalation and leave the issue as it is.

Evert0x commented 4 months ago

Result: Invalid Unique

sherlock-admin3 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

Arabadzhiew: rejected

sherlock-audit / 2024-03-nouns-dao-2-judging

auditsbyradev - `NounsDAOProposals.sol` - The `GRACE_PERIOD` adjustments can lead to reactivate and execute expired Pproposals #37

`NounsDAOProposals.sol` - The `GRACE_PERIOD` adjustments can lead to reactivate and execute expired Pproposals

Summary

Vulnerability Detail

Exploit Scenario: Manipulating Proposal Outcomes via GRACE_PERIOD Adjustment in NounsDAOProposals.sol Contract

Impact

Code Snippet

Tool used

Recommendation

Validation of the Vulnerability

sherlock-audit / 2024-03-nouns-dao-2-judging

auditsbyradev - `NounsDAOProposals.sol` - The `GRACE_PERIOD` adjustments can lead to reactivate and execute expired Pproposals #37

NounsDAOProposals.sol - The GRACE_PERIOD adjustments can lead to reactivate and execute expired Pproposals

Summary

Vulnerability Detail

Exploit Scenario: Manipulating Proposal Outcomes via GRACE_PERIOD Adjustment in NounsDAOProposals.sol Contract

Impact

Code Snippet

Tool used

Recommendation

Validation of the Vulnerability

`NounsDAOProposals.sol` - The `GRACE_PERIOD` adjustments can lead to reactivate and execute expired Pproposals