roguereddwarf - HatsSignerGate + MultiHatsSignerGate: more than maxSignatures can be claimed which leads to DOS in reconcileSignerCount

[sherlock-admin]

roguereddwarf

medium

HatsSignerGate + MultiHatsSignerGate: more than maxSignatures can be claimed which leads to DOS in reconcileSignerCount

Summary

The HatsSignerGate.claimSigner and MultiHatsSignerGate.claimSigner functions allow users to become signers.

It is important that both functions do not allow that there exist more valid signers than maxSigners.

This is because if there are more valid signers than maxSigners, any call to HatsSignerGateBase.reconcileSignerCount reverts, which means that no transactions can be executed.

The only possibility to resolve this is for a valid signer to give up his signer hat. No signer will voluntarily give up his signer hat. And it is wrong that a signer must give it up. Valid signers that have claimed before maxSigners was reached should not be affected by someone trying to become a signer and exceeding maxSigners. In other words the situation where one of the signers needs to give up his signer hat should have never occurred in the first place.

Vulnerability Detail

Think of the following scenario:

1. maxSignatures=10 and there are 10 valid signers
2. The signers execute a transaction that calls Safe.addOwnerWithThreshold such that there are now 11 owners (still there are 10 valid signers)
3. One of the 10 signers is no longer a wearer of the hat and reconcileSignerCount is called. So there are now 9 valid signers and 11 owners
4. The signer that was no longer a wearer of the hat in the previous step now wears the hat again. However reconcileSignerCount is not called. So there are 11 owners and 10 valid signers. The HSG however still thinks there are 9 valid signers.

When a new signer now calls claimSigner, all checks will pass and he will be swapped for the owner that is not a valid signer:

        // 9 >= 10 is false
        if (currentSignerCount >= maxSigs) {
            revert MaxSignersReached();
        }

        // msg.sender is a new signer so he is not yet owner
        if (safe.isOwner(msg.sender)) {
            revert SignerAlreadyClaimed(msg.sender);
        }

        // msg.sender is a valid signer, he wears the signer hat
        if (!isValidSigner(msg.sender)) {
            revert NotSignerHatWearer(msg.sender);
        }

So there are now 11 owners and 11 valid signers. This means when reconcileSignerCount is called, the following lines cause a revert:

    function reconcileSignerCount() public {
        address[] memory owners = safe.getOwners();
        uint256 validSignerCount = _countValidSigners(owners);

        // 11 > 10
        if (validSignerCount > maxSigners) {
            revert MaxSignersReached();
        }

Impact

As mentioned before, we end up in a situation where one of the valid signers has to give up his signer hat in order for the HSG to become operable again.

So one of the valid signers that has rightfully claimed his spot as a signer may lose his privilege to sign transactions.

Code Snippet

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);
        }
    }

https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/MultiHatsSignerGate.sol#L41-L81

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

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

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

        if (!isValidSignerHat(_hatId)) {
            revert InvalidSignerHat(_hatId);
        }

        if (!HATS.isWearerOfHat(msg.sender, _hatId)) {
            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);
        }

        // register the hat used to claim. This will be the hat checked in `checkTransaction()` for this signer
        claimedSignerHats[msg.sender] = _hatId;
    }

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

    function reconcileSignerCount() public {
        address[] memory owners = safe.getOwners();
        uint256 validSignerCount = _countValidSigners(owners);

        if (validSignerCount > maxSigners) {
            revert MaxSignersReached();
        }

        // update the signer count accordingly
        signerCount = validSignerCount;

        uint256 currentThreshold = safe.getThreshold();
        uint256 newThreshold;
        uint256 target = targetThreshold; // save SLOADs

        if (validSignerCount <= target && validSignerCount != currentThreshold) {
            newThreshold = validSignerCount;
        } else if (validSignerCount > target && currentThreshold < target) {
            newThreshold = target;
        }
        if (newThreshold > 0) {
            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);

            bool success = safe.execTransactionFromModule(
                address(safe), // to
                0, // value
                data, // data
                Enum.Operation.Call // operation
            );

            if (!success) {
                revert FailedExecChangeThreshold();
            }
        }
    }

Tool used

Manual Review

Recommendation

The HatsSignerGate.claimSigner and MultiHatsSignerGate.claimSigner functions should call reconcileSignerCount such that they work with the correct amount of signers and the scenario described in this report cannot occur.

diff --git a/src/HatsSignerGate.sol b/src/HatsSignerGate.sol
index 7a02faa..949d390 100644
--- a/src/HatsSignerGate.sol
+++ b/src/HatsSignerGate.sol
@@ -34,6 +34,8 @@ contract HatsSignerGate is HatsSignerGateBase {
     /// @notice Function to become an owner on the safe if you are wearing the signers hat
     /// @dev Reverts if `maxSigners` has been reached, the caller is either invalid or has already claimed. Swaps caller with existing invalid owner if relevant.
     function claimSigner() public virtual {
+        reconcileSignerCount();
+
         uint256 maxSigs = maxSigners; // save SLOADs
         uint256 currentSignerCount = signerCount;

diff --git a/src/MultiHatsSignerGate.sol b/src/MultiHatsSignerGate.sol
index da74536..57041f6 100644
--- a/src/MultiHatsSignerGate.sol
+++ b/src/MultiHatsSignerGate.sol
@@ -39,6 +39,8 @@ contract MultiHatsSignerGate is HatsSignerGateBase {
     /// @dev Reverts if `maxSigners` has been reached, the caller is either invalid or has already claimed. Swaps caller with existing invalid owner if relevant.
     /// @param _hatId The hat id to claim signer rights for
     function claimSigner(uint256 _hatId) public {
+        reconcileSignerCount();
+        
         uint256 maxSigs = maxSigners; // save SLOADs
         uint256 currentSignerCount = signerCount;

[spengrah]

I don't believe this is a duplicate of #46, which deals with number of owners being increased by another module, while the present issue deals with owners being increased by the safe's signers. That means, however, that it be a duplicate of #118 and #170.

[spengrah]

Addressed by https://github.com/Hats-Protocol/hats-zodiac/pull/5

[zobront]

@spengrah This blocks the specific path he used to get validSigners > maxSigners, but are there any other paths to get signers into the safe (such as through Safe directly)?

If not, then most important is just to make sure there is zero way for that to happen through the contract and that invariant is rock solid. I can take another look too.

[spengrah]

@zobront #5 makes two changes relevant to this issue:

1. The zodiac guard functionality now blocks signers from adding a new owner to the Safe (blocking step 2 of the vulnerability scenario).
2. The contract no longer relies on reconcileSignerCount() to update the valid signer count, instead tracking it dynamically via validSignerCount(). This makes step 4 irrelevant, so even if there were a different way to get signers into the safe (which there is not), claimSigner() would always "know" about the extra signer and therefore disallow extraneous claims.

Since we've blocked owners from adding signers via a tx, and we've also blocked other modules from being added, the only way to add a new signer to the safe is via claimSigner(). Crucially, if the number of safe owners is >= maxSigners, instead of simply adding a new signer, claimSigner() will replace an invalid signer with the new signer. Therefore, in combination with the changes described above, there is no possible way to have more than maxSigners owners on the safe, and therefore no way to have more than maxSigners valid signers.

[jacksanford1]

Comment from zobront in Discord about this issue:

Confirmed, that logic seems rock solid. Good stuff.

Interpreting this as "Fix Approved" and adding a "Fix Approved" label on behalf of zobront.