unforgiven - it's possible to perform malicious actions in most of the HatsSignerGate, MultiHatsSignerGate and HatsSignerGateBase contracts functions by reentrancy, because of external call in the `isValidSigner()` and hat's check wearer

[sherlock-admin]

unforgiven

high

it's possible to perform malicious actions in most of the HatsSignerGate, MultiHatsSignerGate and HatsSignerGateBase contracts functions by reentrancy, because of external call in the isValidSigner() and hat's check wearer

Summary

most of the functions in HSG contracts check that signer is valid signer by calling isValidSigner() which calls HATS.isWearerOfHat() which would make external call to toggle and eligible contract addresses that are registered in the Hat contract. because code doesn't have reentrancy guard and doesn't follow check-effect-interaction pattern so it would be possible to perform malicious actions. in detail function claimSigner() and _removeSigner() and _getCorrectThreshold() are vulenable.

Vulnerability Detail

For example this is _removeSigner() code:

function _removeSigner(address _signer) internal {
        bytes memory removeOwnerData;
        address[] memory owners = safe.getOwners();
        uint256 currentSignerCount = signerCount; // save an SLOAD
        uint256 newSignerCount;

        if (currentSignerCount < 2 && owners.length == 1) {
            // signerCount could be 0 after reconcileSignerCount
            // make address(this) the only owner
            removeOwnerData = abi.encodeWithSignature(
                "swapOwner(address,address,address)",
                SENTINEL_OWNERS, // prevOwner
                _signer, // oldOwner
                address(this) // newOwner
            );

            // newSignerCount is already 0
        } else {
            uint256 currentThreshold = safe.getThreshold();
            uint256 newThreshold = currentThreshold;
            uint256 validSignerCount = _countValidSigners(owners);

            if (validSignerCount == currentSignerCount) {
                newSignerCount = currentSignerCount;
            } else {
                newSignerCount = currentSignerCount - 1;
            }

            // ensure that txs can't execute if fewer signers than target threshold
            if (newSignerCount <= targetThreshold) {
                newThreshold = newSignerCount;
            }

            removeOwnerData = abi.encodeWithSignature(
                "removeOwner(address,address,uint256)", _findPrevOwner(owners, _signer), _signer, newThreshold
            );
        }

        // update signerCount
        signerCount = newSignerCount;

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

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

code calls _countValidSigners() which would call hat contract and it would call external address. one can perform reentrancy and call removeSigner() multiple times in reentrancy and because code save the value of the safe.getOwners() in the memory before the external call and use it after the external call so it would code make the wrong changes, for example set higher value of validSignerCount for the safe even so they are changed by reentrancy.

other functions claimSigner/_removeSigner/_getCorrectThreshold has smilar issues. code make perform some checks and save some values in the memory and them makes external calls. (this functions are used by other functions in some cases)

This is claimSigner() code:

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

As you can see code checks maxSigs and owner status then calls isValidSigner() which has external call, so by performing reentrancy and calling this function again recursivly is would be possible to bypass those checks.

Impact

it's possible to perform all sort of reentrancy attacks and bypass checks and make contract to enter unexpected state

Code Snippet

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

Tool used

Manual Review

Recommendation

protect against reentrancy

[spengrah]

This is not an issue because the external calls in Hats.isWearerOfHat() are all staticcalls, so the external contracts cannot change any state.