126.md

cducrest-brainbot

medium

The value of signerCount can be broken

Summary

_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.

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 reconcileSignerCount at the beginning of claimSigner()