From 64da2c10a41db51c1a7db14fd0700f50ba1ccdcd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= <ernestognw@gmail.com>
Date: Tue, 19 Sep 2023 08:35:42 -0600
Subject: [PATCH] Fix `AccessManager._checkAuthorized` in `execute` context
 (#4612)

Co-authored-by: Francisco <fg@frang.io>
---
 contracts/access/manager/AccessManager.sol | 50 +++++++++++++++-------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol
index e83b8a4da..47dc47f73 100644
--- a/contracts/access/manager/AccessManager.sol
+++ b/contracts/access/manager/AccessManager.sol
@@ -136,7 +136,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
         } else 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 (_executionId == _hashExecutionId(target, selector), 0);
+            return (_isExecuting(target, selector), 0);
         } else {
             uint64 roleId = getTargetFunctionRole(target, selector);
             (bool isMember, uint32 currentDelay) = hasRole(roleId, caller);
@@ -760,7 +760,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      */
     function _checkAuthorized() private {
         address caller = _msgSender();
-        (bool immediate, uint32 delay) = _canCallExtended(caller, address(this), _msgData());
+        (bool immediate, uint32 delay) = _canCallSelf(caller, _msgData());
         if (!immediate) {
             if (delay == 0) {
                 (, uint64 requiredRole, ) = _getAdminRestrictions(_msgData());
@@ -833,25 +833,45 @@ contract AccessManager is Context, Multicall, IAccessManager {
      */
     function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool, uint32) {
         if (target == address(this)) {
-            (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
-            if (!enabled) {
-                return (false, 0);
-            }
-
-            (bool inRole, uint32 executionDelay) = hasRole(roleId, caller);
-            if (!inRole) {
-                return (false, 0);
-            }
-
-            // downcast is safe because both options are uint32
-            uint32 delay = uint32(Math.max(operationDelay, executionDelay));
-            return (delay == 0, delay);
+            return _canCallSelf(caller, data);
         } else {
             bytes4 selector = bytes4(data);
             return canCall(caller, target, selector);
         }
     }
 
+    /**
+     * @dev A version of {canCall} that checks for admin restrictions in this contract.
+     */
+    function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) {
+        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), bytes4(data)), 0);
+        }
+
+        (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
+        if (!enabled) {
+            return (false, 0);
+        }
+
+        (bool inRole, uint32 executionDelay) = hasRole(roleId, caller);
+        if (!inRole) {
+            return (false, 0);
+        }
+
+        // downcast is safe because both options are uint32
+        delay = uint32(Math.max(operationDelay, executionDelay));
+        return (delay == 0, delay);
+    }
+
+    /**
+     * @dev Returns true if a call with `target` and `selector` is being executed via {executed}.
+     */
+    function _isExecuting(address target, bytes4 selector) private view returns (bool) {
+        return _executionId == _hashExecutionId(target, selector);
+    }
+
     /**
      * @dev Returns true if a schedule timepoint is past its expiration deadline.
      */