diff --git a/.changeset/flat-deers-end.md b/.changeset/flat-deers-end.md new file mode 100644 index 000000000..61895f2cf --- /dev/null +++ b/.changeset/flat-deers-end.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Governor`: add a public `cancel(uint256)` function. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 8235fb5ec..f82f9fea8 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -40,6 +40,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive Timers.BlockNumber voteEnd; bool executed; bool canceled; + address proposer; } string private _name; @@ -95,9 +96,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive return interfaceId == (type(IGovernor).interfaceId ^ + this.cancel.selector ^ this.castVoteWithReasonAndParams.selector ^ this.castVoteWithReasonAndParamsBySig.selector ^ this.getVotesWithParams.selector) || + interfaceId == (type(IGovernor).interfaceId ^ this.cancel.selector) || interfaceId == type(IGovernor).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); @@ -248,8 +251,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { + address proposer = _msgSender(); + require( - getVotes(_msgSender(), block.number - 1) >= proposalThreshold(), + getVotes(proposer, block.number - 1) >= proposalThreshold(), "Governor: proposer votes below proposal threshold" ); @@ -267,10 +272,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive proposal.voteStart.setDeadline(snapshot); proposal.voteEnd.setDeadline(deadline); + proposal.proposer = proposer; emit ProposalCreated( proposalId, - _msgSender(), + proposer, targets, values, new string[](targets.length), @@ -310,6 +316,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive return proposalId; } + /** + * @dev See {IGovernor-cancel}. + */ + function cancel(uint256 proposalId) public virtual override { + require(state(proposalId) == ProposalState.Pending, "Governor: too late to cancel"); + require(_msgSender() == _proposals[proposalId].proposer, "Governor: only proposer can cancel"); + _cancel(proposalId); + } + /** * @dev Internal execution mechanism. Can be overridden to implement different execution mechanism */ @@ -375,7 +390,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes[] memory calldatas, bytes32 descriptionHash ) internal virtual returns (uint256) { - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + return _cancel(hashProposal(targets, values, calldatas, descriptionHash)); + } + + /** + * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as + * canceled to allow distinguishing it from executed proposals. + * + * Emits a {IGovernor-ProposalCanceled} event. + */ + function _cancel(uint256 proposalId) internal virtual returns (uint256) { ProposalState status = state(proposalId); require( diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index eb3d1fc05..18cf57fb1 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -216,6 +216,14 @@ abstract contract IGovernor is IERC165 { bytes32 descriptionHash ) public payable virtual returns (uint256 proposalId); + /** + * @dev Cancel a proposal. This is restricted to Pending proposal (before the vote starts) and is restricted to + * the proposal's proposer. + * + * Emits a {ProposalCanceled} event. + */ + function cancel(uint256 proposalId) public virtual; + /** * @dev Cast a vote * diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 8d74742c5..912d02f8a 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -99,7 +99,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp ); } - function cancel(uint256 proposalId) public virtual override { + function cancel(uint256 proposalId) public virtual override(IGovernor, Governor) { ProposalDetails storage details = _proposalDetails[proposalId]; require( @@ -107,12 +107,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp "GovernorBravo: proposer above threshold" ); - _cancel( - details.targets, - details.values, - _encodeCalldata(details.signatures, details.calldatas), - details.descriptionHash - ); + _cancel(proposalId); } /** diff --git a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol index d1ec0337d..159c55100 100644 --- a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol @@ -90,11 +90,6 @@ abstract contract IGovernorCompatibilityBravo is IGovernor { */ function execute(uint256 proposalId) public payable virtual; - /** - * @dev Cancels a proposal only if sender is the proposer, or proposer delegates dropped below proposal threshold. - */ - function cancel(uint256 proposalId) public virtual; - /** * @dev Part of the Governor Bravo's interface: _"Gets actions of a proposal"_. */ diff --git a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol index 4356bce7f..2727794f6 100644 --- a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol +++ b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol @@ -66,6 +66,10 @@ abstract contract GovernorCompatibilityBravoMock is return super.execute(targets, values, calldatas, salt); } + function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { + super.cancel(proposalId); + } + function _execute( uint256 proposalId, address[] memory targets, diff --git a/contracts/mocks/wizard/MyGovernor3.sol b/contracts/mocks/wizard/MyGovernor3.sol index 320342290..4192cae94 100644 --- a/contracts/mocks/wizard/MyGovernor3.sol +++ b/contracts/mocks/wizard/MyGovernor3.sol @@ -54,6 +54,10 @@ contract MyGovernor is return super.propose(targets, values, calldatas, description); } + function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { + super.cancel(proposalId); + } + function _execute( uint256 proposalId, address[] memory targets, diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index cc3cc17ef..4c083ed92 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -382,55 +382,109 @@ contract('Governor', function (accounts) { }); describe('cancel', function () { - it('before proposal', async function () { - await expectRevert(this.helper.cancel(), 'Governor: unknown proposal id'); + describe('internal', function () { + it('before proposal', async function () { + await expectRevert(this.helper.cancel('internal'), 'Governor: unknown proposal id'); + }); + + it('after proposal', async function () { + await this.helper.propose(); + + await this.helper.cancel('internal'); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + + await this.helper.waitForSnapshot(); + await expectRevert( + this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }), + 'Governor: vote not currently active', + ); + }); + + it('after vote', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + + await this.helper.cancel('internal'); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + + await this.helper.waitForDeadline(); + await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); + }); + + it('after deadline', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + + await this.helper.cancel('internal'); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + + await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); + }); + + it('after execution', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await this.helper.execute(); + + await expectRevert(this.helper.cancel('internal'), 'Governor: proposal not active'); + }); }); - it('after proposal', async function () { - await this.helper.propose(); + describe('public', function () { + it('before proposal', async function () { + await expectRevert(this.helper.cancel('external'), 'Governor: unknown proposal id'); + }); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + it('after proposal', async function () { + await this.helper.propose(); - await this.helper.waitForSnapshot(); - await expectRevert( - this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }), - 'Governor: vote not currently active', - ); - }); + await this.helper.cancel('external'); + }); - it('after vote', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + it('after proposal - restricted to proposer', async function () { + await this.helper.propose(); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + await expectRevert(this.helper.cancel('external', { from: owner }), 'Governor: only proposer can cancel'); + }); - await this.helper.waitForDeadline(); - await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); - }); + it('after vote started', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(1); // snapshot + 1 block - it('after deadline', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.waitForDeadline(); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); + }); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + it('after vote', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); - }); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); + }); - it('after execution', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.waitForDeadline(); - await this.helper.execute(); + it('after deadline', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); - await expectRevert(this.helper.cancel(), 'Governor: proposal not active'); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); + }); + + it('after execution', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await this.helper.execute(); + + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); + }); }); }); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 231d57b27..3666c4548 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -226,18 +226,18 @@ contract('GovernorCompatibilityBravo', function (accounts) { describe('cancel', function () { it('proposer can cancel', async function () { await this.helper.propose({ from: proposer }); - await this.helper.cancel({ from: proposer }); + await this.helper.cancel('external', { from: proposer }); }); it('anyone can cancel if proposer drop below threshold', async function () { await this.helper.propose({ from: proposer }); await this.token.transfer(voter1, web3.utils.toWei('1'), { from: proposer }); - await this.helper.cancel(); + await this.helper.cancel('external'); }); it('cannot cancel is proposer is still above threshold', async function () { await this.helper.propose({ from: proposer }); - await expectRevert(this.helper.cancel(), 'GovernorBravo: proposer above threshold'); + await expectRevert(this.helper.cancel('external'), 'GovernorBravo: proposer above threshold'); }); }); }); diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 334659e07..fb6803383 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -193,7 +193,7 @@ contract('GovernorTimelockCompound', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.queue(), 'Governor: proposal not successful'); @@ -206,7 +206,7 @@ contract('GovernorTimelockCompound', function (accounts) { await this.helper.waitForDeadline(); await this.helper.queue(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 2e8c75888..4f9b9df73 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -185,7 +185,7 @@ contract('GovernorTimelockControl', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.queue(), 'Governor: proposal not successful'); @@ -198,7 +198,7 @@ contract('GovernorTimelockControl', function (accounts) { await this.helper.waitForDeadline(); await this.helper.queue(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index ff341aa12..f933f389d 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -62,14 +62,19 @@ class GovernorHelper { ); } - cancel(opts = null) { + cancel(visibility = 'external', opts = null) { const proposal = this.currentProposal; - return proposal.useCompatibilityInterface - ? this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)) - : this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( + switch (visibility) { + case 'external': + return this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)); + case 'internal': + return this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( ...concatOpts(proposal.shortProposal, opts), ); + default: + throw new Error(`unsuported visibility "${visibility}"`); + } } vote(vote = {}, opts = null) {