bin2chen - checkTransaction() can skip minThreshold limt

[sherlock-admin]

bin2chen

high

checkTransaction() can skip minThreshold limt

Summary

Malicious users can forge the last few signatures that are not checked by GnosisSafe, skipping the minThreshold limit

Vulnerability Detail

checkTransaction() use for Pre-flight check on a safe transaction to ensure that it s signers are valid.
one very important function is to check that the number of signatures cannot be less than minThreshold
The implementation code is as follows:

    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 {
...
        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
        );
        uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65); //1.<-------get validSigCount from countValidSignatures() 

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

get the number of valid signatures by countingValidSignatures(), and revert if the number is less than minThreshold
countingValidSignatures()'s implementation:

    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;

        for (i; i < sigCount;) {
            (v, r, s) = signatureSplit(signatures, i);
            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))); //<-----------------if v==0 get currentOwner without Signature content verification
            } 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))); //<-----------------if v==1 get currentOwner without Signature content verification
            } 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;  //<---------if valid count++
                }
            }
            // shouldn't overflow given reasonable sigCount
            unchecked {
                ++i;
            }
        }
    }

this method modified from GnosisSafe.sol#checkNSignatures(), and made some changes:

1. If v==0 or v==1, get currentOwner directly, and do not verify the signature content
2. Removal of duplicate owner check

These security restrictions are ignored because signatures come from GnosisSafe, which has already verified by GnosisSafe

But there is a problem: GnosisSafe does verify signatures, but GnosisSafe only verifies the first few signatures of threshold

The GnosisSafe code is as follows:
https://github.com/safe-global/safe-contracts/blob/c36bcab46578a442862d043e12a83fec41143dec/contracts/GnosisSafe.sol#L145

    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) {
....
            checkSignatures(txHash, txHashData, signatures);  //<------call checkSignatures

    function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view {
        // Load threshold to avoid multiple storage loads
        uint256 _threshold = threshold;
        // Check that a threshold is set
        require(_threshold > 0, "GS001");
        checkNSignatures(dataHash, data, signatures, _threshold);  //<--------only check _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;
        for (i = 0; i < requiredSignatures; i++) { //<--------only check requiredSignatures

but HatsSignerGateBase.checkTransaction use all signatures by signatures.length / 65
countValidSignatures(txHash, signatures, signatures.length / 65)
so malicious user can forge the last few signatures of threshold, because the last few signatures are not checked by GnosisSafe

For example:
minThreshold =2
targetThreshold=2
maxSigners=5

safe.threshold = 2
safe.owners=[alice,bob,jack,jimmy]

For some reason alice and bob has lost hats , has become invalid signer, but still in safe.owners[]

Although alice and bob are already invalid Signer, it is still possible to construct a malicious signature to skip the minThreshold limit

signatures:
[0] = alice signature ---> real signature, for safe verification
[1] = bob signature ---> real signature, for safe verification
[2] = jack fake signature (v=1 r=jack s=000) -- > safe will Ignore ， HatsSignerGateBase will count
[3] = jimmy fake signature (v=1 r=jimmy s=000) -- > safe will Ignore，HatsSignerGateBase will count

This gets validSigCount = 2 , which can skip minThreshold limit

Impact

minThreshold mechanism fails ，can malicious execution of illegal transactions

Code Snippet

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

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

https://github.com/safe-global/safe-contracts/blob/c36bcab46578a442862d043e12a83fec41143dec/contracts/GnosisSafe.sol#L145

Tool used

Manual Review

Recommendation

Use safe.getThreshold() instead of signatures.length / 65

    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 {
...
-       uint256 validSigCount = countValidSignatures(txHash, signatures, signatures.length / 65);
+       uint256 validSigCount = countValidSignatures(txHash, signatures, safe.getThreshold());

Duplicate of #50

[bin2chen66]

Escalate for 10 USDC

This issue describes the problem that is duplicative of #50 , not #36

Please check it, thanks!

[sherlock-admin]

Escalate for 10 USDC

This issue describes the problem that is duplicative of #50 , not #36

Please check it, thanks!

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.

[hrishibhat]

Escalation accepted

Valid duplicate of #50

[sherlock-admin]

Escalation accepted

Valid duplicate of #50

This issue's escalations have been accepted!

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