Signature validation will not be performed due to condition always failing in some situations

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/main/contracts/proposals/ProposalExecutionEngine.sol#L234

Vulnerability details

Bug description

The Party protocol has added a functionality to allow Parties to sign messages via the ERC1271 standard (see more about this standard here).

In order to do so, the ProposalExecutionEngine.sol contract incorporates an isValidSignature() , which will allow third parties to verify if a signature is actually valid by querying the Party contract directly:

function isValidSignature(bytes32 hash, bytes memory signature) external view returns (bytes4) {
        IERC1271 validator = getSignatureValidatorForHash(hash);
        if (address(validator) == address(1)) {  // @audit this enables potential to signature replay 
            // Signature set by party to be always valid
            return IERC1271.isValidSignature.selector;
        }
        if (address(validator) != address(0)) {
            return validator.isValidSignature(hash, signature);
        }
        if (tx.origin == address(0)) { 
            validator = getSignatureValidatorForHash(0);
            if (address(validator) == address(0)) {
                // Use global off-chain signature validator
                validator = IERC1271(
                    _GLOBALS.getAddress(LibGlobals.GLOBAL_OFF_CHAIN_SIGNATURE_VALIDATOR)
                );
            }
            return validator.isValidSignature(hash, signature);
        }
        return 0;
    }

As we can see in the code snippet, isValidSignature() will query the validator for a given hash by calling the getSignatureValidatorForHash() function, and then it will perform several checks:

• If the validator address for that specific hash is address(1), the signature will directly be valid
• If the validator address is different from the zero address (meaning a validator is specifically set for that hash), then the validator will be used to check if the signature is actually valid
• If none of the previous conditions are fulfilled, a third check is reached. From the Party docs: “When the tx.origin is address(0) (indicative of an off-chain call), we fallback to the validator stored for the zero hash if none is set for the given hash. If the party hasn't set a validator for the zero hash, the party uses the GLOBAL_OFF_CHAIN_SIGNATURE_VALIDATOR stored in globals." This is where the vulnerability can be found. We can see how the contract checks the condition if (tx.origin == address(0) evaluates to true in order to then be able to finally validate the hash either with the validator set for the 0 hash or the global validator (in case there is no validator set for the 0 hash). The problem is that this third step will never take place under any circumstance due to the fact that it is not possible to perform a transaction where tx.origin evaluates to address(0) , hence, the third block of code will never be executed. Checking if (tx.origin == address(0) is not a proper indicative of an off-chain call.

The issue described can lead to a signature that is valid (a signature that either is set as valid in the validator corresponding to the zero hash, or set as valid in the global off-chain signature validator) to never be evaluated as actually valid. This makes the Party contract not being able to validate signatures that might be valid, breaking the expected signature validation functionality.

Impact

Raising as medium due to the fact that although this vulnerability completely breaks the intended behavior for smart contract signature validation in some situations, the issue can be mitigated by setting a validator for that specific hash.

Proof of Concept

As described in the bug description section, it is not possible to execute a transaction where tx.origin == address(0) , effectively making the block of code corresponding to such condition never be executed:

function isValidSignature(bytes32 hash, bytes memory signature) external view returns (bytes4) {
        ...
        if (tx.origin == address(0)) { // <-- this will always be false!
            validator = getSignatureValidatorForHash(0);
            if (address(validator) == address(0)) {
                // Use global off-chain signature validator
                validator = IERC1271(
                    _GLOBALS.getAddress(LibGlobals.GLOBAL_OFF_CHAIN_SIGNATURE_VALIDATOR)
                );
            }
            return validator.isValidSignature(hash, signature);
        }
        return 0;
    }

Tools used

Manual review

Recommended Mitigation

Rethink how the signature validation should be performed in the fallback situation where getSignatureValidatorForHash(hash) returns address(0) for the given hash, not relying on tx.origin.

Assessed type

Other

[ydspa]

Misunderstanding of the usage of tx.origin == address(0)

Invalid

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid