VerifyingSingletonPaymaster.validatePaymasterUserOp does not work when sigLength == 64

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L97-L111

Vulnerability details

Impact

Openzeppelin canceled support for compact signatures in the ECDSA library after version 4.7.3, that is, it no longer supports recovery of signatures with a length of 64

## 4.7.3

### Breaking changes

 * `ECDSA`: `recover(bytes32,bytes)` and `tryRecover(bytes32,bytes)` no longer accept compact signatures to prevent malleability. Compact signature support remains available using `recover(bytes32,bytes32,bytes32)` and `tryRecover(bytes32,bytes32,bytes32)`.

And biconomy uses the openzeppelin library above version 4.7.3

    "@openzeppelin/contracts": "^4.7.3",
    "@openzeppelin/contracts-upgradeable": "^4.7.3",

In the VerifyingSingletonPaymaster.validatePaymasterUserOp function, the code indicates that the signature with a length of 64 is still supported, but actually when the user uses a signature with a length of 64 in EntryPoint.handleOps, recover will return 0 address, the require statement fails, and validatePaymasterUserOp cannot work.

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

Proof of Concept

OpenZeppelin/openzeppelin-contracts@e1878ac
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/package.json#L61-L62
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L107-L108
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L363-L374

Tools Used

None

Recommended Mitigation Steps

Change to

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

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

low risk

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-b