Fix Review #1

sherlock-admin · 2024-12-05T16:24:09Z

Fix Review of

Repo: Hats-Protocol/hats-zodiac
Commit Hash: a9e3f4f0e968fb332800a468eddcb993fc6d5cd2

sherlock-admin · 2024-12-05T16:24:17Z

hats-zodiac/README.md

+- Delegatecalls can modify Safe state if not properly restricted. Owners should NOT approve delegatecall targets that enable the following:
+  - Directly modifying any of the Safe's state, including the Safe's nonce.
+  - Additional delegatecalls. For example, the MultiSend library that is *not* MultiSendCallOnly should not be approved.
+- HSG validates that approved delegatecalls don't modify critical Safe parameters, but relies on the Safe' nonce to do so.


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:19Z

hats-zodiac/src/HatsSignerGate.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: LGPL-3.0
-pragma solidity >=0.8.13;
+pragma solidity >=0.8.28;


Change related to Hats-Protocol/hats-zodiac#83.

Fixes issues: #19

sherlock-admin · 2024-12-05T16:24:21Z

hats-zodiac/src/HatsSignerGate.sol

@@ -95,28 +95,31 @@ contract HatsSignerGate is
  /*//////////////////////////////////////////////////////////////
                          TRANSIENT STATE
  //////////////////////////////////////////////////////////////*/
-  
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:23Z

hats-zodiac/src/HatsSignerGate.sol

  /// @dev Temporary record of the existing owners on the `safe` when a transaction is submitted
  bytes32 transient _existingOwnersHash;
-  
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:25Z

hats-zodiac/src/HatsSignerGate.sol

  /// @dev Temporary record of the existing threshold on the `safe` when a transaction is submitted
  uint256 transient _existingThreshold;
-  
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:27Z

hats-zodiac/src/HatsSignerGate.sol

  /// @dev Temporary record of the existing fallback handler on the `safe` when a transaction is submitted
  address transient _existingFallbackHandler;
-  
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:29Z

hats-zodiac/src/HatsSignerGate.sol

+
+  /// @dev Temporary record of whether we're inside an execTransaction call
+  bool transient _inSafeExecTransaction;
+
+  /// @dev Temporary record of whether we're inside an execTransactionFromModule call
+  bool transient _inModuleExecTransaction;
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:30Z

hats-zodiac/src/HatsSignerGate.sol

  /// @dev The safe's nonce at the beginning of a transaction
  uint256 transient _initialNonce;
-  
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:32Z

hats-zodiac/src/HatsSignerGate.sol

+  uint256 transient _checkTransactionCounter;
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:34Z

hats-zodiac/src/HatsSignerGate.sol

+  /// @notice Only approved delegatecall targets are allowed.
+  /// @notice This function cannot be entered from within itself, called directly from within Safe.execTransaction, or
+  /// from within an execTransactionFromModule or execTransactionFromModuleReturnData call.


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:36Z

hats-zodiac/src/HatsSignerGate.sol

+    // Disallow entering this function from a) inside a Safe.execTransaction call or b) inside an
+    // execTransactionFromModule call
+    if (_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed();
+    _inSafeExecTransaction = true;
+
+    /// @dev We enforce that this function is called exactly once per Safe.execTransaction call. Leveraging the fact
+    /// that the Safe nonce increments exactly once per Safe.execTransaction call, we require that nonce diff matches
+    /// the number of times this function has been called.
+    // Record the initial nonce of the Safe at the beginning of the transaction


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:38Z

hats-zodiac/src/HatsSignerGate.sol

+    /// that the Safe nonce increments exactly once per Safe.execTransaction call, we require that nonce diff matches
+    /// the number of times this function has been called.
+    // Record the initial nonce of the Safe at the beginning of the transaction
+    if (_checkTransactionCounter == 0) _initialNonce = safe.nonce() - 1;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:40Z

hats-zodiac/src/HatsSignerGate.sol

+    // Increment the entrancy counter
+    _checkTransactionCounter++;
+    // Ensure that this function is called exactly once per Safe.execTransaction call


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:42Z

hats-zodiac/src/HatsSignerGate.sol

+    // Increment the entrancy counter
+    _checkTransactionCounter++;
+    // Ensure that this function is called exactly once per Safe.execTransaction call
+    if (safe.nonce() - _initialNonce != _checkTransactionCounter) revert NoReentryAllowed();


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:44Z

hats-zodiac/src/HatsSignerGate.sol

@@ -450,7 +447,7 @@ contract HatsSignerGate is
        address(0),
        payable(0),
        "",
-        address(0)
+        address(safe)


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:46Z

hats-zodiac/src/HatsSignerGate.sol

@@ -511,18 +508,26 @@ contract HatsSignerGate is
   *         2. changing any modules
   *         3. changing the threshold
   *         4. changing the owners
+   *         5. changing the fallback handler


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:48Z

hats-zodiac/src/HatsSignerGate.sol

+    // Ensure that this is only called in accordance with the Safe execTransaction flow
+    // And that it is not called from inside an execTransactionFromModule call


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:50Z

hats-zodiac/src/HatsSignerGate.sol

   *     CAUTION: If the safe has any authority over the signersHat(s) — i.e. wears their admin hat(s) or is an
   *     eligibility or toggle module — then in some cases protections (3) and (4) may not hold. Proceed with caution if
   *     considering granting such authority to the safe.
   * @dev Modified from
   * https://github.com/gnosis/zodiac-guard-mod/blob/988ebc7b71e352f121a0be5f6ae37e79e47a4541/contracts/ModGuard.sol#L86
   */
  function checkAfterExecution(bytes32, bool) public override {
+    // Ensure that this is only called in accordance with the Safe execTransaction flow
+    // And that it is not called from inside an execTransactionFromModule call
+    if (!_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed();


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:51Z

hats-zodiac/src/HatsSignerGate.sol

+    // Ensure that this is only called in accordance with the Safe execTransaction flow
+    // And that it is not called from inside an execTransactionFromModule call
+    if (!_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed();
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:53Z

hats-zodiac/src/HatsSignerGate.sol

+    // reset the reentrancy guard to enable subsequent legitimate Safe execTransactions in the same transaction
+    _inSafeExecTransaction = false;
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:55Z

hats-zodiac/src/HatsSignerGate.sol

+    // Disallow entering this function from inside a Safe execTransaction call or another execTransactionFromModule call
+    if (_inSafeExecTransaction || _inModuleExecTransaction) revert NoReentryAllowed();


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:57Z

hats-zodiac/src/HatsSignerGate.sol


    // set the reentrancy guard
-    _reentrancyGuard = 1;
+    _inModuleExecTransaction = true;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:24:59Z

hats-zodiac/src/HatsSignerGate.sol

@@ -1015,6 +1016,6 @@ contract HatsSignerGate is
    if (operation_ == Enum.Operation.DelegateCall) _checkSafeState(_safe);

    // reset the reentrancy guard to enable future legitimate calls within the same transaction
-    _reentrancyGuard = 0;
+    _inModuleExecTransaction = false;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:25:01Z

hats-zodiac/src/lib/SafeManagerLib.sol

+  /*//////////////////////////////////////////////////////////////
+                            CUSTOM ERRORS
+  //////////////////////////////////////////////////////////////*/
+
+  /// @notice Emitted when a call to the Safe's execTransactionFromModule fails
+  error SafeTransactionFailed();
+


Change related to Hats-Protocol/hats-zodiac#82.

Fixes issues: #8

sherlock-admin · 2024-12-05T16:25:03Z

hats-zodiac/src/lib/SafeManagerLib.sol

+    bool success =
+      _safe.execTransactionFromModule({ to: address(_safe), value: 0, data: _data, operation: Enum.Operation.Call });
+
+    if (!success) revert SafeTransactionFailed();


Change related to Hats-Protocol/hats-zodiac#82.

Fixes issues: #8

sherlock-admin · 2024-12-05T16:25:05Z

hats-zodiac/test/HatsSignerGate.attacks.t.sol

@@ -4,16 +4,38 @@ pragma solidity ^0.8.13;
 import { Test, console2 } from "../lib/forge-std/src/Test.sol";
 import { WithHSGInstanceTest, Enum } from "./TestSuite.t.sol";
 import { IHatsSignerGate } from "../src/interfaces/IHatsSignerGate.sol";
+import { SafeManagerLib } from "../src/lib/SafeManagerLib.sol";


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:25:07Z

hats-zodiac/test/HatsSignerGate.attacks.t.sol

+  address public maliciousFallbackHandler = makeAddr("maliciousFallbackHandler");
+  address public goodFallbackHandler;
+  bytes public setFallbackAction = abi.encodeWithSignature("setFallbackHandler(address)", maliciousFallbackHandler);
+  bytes public packedCalls;
+  bytes public multiSendData;
+  bytes32 public safeTxHash;
+  bytes public checkTransactionAction;
+  bytes public signatures;
+
+  bytes public checkAfterExecutionAction =
+    abi.encodeWithSignature("checkAfterExecution(bytes32,bool)", bytes32(0), false);
+
+  string public constant CHECK_TRANSACTION_SIGNATURE =
+    "checkTransaction(address,uint256,bytes,uint8,uint256,uint256,uint256,address,address,bytes,address)";
+
+  function setUp() public override {
+    super.setUp();
+
+    goodFallbackHandler = SafeManagerLib.getSafeFallbackHandler(safe);
+  }
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:25:09Z

hats-zodiac/test/HatsSignerGate.attacks.t.sol

  function testSignersCannotAddNewModules() public {
    _addSignersSameHat(2, signerHat);

    bytes memory addModuleData = abi.encodeWithSignature("enableModule(address)", address(tstModule1));

    (bytes memory multisendCall, bytes32 txHash) = _constructSingleActionMultiSendTx(addModuleData);

-    bytes memory signatures = _createNSigsForTx(txHash, 2);
+    signatures = _createNSigsForTx(txHash, 2);


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:25:11Z

hats-zodiac/test/HatsSignerGate.attacks.t.sol

@@ -67,7 +89,7 @@ contract AttacksScenarios is WithHSGInstanceTest {
    // create the tx
    bytes32 txHash = _getTxHash(destAddress, transferValue, Enum.Operation.Call, hex"00", safe);
    // have them sign it
-    bytes memory signatures = _createNSigsForTx(txHash, 2);
+    signatures = _createNSigsForTx(txHash, 2);


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:29Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_delegatecallTargetNotEnabled()
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:31Z

hats-zodiac/test/HatsSignerGate.t.sol

+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:33Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_cannotCallSafe()
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:35Z

hats-zodiac/test/HatsSignerGate.t.sol

+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:37Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_thresholdTooLow(uint8 _operation)
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:39Z

hats-zodiac/test/HatsSignerGate.t.sol

+    // transient state should be cleared
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:41Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_insufficientValidSignatures(uint8 _operation)
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:43Z

hats-zodiac/test/HatsSignerGate.t.sol

+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });
+  }
+
+  function test_revert_inSafeExecTransaction(bool _inModuleExecTransaction)
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(true)
+    inModuleExecTransaction(_inModuleExecTransaction)
+  {
+    vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector);
+    vm.prank(caller);
+    harness.exposed_checkTransaction(
+      address(safe), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0)
+    );
+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:45Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_inModuleExecTransaction(bool _inSafeExecTransaction)
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(_inSafeExecTransaction)
+    inModuleExecTransaction(true)
+  {
+    vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector);
+    vm.prank(caller);
+    harness.exposed_checkTransaction(
+      address(safe), 0, new bytes(0), Enum.Operation.Call, 0, 0, 0, address(0), payable(0), new bytes(0), address(0)
+    );
+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });
+  }
+
+  function test_revert_noReentryAllowed()
+    public
+    callerIsSafe(true)
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:47Z

hats-zodiac/test/HatsSignerGate.t.sol

+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:48Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_happy_checkAfterExecution(bytes32 _txHash, bool _success)
+    public
+    inSafeExecTransaction(true)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:51Z

hats-zodiac/test/HatsSignerGate.t.sol

+
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:53Z

hats-zodiac/test/HatsSignerGate.t.sol

+  function test_revert_guardReverts(bytes32 _txHash, bool _success)
+    public
+    inSafeExecTransaction(true)
+    inModuleExecTransaction(false)
+  {


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:55Z

hats-zodiac/test/HatsSignerGate.t.sol

+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });
+  }
+
+  function test_revert_notInSafeExecTransaction(bytes32 _txHash, bool _success)
+    public
+    inSafeExecTransaction(false)
+    inModuleExecTransaction(false)
+  {
+    // should revert because we are not inside a Safe execTransaction call
+    vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector);
+    harness.exposed_checkAfterExecution(_txHash, _success);
+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });
+  }
+
+  function test_revert_inModuleExecTransaction(bytes32 _txHash, bool _success)
+    public
+    inSafeExecTransaction(true)
+    inModuleExecTransaction(true)
+  {
+    // should revert because we are not inside a Safe execTransaction call
+    vm.expectRevert(IHatsSignerGate.NoReentryAllowed.selector);
+    harness.exposed_checkAfterExecution(_txHash, _success);
+
+    // transient state should be cleared after revert
+    _assertTransientStateVariables({
+      _operation: Enum.Operation(uint8(0)),
+      _existingOwnersHash: bytes32(0),
+      _existingThreshold: 0,
+      _existingFallbackHandler: address(0),
+      _inSafeExecTransaction: false,
+      _inModuleExecTransaction: false,
+      _initialNonce: 0,
+      _checkTransactionCounter: 0
+    });


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:30:57Z

hats-zodiac/test/SafeManagerLib.t.sol

@@ -106,6 +106,7 @@ contract SafeManagerLib_EncodingActions is Test {
 contract SafeManagerLib_ExecutingActions is WithHSGHarnessInstanceTest {
  /// @dev Since execSafeTransactionFromHSG is called by all the other exec* functions, we rely on tests for those
  /// functions to verify that execSafeTransactionFromHSG is working correctly.
+  // TODO: add test for execSafeTransactionFromHSG


Change related to Hats-Protocol/hats-zodiac#82.

Fixes issues: #8

sherlock-admin · 2024-12-05T16:30:59Z

hats-zodiac/test/SafeManagerLib.t.sol

+    // the transaction should revert
+    vm.expectRevert(SafeManagerLib.SafeTransactionFailed.selector);


Change related to Hats-Protocol/hats-zodiac#82.

Fixes issues: #8

sherlock-admin · 2024-12-05T16:31:01Z

hats-zodiac/test/TestSuite.t.sol

+    bool _inSafeExecTransaction,
+    bool _inModuleExecTransaction,


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:04Z

hats-zodiac/test/TestSuite.t.sol

    uint256 _initialNonce,
-    uint256 _entrancyCounter
+    uint256 _checkTransactionCounter


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:06Z

hats-zodiac/test/TestSuite.t.sol

+    {
+      assertEq(uint8(harness.operation()), uint8(_operation), "operation should be set");
+      assertEq(harness.inSafeExecTransaction(), _inSafeExecTransaction, "inSafeExecTransaction should be set");
+      assertEq(harness.inModuleExecTransaction(), _inModuleExecTransaction, "inModuleExecTransaction should be set");
+      assertEq(harness.initialNonce(), _initialNonce, "initial nonce should be set");
+      assertEq(harness.checkTransactionCounter(), _checkTransactionCounter, "checkTransactionCounter should be set");
+      if (_operation == Enum.Operation.DelegateCall) {
+        assertEq(harness.existingOwnersHash(), _existingOwnersHash, "existing owners hash should be set");
+        assertEq(harness.existingThreshold(), _existingThreshold, "existing threshold should be set");
+        assertEq(harness.existingFallbackHandler(), _existingFallbackHandler, "existing fallback handler should be set");
+      } else {
+        assertEq(harness.existingOwnersHash(), bytes32(0), "existing owners hash should be empty");
+        assertEq(harness.existingThreshold(), 0, "existing threshold should be empty");
+        assertEq(harness.existingFallbackHandler(), address(0), "existing fallback handler should be empty");
+      }


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:08Z

hats-zodiac/test/TestSuite.t.sol

+
+  /// @dev Forces the value of the `_inSafeExecTransaction` transient variable
+  modifier inSafeExecTransaction(bool _inSafeExecTransaction) {
+    harness.setInSafeExecTransaction(_inSafeExecTransaction);
+    _;
+  }
+
+  /// @dev Forces the value of the `_inModuleExecTransaction` transient variable
+  modifier inModuleExecTransaction(bool _inModuleExecTransaction) {
+    harness.setInModuleExecTransaction(_inModuleExecTransaction);
+    _;
+  }


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:09Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

+  bool public inModuleExecTransaction;
+  bool public inSafeExecTransaction;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:11Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

  uint256 public initialNonce;
-  uint256 public entrancyCounter;
+  uint256 public checkTransactionCounter;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:13Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

+  function setInModuleExecTransaction(bool inModuleExecTransaction_) public {
+    _inModuleExecTransaction = inModuleExecTransaction_;
+  }
+
+  function setInSafeExecTransaction(bool inSafeExecTransaction_) public {
+    _inSafeExecTransaction = inSafeExecTransaction_;
+  }
+


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:15Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

+  function exposed_inModuleExecTransaction() public view returns (bool) {
+    return _inModuleExecTransaction;
+  }
+
+  function exposed_inSafeExecTransaction() public view returns (bool) {
+    return _inSafeExecTransaction;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:17Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

+  function exposed_checkTransactionCounter() public view returns (uint256) {
+    return _checkTransactionCounter;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:19Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

+    inModuleExecTransaction = _inModuleExecTransaction;
+    inSafeExecTransaction = _inSafeExecTransaction;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:21Z

hats-zodiac/test/harnesses/HatsSignerGateHarness.sol

    initialNonce = _initialNonce;
-    entrancyCounter = _entrancyCounter;
+    checkTransactionCounter = _checkTransactionCounter;


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:23Z

hats-zodiac/test/mocks/TestGuard.sol

@@ -31,11 +31,12 @@ contract TestGuard is BaseGuard {
    executionDisallowed = true;
  }

+  /// @dev Modified to remove the operation restriction


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:25Z

hats-zodiac/test/mocks/TestGuard.sol

  function checkTransaction(
    address to,
    uint256 value,
    bytes memory data,
-    Enum.Operation operation,
+    Enum.Operation, /* operation */


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

sherlock-admin · 2024-12-05T16:31:27Z

hats-zodiac/test/mocks/TestGuard.sol

@@ -47,7 +48,7 @@ contract TestGuard is BaseGuard {
    require(to != address(0), "Cannot send to zero address");
    require(value != 1337, "Cannot send 1337");
    require(bytes3(data) != bytes3(0xbaddad), "Cannot call 0xbaddad");
-    require(operation != Enum.Operation(1), "No delegate calls");
+    // require(operation != Enum.Operation(1), "No delegate calls");


Change related to Hats-Protocol/hats-zodiac#81.

Fixes issues: #3

Fix Review

7508730

sherlock-admin commented Dec 5, 2024

View reviewed changes

		/// @dev Temporary record of the existing owners on the `safe` when a transaction is submitted
		bytes32 transient _existingOwnersHash;

		/// @dev Temporary record of the existing threshold on the `safe` when a transaction is submitted
		uint256 transient _existingThreshold;

		/// @dev Temporary record of the existing fallback handler on the `safe` when a transaction is submitted
		address transient _existingFallbackHandler;

		/// @dev The safe's nonce at the beginning of a transaction
		uint256 transient _initialNonce;

		// Ensure that this is only called in accordance with the Safe execTransaction flow
		// And that it is not called from inside an execTransactionFromModule call

		// reset the reentrancy guard to enable subsequent legitimate Safe execTransactions in the same transaction
		_inSafeExecTransaction = false;

		// Disallow entering this function from inside a Safe execTransaction call or another execTransactionFromModule call
		if (_inSafeExecTransaction \|\| _inModuleExecTransaction) revert NoReentryAllowed();

		// the transaction should revert
		vm.expectRevert(SafeManagerLib.SafeTransactionFailed.selector);

		bool public inModuleExecTransaction;
		bool public inSafeExecTransaction;

		function exposed_checkTransactionCounter() public view returns (uint256) {
		return _checkTransactionCounter;

		inModuleExecTransaction = _inModuleExecTransaction;
		inSafeExecTransaction = _inSafeExecTransaction;

Fix Review #1

Are you sure you want to change the base?

Fix Review #1

Conversation

sherlock-admin commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment