Demonstrate fix for sherlock issue 44

src/HSGLib.sol

@@ -76,3 +76,7 @@ error SignersCannotChangeOwners();
 /// @notice Emmitted when a call to `checkTransaction` or `checkAfterExecution` is not made from the `safe`
 /// @dev Together with `guardEntries`, protects against arbitrary reentrancy attacks by the signers
 error NotCalledFromSafe();
+
+/// @notice Emmitted when attempting to reenter `checkTransaction`
+/// @dev The Safe will catch this error and re-throw with its own error message (`GS013`)
+error NoReentryAllowed();

src/HatsSignerGate.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: CC0
 pragma solidity >=0.8.13;
 
-import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
+// import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
 import { HatsSignerGateBase, IGnosisSafe, Enum } from "./HatsSignerGateBase.sol";
 import "./HSGLib.sol";
 
@@ -72,8 +72,6 @@ contract HatsSignerGate is HatsSignerGateBase {
     /// @param _account The address to check
     /// @return valid Whether `_account` is a valid signer
     function isValidSigner(address _account) public view override returns (bool valid) {
-        // console2.log("ivs 1");
         valid = HATS.isWearerOfHat(_account, signersHatId);
-        // console2.log("ivs 2");
     }
 }

src/HatsSignerGateBase.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: CC0
 pragma solidity >=0.8.13;
 
-import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
+// import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
 import "./HSGLib.sol";
 import { HatsOwnedInitializable } from "hats-auth/HatsOwnedInitializable.sol";
 import { BaseGuard } from "zodiac/guard/BaseGuard.sol";
@@ -23,18 +23,12 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @notice The maximum number of signers allowed for the `safe`
     uint256 public maxSigners;
 
-    // /// @notice The current number of signers on the `safe`
-    // uint256 public signerCount;
-
     /// @notice The version of HatsSignerGate used in this contract
     string public version;
 
     /// @notice The number of modules enabled on the `safe`, as enabled via this contract
     uint256 public enabledModuleCount;
 
-    /// @dev Temporary record of the existing modules on the `safe` when a transaction is submitted
-    bytes32 internal _existingModulesHash;
-
     /// @dev Temporary record of the existing owners on the `safe` when a transaction is submitted
     bytes32 internal _existingOwnersHash;
 
@@ -99,16 +93,22 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         if (_targetThreshold != targetThreshold) {
             _setTargetThreshold(_targetThreshold);
 
-            if (validSignerCount() > 1) _setSafeThreshold(_targetThreshold);
+            uint256 signerCount = validSignerCount();
+            if (signerCount > 1) _setSafeThreshold(_targetThreshold, signerCount);
 
             emit HSGLib.TargetThresholdSet(_targetThreshold);
         }
     }
 
     /// @notice Internal function to set the target threshold
-    /// @dev Reverts if `_targetThreshold` is greater than `maxSigners`
+    /// @dev Reverts if `_targetThreshold` is greater than `maxSigners` or lower than `minThreshold`
     /// @param _targetThreshold The new target threshold to set
     function _setTargetThreshold(uint256 _targetThreshold) internal {
+        // target threshold cannot be lower than min threshold
+        if (_targetThreshold < minThreshold) {
+            revert InvalidTargetThreshold();
+        }
+        // target threshold cannot be greater than max signers
         if (_targetThreshold > maxSigners) {
             revert InvalidTargetThreshold();
         }
@@ -119,13 +119,13 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @notice Internal function to set the threshold for the `safe`
     /// @dev Forwards the threshold-setting call to `safe.ExecTransactionFromModule`
     /// @param _threshold The threshold to set on the `safe`
-    function _setSafeThreshold(uint256 _threshold) internal {
+    /// @param _signerCount The number of valid signers on the `safe`; should be calculated from `validSignerCount()`
+    function _setSafeThreshold(uint256 _threshold, uint256 _signerCount) internal {
         uint256 newThreshold = _threshold;
-        uint256 signerCount = validSignerCount();
 
         // ensure that txs can't execute if fewer signers than target threshold
-        if (signerCount <= _threshold) {
-            newThreshold = signerCount;
+        if (_signerCount <= _threshold) {
+            newThreshold = _signerCount;
         }
         if (newThreshold != safe.getThreshold()) {
             bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);
@@ -162,25 +162,6 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         minThreshold = _minThreshold;
     }
 
-    /// @notice Allows the owner to enable a new module on the `safe`
-    /// @dev Increments the `enabledModuleCount` to include the new module in the allowed list (see `checkTransaction` and `checkAfterExecution`)
-    /// @param _module The address of the module to enable
-    function enableNewModule(address _module) external onlyOwner {
-        ++enabledModuleCount;
-
-        bytes memory data = abi.encodeWithSignature("enableModule(address)", _module);
-        bool success = safe.execTransactionFromModule(
-            address(safe), // to
-            0, // value
-            data, // data
-            Enum.Operation.Call // operation
-        );
-
-        if (!success) {
-            revert FailedExecEnableModule();
-        }
-    }
-
     /// @notice Tallies the number of existing `safe` owners that wear a signer hat and updates the `safe` threshold if necessary
     /// @dev Does NOT remove invalid `safe` owners
     function reconcileSignerCount() public {
@@ -200,7 +181,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
             newThreshold = target;
         }
         if (newThreshold > 0) {
-            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", signerCount);
+            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);
 
             bool success = safe.execTransactionFromModule(
                 address(safe), // to
@@ -215,6 +196,8 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         }
     }
 
+    /// @notice Tallies the number of existing `safe` owners that wear a signer hat
+    /// @return signerCount The number of valid signers on the `safe`
     function validSignerCount() public view returns (uint256 signerCount) {
         signerCount = _countValidSigners(safe.getOwners());
     }
@@ -310,7 +293,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         address ownerToCheck;
         bytes memory data;
 
-        for (uint256 i; i < _ownerCount - 1;) {
+        for (uint256 i; i < _ownerCount;) {
             ownerToCheck = _owners[i];
 
             if (!isValidSigner(ownerToCheck)) {
@@ -440,22 +423,19 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         address // msgSender
     ) external override {
         if (msg.sender != address(safe)) revert NotCalledFromSafe();
-
         // get the safe owners
         address[] memory owners = safe.getOwners();
         {
             // scope to avoid stack too deep errors
             uint256 safeOwnerCount = owners.length;
             // uint256 validSignerCount = _countValidSigners(safe.getOwners());
-
             // ensure that safe threshold is correct
             reconcileSignerCount();
 
             if (safeOwnerCount < minThreshold) {
                 revert BelowMinThreshold(minThreshold, safeOwnerCount);
             }
         }
-
         // get the tx hash; view function
         bytes32 txHash = safe.getTransactionHash(
             // Transaction info
@@ -473,25 +453,22 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
             // We subtract 1 since nonce was just incremented in the parent function call
             safe.nonce() - 1 // view function
         );
-
-        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);
+        uint256 threshold = safe.getThreshold();
+        uint256 validSigCount = countValidSignatures(txHash, signatures, threshold);
 
         // revert if there aren't enough valid signatures
-        if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {
+        if (validSigCount < threshold || validSigCount < minThreshold) {
             revert InvalidSigners();
         }
 
-        // record existing modules for post-flight check
-        // SENTINEL_OWNERS and SENTINEL_MODULES are both address(0x1)
-        (address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);
-        _existingModulesHash = keccak256(abi.encode(modules));
-
         // record existing owners for post-flight check
         _existingOwnersHash = keccak256(abi.encode(owners));
 
         unchecked {
             ++_guardEntries;
         }
+        // revert if re-entry is detected
+        if (_guardEntries > 1) revert NoReentryAllowed();
     }
 
     /**
@@ -520,13 +497,24 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         if (keccak256(abi.encode(owners)) != _existingOwnersHash) {
             revert SignersCannotChangeOwners();
         }
-        // prevent signers from changing the modules
+        // prevent signers from removing this module or adding any other modules
         /// @dev SENTINEL_OWNERS and SENTINEL_MODULES are both address(0x1)
-        (address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount + 1);
-        if (keccak256(abi.encode(modules)) != _existingModulesHash) {
+        (address[] memory modulesWith1, address next) = safe.getModulesPaginated(SENTINEL_OWNERS, 1);
+        // ensure that there is only one module...
+        if (
+            // if the length is 0, we know this module has been removed
+            // forgefmt: disable-next-line
+            modulesWith1.length == 0
+            /* per Safe ModuleManager.sol#137, "If all entries fit into a single page, the next pointer will be 0x1", ie SENTINEL_OWNERS.
+            Therefore, if `next` is not SENTINEL_OWNERS, we know another module has been added. */
+            || next != SENTINEL_OWNERS
+        ) {
+            revert SignersCannotChangeModules();
+        } // ...and that the only module is this contract
+        else if (modulesWith1[0] != address(this)) {
             revert SignersCannotChangeModules();
         }
-        // leave checked to catch underflows triggered by re-erntry attempts
+        // leave checked to catch underflows triggered by re-entry attempts
         --_guardEntries;
     }