Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a governor extension that implements a proposal guardian #5303

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Nov 19, 2024

Fixes #5301

PR Checklist

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

@Amxx Amxx added this to the 5.3 milestone Nov 19, 2024
Copy link

changeset-bot bot commented Nov 19, 2024

⚠️ No Changeset found

Latest commit: e56c8b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Amxx Amxx requested a review from arr00 November 19, 2024 10:23
@arr00
Copy link
Contributor

arr00 commented Nov 20, 2024

#5301

@arr00
Copy link
Contributor

arr00 commented Nov 20, 2024

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
Copy link
Contributor

arr00 commented Nov 25, 2024

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

@arr00 arr00 force-pushed the feature/governor/security_council branch from 9bc440d to d613cc8 Compare December 13, 2024 18:21
@arr00 arr00 changed the title Add a governor extension that implements a security council Add a governor extension that implements a proposal guardian Dec 13, 2024
@arr00 arr00 requested a review from ernestognw December 18, 2024 17:48
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +32 to +38
// if there is no proposal guardian
// ... only the proposer can cancel
// ... no restriction on when the proposer can cancel
uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
address proposer = proposalProposer(proposalId);
if (caller != proposer) revert GovernorOnlyProposer(caller);
return _cancel(targets, values, calldatas, descriptionHash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default behavior in Governor is to only allow the proposer to cancel when the proposal is still pending. I don't think we want to allow proposers to cancel after that.

Also, if we want to keep the default behavior, wouldn't it be better to just?:

Suggested change
// if there is no proposal guardian
// ... only the proposer can cancel
// ... no restriction on when the proposer can cancel
uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
address proposer = proposalProposer(proposalId);
if (caller != proposer) revert GovernorOnlyProposer(caller);
return _cancel(targets, values, calldatas, descriptionHash);
return super.cancel(targets, values, calldatas, descriptionHash);

This would simplify this function imo

@arr00
Copy link
Contributor

arr00 commented Dec 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Governor cancel guardian feature
3 participants