rvierdiiev - HatsSignerGate.claimSigner will revert when signerCount = maxSigners and any of owners is invalid

[sherlock-admin]

rvierdiiev

medium

HatsSignerGate.claimSigner will revert when signerCount = maxSigners and any of owners is invalid

Summary

HatsSignerGate.claimSigner will revert when signerCount = maxSigners and any of owners is invalid. As result new signer will not be able to become owner.

Vulnerability Detail

HatsSignerGate.claimSigner is called by hat wearer in order to become Safe owner. https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGate.sol#L36-L69

    function claimSigner() public virtual {
        uint256 maxSigs = maxSigners; // save SLOADs
        uint256 currentSignerCount = signerCount;

        if (currentSignerCount >= maxSigs) {
            revert MaxSignersReached();
        }

        if (safe.isOwner(msg.sender)) {
            revert SignerAlreadyClaimed(msg.sender);
        }

        if (!isValidSigner(msg.sender)) {
            revert NotSignerHatWearer(msg.sender);
        }

        /* 
        We check the safe owner count in case there are existing owners who are no longer valid signers. 
        If we're already at maxSigners, we'll replace one of the invalid owners by swapping the signer.
        Otherwise, we'll simply add the new signer.
        */
        address[] memory owners = safe.getOwners();
        uint256 ownerCount = owners.length;

        if (ownerCount >= maxSigs) {
            bool swapped = _swapSigner(owners, ownerCount, maxSigs, currentSignerCount, msg.sender);
            if (!swapped) {
                // if there are no invalid owners, we can't add a new signer, so we revert
                revert NoInvalidSignersToReplace();
            }
        } else {
            _grantSigner(owners, currentSignerCount, msg.sender);
        }
    }

The first check in function checks that signerCount(count of valid owners that were updated last time) are less than maxSigners(maximum allowed signers count. Otherwise function will revert.

But it's possible, that between last update of signerCount variable, some owner already became invalid and he should be changed by claimer, but function will revert. Claimer will need to call reconcileSignerCount function in order to update signerCount function and be able to call claimSigner function.

Impact

Claimer can't become a signer as signerCount isn't updated.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

I guess, that you don't need that check. Or the check can be currentSignerCount > maxSigs(without = sign). In case if each owner is valid, function will not swap any owner to claimer. And in case if any owner became invalid, then function will swap it.

[spengrah]

This should be addressed by switching to a dynamically read getSignerCount() instead of relying on the signerCount state variable, as discussed in #111

cc @zobront

[spengrah]

https://github.com/Hats-Protocol/hats-zodiac/pull/6

[zobront]

Escalate for 10 USDC

While this issue is technically correct, there is no risk and it doesn't warrant a Medium.

The worst possible outcome is simply bad UX that the user would need to call reconcileSignerCount() and re-call the claimSigner() function and it would work as expected.

[sherlock-admin]

Escalate for 10 USDC

While this issue is technically correct, there is no risk and it doesn't warrant a Medium.

The worst possible outcome is simply bad UX that the user would need to call reconcileSignerCount() and re-call the claimSigner() function and it would work as expected.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cducrest]

As pointed out in the duplicate https://github.com/sherlock-audit/2023-02-hats-judging/issues/126, this problem could result in a number of valid signers above the maxSigners in the condition that the signerCount value was out of sync when claimSigner() is called and there is at least one invalid signer set as owner of the safe.

I can understand why this issue could be seen as not high impact enough, but the duplicate should prove that it is a valid medium.

[hrishibhat]

Escalation accepted

Not a valid medium Agreed that this issue & its duplicates can be considered low given their impact, which can be mitigated by calling other reconcileSignerCount or by removeSigner.

[sherlock-admin]

Escalation accepted

Not a valid medium Agreed that this issue & its duplicates can be considered low given their impact, which can be mitigated by calling other reconcileSignerCount or by removeSigner.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.