Skip to content

Commit 4681c1c

Browse files
Amxxcoderabbitai[bot]
Amxx
and
coderabbitai[bot]
authored
Fix: Enforce proposal status when calling Governor.execute depending on proposalNeedsQueuing (#6386)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 95a983a commit 4681c1c

File tree

9 files changed

+84
-40
lines changed

9 files changed

+84
-40
lines changed

‎.changeset/silver-falcons-lay.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': minor
3+
---
4+
5+
`Governor`: Strict enforcement of the expected proposal state depending on `proposalNeedsQueuing` when calling `execute`.

‎contracts/governance/Governor.sol‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,10 +394,11 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
394394
bytes32 descriptionHash
395395
) public payable virtual returns (uint256) {
396396
uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
397+
bool needsQueueing = proposalNeedsQueuing(proposalId);
397398

398399
_validateStateBitmap(
399400
proposalId,
400-
_encodeStateBitmap(ProposalState.Succeeded) | _encodeStateBitmap(ProposalState.Queued)
401+
_encodeStateBitmap(needsQueueing ? ProposalState.Queued : ProposalState.Succeeded)
401402
);
402403

403404
// mark as executed before calls to avoid reentrancy

‎contracts/governance/IGovernor.sol‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,6 @@ interface IGovernor is IERC165, IERC6372 {
9191
*/
9292
error GovernorQueueNotImplemented();
9393

94-
/**
95-
* @dev The proposal hasn't been queued yet.
96-
*/
97-
error GovernorNotQueuedProposal(uint256 proposalId);
98-
9994
/**
10095
* @dev The proposal has already been queued.
10196
*/

‎contracts/governance/extensions/GovernorTimelockCompound.sol‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,6 @@ abstract contract GovernorTimelockCompound is Governor {
9494
bytes32 /*descriptionHash*/
9595
) internal virtual override {
9696
uint256 etaSeconds = proposalEta(proposalId);
97-
if (etaSeconds == 0) {
98-
revert GovernorNotQueuedProposal(proposalId);
99-
}
10097
Address.sendValue(payable(_timelock), msg.value);
10198
for (uint256 i = 0; i < targets.length; ++i) {
10299
_timelock.executeTransaction(targets[i], values[i], "", calldatas[i], etaSeconds);

‎test/governance/Governor.test.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ describe('Governor', function () {
404404
.withArgs(
405405
this.proposal.id,
406406
ProposalState.Active,
407-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
407+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
408408
);
409409
});
410410

@@ -417,7 +417,7 @@ describe('Governor', function () {
417417
.withArgs(
418418
this.proposal.id,
419419
ProposalState.Active,
420-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
420+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
421421
);
422422
});
423423

@@ -430,7 +430,7 @@ describe('Governor', function () {
430430
.withArgs(
431431
this.proposal.id,
432432
ProposalState.Active,
433-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
433+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
434434
);
435435
});
436436

@@ -481,7 +481,7 @@ describe('Governor', function () {
481481
.withArgs(
482482
this.proposal.id,
483483
ProposalState.Executed,
484-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
484+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
485485
);
486486
});
487487
});
@@ -569,7 +569,7 @@ describe('Governor', function () {
569569
.withArgs(
570570
this.proposal.id,
571571
ProposalState.Canceled,
572-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
572+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
573573
);
574574
});
575575

@@ -587,7 +587,7 @@ describe('Governor', function () {
587587
.withArgs(
588588
this.proposal.id,
589589
ProposalState.Canceled,
590-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
590+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
591591
);
592592
});
593593

‎test/governance/extensions/GovernorTimelockAccess.test.js‎

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,42 @@ describe('GovernorTimelockAccess', function () {
235235
// Set base delay
236236
await this.mock.$_setBaseDelaySeconds(baseDelay);
237237

238-
await this.helper.setProposal([this.restricted.operation], 'descr');
238+
const { id } = await this.helper.setProposal([this.restricted.operation], 'descr');
239239
await this.helper.propose();
240-
expect(await this.mock.proposalNeedsQueuing(this.helper.currentProposal.id)).to.be.false;
240+
expect(await this.mock.proposalNeedsQueuing(id)).to.be.false;
241+
242+
await this.helper.waitForSnapshot();
243+
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
244+
await this.helper.waitForDeadline();
245+
246+
// No need for queuing, so it should not revert
247+
await expect(this.helper.execute()).to.not.be.reverted;
248+
});
249+
250+
it('does need to queue proposals with base delay', async function () {
251+
const roleId = 1n;
252+
const executionDelay = 0n;
253+
const baseDelay = 10n;
254+
255+
// Set execution delay
256+
await this.manager.connect(this.admin).setTargetFunctionRole(this.receiver, [this.restricted.selector], roleId);
257+
await this.manager.connect(this.admin).grantRole(roleId, this.mock, executionDelay);
258+
259+
// Set base delay
260+
await this.mock.$_setBaseDelaySeconds(baseDelay);
261+
262+
const { id } = await this.helper.setProposal([this.restricted.operation], 'descr');
263+
await this.helper.propose();
264+
expect(await this.mock.proposalNeedsQueuing(id)).to.be.true;
265+
266+
await this.helper.waitForSnapshot();
267+
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
268+
await this.helper.waitForDeadline();
269+
270+
// Not queued, so it should revert
271+
await expect(this.helper.execute())
272+
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
273+
.withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]));
241274
});
242275

243276
it('needs to queue proposals with any delay', async function () {
@@ -257,12 +290,21 @@ describe('GovernorTimelockAccess', function () {
257290
// Set base delay
258291
await this.mock.$_setBaseDelaySeconds(baseDelay);
259292

260-
await this.helper.setProposal(
293+
const { id } = await this.helper.setProposal(
261294
[this.restricted.operation],
262295
`executionDelay=${executionDelay.toString()}}baseDelay=${baseDelay.toString()}}`,
263296
);
264297
await this.helper.propose();
265-
expect(await this.mock.proposalNeedsQueuing(this.helper.currentProposal.id)).to.be.true;
298+
expect(await this.mock.proposalNeedsQueuing(id)).to.be.true;
299+
300+
await this.helper.waitForSnapshot();
301+
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
302+
await this.helper.waitForDeadline();
303+
304+
// Not queued, so it should revert
305+
await expect(this.helper.execute())
306+
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
307+
.withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]));
266308
}
267309
});
268310

@@ -556,7 +598,7 @@ describe('GovernorTimelockAccess', function () {
556598
.withArgs(
557599
this.proposal.id,
558600
ProposalState.Canceled,
559-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
601+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), // proposal needs queueing
560602
);
561603
});
562604

@@ -602,7 +644,7 @@ describe('GovernorTimelockAccess', function () {
602644
.withArgs(
603645
original.currentProposal.id,
604646
ProposalState.Canceled,
605-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
647+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]), // proposal needs queueing
606648
);
607649
});
608650

@@ -628,7 +670,7 @@ describe('GovernorTimelockAccess', function () {
628670
.withArgs(
629671
this.proposal.id,
630672
ProposalState.Canceled,
631-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
673+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), // not queueing necessary
632674
);
633675
});
634676

@@ -649,7 +691,7 @@ describe('GovernorTimelockAccess', function () {
649691
.withArgs(
650692
this.proposal.id,
651693
ProposalState.Canceled,
652-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
694+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]), // not queueing necessary
653695
);
654696
});
655697

‎test/governance/extensions/GovernorTimelockCompound.test.js‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,8 @@ describe('GovernorTimelockCompound', function () {
156156
.to.be.revertedWithCustomError(this.mock, 'GovernorAlreadyQueuedProposal')
157157
.withArgs(id);
158158
await expect(this.helper.execute())
159-
.to.be.revertedWithCustomError(this.mock, 'GovernorNotQueuedProposal')
160-
.withArgs(id);
159+
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
160+
.withArgs(id, ProposalState.Succeeded, GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]));
161161
});
162162
});
163163

@@ -168,11 +168,13 @@ describe('GovernorTimelockCompound', function () {
168168
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
169169
await this.helper.waitForDeadline(1n);
170170

171-
expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Succeeded);
172-
173171
await expect(this.helper.execute())
174-
.to.be.revertedWithCustomError(this.mock, 'GovernorNotQueuedProposal')
175-
.withArgs(this.proposal.id);
172+
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
173+
.withArgs(
174+
this.proposal.id,
175+
ProposalState.Succeeded,
176+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
177+
);
176178
});
177179

178180
it('if too early', async function () {
@@ -204,7 +206,7 @@ describe('GovernorTimelockCompound', function () {
204206
.withArgs(
205207
this.proposal.id,
206208
ProposalState.Expired,
207-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
209+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
208210
);
209211
});
210212

@@ -222,7 +224,7 @@ describe('GovernorTimelockCompound', function () {
222224
.withArgs(
223225
this.proposal.id,
224226
ProposalState.Executed,
225-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
227+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
226228
);
227229
});
228230
});
@@ -317,7 +319,7 @@ describe('GovernorTimelockCompound', function () {
317319
.withArgs(
318320
this.proposal.id,
319321
ProposalState.Canceled,
320-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
322+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
321323
);
322324
});
323325
});

‎test/governance/extensions/GovernorTimelockControl.test.js‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,13 @@ describe('GovernorTimelockControl', function () {
166166
await this.helper.connect(this.voter1).vote({ support: VoteType.For });
167167
await this.helper.waitForDeadline(1n);
168168

169-
expect(await this.mock.state(this.proposal.id)).to.equal(ProposalState.Succeeded);
170-
171169
await expect(this.helper.execute())
172-
.to.be.revertedWithCustomError(this.timelock, 'TimelockUnexpectedOperationState')
173-
.withArgs(this.proposal.timelockid, GovernorHelper.proposalStatesToBitMap(OperationState.Ready));
170+
.to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState')
171+
.withArgs(
172+
this.proposal.id,
173+
ProposalState.Succeeded,
174+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
175+
);
174176
});
175177

176178
it('if too early', async function () {
@@ -201,7 +203,7 @@ describe('GovernorTimelockControl', function () {
201203
.withArgs(
202204
this.proposal.id,
203205
ProposalState.Executed,
204-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
206+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
205207
);
206208
});
207209

@@ -224,7 +226,7 @@ describe('GovernorTimelockControl', function () {
224226
.withArgs(
225227
this.proposal.id,
226228
ProposalState.Executed,
227-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
229+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
228230
);
229231
});
230232
});
@@ -270,7 +272,7 @@ describe('GovernorTimelockControl', function () {
270272
.withArgs(
271273
this.proposal.id,
272274
ProposalState.Canceled,
273-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
275+
GovernorHelper.proposalStatesToBitMap([ProposalState.Queued]),
274276
);
275277
});
276278

‎test/governance/extensions/GovernorVotesQuorumFraction.test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ describe('GovernorVotesQuorumFraction', function () {
9292
.withArgs(
9393
this.proposal.id,
9494
ProposalState.Defeated,
95-
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded, ProposalState.Queued]),
95+
GovernorHelper.proposalStatesToBitMap([ProposalState.Succeeded]),
9696
);
9797
});
9898

0 commit comments

Comments
 (0)