processAndSortSignatures Function Fails to Handle Multiple Valid Signatures for Owners, Leading to Reversions in checkNSignatures Function if Threshold is Greater Than 1

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

Description: Description\ Description TheprocessAndSortSignatures function in the Helpers contract is designed to process and sort signatures for a Safe Multisig Wallet transaction. However, the current implementation only includes the first valid signature for each owner and ignores any subsequent valid signatures. This behavior can lead to incomplete signature validation and potential issues in transaction processing, especially when the checkNSignatures function in safe contract requires multiple valid signatures to meet the threshold.

Attack Scenario\

Attachments

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

Flow Explanation

Function Definition: The function takes in a dataHash, signatures, and an array of owners.
Signature Count: It calculates the number of signatures by dividing the length of the signatures array by 65 (each signature is 65 bytes).
Loop Through Owners: It iterates over each owner in the owners array.
Loop Through Signatures: For each owner, it iterates over the signatures to find a valid one.
Signature Validation: It validates the signature by checking if the signer matches the current owner.
Concatenation: If a valid signature is found, it concatenates the signature to the concatenatedSignatures array and breaks out of the inner loop.

Issue

-The break statement in the inner loop causes the function to stop checking for additional valid signatures for the current owner after finding the first valid signature.

-This results in only the first valid signature being included in the concatenatedSignatures array, ignoring any subsequent valid signatures for the same owner.

 leadSafe.checkSignatures(
                keccak256(palmeraTxHashData),
                palmeraTxHashData,
                sortedSignatures
            );

-When the checkSignatures function is called, it may fail if the threshold is greater than 1, as the processAndSortSignatures function does not return all valid signatures.

 function checkSignatures(bytes32 dataHash, bytes memory signatures) public view override {
        // Load threshold to avoid multiple storage loads
        uint256 _threshold = threshold;
        // Check that a threshold is set
        if (_threshold == 0) revertWithError("GS001");
        checkNSignatures(msg.sender, dataHash, signatures, _threshold);
    }

    /**
     * @inheritdoc ISafe
     */
    function checkNSignatures(
        address executor,
        bytes32 dataHash,
        bytes memory signatures,
        uint256 requiredSignatures
    ) public view override {
        // Check that the provided signature data is not too short
    --->    if (signatures.length < requiredSignatures.mul(65)) revertWithError("GS020");

Revised Code File (Optional)

hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

processAndSortSignatures Function Fails to Handle Multiple Valid Signatures for Owners, Leading to Reversions in checkNSignatures Function if Threshold is Greater Than 1 #59