Add a governor extension that implements a proposal guardian by Amxx · Pull Request #5303 · OpenZeppelin/openzeppelin-contracts (original) (raw)

@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.