processAndSortSignatures is missing case for v = 1

[hats-bug-reporter[bot]]

Github username: -- Twitter username: SBSecurity_ Submission hash (on-chain): 0x9ab6a6d445d708b08312713874fce44f68c7caec5f5f8e24c4652de0fbc03902 Severity: medium

Description: Description\ processAndSortSignatures() is missing case when v = 1, and use the wrong signer.

When v = 1 it should return the signer from r param like it is done is the SAFE contract

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
                if (executor != currentOwner && approvedHashes[currentOwner][dataHash] == 0) revertWithError("GS025");

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

1. Proof of Concept (PoC) File

function processAndSortSignatures(
        bytes32 dataHash,
        bytes memory signatures,
        address[] memory owners
    ) internal pure returns (bytes memory) {
        uint256 count = signatures.length / 65;
        bytes memory concatenatedSignatures;

        for (uint256 j; j < owners.length;) {
            address currentOwner = owners[j];
            for (uint256 i; i < count;) {
                (uint8 v, bytes32 r, bytes32 s) = signatureSplit(signatures, i);

                address signer;
                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
                    signer = address(uint160(uint256(r)));
                } else {
                    // "eth_sign_flow" signatures are specified as v > 30 and are handled differently
                    // if not handle like EOA signature
                    (uint8 v1, bytes32 hashData) = v > 30
                        ? (
                            v - 4,
                            keccak256(
                                abi.encodePacked(
                                    "\x19Ethereum Signed Message:\n32", dataHash
                                )
                                )
                        )
                        : (v, dataHash);
                    signer = ecrecover(hashData, v1, r, s);
                }

                bytes memory signature = abi.encodePacked(r, s, v);
                if (signer == currentOwner) {
                    concatenatedSignatures =
                        abi.encodePacked(concatenatedSignatures, signature);
                    break;
                }
                unchecked {
                    ++i;
                }
            }
            unchecked {
                ++j;
            }
        }
        return concatenatedSignatures;
    }

2. Revised Code File (Optional)

[alfredolopez80]

Non-issue, it is intentional not to support approvedHashes, because it is something that is managed directly from a safe storage, and that's why we do not plan to support it in our module.

if any user, use this functionality with our execTransactionOnBehalf the signature not will be concatenated, and probably fail!!, and that's why we will left well docummented that in our protocol.