roguereddwarf - HatsSignerGateFactory: Should revert if there are more than 5 existing modules

[sherlock-admin]

roguereddwarf

medium

HatsSignerGateFactory: Should revert if there are more than 5 existing modules

Summary

The HatsSignerGateFactory contract allows to deploy HatsSignerGate and MultiHatsSignerGate contracts and attach them to existing Safes.

The functions that are used to do this are HatsSignerGateFactory.deployHatsSignerGate and HatsSignerGateFactory.deployMultiHatsSignerGate.

It is known to the sponsor that it is unsafe to do this for Safes that have more than 5 modules registered.
We can see this by their comments:

    /// @dev Do not attach HatsSignerGate to a Safe with more than 5 existing modules; its signers will not be able to execute any transactions

So the signers will not be able to execute any transactions.

Vulnerability Detail

I will not explain why this danger exists because it is known to the sponsor.

So here just a quick summary: When a transaction is executed the HatsSignerGate checks that no new modules were added. However these checks are based on a wrong module count if the number of initial modules is greater than 5.
The HatsSignerGate will think that the transaction by the signers has added a new module and will revert.

I argue that this is very unsafe.

The code should check and revert if there are more than 5 modules registered.

If signers of the Safe are not aware of this limitation which is likely because the danger of this function is only mentioned in a comment, they can lose access to all their funds and all other privileges associated with the Safe.

Impact

Signers lose access to the Safe because all transactions will revert.

Code Snippet

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateFactory.sol#L124-L141

    // option 2: deploy a new signer gate and attach it to an existing Safe
    /// @dev Do not attach HatsSignerGate to a Safe with more than 5 existing modules; its signers will not be able to execute any transactions
    function deployHatsSignerGate(
        uint256 _ownerHatId,
        uint256 _signersHatId,
        address _safe, // existing Gnosis Safe that the signers will join
        uint256 _minThreshold,
        uint256 _targetThreshold,
        uint256 _maxSigners
    ) public returns (address hsg) {
        // count up the existing modules on the safe
        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 5);
        uint256 existingModuleCount = modules.length;


        return _deployHatsSignerGate(
            _ownerHatId, _signersHatId, _safe, _minThreshold, _targetThreshold, _maxSigners, existingModuleCount
        );
    }

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateFactory.sol#L243-L258

    function deployMultiHatsSignerGate(
        uint256 _ownerHatId,
        uint256[] calldata _signersHatIds,
        address _safe, // existing Gnosis Safe that the signers will join
        uint256 _minThreshold,
        uint256 _targetThreshold,
        uint256 _maxSigners
    ) public returns (address mhsg) {
        // count up the existing modules on the safe
        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 5);
        uint256 existingModuleCount = modules.length;


        return _deployMultiHatsSignerGate(
            _ownerHatId, _signersHatIds, _safe, _minThreshold, _targetThreshold, _maxSigners, existingModuleCount
        );
    }

Tool used

Manual Review

Recommendation

I recommend to check if there are more than 5 modules registered and revert if this is the case.

Fix:

diff --git a/src/HatsSignerGateFactory.sol b/src/HatsSignerGateFactory.sol
index 57d40cd..0f9e2f4 100644
--- a/src/HatsSignerGateFactory.sol
+++ b/src/HatsSignerGateFactory.sol
@@ -132,8 +132,11 @@ contract HatsSignerGateFactory {
         uint256 _maxSigners
     ) public returns (address hsg) {
         // count up the existing modules on the safe
-        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 5);
+        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 6);
         uint256 existingModuleCount = modules.length;
+        if (modules.length > 5) {
+            revert TooManyModules();
+        }
 
         return _deployHatsSignerGate(
             _ownerHatId, _signersHatId, _safe, _minThreshold, _targetThreshold, _maxSigners, existingModuleCount
@@ -249,7 +252,10 @@ contract HatsSignerGateFactory {
         uint256 _maxSigners
     ) public returns (address mhsg) {
         // count up the existing modules on the safe
-        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 5);
+        (address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 6);
+        if (modules.length > 5) {
+            revert TooManyModules();
+        }
         uint256 existingModuleCount = modules.length;
 
         return _deployMultiHatsSignerGate(

Duplicate of #43