AtomWallet does not fully respect ERC-4337 standard

[hats-bug-reporter]

Github username: --
Twitter username: @burnerelu
Submission hash (on-chain): 0x0f336c0e954ad4133dff4950831ca5d667b42939442ee9ffca1dff0b77507a94
Severity: low

Description:
Description
ERC-4337 states the following: "(The account) .. 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 MUST revert." (https://eips.ethereum.org/EIPS/eip-4337).

In the current AtomWallet implementation, the error return value of the ECDSA.tryRecover function is not checked. This way, regardless of the error signaled by the tryRecover function, the _validateSignature implementation will not revert.

Attack Scenario
Not applicable

Attachments

1. Proof of Concept (PoC) File

validateSigOriginal.sol - contains the function that does not respect the ERC-4337 standard

2. Revised Code File (Optional)
   validateSigProposal.sol - contains two versions of the modified function - one that treats all ECDSA error types in a similar fashion and reverts using a generic error, and one that reverts with specific error types that also take errorArg into account.

A third option would be to call ECDSA.recover instead of ECDSA.tryRecover, as it is done in example https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/samples/SimpleAccount.sol

Files:

• validateSigProposal.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmavGXVDbExrNaFRb66fCX7HvCoY8j8RvXtytR9EqxMD9T)
• validateSigOriginal.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmZ6yAGWfCjD5LRCzaMgzJaQ2U2928tDSs5Z2DorpvurdF)

[mihailo-maksa]

Our detailed perspective:

• Please paste the code files directly here instead of providing download links for easier review and validation.
• The proposed fix should address cases where the signature is invalid, not just where there is a mismatch. Specifically, the _validateSignature function should be updated to check the error return value of the ECDSA.tryRecover function and revert accordingly if the signature is invalid.
• To comply with the ERC-4337 standard, our implementation will ensure that the validation of the signature results in SIG_VALIDATION_FAILED when there is a mismatch, while any other error must cause a revert.
• In conclusion, we acknowledge this as a minor issue and will implement the necessary changes to ensure full compliance with the ERC-4337 standard, enhancing the robustness of our contract's signature validation process.

[burnerelu]

Hi @mihailo-maksa,

Sorry for being a bit clumsy about it, it's my first audit and I'm still getting familiar with the WoW.
I agree with your breakdown and your conclusion, but I do have a small remark/question. By looking at the rules on the competition page, I've seen the following description for issues marked as Low:

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

I considered that this description perfectly fits this issue, as the intention was to rigorously implement the ERC-4337 standard, and the actual behavior was slightly different by failing to revert in some cases. Therefore, I've marked this issue as a "Low".

I noticed that you've added the "Minor" label to it. Did I make a mistake in registering the defect as a "Low", or does the Minor label mean something else? I didn't manage to find anything about the "Minor" severity in the rules of the contest.

As mentioned, this is my first ever audit so I'm probably missing lots of common knowledge. Thanks for the feedback and for validating my first-ever finding! :)

[mihailo-maksa]

Hi @burnerelu,

Thank you for your first-ever finding, and we're pleased to be the place where you started your web3 security journey in audit competitions. We appreciate your effort and the work you've put into identifying the issue.

Regarding your query, our criteria for a "Low" severity issue require that the contract does not function as expected, with no loss of funds for users. In this case, it was not expected anywhere in our documentation and specifications to revert in _validateSignature originally. Your issue describes an improvement to better conform to the existing ERC-4337 standard, rather than a security vulnerability on its own.

Therefore, this issue is considered a "Minor" issue, which still qualifies for a smaller payout. Congratulations on your finding, and we look forward to more contributions from you in the future!

Best regards,
Mihailo