Include missing _cancel(uint256 proposal_id) in GovernorTimelockControl

[sebsylvester]

🧐 Motivation
When using the Governor contract with the GovernorTimelockControl, one needs to override cancel(uint256 proposalId) because both Pending and Queued proposals must be cancellable. The problem is that it's not possible to invoke the inherited function _cancel(address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash from inside the overidden public cancel function.
)

📝 Details
A solution would be to let the GovernorTimelockControl override both the internal _cancel functions it inherits from Governor, in which case the four-argument function simply forwards to the single-argument function (as is the case in the Governor contract). The logic currently inside _cancel would move to the new single-argument function.

The downside is that it's a breaking change for anyone using the GovernorTimelockControl.
I will submit a PR with a link to this issue.

[Amxx]

Hello @sebsylvester, and thank you for raising that issue !

Currently we have two different signatures for the internal _cancel function:

function _cancel(address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash);
function _cancel(uint256 proposalId)

We can see that the first one is "just" an alias to the second one. The second one was introduced in #3983 when we wanted to introduce a public cancel function. We used this new signature because that is the signature that compound uses.

We missed something important when doing so!

Right now, in master, we have

flowchart LR
    id1{{"cancel(uint256)"}}
    id2{{"_cancel(uint256)"}}
    id3{{"_cancel(address[],uint256[],bytes[],bytes32)"}}
    id1 --> id2
    id3 --> id2

This is good, except the public function completely skips the "full params" internal one.

On the other hand, we have 2 other contracts (beside the core Governor.sol) that deal with cancelation:

• GovernorTimelockCompound
• GovernorTimelockControl

In both cases we override the "full params" internal one, which means that in both cases the custom logic (that cancels on the timelock) is NOT executed when calling the public function.

In the case of GovernorTimelockControl, we don't actually need the full parameters, so we could rewrite GovernorTimelockControl to overide the simple _cancel(uint256) variant. That would work well, except maybe for some additional overrides that would have to be specified. That is what #4053 does.

In the case of GovernorTimelockCompound its way more complex, because cancelling on a CompoundTimelock requires the parameters ... that we don't store. This means that there is no way to have the public cancel(uint256) trigger the CompoundTimelock.cancelTransaction with the data we currently store.

TLDR: the current implementation of the public cancel is not good. Fortunatelly it was never released but we need to fix that for 4.9

[Amxx]

Also, its worrying (but logic) that the tests did not catch that:

• the core tests where we added the public cancel(uint256) tests are not extensions aware.
• the extensions tests are done using the internal _cancel(address[],uint256[],bytes[],bytes32) function.

[frangio]

I see 2 viable options:

1. Change cancel(uint256) into cancel(address[],uint256[],bytes[],bytes32).
2. Add a storage mapping to GovernorTimelockCompound such that the proposalId can be mapped back to the proposal parameters.

We discussed how to implement option 2 without having redundant storage with GovernorCompatibilityBravo. Using a new module ("GovernorStorage") through inheritance is not a great option because it messes up the storage layout too much, an issue for upgradeable contracts. We could also do something a little nasty: have the mapping inline in GovernorTimelockCompound and define functions _storeProposal and getActions, such that if it is combined with the Bravo module, only one of the storage mappings gets used. This... works, but may be confusing.

Our best option might be option 1, with a roadmap to having cancel(uint256) in a good way in 5.0. For example, introducing GovernorStorage and using it in both TimelockCompound and Bravo modules.

[Amxx]

Thank you again for raising that issue. As a small token of appreciation we would like to award you a contributor gitpoap!

@gitpoap-bot @sebsylvester

[gitpoap-bot]

Congrats, @sebsylvester ! You've earned a GitPOAP for your contribution!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.