juancito - Transactions will be frozen if incorrect settings are used during a deployment on HatsSignerGateFactory

[sherlock-admin]

juancito

medium

Transactions will be frozen if incorrect settings are used during a deployment on HatsSignerGateFactory

Summary

Transactions executed on the HatsSignerGate or the MultiHatsSignerGate must pass a pre-flight check and a post-flight check in order to be successful (checkTransaction(...) and checkAfterExecution(...)).

One check is to prevent safe signers from changing any modules:

// checkTransaction(...)
// record existing modules for post-flight check
(address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);
_existingModulesHash = keccak256(abi.encode(modules));

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

So, if any extra module is found on the checkAfterExecution, it will result on a different hash, and the transaction will revert.

This will always happen if enabledModuleCount does not reflect the real number of modules.

Vulnerability Detail

An incorrect enabledModuleCount value, making the transactions fail can be produced due to:

• Using the HatsSignerGateFactory._deployMultiHatsSignerGate(...) with a _existingModuleCount value lower than the actual modules count on the assigned safe. This function presumably has a wrong visibility scope, but can lead to these scenarios if left as it is.
• Attaching a Safe with more than 5 existing modules. This is explicitly said on the documented contract, but there is no strict check to prevent it from any other interface that interacts with the contract.

On any case, there will be more modules than the actual enabledModuleCount on the contract, leading to the described transaction reverts.

Impact

• Transactions performed on an HSG with incorrect module settings will be frozen and revert every time.
• The enabledModuleCount public variable will reflect an incorrect value.

Code Snippet

// HatsSignerGateFactory.sol
function _deployMultiHatsSignerGate(
    // ...
    uint256 _existingModuleCount
) public returns (address mhsg) { // @audit
    bytes memory initializeParams = abi.encode(
        // ...
        _existingModuleCount
    );

    mhsg = moduleProxyFactory.deployModule(
        multiHatsSignerGateSingleton, abi.encodeWithSignature("setUp(bytes)", initializeParams), ++nonce
    );

    // ...
}

• https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateFactory.sol#L260-L288

(address[] memory modules,) = GnosisSafe(payable(_safe)).getModulesPaginated(SENTINEL_MODULES, 5);

• https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateFactory.sol#L135
• https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateFactory.sol#L252

Tool used

Manual Review

Recommendation

• Change the visibility of the _deployMultiHatsSignerGate function to internal
• Check for a higher number of modules on the Safe and revert the deployment if the number is bigger than 5

Duplicate of #43