cducrest-brainbot - Usage of HSG for existing safe can brick safe

[sherlock-admin]

cducrest-brainbot

high

Usage of HSG for existing safe can brick safe

Summary

The HatsSignerGateFactory allows for deployment of HSG / MHSG for existing safes. I believe the intention is to let the user call enableModule() and setGuard() on the safe after deployme with the HSG / MHSG address.

This can result in unmatching values of maxSigners in the HSG and number of valid signers of the safe. That will prevent further interaction with the safe rendering it unusable.

Vulnerability Detail

If a safe has 10 owners with valid hats, and a HSG / MHSG is deployed with a value of maxSigners < 10 and this HSG / MHSG is wired to the safe, the checks for validSignerCount <= maxSigners will revert in the HSG.

These checks are present in reconcileSignerCount and claimSigner. However reconcileSignerCount is a core function that is called by checkTransaction(), the pre-flight check on the safe transaction.

Impact

The safe will not be able to execute any transaction until the number of valid signers is lowered (some hat wearers give up their hats / some hats turns invalid ...)

Code Snippet

reconcileSignerCount checks maxSigners value:

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

It is called during checkTransaction:

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

The value is set once during setup and not changeable:

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

Tool used

Manual Review

Recommendation

Allow this value to be changed by owner, or have a function that checks the HSG is safe before making it active after it is wired to an existing safe.

[spengrah]

I like the recommendation to add a function to check that HSG is can be safely wired up to an existing Safe.

Another approach could be to check in the factory that maxSigners >= hat.currentSupply — or use the recommendation from #104 of ensuring that maxSigners > safe.getOwners() — but this wouldn't completely solve it because more hats could be minted to existing signers between deployment and wiring up.

cc @zobront

[zobront]

@spengrah I agree that the check needs to be at the "wiring up" time and not earlier.

[spengrah]

Re-adding the severity dispute tag since existing signers (who have full control over the safe prior to attaching HSG) would be both the ones attaching HSG in the first place and needing to renounce or be revoked if they made a mistake to attach. To me, this means that this is not a high severity bug, since attaching / fixing an attachment of HSG are incentive-aligned.

[MLON33]

zobront added "Fix Approved" label

[jacksanford1]

Adding the fix PR from spengrah for reference: https://github.com/Hats-Protocol/hats-zodiac/pull/14