diff --git a/.changeset/silver-falcons-lay.md b/.changeset/silver-falcons-lay.md new file mode 100644 index 00000000000..f3296013128 --- /dev/null +++ b/.changeset/silver-falcons-lay.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Governor`: Strict enforcement of the expected proposal state depending on `proposalNeedsQueuing` when calling `execute`. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index baffd23e24f..6cdf900590f 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -394,10 +394,11 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bytes32 descriptionHash ) public payable virtual returns (uint256) { uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash); + bool needsQueueing = proposalNeedsQueuing(proposalId); _validateStateBitmap( proposalId, - _encodeStateBitmap(ProposalState.Succeeded) | _encodeStateBitmap(ProposalState.Queued) + _encodeStateBitmap(needsQueueing ? ProposalState.Queued : ProposalState.Succeeded) ); // mark as executed before calls to avoid reentrancy diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 0988b4e91ca..916dd41802b 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -91,11 +91,6 @@ interface IGovernor is IERC165, IERC6372 { */ error GovernorQueueNotImplemented(); - /** - * @dev The proposal hasn't been queued yet. - */ - error GovernorNotQueuedProposal(uint256 proposalId); - /** * @dev The proposal has already been queued. */ diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 8f6183e8b89..c3225f19865 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -94,9 +94,6 @@ abstract contract GovernorTimelockCompound is Governor { bytes32 /*descriptionHash*/ ) internal virtual override { uint256 etaSeconds = proposalEta(proposalId); - if (etaSeconds == 0) { - revert GovernorNotQueuedProposal(proposalId); - } Address.sendValue(payable(_timelock), msg.value); for (uint256 i = 0; i < targets.length; ++i) { _timelock.executeTransaction(targets[i], values[i], "", calldatas[i], etaSeconds); diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index ebb5a3884d1..961cd149d5c 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -404,7 +404,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Active, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); @@ -417,7 +417,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Active, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); @@ -430,7 +430,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Active, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); @@ -481,7 +481,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Executed, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); }); @@ -569,7 +569,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); @@ -587,7 +587,7 @@ describe('Governor', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); }); diff --git a/test/governance/extensions/GovernorTimelockAccess.test.js b/test/governance/extensions/GovernorTimelockAccess.test.js index 5eea6478abd..3772a4aa956 100644 --- a/test/governance/extensions/GovernorTimelockAccess.test.js +++ b/test/governance/extensions/GovernorTimelockAccess.test.js @@ -235,9 +235,42 @@ describe('GovernorTimelockAccess', function () { // Set base delay await this.mock.$_setBaseDelaySeconds(baseDelay); - await this.helper.setProposal([this.restricted.operation], 'descr'); + const { id } = await this.helper.setProposal([this.restricted.operation], 'descr'); await this.helper.propose(); - expect(await this.mock.proposalNeedsQueuing(this.helper.currentProposal.id)).to.be.false; + expect(await this.mock.proposalNeedsQueuing(id)).to.be.false; + + await this.helper.waitForSnapshot(); + await this.helper.connect(this.voter1).vote({ support: VoteType.For }); + await this.helper.waitForDeadline(); + + // No need for queuing, so it should not revert + await expect(this.helper.execute()).to.not.be.reverted; + }); + + it('does need to queue proposals with base delay', async function () { + const roleId = 1n; + const executionDelay = 0n; + const baseDelay = 10n; + + // Set execution delay + await this.manager.connect(this.admin).setTargetFunctionRole(this.receiver, [this.restricted.selector], roleId); + await this.manager.connect(this.admin).grantRole(roleId, this.mock, executionDelay); + + // Set base delay + await this.mock.$_setBaseDelaySeconds(baseDelay); + + const { id } = await this.helper.setProposal([this.restricted.operation], 'descr'); + await this.helper.propose(); + expect(await this.mock.proposalNeedsQueuing(id)).to.be.true; + + await this.helper.waitForSnapshot(); + await this.helper.connect(this.voter1).vote({ support: VoteType.For }); + await this.helper.waitForDeadline(); + + // Not queued, so it should revert + await expect(this.helper.execute()) + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued])); }); it('needs to queue proposals with any delay', async function () { @@ -257,12 +290,21 @@ describe('GovernorTimelockAccess', function () { // Set base delay await this.mock.$_setBaseDelaySeconds(baseDelay); - await this.helper.setProposal( + const { id } = await this.helper.setProposal( [this.restricted.operation], `executionDelay=${executionDelay.toString()}}baseDelay=${baseDelay.toString()}}`, ); await this.helper.propose(); - expect(await this.mock.proposalNeedsQueuing(this.helper.currentProposal.id)).to.be.true; + expect(await this.mock.proposalNeedsQueuing(id)).to.be.true; + + await this.helper.waitForSnapshot(); + await this.helper.connect(this.voter1).vote({ support: VoteType.For }); + await this.helper.waitForDeadline(); + + // Not queued, so it should revert + await expect(this.helper.execute()) + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued])); } }); @@ -556,7 +598,7 @@ describe('GovernorTimelockAccess', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), // proposal needs queueing ); }); @@ -602,7 +644,7 @@ describe('GovernorTimelockAccess', function () { .withArgs( original.currentProposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), // proposal needs queueing ); }); @@ -628,7 +670,7 @@ describe('GovernorTimelockAccess', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), // not queueing necessary ); }); @@ -649,7 +691,7 @@ describe('GovernorTimelockAccess', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), // not queueing necessary ); }); diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index cd82481d500..fc8e1e561b2 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -156,8 +156,8 @@ describe('GovernorTimelockCompound', function () { .to.be.revertedWithCustomError(this.mock, 'GovernorAlreadyQueuedProposal') .withArgs(id); await expect(this.helper.execute()) - .to.be.revertedWithCustomError(this.mock, 'GovernorNotQueuedProposal') - .withArgs(id); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued])); }); }); @@ -168,11 +168,13 @@ describe('GovernorTimelockCompound', function () { await this.helper.connect(this.voter1).vote({ support: VoteType.For }); await this.helper.waitForDeadline(1n); - expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Succeeded); - await expect(this.helper.execute()) - .to.be.revertedWithCustomError(this.mock, 'GovernorNotQueuedProposal') - .withArgs(this.proposal.id); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs( + this.proposal.id, + ProposalState.Succeeded, + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), + ); }); it('if too early', async function () { @@ -204,7 +206,7 @@ describe('GovernorTimelockCompound', function () { .withArgs( this.proposal.id, ProposalState.Expired, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); @@ -222,7 +224,7 @@ describe('GovernorTimelockCompound', function () { .withArgs( this.proposal.id, ProposalState.Executed, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); }); @@ -317,7 +319,7 @@ describe('GovernorTimelockCompound', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 507c7e27832..02e1ff2bc8d 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -166,11 +166,13 @@ describe('GovernorTimelockControl', function () { await this.helper.connect(this.voter1).vote({ support: VoteType.For }); await this.helper.waitForDeadline(1n); - expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Succeeded); - await expect(this.helper.execute()) - .to.be.revertedWithCustomError(this.timelock, 'TimelockUnexpectedOperationState') - .withArgs(this.proposal.timelockid, GovernorHelper.proposalStatesToBitMap(OperationState.Ready)); + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs( + this.proposal.id, + ProposalState.Succeeded, + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), + ); }); it('if too early', async function () { @@ -201,7 +203,7 @@ describe('GovernorTimelockControl', function () { .withArgs( this.proposal.id, ProposalState.Executed, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); @@ -224,7 +226,7 @@ describe('GovernorTimelockControl', function () { .withArgs( this.proposal.id, ProposalState.Executed, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); }); @@ -270,7 +272,7 @@ describe('GovernorTimelockControl', function () { .withArgs( this.proposal.id, ProposalState.Canceled, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), ); }); diff --git a/test/governance/extensions/GovernorVotesQuorumFraction.test.js b/test/governance/extensions/GovernorVotesQuorumFraction.test.js index 99afd393173..6dafaa9e3fc 100644 --- a/test/governance/extensions/GovernorVotesQuorumFraction.test.js +++ b/test/governance/extensions/GovernorVotesQuorumFraction.test.js @@ -92,7 +92,7 @@ describe('GovernorVotesQuorumFraction', function () { .withArgs( this.proposal.id, ProposalState.Defeated, - GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]), + GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), ); });