Add a governor extension that implements a proposal guardian

[Amxx]

Fixes #5301

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 276185d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[arr00]

#5301

[arr00]

Looks good other than some comments. Would it make sense to call it something like GovernorCancelCouncil to be more explicit? I could imagine daos having multiple different "security" related councils.

[arr00]

Compound Governance already has a feature like this where the council is called the proposalGuardian. If we aren't too against the wording we could reuse it.

https://github.com/compound-finance/compound-governance/blob/15614c913d548c7a7a4a3f3543069562d120eb7d/contracts/GovernorBravoDelegate.sol#L768-L784

[ernestognw]

What's the rationale to allow a proposer to cancel a proposal at any time?

I think the contract would be simpler if it just focus on allowing the guardian to cancel at any time and otherwise fallback to their original behavior with super.

[arr00]

What's the rationale to allow a proposer to cancel a proposal at any time?

I think the contract would be simpler if it just focus on allowing the guardian to cancel at any time and otherwise fallback to their original behavior with super.

The inspiration for this PR is in #5260 which was then replaced by #5301. There is some context missing but will try to give a TLDR.

• The assumption that governance correctly identifies all faulty proposals is flawed.
  • There are times that a proposal needs to be created strategically but then need to be cancelled.
  • Proposals very often have to be cancelled after the pending period (see compound governance)
• Compound governance allows cancellation by proposers at any point which is what Enable proposal cancelation by proposer after pending period #5260 advocated for
• @Amxx pointed out that this gives a possibly exploitive permission to the proposer over the community as they could manipulate governance by cancelling at any point.
  • Say governance wants to do A but proposer X wants A to be delayed by a week. Proposer X would frontrun another proposer to propose A so other community members wouldn't propose. Proposer X cancels proposal A right before execution, requiring a whole new proposal process.
• While I don't know of any occurrences of the above, the possibility makes it preferred that the cancellation comes from a trusted community proposal guardian (hopefully a multisig)
  • If governance does not actually set the cancel guardian, it is definitely preferred to pass this authority over to the proposers.

[ernestognw]

@Amxx pointed out that this gives a possibly exploitive permission to the proposer over the community as they could manipulate governance by cancelling at any point.

Right, this was my interpretation as well. I also don't have any concrete example of such an exploit.

Getting back to the whole idea of enhancing cancellation capabilities, one concern I have with this design is the pattern:

if (...)  {
  ...
  _cancel(...);
} else if (...) {
  ...
  _cancel(...);
} else {
  super.cancel();
}

The reasoning is that we're allowing to bypass cancel side effects. By not calling super in certain situations (i.e. whether the caller is the proposer or the guardian). This is why I wanted to reduce the branches as much as possible

Consider a contract that inherits from GovernorProposalGuardian, but also inherits from an abstract contract that counts how many cancellations have been made (e.g. GovernorCancellationCounter), so that:

contract GovernorCancellationCounter {
  uint256 cancellations;

  function cancel(...) public override virtual returns (uint256) {
      cancellations++;
      super.cancel(...);
    }
}

contract MyGovernor is ..., GovernorCancellationCounter, GovernorProposalGuardian { ... } // Order is important

In this case, the count would be missing the internal branches that don't call super. Although it's easier to override _cancel, I would say there may be legitimate reasons to prefer cancel.

I'd feel more comfortable if Governor implements an internal:

function _validateCancel(...) internal virtual {
    // Current logic
    uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
    _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
    if (_msgSender() != proposalProposer(proposalId)) {
        revert GovernorOnlyProposer(_msgSender());
    }
}

...


function cancel(...) public virtual returns (uint256) {
  _validateCancel();
  return _cancel(targets, values, calldatas, descriptionHash);
}

I think this way users can override _validateCancel and we can use it in GovernorProposalGuardian to implement the newer branches without missing the super.cancel.

[arr00]

Should the proposal guardian have the ability to give up this role? I could see an argument in either direction but think it's worth a brief discussion before merging.

For:
The proposal guardian has a critical role in the governance system--they need to catch potentially broken proposals. These proposals can cause significant issues if they go unmediated. The designers of the governance system explicitly defined a proposal guardian functionality so they want these types of proposals to be cancelled. What if for some reason, the proposal guardian is no longer able to act in the role? They need to be able to transition this role to proposers until governance has time to update the role to a new address.

Against:
Proposals should belong to the community after the pending period--if a malicious actor has the ability to cancel a proposal at any time, they may do so at the last second and stall governance. The proposal guardian can signal to governance that they would like to step down and governance should swap them out in a timely manner.

I think I lean towards for since the frequency of broken proposals out in the wild is quite high. If the proposal guardian isn't able to act effectively, the DAO needs the proposers to have this ability.

[Amxx]

When there is a guardian, proposer still have the ability to cancel "early".
Additionally, if the guardian is unable to act, malicious proposal can still be "blocked" by voters saying no.

IMO:

• if this feature is present, "disabling" it is possible through an override that always revert, but the function remains present in the ABI (and bytecode)
• if this feature is not present, its quite easy to add

function transferProposalGuardian(address newProposalGuardian) public {
    require(_msgSender == proposalGuardian());
    _setProposalGuardian(newProposalGuardian);
}

When in doubts, or when we think not everyone needs something, I'm always in favor of not forving the feature. In that particular case, the Governor is already close to the limit en terms of bytecode size, so I would avoid increassing it for something users may not want/need.

[ernestognw]

LGTM