Old proposals can be executed if the quorum is lowered

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L396-L397

Vulnerability details

Impact

Old proposals below the quorum can be executed in the future when the quorum is lowered. This may cause unwanted and unexpected effects on the ecosystem.

Proof of Concept

Users can create various proposals using _possiblyCreateProposal. Proposals can be executed and deleted with finalizeBallot. Initially, canFinalizeBallot is called to check if the proposal passed the quorum. If not, canFinalizeBallot returns false, the proposal remains active, regardless of the votes, and cannot be closed.

However, this can lead to an issue where, if the quorum is lowered, some proposals that didn't get the required number of votes can be executed upon the lowering.

Example:

• Assume quorum is at 20%.

1. Proposal to SEND_SALT to an investor's address gets 60% YES and 40% NO but doesn't pass the quorum threshold - 60% of total staked salt (requires 3x the quorum).
2. After a few months or a year, users decide to lower the quorum.
3. Vote after vote passes, lowering the quorum to 10%.
4. Now the old proposal to SEND_SALT returns true from canFinalizeBallot as it has enough votes.
5. A user executes the old proposal and sends the salt to the appointed address.
6. The address is no longer active, and the salt is stuck there.

This can be a common case, and it can happen with all kinds of proposals.

Tools Used

Manual review.

Recommended Mitigation Steps

Either set an expiration date for every proposal or modify the structure of finalizeBallot to include markBallotAsFinalized:

function finalizeBallot(uint256 ballotID) external nonReentrant {
-    require(proposals.canFinalizeBallot(ballotID), "The ballot is not yet able to be finalized");

+   if (proposals.canFinalizeBallot(ballotID)) {
        Ballot memory ballot = proposals.ballotForID(ballotID);

        if (ballot.ballotType == BallotType.PARAMETER) {
            _finalizeParameterBallot(ballotID);
        } else if (ballot.ballotType == BallotType.WHITELIST_TOKEN) {
            _finalizeTokenWhitelisting(ballotID);
        } else {
            _finalizeApprovalBallot(ballotID);
        }
+       proposals.markBallotAsFinalized(ballotID);
    }
}

Assessed type

Error

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

This is acceptable behavior. Part of the reason to lower quorum requirements will be to push through old proposals.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Downgrading to QA as this is a possible design although it requires being careful when changing the quorum

[c4-judge]

Picodes marked the issue as grade-b

[0x3b33]

Thanks for the fast judging @Picodes.

Short summary - The issue above describes how if a quorum is lowered old proposals will be executed, because now they can pass the much lower quorum.

When this issue is showed it can be considered as obvious that such thing is expected, however it can still cause damage to the protocol, as most failed proposals should never be executed, they have a reason to be failed. Such proposals can be either malicious, wasteful or just outdated. They could have been great options at the time, but now when the quorum is lowered to send funds to dead contracts.

Bellow is a reference describing that OZ found in GovernorVotesQuorumFraction the same issue in their code (marked as HIGH) and fixed it, as it can cause sudden old proposals to pass:

https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-xrc4-737v-9q75

Thanks for your time!

[shealtielanz]

Hey I think @0x3b33 is correct here as He is showing the “effects” of the “acceptable behavior” His explanations right here summarizes everything there’s to say.. And OZ had found similar issue too. Please look into it sir Thank you for your time @Picodes

[Picodes]

The impact here at most is that you cannot decrease the quorum because it would lead to this

[Picodes]

I feel in OZ's case it's understandable to flag this as high because it wasn't the intended design but here if it's what the sponsor wanted to do we have no reason to flag this as valid

[0x3b33]

The impact here at most is that you cannot decrease the quorum because it would lead to this

The actual impact is that a faulty proposal will pass when the quorum is lowered. This can lead to loss of funds, faulty upgrades, improper allocations and many other issues.

The sponsor explained that this issue is known, however I think if it was know it should have been described in the docs or in the Readme for the project.

I still think this is an issue, it's acceptable if the sponsor doesn't want it to be fixed, however that doesn't stop it from being marked as med and acknowledged.

[Picodes]

I can understand your reasoning. Basically, you are saying that the current design is dangerous because it could lead to a mistake, or that the protocol could be stuck and not be able to lower the quorum. For the first scenario (the DAO making a mistake and a faulty proposal passing) it would be QA because that's usually how configuration mistakes are handled. For the second scenario, it depends on the intent of the sponsor. The goal is to determine if "a functionality of the protocol is broken".

If the original intent was to be able to lower the quorum to pass old proposals, then I still don't feel that this fits the definition of Medium severity "assets not at direct risk, but the function of the protocol or its availability could be impacted"

[Picodes]

@0x3b33 after discussing this with an other judge, I think it makes sense to also flag this issue as a valid duplicate of #362. Indeed, the main issue here is also the fact that proposals below quorum can't be finalized even if they have expired.

[0x3b33]

Thank you for spending the extra time, I really appreciate it.