From 37f874f717548736b0d44c7b2f0e68b0e5016625 Mon Sep 17 00:00:00 2001
From: ernestognw <ernestognw@gmail.com>
Date: Wed, 10 Apr 2024 14:37:32 -0600
Subject: [PATCH 1/8] Extend `onlyAuthorized` to support extra functions

---
 .changeset/light-news-listen.md               |  5 ++
 contracts/access/manager/AccessManager.sol    | 37 +++++++-----
 contracts/mocks/AccessManagerMock.sol         | 21 +++++++
 test/access/manager/AccessManager.behavior.js | 53 +++++++++++++++++
 test/access/manager/AccessManager.test.js     | 57 ++++++++++++++++++-
 5 files changed, 159 insertions(+), 14 deletions(-)
 create mode 100644 .changeset/light-news-listen.md
 create mode 100644 contracts/mocks/AccessManagerMock.sol

diff --git a/.changeset/light-news-listen.md b/.changeset/light-news-listen.md
new file mode 100644
index 00000000000..02eda145394
--- /dev/null
+++ b/.changeset/light-news-listen.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`AccessManager`: Allow the `onlyAuthorize` modifier to restrict functions added to the manager.
diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol
index e59c50e665b..4711a88fedd 100644
--- a/contracts/access/manager/AccessManager.sol
+++ b/contracts/access/manager/AccessManager.sol
@@ -586,14 +586,15 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     // ================================================= ADMIN LOGIC ==================================================
     /**
-     * @dev Check if the current call is authorized according to admin logic.
+     * @dev Check if the current call is authorized according to admin and roles logic.
+     *
+     * WARNING: Carefully review the considerations of {AccessManaged-restricted} since they apply to this modifier.
      */
     function _checkAuthorized() private {
         address caller = _msgSender();
-        (bool immediate, uint32 delay) = _canCallSelf(caller, _msgData());
+        (bool immediate, uint64 requiredRole, uint32 delay) = _canCallSelf(caller, _msgData());
         if (!immediate) {
             if (delay == 0) {
-                (, uint64 requiredRole, ) = _getAdminRestrictions(_msgData());
                 revert AccessManagerUnauthorizedAccount(caller, requiredRole);
             } else {
                 _consumeScheduledOp(hashOperation(caller, address(this), _msgData()));
@@ -666,39 +667,49 @@ contract AccessManager is Context, Multicall, IAccessManager {
         bytes calldata data
     ) private view returns (bool immediate, uint32 delay) {
         if (target == address(this)) {
-            return _canCallSelf(caller, data);
+            (immediate, , delay) = _canCallSelf(caller, data);
+            return (immediate, delay);
         } else {
             return data.length < 4 ? (false, 0) : canCall(caller, target, _checkSelector(data));
         }
     }
 
     /**
-     * @dev A version of {canCall} that checks for admin restrictions in this contract.
+     * @dev A version of {canCall} that checks for restrictions in this contract.
      */
-    function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) {
+    function _canCallSelf(
+        address caller,
+        bytes calldata data
+    ) private view returns (bool immediate, uint64 requiredRole, uint32 delay) {
         if (data.length < 4) {
-            return (false, 0);
+            return (false, 0, 0);
         }
 
+        bytes4 selector = _checkSelector(data);
+
+        (bool isAdminRestriction, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
+
         if (caller == address(this)) {
             // Caller is AccessManager, this means the call was sent through {execute} and it already checked
             // permissions. We verify that the call "identifier", which is set during {execute}, is correct.
-            return (_isExecuting(address(this), _checkSelector(data)), 0);
+            return (_isExecuting(address(this), selector), roleId, 0);
         }
 
-        (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
-        if (!enabled) {
-            return (false, 0);
+        if (!isAdminRestriction) {
+            if (isTargetClosed(address(this))) {
+                return (false, roleId, 0);
+            }
+            roleId = getTargetFunctionRole(address(this), selector);
         }
 
         (bool inRole, uint32 executionDelay) = hasRole(roleId, caller);
         if (!inRole) {
-            return (false, 0);
+            return (false, roleId, 0);
         }
 
         // downcast is safe because both options are uint32
         delay = uint32(Math.max(operationDelay, executionDelay));
-        return (delay == 0, delay);
+        return (delay == 0, roleId, delay);
     }
 
     /**
diff --git a/contracts/mocks/AccessManagerMock.sol b/contracts/mocks/AccessManagerMock.sol
new file mode 100644
index 00000000000..952c761199d
--- /dev/null
+++ b/contracts/mocks/AccessManagerMock.sol
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+import {AccessManager} from "../access/manager/AccessManager.sol";
+import {StorageSlot} from "../utils/StorageSlot.sol";
+
+contract AccessManagerMock is AccessManager {
+    event CalledRestricted(address caller);
+    event CalledUnrestricted(address caller);
+
+    constructor(address initialAdmin) AccessManager(initialAdmin) {}
+
+    function fnRestricted() public onlyAuthorized {
+        emit CalledRestricted(msg.sender);
+    }
+
+    function fnUnrestricted() public {
+        emit CalledUnrestricted(msg.sender);
+    }
+}
diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js
index c9e236eb0f8..27c601d48c4 100644
--- a/test/access/manager/AccessManager.behavior.js
+++ b/test/access/manager/AccessManager.behavior.js
@@ -193,9 +193,62 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
   });
 }
 
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeASelfRestrictedOperation() {
+  const getAccessPath = LIKE_COMMON_GET_ACCESS;
+
+  function testScheduleOperation(mineDelay) {
+    return function self() {
+      self.mineDelay = mineDelay;
+      beforeEach('sets execution delay', async function () {
+        this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
+      });
+      testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE);
+    };
+  }
+
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay =
+    testScheduleOperation(true);
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = testScheduleOperation(false);
+
+  beforeEach('set target as manager', function () {
+    this.target = this.manager;
+  });
+
+  const isExecutingPath = LIKE_COMMON_IS_EXECUTING;
+  isExecutingPath.notExecuting = function () {
+    it('reverts as AccessManagerUnauthorizedAccount', async function () {
+      await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata }))
+        .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount')
+        .withArgs(this.caller, 0); // There's no way to know the required role of a function executed by the manager since the selector will be that of "execute"
+    });
+  };
+
+  testAsRestrictedOperation({
+    callerIsTheManager: isExecutingPath,
+    callerIsNotTheManager() {
+      testAsHasRole({
+        publicRoleIsRequired() {
+          it('succeeds called directly', async function () {
+            await this.caller.sendTransaction({ to: this.target, data: this.calldata });
+          });
+
+          it('succeeds via execute', async function () {
+            await this.manager.connect(this.caller).execute(this.target, this.calldata);
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      });
+    },
+  });
+}
+
 module.exports = {
   shouldBehaveLikeDelayedAdminOperation,
   shouldBehaveLikeNotDelayedAdminOperation,
   shouldBehaveLikeRoleAdminOperation,
   shouldBehaveLikeAManagedRestrictedOperation,
+  shouldBehaveLikeASelfRestrictedOperation,
 };
diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js
index 07d7b5a19c5..94a940b4779 100644
--- a/test/access/manager/AccessManager.test.js
+++ b/test/access/manager/AccessManager.test.js
@@ -23,6 +23,7 @@ const {
   shouldBehaveLikeNotDelayedAdminOperation,
   shouldBehaveLikeRoleAdminOperation,
   shouldBehaveLikeAManagedRestrictedOperation,
+  shouldBehaveLikeASelfRestrictedOperation,
 } = require('./AccessManager.behavior');
 
 const {
@@ -48,7 +49,7 @@ async function fixture() {
   roles.SOME.members = [member];
   roles.PUBLIC.members = [admin, roleAdmin, roleGuardian, member, user, other];
 
-  const manager = await ethers.deployContract('$AccessManager', [admin]);
+  const manager = await ethers.deployContract('$AccessManagerMock', [admin]);
   const target = await ethers.deployContract('$AccessManagedTarget', [manager]);
 
   for (const { id: roleId, admin, guardian, members } of Object.values(roles)) {
@@ -1670,6 +1671,60 @@ describe('AccessManager', function () {
     });
   });
 
+  describe('access managed self operations', function () {
+    describe('when calling a restricted target function', function () {
+      beforeEach('set required role', function () {
+        this.method = this.target.fnRestricted.getFragment();
+        this.role = { id: 785913n };
+        this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
+      });
+
+      describe('restrictions', function () {
+        beforeEach('set method and args', function () {
+          this.calldata = this.target.interface.encodeFunctionData(this.method, []);
+          this.caller = this.user;
+        });
+
+        shouldBehaveLikeASelfRestrictedOperation();
+      });
+
+      it('succeeds called by a role member', async function () {
+        await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
+
+        await expect(
+          this.target.connect(this.user)[this.method.selector]({
+            data: this.calldata,
+          }),
+        )
+          .to.emit(this.target, 'CalledRestricted')
+          .withArgs(this.user);
+      });
+    });
+
+    describe('when calling a non-restricted target function', function () {
+      const method = 'fnUnrestricted()';
+
+      beforeEach('set required role', async function () {
+        this.role = { id: 879435n };
+        await this.manager.$_setTargetFunctionRole(
+          this.target,
+          this.target[method].getFragment().selector,
+          this.role.id,
+        );
+      });
+
+      it('succeeds called by anyone', async function () {
+        await expect(
+          this.target.connect(this.user)[method]({
+            data: this.calldata,
+          }),
+        )
+          .to.emit(this.target, 'CalledUnrestricted')
+          .withArgs(this.user);
+      });
+    });
+  });
+
   describe('access managed target operations', function () {
     describe('when calling a restricted target function', function () {
       beforeEach('set required role', function () {

From d5be4dddab80602c521a48b97fb2f813b7af7b90 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= <ernestognw@gmail.com>
Date: Wed, 10 Apr 2024 15:13:04 -0600
Subject: [PATCH 2/8] Update .changeset/light-news-listen.md

---
 .changeset/light-news-listen.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.changeset/light-news-listen.md b/.changeset/light-news-listen.md
index 02eda145394..1572d908139 100644
--- a/.changeset/light-news-listen.md
+++ b/.changeset/light-news-listen.md
@@ -2,4 +2,4 @@
 'openzeppelin-solidity': minor
 ---
 
-`AccessManager`: Allow the `onlyAuthorize` modifier to restrict functions added to the manager.
+`AccessManager`: Allow the `onlyAuthorized` modifier to restrict functions added to the manager.

From f2d58aa4e99dd7880b2518e7b1afcb0819d8f153 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 11 Apr 2024 21:29:28 +0200
Subject: [PATCH 3/8] simplification

---
 contracts/access/manager/AccessManager.sol | 37 +++++++++-------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol
index 4711a88fedd..382bfcd7e4a 100644
--- a/contracts/access/manager/AccessManager.sol
+++ b/contracts/access/manager/AccessManager.sol
@@ -592,9 +592,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
      */
     function _checkAuthorized() private {
         address caller = _msgSender();
-        (bool immediate, uint64 requiredRole, uint32 delay) = _canCallSelf(caller, _msgData());
+        (bool immediate, uint32 delay) = _canCallSelf(caller, _msgData());
         if (!immediate) {
             if (delay == 0) {
+                (, uint64 requiredRole, ) = _getAdminRestrictions(_msgData());
                 revert AccessManagerUnauthorizedAccount(caller, requiredRole);
             } else {
                 _consumeScheduledOp(hashOperation(caller, address(this), _msgData()));
@@ -612,7 +613,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      */
     function _getAdminRestrictions(
         bytes calldata data
-    ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) {
+    ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) {
         if (data.length < 4) {
             return (false, 0, 0);
         }
@@ -649,7 +650,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
             return (true, getRoleAdmin(roleId), 0);
         }
 
-        return (false, 0, 0);
+        return (false, getTargetFunctionRole(address(this), selector), 0);
     }
 
     // =================================================== HELPERS ====================================================
@@ -667,8 +668,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
         bytes calldata data
     ) private view returns (bool immediate, uint32 delay) {
         if (target == address(this)) {
-            (immediate, , delay) = _canCallSelf(caller, data);
-            return (immediate, delay);
+            return _canCallSelf(caller, data);
         } else {
             return data.length < 4 ? (false, 0) : canCall(caller, target, _checkSelector(data));
         }
@@ -677,39 +677,32 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev A version of {canCall} that checks for restrictions in this contract.
      */
-    function _canCallSelf(
-        address caller,
-        bytes calldata data
-    ) private view returns (bool immediate, uint64 requiredRole, uint32 delay) {
+    function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) {
         if (data.length < 4) {
-            return (false, 0, 0);
+            return (false, 0);
         }
 
-        bytes4 selector = _checkSelector(data);
-
-        (bool isAdminRestriction, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
-
         if (caller == address(this)) {
             // Caller is AccessManager, this means the call was sent through {execute} and it already checked
             // permissions. We verify that the call "identifier", which is set during {execute}, is correct.
-            return (_isExecuting(address(this), selector), roleId, 0);
+            return (_isExecuting(address(this), _checkSelector(data)), 0);
         }
 
-        if (!isAdminRestriction) {
-            if (isTargetClosed(address(this))) {
-                return (false, roleId, 0);
-            }
-            roleId = getTargetFunctionRole(address(this), selector);
+        (bool adminRestricted, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
+
+        // isTragetClosed apply to non-admin-restricted function
+        if (!adminRestricted && isTargetClosed(address(this))) {
+            return (false, 0);
         }
 
         (bool inRole, uint32 executionDelay) = hasRole(roleId, caller);
         if (!inRole) {
-            return (false, roleId, 0);
+            return (false, 0);
         }
 
         // downcast is safe because both options are uint32
         delay = uint32(Math.max(operationDelay, executionDelay));
-        return (delay == 0, roleId, delay);
+        return (delay == 0, delay);
     }
 
     /**

From d0fd2ab9b3d89b7bf42e0b1d4a490d5c283aceb8 Mon Sep 17 00:00:00 2001
From: ernestognw <ernestognw@gmail.com>
Date: Thu, 11 Apr 2024 13:46:55 -0600
Subject: [PATCH 4/8] Address test comments

---
 test/access/manager/AccessManager.behavior.js |  2 +-
 test/access/manager/AccessManager.test.js     | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js
index 27c601d48c4..cfc567cebc6 100644
--- a/test/access/manager/AccessManager.behavior.js
+++ b/test/access/manager/AccessManager.behavior.js
@@ -194,7 +194,7 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
 }
 
 /**
- * @requires this.{manager,roles,calldata,role}
+ * @requires this.{target,manager,roles,calldata,role}
  */
 function shouldBehaveLikeASelfRestrictedOperation() {
   const getAccessPath = LIKE_COMMON_GET_ACCESS;
diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js
index 94a940b4779..0b15395f54d 100644
--- a/test/access/manager/AccessManager.test.js
+++ b/test/access/manager/AccessManager.test.js
@@ -1674,15 +1674,16 @@ describe('AccessManager', function () {
   describe('access managed self operations', function () {
     describe('when calling a restricted target function', function () {
       beforeEach('set required role', function () {
-        this.method = this.target.fnRestricted.getFragment();
+        this.method = this.manager.fnRestricted.getFragment();
         this.role = { id: 785913n };
         this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
       });
 
       describe('restrictions', function () {
         beforeEach('set method and args', function () {
-          this.calldata = this.target.interface.encodeFunctionData(this.method, []);
+          this.calldata = this.manager.interface.encodeFunctionData(this.method, []);
           this.caller = this.user;
+          this.target = this.manager; // For shouldBehaveLikeASelfRestrictedOperation
         });
 
         shouldBehaveLikeASelfRestrictedOperation();
@@ -1692,11 +1693,11 @@ describe('AccessManager', function () {
         await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
 
         await expect(
-          this.target.connect(this.user)[this.method.selector]({
+          this.manager.connect(this.user)[this.method.selector]({
             data: this.calldata,
           }),
         )
-          .to.emit(this.target, 'CalledRestricted')
+          .to.emit(this.manager, 'CalledRestricted')
           .withArgs(this.user);
       });
     });
@@ -1707,19 +1708,19 @@ describe('AccessManager', function () {
       beforeEach('set required role', async function () {
         this.role = { id: 879435n };
         await this.manager.$_setTargetFunctionRole(
-          this.target,
-          this.target[method].getFragment().selector,
+          this.manager,
+          this.manager[method].getFragment().selector,
           this.role.id,
         );
       });
 
       it('succeeds called by anyone', async function () {
         await expect(
-          this.target.connect(this.user)[method]({
+          this.manager.connect(this.user)[method]({
             data: this.calldata,
           }),
         )
-          .to.emit(this.target, 'CalledUnrestricted')
+          .to.emit(this.manager, 'CalledUnrestricted')
           .withArgs(this.user);
       });
     });

From 3bda5441fd76a6429ed3479d40109e744b8d00c4 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 23 May 2024 15:36:03 +0200
Subject: [PATCH 5/8] fix partch

---
 certora/diff/access_manager_AccessManager.sol.patch | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/certora/diff/access_manager_AccessManager.sol.patch b/certora/diff/access_manager_AccessManager.sol.patch
index 29ff9234677..cfb6cdf2f06 100644
--- a/certora/diff/access_manager_AccessManager.sol.patch
+++ b/certora/diff/access_manager_AccessManager.sol.patch
@@ -64,8 +64,8 @@
       */
      function _getAdminRestrictions(
          bytes calldata data
--    ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) {
-+    ) internal view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV
+-    ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) {
++    ) internal view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV
          if (data.length < 4) {
              return (false, 0, 0);
          }

From 8306341ea18d37bfbdc3ef476a8db0088e2e3ef9 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Thu, 23 May 2024 16:15:34 +0200
Subject: [PATCH 6/8] fix tests

---
 test/access/manager/AccessManager.behavior.js |  2 +-
 test/access/manager/AccessManager.test.js     | 51 ++++++++-----------
 2 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js
index cfc567cebc6..7810535aa01 100644
--- a/test/access/manager/AccessManager.behavior.js
+++ b/test/access/manager/AccessManager.behavior.js
@@ -222,7 +222,7 @@ function shouldBehaveLikeASelfRestrictedOperation() {
     it('reverts as AccessManagerUnauthorizedAccount', async function () {
       await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata }))
         .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount')
-        .withArgs(this.caller, 0); // There's no way to know the required role of a function executed by the manager since the selector will be that of "execute"
+        .withArgs(this.caller, this.role?.id ?? 0n);
     });
   };
 
diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js
index 0b15395f54d..3936802156b 100644
--- a/test/access/manager/AccessManager.test.js
+++ b/test/access/manager/AccessManager.test.js
@@ -1673,17 +1673,21 @@ describe('AccessManager', function () {
 
   describe('access managed self operations', function () {
     describe('when calling a restricted target function', function () {
-      beforeEach('set required role', function () {
-        this.method = this.manager.fnRestricted.getFragment();
+      const method = 'fnRestricted()';
+
+      beforeEach('set required role', async function () {
         this.role = { id: 785913n };
-        this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
+        await this.manager.$_setTargetFunctionRole(
+          this.manager,
+          this.manager[method].getFragment().selector,
+          this.role.id,
+        );
       });
 
       describe('restrictions', function () {
         beforeEach('set method and args', function () {
-          this.calldata = this.manager.interface.encodeFunctionData(this.method, []);
           this.caller = this.user;
-          this.target = this.manager; // For shouldBehaveLikeASelfRestrictedOperation
+          this.calldata = this.manager.interface.encodeFunctionData(method, []);
         });
 
         shouldBehaveLikeASelfRestrictedOperation();
@@ -1692,11 +1696,7 @@ describe('AccessManager', function () {
       it('succeeds called by a role member', async function () {
         await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
 
-        await expect(
-          this.manager.connect(this.user)[this.method.selector]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.manager.connect(this.user)[method]())
           .to.emit(this.manager, 'CalledRestricted')
           .withArgs(this.user);
       });
@@ -1715,11 +1715,7 @@ describe('AccessManager', function () {
       });
 
       it('succeeds called by anyone', async function () {
-        await expect(
-          this.manager.connect(this.user)[method]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.manager.connect(this.user)[method]())
           .to.emit(this.manager, 'CalledUnrestricted')
           .withArgs(this.user);
       });
@@ -1728,16 +1724,21 @@ describe('AccessManager', function () {
 
   describe('access managed target operations', function () {
     describe('when calling a restricted target function', function () {
-      beforeEach('set required role', function () {
-        this.method = this.target.fnRestricted.getFragment();
+      const method = 'fnRestricted()';
+
+      beforeEach('set required role', async function () {
         this.role = { id: 3597243n };
-        this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
+        await this.manager.$_setTargetFunctionRole(
+          this.target,
+          this.target[method].getFragment().selector,
+          this.role.id,
+        );
       });
 
       describe('restrictions', function () {
         beforeEach('set method and args', function () {
-          this.calldata = this.target.interface.encodeFunctionData(this.method, []);
           this.caller = this.user;
+          this.calldata = this.target.interface.encodeFunctionData(method, []);
         });
 
         shouldBehaveLikeAManagedRestrictedOperation();
@@ -1746,11 +1747,7 @@ describe('AccessManager', function () {
       it('succeeds called by a role member', async function () {
         await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
 
-        await expect(
-          this.target.connect(this.user)[this.method.selector]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.target.connect(this.user)[method]())
           .to.emit(this.target, 'CalledRestricted')
           .withArgs(this.user);
       });
@@ -1769,11 +1766,7 @@ describe('AccessManager', function () {
       });
 
       it('succeeds called by anyone', async function () {
-        await expect(
-          this.target.connect(this.user)[method]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.target.connect(this.user)[method]())
           .to.emit(this.target, 'CalledUnrestricted')
           .withArgs(this.user);
       });

From 33a21f80111c342f8973a3c80e546c9e32fb498e Mon Sep 17 00:00:00 2001
From: ernestognw <ernestognw@gmail.com>
Date: Fri, 24 May 2024 14:33:38 -0600
Subject: [PATCH 7/8] Allow closing the AccessManager

---
 contracts/access/manager/AccessManager.sol    |  3 ---
 contracts/access/manager/IAccessManager.sol   |  7 +++--
 test/access/manager/AccessManager.behavior.js | 27 ++++++++++---------
 test/access/manager/AccessManager.test.js     | 16 ++++++++---
 4 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol
index 382bfcd7e4a..76abea41c9a 100644
--- a/contracts/access/manager/AccessManager.sol
+++ b/contracts/access/manager/AccessManager.sol
@@ -412,9 +412,6 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * Emits a {TargetClosed} event.
      */
     function _setTargetClosed(address target, bool closed) internal virtual {
-        if (target == address(this)) {
-            revert AccessManagerLockedAccount(target);
-        }
         _targets[target].closed = closed;
         emit TargetClosed(target, closed);
     }
diff --git a/contracts/access/manager/IAccessManager.sol b/contracts/access/manager/IAccessManager.sol
index 697301d1c4a..50637e90dbf 100644
--- a/contracts/access/manager/IAccessManager.sol
+++ b/contracts/access/manager/IAccessManager.sol
@@ -82,7 +82,6 @@ interface IAccessManager {
     error AccessManagerNotScheduled(bytes32 operationId);
     error AccessManagerNotReady(bytes32 operationId);
     error AccessManagerExpired(bytes32 operationId);
-    error AccessManagerLockedAccount(address account);
     error AccessManagerLockedRole(uint64 roleId);
     error AccessManagerBadConfirmation();
     error AccessManagerUnauthorizedAccount(address msgsender, uint64 roleId);
@@ -108,7 +107,7 @@ interface IAccessManager {
      * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail
      * to identify the indirect workflow, and will consider calls that require a delay to be forbidden.
      *
-     * NOTE: This function does not report the permissions of this manager itself. These are defined by the
+     * NOTE: This function does not report the permissions of the admin functions in the manager itself. These are defined by the
      * {AccessManager} documentation.
      */
     function canCall(
@@ -134,6 +133,8 @@ interface IAccessManager {
 
     /**
      * @dev Get whether the contract is closed disabling any access. Otherwise role permissions are applied.
+     *
+     * NOTE: When the manager itself is closed, admin functions are still accessible to avoid locking the contract.
      */
     function isTargetClosed(address target) external view returns (bool);
 
@@ -308,6 +309,8 @@ interface IAccessManager {
     /**
      * @dev Set the closed flag for a contract.
      *
+     * Closing the manager itself won't disable access to admin methods to avoid locking the contract.
+     *
      * Requirements:
      *
      * - the caller must be a global admin
diff --git a/test/access/manager/AccessManager.behavior.js b/test/access/manager/AccessManager.behavior.js
index 7810535aa01..385da578c0b 100644
--- a/test/access/manager/AccessManager.behavior.js
+++ b/test/access/manager/AccessManager.behavior.js
@@ -197,6 +197,14 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
  * @requires this.{target,manager,roles,calldata,role}
  */
 function shouldBehaveLikeASelfRestrictedOperation() {
+  function revertUnauthorized() {
+    it('reverts as AccessManagerUnauthorizedAccount', async function () {
+      await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata }))
+        .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount')
+        .withArgs(this.caller, this.role?.id ?? 0n);
+    });
+  }
+
   const getAccessPath = LIKE_COMMON_GET_ACCESS;
 
   function testScheduleOperation(mineDelay) {
@@ -218,18 +226,13 @@ function shouldBehaveLikeASelfRestrictedOperation() {
   });
 
   const isExecutingPath = LIKE_COMMON_IS_EXECUTING;
-  isExecutingPath.notExecuting = function () {
-    it('reverts as AccessManagerUnauthorizedAccount', async function () {
-      await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata }))
-        .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount')
-        .withArgs(this.caller, this.role?.id ?? 0n);
-    });
-  };
+  isExecutingPath.notExecuting = revertUnauthorized;
 
-  testAsRestrictedOperation({
-    callerIsTheManager: isExecutingPath,
-    callerIsNotTheManager() {
-      testAsHasRole({
+  testAsCanCall({
+    closed: revertUnauthorized,
+    open: {
+      callerIsTheManager: isExecutingPath,
+      callerIsNotTheManager: {
         publicRoleIsRequired() {
           it('succeeds called directly', async function () {
             await this.caller.sendTransaction({ to: this.target, data: this.calldata });
@@ -240,7 +243,7 @@ function shouldBehaveLikeASelfRestrictedOperation() {
           });
         },
         specificRoleIsRequired: getAccessPath,
-      });
+      },
     },
   });
 }
diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js
index 3936802156b..7726831b268 100644
--- a/test/access/manager/AccessManager.test.js
+++ b/test/access/manager/AccessManager.test.js
@@ -1106,10 +1106,18 @@ describe('AccessManager', function () {
           expect(await this.manager.isTargetClosed(this.target)).to.be.false;
         });
 
-        it('reverts if closing the manager', async function () {
-          await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true))
-            .to.be.revertedWithCustomError(this.manager, 'AccessManagerLockedAccount')
-            .withArgs(this.manager);
+        describe('when the target is the manager', async function () {
+          it('closes and opens the manager', async function () {
+            await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true))
+              .to.emit(this.manager, 'TargetClosed')
+              .withArgs(this.manager, true);
+            expect(await this.manager.isTargetClosed(this.manager)).to.be.true;
+
+            await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, false))
+              .to.emit(this.manager, 'TargetClosed')
+              .withArgs(this.manager, false);
+            expect(await this.manager.isTargetClosed(this.manager)).to.be.false;
+          });
         });
       });
 

From 5f81c0f3cc7489a2937ecbe0f6615b53002432b1 Mon Sep 17 00:00:00 2001
From: ernestognw <ernestognw@gmail.com>
Date: Fri, 24 May 2024 14:38:46 -0600
Subject: [PATCH 8/8] Codespell

---
 CODE_OF_CONDUCT.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md
index cd7770dac27..f7cdce98e08 100644
--- a/CODE_OF_CONDUCT.md
+++ b/CODE_OF_CONDUCT.md
@@ -6,7 +6,7 @@ In the interest of fostering an open and welcoming environment, we as
 contributors and maintainers pledge to making participation in our project and
 our community a harassment-free experience for everyone, regardless of age, body
 size, disability, ethnicity, sex characteristics, gender identity and expression,
-level of experience, education, socio-economic status, nationality, personal
+level of experience, education, socioeconomic status, nationality, personal
 appearance, race, religion, or sexual identity and orientation.
 
 ## Our Standards