AmbireReview01.md

	Ambire Wallet V2 (ERC4337, Paymaster, DKIM Recovery) Prepared by: Curiousapple, Independent Security Researcher Duration: 4 days Date of Delivery: 24 Oct 2023

About

Ambire

Ambire is one of the pioneers of smart contract wallets, with version 1 launched in late 2021. This particular update, version 2, focuses on their implementation of EIP4337, Paymaster, and a novel recovery scheme, DKIM. The DKIM recovery system allows users to perform self-custodial email/password authentication on-chain and recover their wallet.
More details about DKIM recovery can be found here

Curiousapple 🦇

Abhishek Vispute, known online as 'Curiousapple', is an independent smart contract security researcher. Previously, he served as a lead smart contract auditor at Macro and is currently working independently.
His auditing experience covers diverse set of protocols, including DeFi, Bridges, NFTs, DAOs, and Games, in all of which he has discovered severe bugs.
You can find his previous work and contact here

Scope

Repo: ambire-common
Branch: v2
Commit: f411456c06a409bbcbbee0c12c7496916202860b
Contracts:

1. contracts/AmbireAccount.sol
2. contracts/AmbirePaymaster.sol
3. contracts/DKIMRecoverySigValidator.sol

Please note that none of the following dependencies or libraries were in scope, as they were assumed to be functioning correctly.

• SignatureValidator.sol
• Strings.sol
• Base64
• BytesUtils
• RSASHA256
• DNSSECImpl
• RRUtils
• OpenZeppelinStrings

Summary of Findings

No severe issues were found.

Acronym
C	Critical
H	High
H	Medium
L	Low
Q	Quality

ID	Title	Status
C-01	Anyone can take control of someone's wallet by encoding the address inside the subject	Fixed
M-01	Users can replay Ambire's paymaster signatures and make Ambire pay more than intended if Entrypoint is changed	Fixed
Q-01	The old nomenclature of "Identity" is still being followed	Fixed
I	Informationals	-

Detailed Findings

[C-01] Anyone can take control of someone's wallet by encoding the from address inside the subject

Impact: High
Likelihood: High

Ambire allows users to recover their wallets using DKIM signatures if the DKIM signature is done on headers equal to what the user has specified in their account info. However, due to a flaw in the parsing logic of _verifyHeaders, anyone can spoof the emailFrom address and take control of the wallet.

Let's take the following header as an example:

to: adamcrein@gmail.com
subject: Give permissions to 0x70997970c51812dc3a010c7d01b50e0d17dc79c8 SigMode 0
message-id: <CAKXuq_X92FgFsmP_jvEyeTJDprj9_QG_fV=K4F3DWK50-gGQ0g@mail.gmail.com>
date: Mon, 21 Aug 2023 05:01:56 +0300
from: test testov <alice@gmail.com>

While verifying headers, Ambire's current logic is as follows:

• The "to" header should start with "emailTo" (startsWith).
• The "subject" header should contain the string "subject:Give..." (split).
• The "from" header should contain "emailFrom" after "from:" (find).

All of these checks are performed from top to bottom, left to right.

Now, to gain control of Alice's wallet, what if I pass the following subject:

to: adamcrein@gmail.com
subject: **from:test testov <alice@gmail.com> subject:Give permissions to 0x70997970c51812dc3a010c7d01b50e0d17dc79c8 SigMode 0**
message-id: <CAKXuq_X92FgFsmP_jvEyeTJDprj9_QG_fV=K4F3DWK50-gGQ0g@mail.gmail.com>
date: Mon, 21 Aug 2023 05:01:56 +0300
from: mr.robot <attack@gmail.com>

The subject will be identified as "subject:Give permissions to 0x70997970c51812dc3a010c7d01b50e0d17dc79c8 SigMode 0" since it exists as expected by the verifyHeaders logic. Ambire's verifyHeaders will start from the top and identify the "from" as the one I spoofed in the subject (alice@gmail.com) and not the actual one (attack@gmail.com) since the search stops at the first find. This would basically allow someone to gain access to user wallets for matching selectors and all user wallets who accept unknown selectors.

Recommendation

Consider correcting the parsing logic so that it is independent of user-dependent fields.

Status

Fixed

---

[M-01] Users can replay Ambire's paymaster signatures and make Ambire pay more than intended if Entrypoint is changed

Impact: Medium
Likelihood: Medium

Entrypoints can change and each one has its own nonce management. Now the question is, if the entrypoint is changed from A to B, can a transaction that was already executed using A for nonce N1 be replayed on B?

Entrypoint resolves this attack vector by encoding the entrypoint address to the userOp hash.
getUserOpHash

The vulnerable case in Ambire’s scenario is validatePaymasterUserOp.

Ambire's validatePaymasterUserOp ignores the passed userOphash from the entrypoint and derives its own hash, due to cyclic dependency.
Source

Recommendation

Consider adding entrypoint to the signed digest of validatePaymasterUserOp

Status

Fixed

---

[Q-01] The old nomenclature of "Identity" is still being followed

Instances :

1. require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
2. require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
3. require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
4. require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
5. // using our own code generation to insert SSTOREs to initialize privileges (IdentityProxyDeploy.js)

Recommendation

Consider replacing the old nomenclature of "Identity" with "AmbireAccount".

Status

Fixed

---

Informationals

1. Any call made to AmbireAccount will be successful if fallbackHandler == address(0).
   
2. Since chain.id is not included in external signatures (DKIM), a DKIM recovery on one chain of a wallet could be executed on another chain by anyone.
   
3. Low-level calls will return success for all cases if to is not a contract.
   
4. Once given DKIM key is disabled it can not be used again since key.isExisting persists.
5. There is no guarantee that all ExecuteArgs of executeMultiple() would be executed at once only. Since the signature verification is done on individual set of toExec.calls, anyone can call execute() directly and execute individual actions if the nonce is in sync.
   For example, let's consider Alice calls executeMultiple⇒[Set A, Set B].
   Anyone can execute actions of set A without executing actions from set B. Once the nonce is incremented by another action, the same applies to set B.

---

Disclaimer

curiousapple's review is limited to identifying potential vulnerabilities in the code. It does not investigate security practices, operational security, or evaluate the code relative to a standard or specification.
curiousapple makes no warranties, either express or implied, regarding the code's merchantability, fitness for a particular purpose, or that it's free from defects.
curiousapple will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, procurement costs of substitute goods or services, or any claim by any other party.
curiousapple will not be liable for any consequential, incidental, special, indirect, or exemplary damages, even if it has been advised of the possibility of such damages.
This review does not constitute investment advice, is not an endorsement, and is not a guarantee as to the absolute security of the project.
By deploying or using the code, users agree to use the code at their own risk.
curiousapple is not responsible for the content or operation of any third-party websites or software linked or referenced in the review, and shall have no liability for the use of such.
curiousapple is same as "Abhishek Vispute" in this context.