Reverting when the AA Wallet is already deployed violates ERC4337

[hats-bug-reporter[bot]]

Github username: @Al-Qa-qa Twitter username: al_qa_qa Submission hash (on-chain): 0x36e29ca734d41d7c5b1ad387c553567829edc763f4e2e19633f64d7886154c98 Severity: medium

Description: Description\ We we deploy our AA Atom wallets using EthMultiVault (SingleTone), we are reverting the transaction if the result was address(0).

EthMultiVault.sol#L375-L381

        assembly {
            atomWallet := create2(0, add(data, 0x20), mload(data), salt)
        }

        if (atomWallet == address(0)) {
            revert Errors.MultiVault_DeployAccountFailed();
        }

This will occur (returning 0) in case of hash collision, or the contract is already deployed.

The problem lies is that according to ERC4337, the SingleTone Factory, which is the contract that is used to create AA wallets, should not revert when deploying an already existed Wallet, and instead it should return the AA address.

EIP4337#first-time-account-creation

it’s expected to return the wallet address even if the wallet has already been created. This is to make it easier for clients to query the address without knowing if the wallet has already been deployed, by simulating a call to entryPoint.getSenderAddress(), which calls the factory under the hood

As stated in the EIP, this helps Clients in simulation process, when doing transaction from a counterfactional wallet, and besides this, It is not the standard way.

Recommendations\

Return the address of the Atom wallet if it is already existed, and the deployment process failed.

EthMultiVault::deployAtomWallet()

if (atomWallet == address(0)) {
-           revert Errors.MultiVault_DeployAccountFailed();
+           atomWallet = computeAtomWalletAddr(atomId);

• if (atomWallet.code.length != 0) {
• revert("Atom Wallet Creation failed, and it is not yet deployed");
• } }

[mihailo-maksa]

The reported issue concerning the deployment logic of AA Atom wallets not aligning with ERC4337 has been reviewed. Here is our detailed perspective:

Enhancement Suggestion: The suggestion to modify the deployment logic to return the address of the already deployed atom wallet instead of reverting is a valid enhancement. This would align the implementation with the ERC4337 standard, which facilitates easier querying and simulation processes for clients.

Current Functionality: The current implementation reverts if the atom wallet deployment returns address(0), indicating either a hash collision or that the contract is already deployed. This approach ensures that the deployment process fails explicitly in case of issues.

Standard Compliance: According to ERC4337, the factory should return the wallet address even if it has already been created. This helps clients in simulation processes and ensures standard compliance.

Severity Assessment: Since this issue does not introduce any vulnerabilities or risks to users and is primarily about aligning with a standard, it is classified as a low severity enhancement.

Conclusion: Modifying the deployment logic to return the address of the already deployed atom wallet is a useful enhancement to align with ERC4337. However, given that we already have a getter function (computeAtomWalletAddr), and the potential increase in gas costs, this change may not be immediately necessary.

Status: This issue is an enhancement.

Suggested Fix: Suggested fix makes sense and can potentially be accepted.

Extra Considerations:

• This change aligns the contract with ERC4337, facilitating easier querying and simulation for clients.
• We already have a getter function (computeAtomWalletAddr) that can retrieve the wallet address.
• Reverting on failure is currently cheaper in terms of gas costs, and the suggested change may increase gas consumption.
• This consideration may also be relevant when deciding on whether to include a mapping of vault ID to atom wallet address.

Comment for the Reporter: Thank you for highlighting this potential issue. Modifying the deployment logic to align with ERC4337 is a good enhancement. However, since reverting is currently a cheaper option and we already have a getter function, this change is not immediately necessary. We can still consider a lower payout for this valid suggestion.

[Al-Qa-qa]

Hi @mihailo-maksa,

Agree with you about gas costs, but unimplementing EIP standards is considered an issue, beside that this will affect clients that batching transactions (user operations).

Unimplementing the EIP standards are categorized from LOWS/MEDIUM, and as illustrated in the report and EIP page, this will affect clients, and may affects users too.

When dealing with clients, they test transactions, and in case of this implementation, testing the transaction (UserOperation) will revert when clients call EntryPoint::getSenderAddress(). which will make them do not process this UserOperation.