AccessManager: Avoid resetting nonce when consuming a scheduled operation (#4603)

Co-authored-by: Francisco <fg@frang.io>

contracts/access/manager/AccessManager.sol

@@ -572,15 +572,14 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
         uint48 minWhen = Time.timestamp() + setback;
 
-        if (when == 0) {
+        // if call is not authorized, or if requested timing is too soon
-            when = minWhen;
+        if ((!immediate && setback == 0) || (when > 0 && when < minWhen)) {
-        }
-
-        // If caller is not authorised, revert
-        if (!immediate && (setback == 0 || when < minWhen)) {
             revert AccessManagerUnauthorizedCall(caller, target, bytes4(data[0:4]));
         }
 
+        // Reuse variable due to stack too deep
+        when = uint48(Math.max(when, minWhen)); // cast is safe: both inputs are uint48
+
         // If caller is authorised, schedule operation
         operationId = hashOperation(caller, target, data);
 
@@ -686,7 +685,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
             revert AccessManagerExpired(operationId);
         }
 
-        delete _schedules[operationId];
+        delete _schedules[operationId].timepoint; // reset the timepoint, keep the nonce
         emit OperationExecuted(operationId, nonce);
 
         return nonce;
@@ -718,7 +717,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
             }
         }
 
-        delete _schedules[operationId].timepoint;
+        delete _schedules[operationId].timepoint; // reset the timepoint, keep the nonce
         uint32 nonce = _schedules[operationId].nonce;
         emit OperationCanceled(operationId, nonce);
 

test/access/manager/AccessManager.test.js

@@ -703,9 +703,14 @@ contract('AccessManager', function (accounts) {
           it('Calling indirectly: only execute', async function () {
             // execute without schedule
             if (directSuccess) {
+              const nonceBefore = await this.manager.getNonce(this.opId);
               const { receipt, tx } = await this.execute();
+
               expectEvent.notEmitted(receipt, 'OperationExecuted', { operationId: this.opId });
               await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
+
+              // nonce is not modified
+              expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore);
             } else if (indirectSuccess) {
               await expectRevertCustomError(this.execute(), 'AccessManagerNotScheduled', [this.opId]);
             } else {
@@ -715,6 +720,7 @@ contract('AccessManager', function (accounts) {
 
           it('Calling indirectly: schedule and execute', async function () {
             if (directSuccess || indirectSuccess) {
+              const nonceBefore = await this.manager.getNonce(this.opId);
               const { receipt } = await this.schedule();
               const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
 
@@ -730,14 +736,21 @@ contract('AccessManager', function (accounts) {
                 timestamp.add(directSuccess ? web3.utils.toBN(0) : delay),
               );
 
+              // nonce is incremented
+              expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
+
               // execute without wait
               if (directSuccess) {
                 const { receipt, tx } = await this.execute();
+
                 await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
                 if (delay && fnRole !== ROLES.PUBLIC) {
                   expectEvent(receipt, 'OperationExecuted', { operationId: this.opId });
                   expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0');
                 }
+
+                // nonce is not modified by execute
+                expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
               } else if (indirectSuccess) {
                 await expectRevertCustomError(this.execute(), 'AccessManagerNotReady', [this.opId]);
               } else {
@@ -750,6 +763,7 @@ contract('AccessManager', function (accounts) {
 
           it('Calling indirectly: schedule wait and execute', async function () {
             if (directSuccess || indirectSuccess) {
+              const nonceBefore = await this.manager.getNonce(this.opId);
               const { receipt } = await this.schedule();
               const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
 
@@ -765,17 +779,24 @@ contract('AccessManager', function (accounts) {
                 timestamp.add(directSuccess ? web3.utils.toBN(0) : delay),
               );
 
+              // nonce is incremented
+              expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
+
               // wait
               await time.increase(delay ?? 0);
 
               // execute without wait
               if (directSuccess || indirectSuccess) {
                 const { receipt, tx } = await this.execute();
+
                 await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
                 if (delay && fnRole !== ROLES.PUBLIC) {
                   expectEvent(receipt, 'OperationExecuted', { operationId: this.opId });
                   expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0');
                 }
+
+                // nonce is not modified by execute
+                expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
               } else {
                 await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
               }
@@ -786,6 +807,7 @@ contract('AccessManager', function (accounts) {
 
           it('Calling directly: schedule and call', async function () {
             if (directSuccess || indirectSuccess) {
+              const nonceBefore = await this.manager.getNonce(this.opId);
               const { receipt } = await this.schedule();
               const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
 
@@ -800,6 +822,9 @@ contract('AccessManager', function (accounts) {
               const schedule = timestamp.add(directSuccess ? web3.utils.toBN(0) : delay);
               expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal(schedule);
 
+              // nonce is incremented
+              expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
+
               // execute without wait
               const promise = this.direct();
               if (directSuccess) {
@@ -807,6 +832,9 @@ contract('AccessManager', function (accounts) {
 
                 // schedule is not reset
                 expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal(schedule);
+
+                // nonce is not modified by execute
+                expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
               } else if (indirectSuccess) {
                 await expectRevertCustomError(promise, 'AccessManagerNotReady', [this.opId]);
               } else {
@@ -819,6 +847,7 @@ contract('AccessManager', function (accounts) {
 
           it('Calling directly: schedule wait and call', async function () {
             if (directSuccess || indirectSuccess) {
+              const nonceBefore = await this.manager.getNonce(this.opId);
               const { receipt } = await this.schedule();
               const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
 
@@ -833,6 +862,9 @@ contract('AccessManager', function (accounts) {
               const schedule = timestamp.add(directSuccess ? web3.utils.toBN(0) : delay);
               expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal(schedule);
 
+              // nonce is incremented
+              expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
+
               // wait
               await time.increase(delay ?? 0);
 
@@ -843,6 +875,9 @@ contract('AccessManager', function (accounts) {
 
                 // schedule is not reset
                 expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal(schedule);
+
+                // nonce is not modified by execute
+                expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
               } else if (indirectSuccess) {
                 const receipt = await promise;
 
@@ -853,6 +888,9 @@ contract('AccessManager', function (accounts) {
 
                 // schedule is reset
                 expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0');
+
+                // nonce is not modified by execute
+                expect(await this.manager.getNonce(this.opId)).to.be.bignumber.equal(nonceBefore.addn(1));
               } else {
                 await expectRevertCustomError(this.direct(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
               }