Add a minimum delay on all admin update operations (#4557)

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

contracts/access/manager/AccessManager.sol

@@ -154,10 +154,12 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev Minimum setback for delay updates. Defaults to 1 day.
+     * @dev Minimum setback for all delay updates, with the exception of execution delays, which
+     * can be increased without setback (and in the event of an accidental increase can be reset
+     * via {revokeGroup}). Defaults to 5 days.
      */
     function minSetback() public view virtual returns (uint32) {
-        return 0; // TODO: set to 1 day
+        return 5 days;
     }
 
     /**
@@ -367,9 +369,11 @@ contract AccessManager is Context, Multicall, IAccessManager {
         uint48 since;
 
         if (inGroup) {
+            // No setback here. Value can be reset by doing revoke + grant, effectively allowing the admin to perform
+            // any change to the execution delay within the duration of the group admin delay.
             (_groups[groupId].members[account].delay, since) = _groups[groupId].members[account].delay.withUpdate(
                 executionDelay,
-                minSetback()
+                0
             );
         } else {
             since = Time.timestamp() + grantDelay;

test/access/manager/AccessManager.test.js

@@ -22,6 +22,8 @@ const classId = web3.utils.toBN(1);
 const executeDelay = web3.utils.toBN(10);
 const grantDelay = web3.utils.toBN(10);
 
+const MINSETBACK = time.duration.days(5);
+
 const formatAccess = access => [access[0], access[1].toString()];
 
 contract('AccessManager', function (accounts) {
@@ -37,6 +39,10 @@ contract('AccessManager', function (accounts) {
     await this.manager.$_grantGroup(GROUPS.SOME, member, 0, 0);
   });
 
+  it('default minsetback is 1 day', async function () {
+    expect(await this.manager.minSetback()).to.be.bignumber.equal(MINSETBACK);
+  });
+
   it('groups are correctly initialized', async function () {
     // group admin
     expect(await this.manager.getGroupAdmin(GROUPS.ADMIN)).to.be.bignumber.equal(GROUPS.ADMIN);
@@ -161,6 +167,7 @@ contract('AccessManager', function (accounts) {
       describe('with a grant delay', function () {
         beforeEach(async function () {
           await this.manager.$_setGrantDelay(GROUPS.SOME, grantDelay);
+          await time.increase(MINSETBACK);
         });
 
         it('granted group is not active immediatly', async function () {
@@ -350,6 +357,7 @@ contract('AccessManager', function (accounts) {
         const oldDelay = web3.utils.toBN(10);
         const newDelay = web3.utils.toBN(100);
 
+        // group is already granted (with no delay) in the initial setup. this update takes time.
         await this.manager.$_grantGroup(GROUPS.SOME, member, 0, oldDelay);
 
         const accessBefore = await this.manager.getAccess(GROUPS.SOME, member);
@@ -380,6 +388,7 @@ contract('AccessManager', function (accounts) {
         const oldDelay = web3.utils.toBN(100);
         const newDelay = web3.utils.toBN(10);
 
+        // group is already granted (with no delay) in the initial setup. this update takes time.
         await this.manager.$_grantGroup(GROUPS.SOME, member, 0, oldDelay);
 
         const accessBefore = await this.manager.getAccess(GROUPS.SOME, member);
@@ -391,27 +400,37 @@ contract('AccessManager', function (accounts) {
           from: manager,
         });
         const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
+        const setback = oldDelay.sub(newDelay);
 
         expectEvent(receipt, 'GroupGranted', {
           groupId: GROUPS.SOME,
           account: member,
-          since: timestamp.add(oldDelay).sub(newDelay),
+          since: timestamp.add(setback),
           delay: newDelay,
         });
 
-        // delayed effect
+        // no immediate effect
         const accessAfter = await this.manager.getAccess(GROUPS.SOME, member);
         expect(accessAfter[1]).to.be.bignumber.equal(oldDelay); // currentDelay
         expect(accessAfter[2]).to.be.bignumber.equal(newDelay); // pendingDelay
-        expect(accessAfter[3]).to.be.bignumber.equal(timestamp.add(oldDelay).sub(newDelay)); // effect
+        expect(accessAfter[3]).to.be.bignumber.equal(timestamp.add(setback)); // effect
+
+        // delayed effect
+        await time.increase(setback);
+        const accessAfterSetback = await this.manager.getAccess(GROUPS.SOME, member);
+        expect(accessAfterSetback[1]).to.be.bignumber.equal(newDelay); // currentDelay
+        expect(accessAfterSetback[2]).to.be.bignumber.equal('0'); // pendingDelay
+        expect(accessAfterSetback[3]).to.be.bignumber.equal('0'); // effect
       });
 
       it('can set a user execution delay during the grant delay', async function () {
         await this.manager.$_grantGroup(GROUPS.SOME, other, 10, 0);
+        // here: "other" is pending to get the group, but doesn't yet have it.
 
         const { receipt } = await this.manager.grantGroup(GROUPS.SOME, other, executeDelay, { from: manager });
         const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
 
+        // increasing the execution delay from 0 to executeDelay is immediate
         expectEvent(receipt, 'GroupGranted', {
           groupId: GROUPS.SOME,
           account: other,
@@ -425,38 +444,75 @@ contract('AccessManager', function (accounts) {
       it('increasing the delay has immediate effect', async function () {
         const oldDelay = web3.utils.toBN(10);
         const newDelay = web3.utils.toBN(100);
+
         await this.manager.$_setGrantDelay(GROUPS.SOME, oldDelay);
+        await time.increase(MINSETBACK);
 
         expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
 
         const { receipt } = await this.manager.setGrantDelay(GROUPS.SOME, newDelay, { from: admin });
         const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
+        const setback = web3.utils.BN.max(MINSETBACK, oldDelay.sub(newDelay));
 
-        expectEvent(receipt, 'GroupGrantDelayChanged', { groupId: GROUPS.SOME, delay: newDelay, since: timestamp });
+        expect(setback).to.be.bignumber.equal(MINSETBACK);
+        expectEvent(receipt, 'GroupGrantDelayChanged', {
+          groupId: GROUPS.SOME,
+          delay: newDelay,
+          since: timestamp.add(setback),
+        });
 
+        expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
+        await time.increase(setback);
         expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(newDelay);
       });
 
-      it('increasing the delay has delay effect', async function () {
+      it('increasing the delay has delay effect #1', async function () {
         const oldDelay = web3.utils.toBN(100);
         const newDelay = web3.utils.toBN(10);
+
         await this.manager.$_setGrantDelay(GROUPS.SOME, oldDelay);
+        await time.increase(MINSETBACK);
 
         expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
 
         const { receipt } = await this.manager.setGrantDelay(GROUPS.SOME, newDelay, { from: admin });
         const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
+        const setback = web3.utils.BN.max(MINSETBACK, oldDelay.sub(newDelay));
 
+        expect(setback).to.be.bignumber.equal(MINSETBACK);
         expectEvent(receipt, 'GroupGrantDelayChanged', {
           groupId: GROUPS.SOME,
           delay: newDelay,
-          since: timestamp.add(oldDelay).sub(newDelay),
+          since: timestamp.add(setback),
         });
 
         expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
+        await time.increase(setback);
+        expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(newDelay);
+      });
 
-        await time.increase(oldDelay.sub(newDelay));
+      it('increasing the delay has delay effect #2', async function () {
+        const oldDelay = time.duration.days(30); // more than the minsetback
+        const newDelay = web3.utils.toBN(10);
+
+        await this.manager.$_setGrantDelay(GROUPS.SOME, oldDelay);
+        await time.increase(MINSETBACK);
 
+        expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
+
+        const { receipt } = await this.manager.setGrantDelay(GROUPS.SOME, newDelay, { from: admin });
+        const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
+        const setback = web3.utils.BN.max(MINSETBACK, oldDelay.sub(newDelay));
+
+        expect(setback).to.be.bignumber.gt(MINSETBACK);
+        expectEvent(receipt, 'GroupGrantDelayChanged', {
+          groupId: GROUPS.SOME,
+          delay: newDelay,
+          since: timestamp.add(setback),
+        });
+
+        expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(oldDelay);
+        await time.increase(setback);
         expect(await this.manager.getGroupGrantDelay(GROUPS.SOME)).to.be.bignumber.equal(newDelay);
       });
 
@@ -1050,8 +1106,8 @@ contract('AccessManager', function (accounts) {
 
   // TODO: test all admin functions
   describe('class delays', function () {
-    const otherClassId = '2';
-    const delay = '1000';
+    const otherClassId = web3.utils.toBN(2);
+    const delay = web3.utils.toBN(1000);
 
     beforeEach('set contract class', async function () {
       this.target = await AccessManagedTarget.new(this.manager.address);
@@ -1065,9 +1121,7 @@ contract('AccessManager', function (accounts) {
       await this.call();
     });
 
-    // TODO: here we need to check increase and decrease.
-    // - Increasing should have immediate effect
-    // - Decreasing should take time.
+    // TODO: here we need to check increase and decrease. Both should have be affected by the minsetback.
     describe('with delay', function () {
       beforeEach('set admin delay', async function () {
         this.tx = await this.manager.setClassAdminDelay(classId, delay, { from: admin });
@@ -1077,28 +1131,38 @@ contract('AccessManager', function (accounts) {
       });
 
       it('emits event and sets delay', async function () {
-        const since = await clockFromReceipt.timestamp(this.tx.receipt).then(web3.utils.toBN);
-        expectEvent(this.tx.receipt, 'ClassAdminDelayUpdated', { classId, delay, since });
+        const timepoint = await clockFromReceipt.timestamp(this.tx.receipt).then(web3.utils.toBN);
 
-        expect(await this.manager.getClassAdminDelay(classId)).to.be.bignumber.equal(delay);
-      });
+        expectEvent(this.tx.receipt, 'ClassAdminDelayUpdated', { classId, delay, since: timepoint.add(MINSETBACK) });
 
-      it('without prior scheduling: reverts', async function () {
-        await expectRevertCustomError(this.call(), 'AccessManagerNotScheduled', [this.opId]);
+        // wait for delay to become active
+        expect(await this.manager.getClassAdminDelay(classId)).to.be.bignumber.equal('0');
+        await time.increase(MINSETBACK);
+        expect(await this.manager.getClassAdminDelay(classId)).to.be.bignumber.equal(delay);
       });
 
-      describe('with prior scheduling', async function () {
-        beforeEach('schedule', async function () {
-          await this.manager.schedule(this.manager.address, this.data, 0, { from: admin });
+      describe('after setback', function () {
+        beforeEach('wait', async function () {
+          await time.increase(MINSETBACK);
         });
 
-        it('without delay: reverts', async function () {
-          await expectRevertCustomError(this.call(), 'AccessManagerNotReady', [this.opId]);
+        it('without prior scheduling: reverts', async function () {
+          await expectRevertCustomError(this.call(), 'AccessManagerNotScheduled', [this.opId]);
         });
 
-        it('with delay: succeeds', async function () {
-          await time.increase(delay);
-          await this.call();
+        describe('with prior scheduling', async function () {
+          beforeEach('schedule', async function () {
+            await this.manager.schedule(this.manager.address, this.data, 0, { from: admin });
+          });
+
+          it('without delay: reverts', async function () {
+            await expectRevertCustomError(this.call(), 'AccessManagerNotReady', [this.opId]);
+          });
+
+          it('with delay: succeeds', async function () {
+            await time.increase(delay);
+            await this.call();
+          });
         });
       });
     });