Safe can break if external module can add additional module

Summary

If an external module adds another module the safe can not execute multisig transactions anymore.

Vulnerability Detail

The HatsSignerGateBase checks that no additional modules were added as part of a multisig transaction:

//pre-flight
(address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);
_existingModulesHash = keccak256(abi.encode(modules));

...

//post-flight
(address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount + 1);
if (keccak256(abi.encode(modules)) != _existingModulesHash) {
    revert SignersCannotChangeModules();
}

The problem is that enabledModuleCount only accounts for modules that were added with HatsSignerGateBase.enableNewModule. If another module has the capability to add modules, this will cause a reversion in the post-flight check. The reason is that the pre-flight check only gets the addresses up to enabledModuleCount to calculate the hash, but the actual amount of modules might be higher. The post-flight check retrieves enabledModuleCount + 1 to prevent other modules from being added as part of a multisig transaction. However, in this scenario the added module has been already there and has just not been taken into account pre-flight.

Impact

Breaking core functionality

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Determine the correct module count by calling safe.getModulesPaginated with a larger page size (multiple calls might be necessary if page size too large, although unlikely)

Duplicate of #42

sherlock-audit / 2023-02-hats-judging

minhtrng - Safe can break if external module can add additional module #117