cducrest-brainbot - Prevent deployment of HSG when safe has more than 5 modules

[sherlock-admin]

cducrest-brainbot

medium

Prevent deployment of HSG when safe has more than 5 modules

Summary

I could not figure out why it is unsafe to use a gnosis safe with more than 5 enabled modules (as described in the code), but the current proxy factory will produce a HSG with wrong enabledModuleCount when we attempt to deploy a new HSG with an existing safe with more than 5 modules.

Vulnerability Detail

If the protocol considers it unsafe to have a safe with a guard HSG with more than 5 pre-existing modules, it should restrain it. In the current implementation, it is not restrained and result in a wrong value of enabledModuleCount.

Impact

HSG with wrong value of enabledModuleCount / bricked safe contracts.

Code Snippet

deployHatsSignerGate only gets the existing modules with a limit of 5:

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

Reference of get getModulesPaginated:

https://github.com/safe-global/safe-contracts/blob/6f4355ecf38f7a842f9f173f25429def2bcbfae9/contracts/base/ModuleManager.sol#L143

This is used to set the value of enabledModuleCount in the HSG:

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L83

The problem is the same for deployment of MultiHatsSignerGate.

Tool used

Manual Review

Recommendation

In the proxy factory, get the modules with a limit of 6 and revert if the 6th value is non-zero. Note that a user could still deploy the HSG with the correct enabledModuleCount, add a module to the safe, and then bind the HSG to the safe. For a definite fix you'd need to have to call a function on the HSG that checks the module count of the safe before it is active.

Duplicate of #43