Incorrect signature check in the `validatePaymasterUserOp` function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L77 https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L109

Vulnerability details

Description

In the validatePaymasterUserOp function from the VerifyingSingletonPaymaster contract there is the following check of the signature provided by the verifyingSigner:

/**
 * return the hash we're going to sign off-chain (and validate on-chain)
 * this method is called by the off-chain service, to sign the request.
 * it is called on-chain from the validatePaymasterUserOp, to validate the signature.
 * note that this signature covers all fields of the UserOperation, except the "paymasterAndData",
 * which will carry the signature itself.
 */
function getHash(UserOperation calldata userOp)
public pure returns (bytes32) {
    //can't use userOp.hash(), since it contains also the paymasterAndData itself.
    return keccak256(abi.encode(
            userOp.getSender(),
            userOp.nonce,
            keccak256(userOp.initCode),
            keccak256(userOp.callData),
            userOp.callGasLimit,
            userOp.verificationGasLimit,
            userOp.preVerificationGas,
            userOp.maxFeePerGas,
            userOp.maxPriorityFeePerGas
        ));
}

/**
 * verify our external signer signed this request.
 * the "paymasterAndData" is expected to be the paymaster and a signature over the entire request params
 */
function validatePaymasterUserOp(UserOperation calldata userOp, bytes32 /*userOpHash*/, uint256 requiredPreFund)
external view override returns (bytes memory context, uint256 deadline) {
    (requiredPreFund);
    bytes32 hash = getHash(userOp);

    PaymasterData memory paymasterData = userOp.decodePaymasterData();
    uint256 sigLength = paymasterData.signatureLength;

    //ECDSA library supports both 64 and 65-byte long signatures.
    // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and not "ECDSA"
    require(sigLength == 64 || sigLength == 65, "VerifyingPaymaster: invalid signature length in paymasterAndData");
    require(verifyingSigner == hash.toEthSignedMessageHash().recover(paymasterData.signature), "VerifyingPaymaster: wrong signature");
    require(requiredPreFund <= paymasterIdBalances[paymasterData.paymasterId], "Insufficient balance for paymaster id");
    return (userOp.paymasterContext(paymasterData), 0);
}

The paymasterId value is not checked to be signed by the verifyingSigner, as it is not part of the preimage of the hash value. Also, there are no additional checks on this value anywhere else.

The chainId value is not checked to be signed by the verifyingSigner, as it is also not part of the preimage of the hash value. All in all, signature provided by the verifyingSigner is vulnarable to replay attack on the different chains. For sure, it can be used only for the UserOperation with the same parameters that are part of the hash preimage in the getHash function.

Impact

The possibility of replacement of paymasterId value as it is not signed by the verifyingSigner entity.

The possibility of a cross-chain replay attack on the provided verifyingSigners' signature as chainId is not a part of the data that it is signing.

Recommended Mitigation Steps

Add the paymasterId and chainId to the preimage of the hash that should be signed by the verifyingSigner:

function getHash(UserOperation calldata userOp, address paymasterId)
public pure returns (bytes32) {
    //can't use userOp.hash(), since it contains also the paymasterAndData itself.
    return keccak256(abi.encode(
            userOp.getSender(),
            userOp.nonce,
            keccak256(userOp.initCode),
            keccak256(userOp.callData),
            userOp.callGasLimit,
            userOp.verificationGasLimit,
            userOp.preVerificationGas,
            userOp.maxFeePerGas,
            userOp.maxPriorityFeePerGas,
            paymasterId,
            block.chainid
        ));
}

[c4-judge]

gzeon-c4 marked the issue as primary issue

[livingrockrises]

lack of proof: "The possibility of replacement of paymasterId value as it is not signed by the verifyingSigner entity."

[livingrockrises]

warden has missed other cases of signature replay with this.

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b

[Barichek]

Hey @gzeon-c4 @livingrockrises! With all due respect, I do not agree with the decision.

The report describes the signature replayability. Specifically, the validatePaymasterUserOp function checks the signature of the verifyingSigner. The hash preimage does not contain the paymasterId and chainId values, which leads to the possibility of reusing such a signature with these values changed.

Regarding your previous message:

lack of proof: "The possibility of replacement of paymasterId value as it is not signed by the verifyingSigner entity."

there is the following part of the report:

The paymasterId value is not checked to be signed by the verifyingSigner, as it is not part of the preimage of the hash value. Also, there are no additional checks on this value anywhere else.

[livingrockrises]

hmm what I mentioned in issue #466 happy to discuss further

[c4-judge]

gzeon-c4 marked the issue as grade-c

[gzeon-c4]

Marking as grade-c since the whole issue is upgraded to M in #542.

[vladbochok]

@gzeon-c4 I am a bit confused. Does it mean that this report is evaluated as M finding?

[gzeon-c4]

@gzeon-c4 I am a bit confused. Does it mean that this report is evaluated as M finding?

Yes, the system don't let me upgrade this to M directly, instead I have to create #542 as a placeholder.