The SmartAccount's validateUserOp function should return SIG_VALIDATION_FAILED rather than revert if the signature is invalid

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L506-L513
https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L63

Vulnerability details

Impact

According to the Biconomy's docs (check contest overview),

Biconomy Smart Account is a smart contract wallet that builds on core concepts of Gnosis / Argent safes and implements an interface to support calls from account abstraction Entry Point contract

As per Biconomy, it doesn't support signature aggregation at the moment, and according to Account Abstraction specs EIP-4337

If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error should revert

However, the SmartAccount reverts rather than returns SIG_VALIDATION_FAILED. This migh lead to incompatibility since any EntryPoint that would be used in future will expect a returned value in case of invalid signature instead of a revert.

Proof of Concept

validateUserOp function (in BaseSmartAccount) calls _validateSignature to validate the userOp (UserOperation). However, _validateSignature reverts if the signature is invalid rather than returning SIG_VALIDATION_FAILED.

Check SmartAccount contract for

    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
    internal override virtual returns (uint256 deadline) {
        bytes32 hash = userOpHash.toEthSignedMessageHash();
        //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
        // solhint-disable-next-line avoid-tx-origin
        require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
        return 0;
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

If the signature is invalid return SIG_VALIDATION_FAILED. for any other error, revert.

Note: adjust the EntryPoint contract according to the suggested change above.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

not high risk

[livingrockrises]

you're right. it has to be rebased with the latest code (0.4.0).

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[c4-judge]

gzeon-c4 marked the issue as selected for report

[vladbochok]

Hey @gzeon-c4 @livingrockrises!

The #498 describes that validateUserOp() SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch.

This is currently not the case, as the case when the account does not support signature aggregation is not supported right now in the code. The validateUserOp() reverts everytime if the recovered signature does not match.

At the same time, the nature of these reports is very similar, so it does not give impact examples, but claim to be incompatible with the standard. All in all, I think this report and its duplicates should be evaluated as duplicates of #498, and not a separate issue.

[livingrockrises]

yes it merely talks about contracts should be upgraded as entry point upgrades. when we scoped for C4 ERC4337 was at v0.3.0 now they are at 0.4.0 plus new commits that will become 0.5.0 with these interface updates. we have been already rebasing our wallet source with latest versions. just that it couldn't be sent for c4 audit scope end of year

[vladbochok]

@gzeoneth Could you please take a look at my message above?

[gzeon-c4]

Hey @gzeon-c4 @livingrockrises!

The #498 describes that validateUserOp() SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch.

This is currently not the case, as the case when the account does not support signature aggregation is not supported right now in the code. The validateUserOp() reverts everytime if the recovered signature does not match.

At the same time, the nature of these reports is very similar, so it does not give impact examples, but claim to be incompatible with the standard. All in all, I think this report and its duplicates should be evaluated as duplicates of #498, and not a separate issue.

make sense

[c4-judge]

gzeon-c4 marked the issue as duplicate of #498

[c4-judge]

gzeon-c4 marked the issue as not selected for report

[c4-judge]

gzeon-c4 marked the issue as satisfactory