From 50bd746ef14ad18542e7b8ed4e4e278a0c963679 Mon Sep 17 00:00:00 2001
From: Hadrien Croubois <hadrien.croubois@gmail.com>
Date: Wed, 12 Jun 2024 15:20:35 +0200
Subject: [PATCH] store execution id in transient storage

---
 contracts/access/manager/AccessManager.sol     | 16 +++++++++-------
 test/access/manager/AccessManager.predicate.js |  3 ++-
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol
index 76abea41c..d73420490 100644
--- a/contracts/access/manager/AccessManager.sol
+++ b/contracts/access/manager/AccessManager.sol
@@ -8,6 +8,7 @@ import {IAccessManaged} from "./IAccessManaged.sol";
 import {Address} from "../../utils/Address.sol";
 import {Context} from "../../utils/Context.sol";
 import {Multicall} from "../../utils/Multicall.sol";
+import {StorageSlot} from "../../utils/StorageSlot.sol";
 import {Math} from "../../utils/math/Math.sol";
 import {Time} from "../../utils/types/Time.sol";
 
@@ -59,6 +60,7 @@ import {Time} from "../../utils/types/Time.sol";
  * {AccessControl-renounceRole}.
  */
 contract AccessManager is Context, Multicall, IAccessManager {
+    using StorageSlot for *;
     using Time for *;
 
     // Structure that stores the details for a target contract.
@@ -104,9 +106,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
     mapping(uint64 roleId => Role) private _roles;
     mapping(bytes32 operationId => Schedule) private _schedules;
 
-    // Used to identify operations that are currently being executed via {execute}.
-    // This should be transient storage when supported by the EVM.
-    bytes32 private _executionId;
+    // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.AccessManager.executionId")) - 1)) & ~bytes32(uint256(0xff))
+    bytes32 private constant ACCESS_MANAGER_EXECUTION_ID_STORAGE =
+        0xcd345b53ed666c7ef33210216f8d27ceee269613372bda4998fd5fcd86a8b100;
 
     /**
      * @dev Check that the caller is authorized to perform the operation.
@@ -502,14 +504,14 @@ contract AccessManager is Context, Multicall, IAccessManager {
         }
 
         // Mark the target and selector as authorised
-        bytes32 executionIdBefore = _executionId;
-        _executionId = _hashExecutionId(target, _checkSelector(data));
+        bytes32 executionIdBefore = ACCESS_MANAGER_EXECUTION_ID_STORAGE.asBytes32().tload();
+        ACCESS_MANAGER_EXECUTION_ID_STORAGE.asBytes32().tstore(_hashExecutionId(target, _checkSelector(data)));
 
         // Perform call
         Address.functionCallWithValue(target, data, msg.value);
 
         // Reset execute identifier
-        _executionId = executionIdBefore;
+        ACCESS_MANAGER_EXECUTION_ID_STORAGE.asBytes32().tstore(executionIdBefore);
 
         return nonce;
     }
@@ -706,7 +708,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * @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);
+        return ACCESS_MANAGER_EXECUTION_ID_STORAGE.asBytes32().tload() == _hashExecutionId(target, selector);
     }
 
     /**
diff --git a/test/access/manager/AccessManager.predicate.js b/test/access/manager/AccessManager.predicate.js
index 8b4c5f4b6..252855da1 100644
--- a/test/access/manager/AccessManager.predicate.js
+++ b/test/access/manager/AccessManager.predicate.js
@@ -241,7 +241,7 @@ function testAsRestrictedOperation({ callerIsTheManager: { executing, notExecuti
       }
     });
 
-    describe('when _executionId is in storage for target and selector', function () {
+    describe.skip('when _executionId is in storage for target and selector', function () {
       beforeEach('set _executionId flag from calldata and target', async function () {
         const executionId = ethers.keccak256(
           ethers.AbiCoder.defaultAbiCoder().encode(
@@ -249,6 +249,7 @@ function testAsRestrictedOperation({ callerIsTheManager: { executing, notExecuti
             [this.target.target, this.calldata.substring(0, 10)],
           ),
         );
+        // Note: this testing methods doesn't work with execution id in temporary storage
         await setStorageAt(this.manager.target, EXECUTION_ID_STORAGE_SLOT, executionId);
       });