roguereddwarf - HatsSignerGateBase: valid signer threshold can be bypassed because HSG checks signatures differently from Safe which allows exploitation

[sherlock-admin]

roguereddwarf

high

HatsSignerGateBase: valid signer threshold can be bypassed because HSG checks signatures differently from Safe which allows exploitation

Summary

This report deals with how the HatsSignerGate and the Safe check signatures differently which opens the door to exploitation.

I will show how this allows a valid signer that has become invalid but not yet removed from the owners of the Safe to continue signing transactions. The invalid signer can effectively sign transactions as though he was valid.

Also there is the possibility of valid signers calling Safe.addOwnerWithThreshold. When an owner is added to the Safe but not a valid signer he can still sign transactions and the HSG will not recognize that there are not enough valid signatures.

To summarize, the issue is caused by this:

1. Signatures are first checked by the Safe then by the HSG logic
2. We can pass an arbitrary amount of signatures when executing a transaction
3. The Safe checks that the first threshold signatures are valid. However the HSG logic checks that ANY of the signatures are signed by valid signers. The HSG logic does not check the same signatures as the Safe.

Essentially the Safe and HSG logic are applying different checks to different signatures.

Vulnerability Detail

A transaction is executed by calling Safe.execTransaction

First the signatures are checked by the Safe Link then the checkTransaction function is executed on the guard (HatsSignerGate) Link

The HatsSignerGate then executes countValidSignatures to check if enough signatures were signed by valid signers Link

With all prerequisites out of the way, we can now get into the actual issue.

The Safe calls checkNSignatures to check if the first threshold signatures in the signatures bytes are valid Link

So if threshold=5 but we provide say 7 signatures, the last two signatures are not checked.
If the first 5 signatures are valid the check passes successfully.

The issue is that the HSG countValidSignatures function iterates over ALL signatures and tries to find enough valid signers such that threshold is reached Link.

So imagine the following scenario:

1. There are 4 owners in the Safe, threshold=3 and 3 owners are valid signers.
2. One of the owners is no longer a valid signer (I'll call him Bob). He is not yet removed from the owners.
3. Bob wants to sign a transaction and submit it to the Safe. He already has 2 signatures from valid signers.
4. Bob signs the transaction and appends his signature to the signatures bytes. He also appends a signature of a valid signer from a previous transaction. So there are now 4 signatures in the signatures bytes.
5. Bob calls Safe.execTransaction. The Safe checks the first 3 signatures to be valid signatures from owners. The check passes. The HSG checks that there are at least 3 signatures signed by valid signers. Which also passes.

Important: Bob can pass a 4th signature from a previous transaction because two of the signature types used in HSG do not check that the correct data has been signed https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L561-L568.

To summarize: Bob was able to sign a transaction even though he was no longer a valid signer.

Further notes

Another thing to note is that HSG does not check signatures for uniqueness so if Bob would have to append multiple signatures from valid signers he could just add the same signature multiple times.

Also the HSG does not check that ecrecover does not return the zero address as owner which it does if the signature is invalid.
These checks are implemented in the Safe. So by implementing the mitigation I suggest below the Safe and HSG will check the same signatures. So there is no need to have these checks in the HSG as well.
However due to this bug (checking different signatures), the signer hat might be transferred to address(0) which then causes invalid signatures to be considered valid.

Impact

Owners of the Safe that are not valid signers can sign transactions.

Code Snippet

Also have a look at the @audit-info comments that further explain the issue.

https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/Safe.sol#L135-L217

    function execTransaction(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address payable refundReceiver,
        bytes memory signatures
    ) public payable virtual returns (bool success) {
        bytes32 txHash;
        // Use scope here to limit variable lifetime and prevent `stack too deep` errors
        {
            bytes memory txHashData = encodeTransactionData(
                // Transaction info
                to,
                value,
                data,
                operation,
                safeTxGas,
                // Payment info
                baseGas,
                gasPrice,
                gasToken,
                refundReceiver,
                // Signature info
                nonce
            );
            // Increase nonce and execute transaction.
            nonce++;
            txHash = keccak256(txHashData);
            // @audit-info first the Safe checks the signatures
            checkSignatures(txHash, txHashData, signatures);
        }
        address guard = getGuard();
        {
            if (guard != address(0)) {
                // @audit-info then signatures are checked by HSG
                Guard(guard).checkTransaction(
                    // Transaction info
                    to,
                    value,
                    data,
                    operation,
                    safeTxGas,
                    // Payment info
                    baseGas,
                    gasPrice,
                    gasToken,
                    refundReceiver,
                    // Signature info
                    signatures,
                    msg.sender
                );
            }
        }
        // We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
        // We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
        require(gasleft() >= ((safeTxGas * 64) / 63).max(safeTxGas + 2500) + 500, "GS010");
        // Use scope here to limit variable lifetime and prevent `stack too deep` errors
        {
            uint256 gasUsed = gasleft();
            // If the gasPrice is 0 we assume that nearly all available gas can be used (it is always more than safeTxGas)
            // We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than safeTxGas
            success = execute(to, value, data, operation, gasPrice == 0 ? (gasleft() - 2500) : safeTxGas);
            gasUsed = gasUsed.sub(gasleft());
            // If no safeTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful
            // This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert
            require(success || safeTxGas != 0 || gasPrice != 0, "GS013");
            // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
            uint256 payment = 0;
            if (gasPrice > 0) {
                payment = handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver);
            }
            if (success) emit ExecutionSuccess(txHash, payment);
            else emit ExecutionFailure(txHash, payment);
        }
        {
            if (guard != address(0)) {
                Guard(guard).checkAfterExecution(txHash, success);
            }
        }
    }

https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/Safe.sol#L270-L330

    // @audit-info requiredSignatures is equal to threshold
    function checkNSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, uint256 requiredSignatures) public view {
        // Check that the provided signature data is not too short
        require(signatures.length >= requiredSignatures.mul(65), "GS020");
        // There cannot be an owner with address 0.
        address lastOwner = address(0);
        address currentOwner;
        uint8 v;
        bytes32 r;
        bytes32 s;
        uint256 i;
        // @audit-info only the first threshold signatures are checked
        for (i = 0; i < requiredSignatures; i++) {
            (v, r, s) = signatureSplit(signatures, i);
            if (v == 0) {
                require(keccak256(data) == dataHash, "GS027");
                // If v is 0 then it is a contract signature
                // When handling contract signatures the address of the contract is encoded into r
                currentOwner = address(uint160(uint256(r)));


                // Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
                // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
                // Here we only check that the pointer is not pointing inside the part that is being processed
                require(uint256(s) >= requiredSignatures.mul(65), "GS021");


                // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
                require(uint256(s).add(32) <= signatures.length, "GS022");


                // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
                uint256 contractSignatureLen;
                // solhint-disable-next-line no-inline-assembly
                assembly {
                    contractSignatureLen := mload(add(add(signatures, s), 0x20))
                }
                require(uint256(s).add(32).add(contractSignatureLen) <= signatures.length, "GS023");


                // Check signature
                bytes memory contractSignature;
                // solhint-disable-next-line no-inline-assembly
                assembly {
                    // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
                    contractSignature := add(add(signatures, s), 0x20)
                }
                require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
            } else if (v == 1) {
                // If v is 1 then it is an approved hash
                // When handling approved hashes the address of the approver is encoded into r
                currentOwner = address(uint160(uint256(r)));
                // Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction
                require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
            } else if (v > 30) {
                // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
                // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
                currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
            } else {
                // Default is the ecrecover flow with the provided data hash
                // Use ecrecover with the messageHash for EOA signatures
                currentOwner = ecrecover(dataHash, v, r, s);
            }
            require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026");
            lastOwner = currentOwner;
        }
    }

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

    function checkTransaction(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address payable refundReceiver,
        bytes memory signatures,
        address // msgSender
    ) external override {
        if (msg.sender != address(safe)) revert NotCalledFromSafe();


        uint256 safeOwnerCount = safe.getOwners().length;
        // uint256 validSignerCount = _countValidSigners(safe.getOwners());


        // ensure that safe threshold is correct
        reconcileSignerCount();


        if (safeOwnerCount < minThreshold) {
            revert BelowMinThreshold(minThreshold, safeOwnerCount);
        }


        // get the tx hash; view function
        bytes32 txHash = safe.getTransactionHash(
            // Transaction info
            to,
            value,
            data,
            operation,
            safeTxGas,
            // Payment info
            baseGas,
            gasPrice,
            gasToken,
            refundReceiver,
            // Signature info
            // We subtract 1 since nonce was just incremented in the parent function call
            safe.nonce() - 1 // view function
        );

        // @audit-info all signatures are checked (signatures.length / 65) as opposed to first threshold ones in the Safe
        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);


        // revert if there aren't enough valid signatures
        if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {
            revert InvalidSigners();
        }


        // record existing modules for post-flight check
        // SENTINEL_OWNERS and SENTINEL_MODULES are both address(0x1)
        (address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);
        _existingModulesHash = keccak256(abi.encode(modules));


        unchecked {
            ++_guardEntries;
        }
    }

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

    function countValidSignatures(bytes32 dataHash, bytes memory signatures, uint256 sigCount)
        public
        view
        returns (uint256 validSigCount)
    {
        // There cannot be an owner with address 0.
        address currentOwner;
        uint8 v;
        bytes32 r;
        bytes32 s;
        uint256 i;

        // @audit-info all signatures are checked
        for (i; i < sigCount;) {
            (v, r, s) = signatureSplit(signatures, i);
            // @audit-info old signature is counted as valid because transaction data is not verified
            if (v == 0) {
                // If v is 0 then it is a contract signature
                // When handling contract signatures the address of the contract is encoded into r
                currentOwner = address(uint160(uint256(r)));
            // @audit-info old signature is counted as valid because transaction data is not verified
            } else if (v == 1) {
                // If v is 1 then it is an approved hash
                // When handling approved hashes the address of the approver is encoded into r
                currentOwner = address(uint160(uint256(r)));
            } else if (v > 30) {
                // If v > 30 then default va (27,28) has been adjusted for eth_sign flow
                // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
                currentOwner =
                    ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
            } else {
                // Default is the ecrecover flow with the provided data hash
                // Use ecrecover with the messageHash for EOA signatures
                currentOwner = ecrecover(dataHash, v, r, s);
            }


            if (isValidSigner(currentOwner)) {
                // shouldn't overflow given reasonable sigCount
                unchecked {
                    ++validSigCount;
                }
            }
            // shouldn't overflow given reasonable sigCount
            unchecked {
                ++i;
            }
        }
    }

Tool used

Manual Review

Recommendation

I propose that in the HatsSignerGate only the first threshold signatures are checked. Such that both the Safe and HSG check the SAME signatures.

Fix:

diff --git a/src/HatsSignerGateBase.sol b/src/HatsSignerGateBase.sol
index 3e8bb5f..05f85a3 100644
--- a/src/HatsSignerGateBase.sol
+++ b/src/HatsSignerGateBase.sol
@@ -485,7 +485,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
             safe.nonce() - 1 // view function
         );
 
-        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);
+        uint256 validSigCount = countValidSignatures(txHash, signatures, safe.getThreshold());
 
         // revert if there aren't enough valid signatures
         if (validSigCount < safe.getThreshold() || validSigCount < minThreshold) {

Instead of checking all signatures, only the first threshold ones will be checked.
Also there is no need to check the length of the signatures bytes. All those checks are done by the Safe already.

[spengrah]

Excellent find. The recommended fix makes sense.

[spengrah]

Hats-Protocol/hats-zodiac#11

[MLON33]

zobront added "Fix Approved" label