GimelSec - reconcileSignerCount() would be blocked if validSignerCount > maxSigners, Safe would not be able to execute any transactions, all assets would be locked.

[sherlock-admin]

GimelSec

high

reconcileSignerCount() would be blocked if validSignerCount > maxSigners, Safe would not be able to execute any transactions, all assets would be locked.

Summary

reconcileSignerCount() would be blocked if validSignerCount > maxSigners, Safe would not be able to execute any transactions, all assets would be locked.

Vulnerability Detail

In HatsSignerGateFactory, there are many options to use a signer gate, for example, option 2: deploy a new signer gate and attach it to an existing Safe. A common scenario is that some DAOs already have a Safe, so they will select option 2 to attach a signer gate to the existing Safe.

Suppose a DAO has a Safe that all owners have worn a signer hat:

• Safe owners: 10
• We expect validSignerCount (aka _countValidSigners(owners)) is 10

Before they create a signer gate, they discuss that the DAO should be reduced to 6 owners, so they create a signer gate with the maxSigners parameter:

• maxSigners: 6

After they attach the signer gate, the Safe will be locked.
Because reconcileSignerCount() will always be reverted due to L187 validSignerCount > maxSigners.

Availability is important. the reconcileSignerCount() should not break checkTransaction().
If the reconcileSignerCount() will always be reverted, the checkTransaction() will also be reverted, the Safe execTransaction() will also be reverted. The Safe could not execute any transactions anymore, all assets would be locked.

Solutions?
Hats admin could set eligibility or toggle to disable some owners' hats, but in the real common case, these DAOs are going to be decentralized, the hats admin will not be owned by only one person. A common scenario is that the Safe owns the top hat, and the top hat manages DAO members' hats (i.e. valid signer hats).
But when this issue happens, the Safe could not execute any transactions, it means that nobody would be able to disable (or set eligibility and toggle) some owners' hats.

In another scenario, the DAOs would always set the hat to be immutable, that also makes Safe be locked.

Also, the signer gate does not allow anyone to disable the module, owners could not disable the module by fallbacking the signer gate.

A solution is that 4 of 10 owners should burn their hats token (aka renounceHat). But in decentralization, nobody wants to renounce hats first, because owners who were originally discussing to be kicked out can stay on as long as other owners renounce hats. If no owners give up their rights, the Safe will be locked forever.

Another scenario example to trigger this issue is that the Safe (the DAO) could call safe.addOwnerWithThreshold() to add new owners, who don't have valid signer hats yet. When these new owners receive valid signer hats, the issue will be triggered.

Impact

Safe would not be able to execute any transactions, all assets would be locked.

Code Snippet

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

Tool used

Manual Review

Recommendation

The reconcileSignerCount() should not be reverted and should gracefully return success bool.

Another check worth doing is to ensure maxSigners > safe.getOwners() when enabling the module.

Duplicate of #51

[spengrah]

I don't think this is a duplicate of #46, but rather of #93.