_swapSigner() of HatsSignerGateBase increments signerCount after a successful swap of previously invalid user. This increment should only be done if the previous value of signerCount was correct, i.e. reconcileSignerCount was called prior. But the surrounding check does not enforce it.
Vulnerability Detail
claimSigner() of HatsSignerGate or MultiHatsSignerGate will call _swapSigner if safe.getOwners().length >= maxSigners.
Knowing reconcileSignerCount() does not remove invalid safe owners, it could be that the safe has more owners than the number of maxSigners.
_swapSigner() will successfully replace an invalid signer of the underlying safe with the new signer if there is one. It will then check if (signerCount < maxSigners) and increment signerCount if so. If reconcileSignerCount() has not been called beforehand, the value of signerCount could be outdated compared to the actual value of safe owners that are valid signers.
Impact
We increase the value of signerCount when we should not have, possibly resulting in a number of valid signers above the maxSigners value.
cducrest-brainbot
medium
The value of signerCount can be broken
Summary
_swapSigner()ofHatsSignerGateBaseincrementssignerCountafter a successful swap of previously invalid user. This increment should only be done if the previous value ofsignerCountwas correct, i.e.reconcileSignerCountwas called prior. But the surrounding check does not enforce it.Vulnerability Detail
claimSigner()ofHatsSignerGateorMultiHatsSignerGatewill call_swapSignerifsafe.getOwners().length >= maxSigners.Knowing
reconcileSignerCount()does not remove invalid safe owners, it could be that the safe has more owners than the number of maxSigners._swapSigner()will successfully replace an invalid signer of the underlying safe with the new signer if there is one. It will then checkif (signerCount < maxSigners)and incrementsignerCountif so. IfreconcileSignerCount()has not been called beforehand, the value ofsignerCountcould be outdated compared to the actual value of safe owners that are valid signers.Impact
We increase the value of signerCount when we should not have, possibly resulting in a number of valid signers above the maxSigners value.
Code Snippet
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L343
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGate.sol#L60-L65
Tool used
Manual Review
Recommendation
Call
reconcileSignerCountat the beginning ofclaimSigner()Duplicate of #27