Fix: Enforce proposal status when calling Governor.execute depending on proposalNeedsQueuing

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

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

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.
      */

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);

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]),
               );
           });
 

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
             );
         });
 

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]),
             );
         });
       });

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]),
             );
         });
 

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]),
           );
       });