April 12, 2023
This security assessment was prepared by OpenZeppelin.
EIP-4337 is a specification to add account abstraction functionality to the Ethereum mainnet without modifying the consensus rules. The Ethereum Foundation asked us to review the latest version revision of their specification and reference implementation.
We audited the eth-infinitism/account-abstraction repository at the 6dea6d8752f64914dd95d932f673ba0f9ff8e144 commit.
In scope were the following contracts:
contracts
├── bls
│ ├── BLSAccount.sol
│ ├── BLSAccountFactory.sol
│ ├── BLSSignatureAggregator.sol
│ └── IBLSAccount.sol
├── core
│ ├── BaseAccount.sol
│ ├── BasePaymaster.sol
│ ├── EntryPoint.sol
│ ├── SenderCreator.sol
│ └── StakeManager.sol
├── gnosis
│ ├── EIP4337Fallback.sol
│ ├── EIP4337Manager.sol
│ └── GnosisAccountFactory.sol
├── interfaces
│ ├── IAccount.sol
│ ├── IAggregatedAccount.sol
│ ├── IAggregator.sol
│ ├── ICreate2Deployer.sol
│ ├── IEntryPoint.sol
│ ├── IPaymaster.sol
│ ├── IStakeManager.sol
│ └── UserOperation.sol
├── samples
│ ├── DepositPaymaster.sol
│ ├── IOracle.sol
│ ├── SimpleAccount.sol
│ ├── SimpleAccountFactory.sol
│ ├── TestAggregatedAccount.sol
│ ├── TestAggregatedAccountFactory.sol
│ ├── TestSignatureAggregator.sol
│ ├── TokenPaymaster.sol
│ └── VerifyingPaymaster.sol
└── utils
└── Exec.sol
Originally BLSHelper.sol was in scope, but we agreed to deprioritize a complete review during the audit.
After the audit, the Ethereum Foundation asked us to review three new pull requests:
1b85cfb: creates a canonical structure for the user operation hash preimage to prevent possible hash collisions between different user operations.19918cd: moves nonce uniqueness validation to the EntryPoint contract. This now prevents accounts from reusing a nonce across multiple operations, but the new “key” and “sequence number” distinction provides some flexibility with operation ordering.9b5f2e4: adds a reentrancy guard and a new BeforeExecution event to the handleOps and handleAggregatedOps functions. This prevents a possible event-ordering confusion that could occur if the functions were called recursively.As part of the fix review process and our review of these changes, we reviewed the pull requests that affect in-scope contracts up to commit 9b5f2e4.
The system architecture is described in our original audit report, and now contains a series of important changes.
For instance, users and paymasters can now both change the EVM state when validating an operation. This is more general and mitigates the need for a paymaster to have after-revert functionality, which may be removed in a future version. To support this change, additional storage restrictions (described in the EIP) have been added to ensure all validations in a batch access non-overlapping sets of storage slots. In addition, user operations can delegate their validation to an “Aggregator” smart contract, which allows all operations that share an aggregator to be validated together. Aggregators are subject to the same staking and throttling rules as paymasters.
To forestall possible confusion, it is worth noting that in this context, “aggregation” refers to any mechanism that can authenticate independent user operations efficiently. The sample BLSSignatureAggregator contract efficiently validates several BLS signatures over different user operations, but does not use the standard BLS Signature Aggregation technique, which produces a combined signature over a single message. Regardless, the system supports accounts with arbitrary validation logic, so anyone could deploy an account that accepts aggregate BLS signatures over a single message (to produce a multi-signature wallet, for example).
There are also a few incremental changes:
Client reported: The Ethereum Foundation identified this issue during the audit.
During simulation, the EntryPoint contract invokes a view function on the sender contract, before proceeding with the regular validation. Since the first access of any storage slot is more expensive than subsequent accesses, the view function could perform the initial “cold accesses” to allow the regular validation function to use “warm accesses”. If the different gas costs determined whether the validation function ran out of gas, the validation would succeed during simulation but fail on-chain. In this scenario, the bundler would have to pay for the failed transaction.
Update: Resolved in pull request #216 and merged at commit 1f505c5. The aggregator logic has been redesigned, which makes this issue obsolete.
Client reported: The Ethereum Foundation identified this issue before the audit.
The EIP forbids accounts from using the BASEFEE opcode during validation, to prevent them from detecting when they are being simulated offline. However, the EntryPoint contract passes the required pre-fund to the account, which depends on the base fee, thereby leaking this value.
Update: Resolved in pull request #171 and merged at commit b34b7a0. The prefund amount now uses the maximum possible gas price.
Client reported: The Ethereum Foundation shared this issue with us during the audit after it was reported by leekt.
The VerifyingPaymaster contract requires the trusted signer to sign a hash of a user operation. However, the signature is under-specified. In particular:
Update: Resolved in pull request #184 and merged at commit 48854ef.
EIP4337ManagerClient reported: The Ethereum Foundation shared this issue with us during the audit after it was reported by leekt.
The EIP4337Manager contract is intended to augment GnosisSafe contracts, by providing a user op validation function. Safe contracts (technically their proxies) are intended to use delegatecall to access this function.
However, anyone can configure the manager contract with new modules. Since the manager contract inherits GnosisSafe functionality, the new modules can trigger arbitrary function calls and potentially self-destruct the contract. This would effectively disable the manager module for all safes that used it.
Update: Resolved in pull request #208 and merged at commit d92fec8.
Client reported: The Ethereum Foundation identified this issue during the audit.
The EntryPoint contract has four locations where an external function call can revert with an arbitrarily large message that the EntryPoint must copy to its own memory. Each instance has a different practical consequence:
callGasLimit. If this occurs on-chain, the user (or the paymaster) will still be charged for the extra gas consumed. If instead the entire bundle reverted, the FailedOp error would not be returned, so the bundler would not easily recognize the problematic operation.FailedOp error.Update: Partially resolved in pull request #178 and merged at commit 9c00e78. Only the user operation revert reason was limited.
The BLSSignatureAggregator exposes a mechanism to let the bundler validate individual signatures before constructing the bundle. Successful operations are grouped so the bundler can combine their signatures off-chain and the EntryPoint can validate them together on-chain. However, it is possible for an account to construct an operation that will pass the individual-signature check and still fail the combined-signature check.
In particular, if the public key it exposes during the individual validation is different from the one used during the combined validation, the two validations will be inconsistent even though the signature is the same. This could occur if the last 4 words of the initCode do not match the public key (because the initCode has additional data, or if they do not use the expected creation function). It could also occur if the user’s validation function (which is not invoked during the individual signature validation) changes the public key that is returned by getBlsPublicKey.
If a bundler constructs a bundle with these operations, it will be unable to validate the combined signature and will attribute the fault to the aggregator, which will cause the aggregator to be throttled and user operations with the same aggregator will not be processed.
Consider synchronizing the two validation functions so they both use the same public key.
Update: Resolved in pull request #195 as well as commit 268f103 of pull request #216, which were merged at commits 1cc1c97 and 1f505c5 respectively.
EntryPoint [samples]The comments describing the initialize function of the SimpleAccount contract claim there should be a mechanism to replace the EntryPoint contract. This does not match the behavior of the function it describes, and in fact, there is no mechanism to replace the EntryPoint contract without upgrading the whole account.
Consider updating the comment to match the behavior, and introducing a mechanism to replace the EntryPoint contract if that functionality is desired.
Update: Resolved in pull request #192 and merged at commit 82685b2. A @dev comment was added to the docstring of the initialize function to clarify that the _entryPoint storage variable is not a parameter of the initializer because an upgrade is required to change the EntryPoint address.
The documentation for the SIG_VALIDATION_FAILED constant states that validateUserOp must return this value instead of reverting if signature validation fails. The SimpleAccount contract correctly follows the specification, however in the EIP4337Manager contract, the validateUserOp function reverts if the signature validation fails. This means the simulateValidation function will revert without providing a ValidationResult object.
Consider changing the logic so that validateUserOp returns SIG_VALIDATION_FAILED in all cases where an invalid signature is encountered.
Update: Resolved in pull request #181 and merged at commit 1dfb173.
The EntryPoint contract decrements the operation expiry timestamp in order to convert 0 (which should be interpreted as “no expiry”) to the maximum uint64 value. However, every other possible expiry value is now off by one. In the interest of predictability, consider only modifying the 0 timestamp.
Update: Resolved in pull request #193 and merged at commit 973c0ac.
Several docstrings and inline comments throughout the code base were found to be incorrect or misleading. In particular:
BaseAccount.sol:
sigTimeRange as “signature and time-range for this operation”, but it contains the signature validity, not the signature itself.BLSSignatureAggregator.sol:
simulateUserOperation. The function name should be simulateValidation.EIP4337Manager.sol:
GnosisSafeStorage, but it actually inherits GnosisSafe.EntryPoint.sol:
paymasterAndData as one of the dynamic byte arrays being excluded from MemoryUserOp._validatePaymasterPrepayment validates that the paymaster is staked, but the function does not perform this check.IPaymaster.sol:
validUntil and validAfter timestamps are 4 bytes in length, but these are 8-byte (uint64) values.IStakeManager.sol:
DepositInfo docstring explains that the variable sizes were chosen so that deposit and staked fit into a single uint256 word, but the 3rd parameter stake will also fit.SimpleAccount.sol:
execFromEntryPoint function, which no longer exists.execute says “called directly from owner, not by entryPoint”, but the _requireFromEntryPointOrOwner function allows execute to be called by the EntryPoint. The comment isn’t clear on whether it is a suggestion, or a restriction to be enforced.initialize function._requireFromEntryPointOrOwner function.IEntryPoint.sol:
@success parameter is listed in the wrong order.UserOperation.sol:
callGasLimit parameter has no @param statement.Update: Resolved in pull request #194 and pull request #216, which were merged at commits faf305e and 1f505c5 respectively.
The EIP states that when a FailedOp is detected, all other operations from the same paymaster should be removed from the current batch. However, this should only apply to FailedOp errors that explicitly mention the paymaster, which imply the paymaster was at fault. Operations that fail for unrelated reasons should not penalize their paymaster.
The EIP also states that userOp validation cannot call the handleOps method. This restriction should also apply to handleAggregatedOps.
Consider clarifying these points in the EIP.
Update: Partially resolved in pull request #196 and merged at 5929ff8. The updated EIP mistakenly refers to the EntryPoint’s depositTo function as depositFor.
The StakeLocked event specifies a withdrawTime parameter, but the argument passed in is the new unstake delay. Consider renaming the event parameter to match its actual usage.
Update: Resolved in pull request #197 and merged at commit 545a15c.
Throughout the codebase there are several parts that do not have docstrings. For instance:
BLSAccount.solBLSAccount.solBLSAccount.solBLSAccount.solBLSSignatureAggregator.solBLSSignatureAggregator.solBLSSignatureAggregator.solIBLSAccount.solBasePaymaster.solBasePaymaster.solBasePaymaster.solEntryPoint.solStakeManager.solEIP4337Fallback.solGnosisAccountFactory.solIStakeManager.solUserOperation.solDepositPaymaster.solSimpleAccount.solSimpleAccount.solTestAggregatedAccount.solTestAggregatedAccount.solTestSignatureAggregator.solTestSignatureAggregator.solTestSignatureAggregator.solTokenPaymaster.solExec.solConsider thoroughly documenting all functions and their parameters, especially public APIs. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Partially resolved in pull request #212 and merged at commit eeb93b2. The recommended changes to GnosisAccountFactory.sol were not implemented.
Within the codebase there are some require statements that lack error messages:
require statement on line 105 of BasePaymaster.solrequire statement on line 49 of DepositPaymaster.solrequire statement on line 137 of SimpleAccount.solConsider including specific, informative error messages in require statements to improve overall code clarity and facilitate troubleshooting whenever a requirement is not satisfied.
Update: Resolved in pull request #198 and merged at commit 182b7d3. Error messages were added to the deficient require statements in BasePaymaster.sol and DepositPaymaster.sol, and the require statement in SimpleAccount.sol was eliminated as part of a code change.
The EIP states that an aggregated account should support the getAggregationInfo function, and that this function should return the account’s public key, and possibly other data. However, the BLSAccount contract does not contain a getAggregationInfo function. Consider renaming the getBlsPublicKey function to getAggregationInfo.
Update: Resolved in pull request #199 and merged at commit 12d2ac0. The EIP now uses the getBlsPublicKey function as an example.
The SimpleAccountFactory creates a new implementation contract but does not initialize it. This means that anyone can initialize the implementation contract to become its owner.
The consequences depend on the version of OpenZeppelin contracts in use. The project requires release 4.2 and later, but release 4.8 is locked. The onlyProxy modifier was introduced in release 4.3.2 to protect the upgrade mechanism. Without this modifier, the owner is authorized to call the upgrade functions on the implementation contract directly, which lets them selfdestruct it.
With the locked version, the implementation owner can execute arbitrary calls from the implementation contract, but should not be able to interfere with the operation of the proxies.
Nevertheless, to reduce the attack surface, consider restricting the versions of OpenZeppelin contracts that are supported and disabling the initializer in the constructor of the SimpleAccount contract, to prevent anyone from claiming ownership.
Update: Resolved in pull request #201 and merged at commit 4004ebf.
The EntryPoint contract can emit a FailedOp error where the reason parameter provides additional context for troubleshooting purposes. However, there are two locations (line 375 and line 417) where an untrusted contract can provide the reason, potentially including misleading error codes. For example, the sender validateUserOp function might revert with "AA90 invalid beneficiary", which might cause confusion during simulation.
Consider prefixing the externally provided revert reasons with a uniquely identifying error code.
Update: Resolved in pull request #200 and merged at commit 3d8f450.
It is not an uncommon practice to use abi.encodeWithSignature or abi.encodeWithSelector to generate calldata for a low-level call. However, the first option is not safe from typographical errors, and the second option is not type-safe. The result is that both of these methods are error-prone and should be considered unsafe.
Within EIP4337Manager.sol, there are some occurrences of unsafe ABI encodings being used:
Consider replacing all occurrences of unsafe ABI encodings with abi.encodeCall, which checks whether the supplied values actually match the types expected by the called function, and also avoids typographical errors.
Note that a bug related to the use of string literals as inputs to abi.encodeCall was fixed in version 0.8.13, so developers should exercise caution when using this function with earlier versions of Solidity.
Update: Resolved in pull request #220 and merged at commit c0a69bf. The first example is an invalid recommendation because it is encoding an error.
uint/int as uint256/int256 [core and samples]Throughout the codebase, there are multiple instances of int and uint being used, as opposed to int256 and uint256. In favor of explicitness, consider replacing all instances of int with int256, and uint with uint256.
Update: Partially resolved in pull request #215 and merged at commit 998fa7d. Most instances have been addressed but there are some uint types remaining.
To provide additional clarity regarding whether a given contract file contains core, sample, or test code, consider the following recommendations to move project files:
samples directory, TestAggregatedAccount.sol, TestAggregatedAccountFactory.sol, and TestSignatureAggregator.sol contain test contracts similar to those found in the contracts/test directory. Consider relocating these files to the contracts/test directory.bls and gnosis directories contain sample account implementations, but do not reside in the samples directory. Consider moving these items to the samples directory.Update: Resolved in pull request #217 and merged at commit f82cbbb.
The IAggregatorAccount interface extends the base IAccount interface by adding the ability to expose a signature aggregator associated with the account. To add support for handling aggregated user operations, the validateUserOp function in IAccount now includes an aggregator address parameter. Accounts not associated with an aggregator must provide a null address for this parameter. This represents an anti-pattern where a base class is aware of features only relevant to a derived class.
To address this case and future enhancements of the protocol, consider replacing the aggregator parameter in validateUserOp with a more generic extensions parameter that can be used to specify the aggregator as well as any future account-specific extensions.
Update: Resolved in pull request #216 and merged at commit 1f505c5.
The packSigTimeRange function of the BaseAccount contract implicitly assumes the timestamps fit within 8 bytes. Consider enforcing this assumption by using uint64 parameters.
Update: Resolved in pull request #203 and merged at commit fa46d5b.
The BLSAccount contract emits an event when the public key is changed, but not when it is initialized. To complete the event history, consider emitting the event on initialization as well.
Update: Resolved in pull request #204 and merged at commit 2600d7e.
The aggregator parameter in the SignatureAggregatorChanged event is not indexed. Consider indexing the event parameter to avoid hindering the task of off-chain services searching and filtering for specific events.
Update: Resolved in pull request #202 and merged at commit 1633c06.
To favor explicitness and readability, there are several locations in the contracts that may benefit from better naming. Our suggestions are:
BaseAccount.sol:
packSigTimeRange function is internal but is not prefixed with “_”. Consider renaming to _packSigTimeRange.BasePaymaster.sol:
packSigTimeRange function is internal but is not prefixed with “_”. Consider renaming to _packSigTimeRange.BLSSignatureAggregator.sol:
hashPublicKey to publicKeyHash for consistency.EIP4337Manager.sol:
_msgSender to msgSender for consistency.IAggregator.sol:
aggregateSignatures function from aggregatesSignature to aggregatedSignature.IEntryPoint.sol:
ExecutionResult error uses validBefore instead of validUntil. For consistency, consider changing the parameter name to validUntil.ReturnInfo struct’s documentation for the validAfter parameter indicates it is inclusive. Consider renaming it to validFrom throughout the entire codebase.AggregatorStakeInfo struct, consider renaming actualAggregator to aggregator (also in the comment here).SenderCreator.sol:
createSender function, consider renaming the initAddress variable to factory to be consistent with the EntryPoint contract.SimpleAccount.sol:
addDeposit function, consider renaming the req variable to success.StakeManager.sol:
internalIncrementDeposit is an internal function that uses “internal” as its prefix instead of “_”. Consider changing to _incrementDeposit.getStakeInfo function is internal but not prefixed with “_”. Consider renaming the function to _getStakeInfo.addr parameter of getStakeInfo to account._unstakeDelaySec in StakeManager now that there is no longer a storage variable named unstakeDelaySec.Update: Resolved in pull request #221 and merged at commit 7bd9909.
The Solidity Style Guide specifies a recommended order for the layout of elements within a contract file in order to facilitate finding declarations grouped together in predictable locations. Within the codebase, this recommendation is not followed in several places:
BLSAccount.sol: The PublicKeyChanged event is defined between two functions.BLSSignatureAggregator.sol: Constant value N is defined between two functions.IEntryPoint.sol: Starting at line 70, error and struct definitions are intermingled with function definitions.IPaymaster.sol: The PostOpMode enum is defined after all functions.SimpleAccount.sol: The _entryPoint variable, SimpleAccountInitialized event, and onlyOwner modifier are defined after several function definitions.To improve the project’s overall legibility, consider standardizing ordering throughout the codebase, as recommended by the Solidity Style Guide.
Update: Partially resolved in pull request #211 and merged at commit ca1b649. In IEntryPoint.sol, the error definitions were relocated but several struct definitions remain defined in between functions.
The StakeManager allows deposits up to the maximum uint112 value, but the stake must be strictly less than the maximum unit112 value. Consider using the same maximum in both cases for consistency.
Update: Resolved in pull request #209 at commit 419b7b0.
The following instances of TODO comments were found in the codebase:
TODO comments are more likely to be overlooked and remain unresolved if they are not being tracked formally as issues. Over time, important information regarding the original motivation, plan, and supporting details may be lost. Consider removing all instances of TODO comments and tracking them in the issues backlog instead. Alternatively, consider linking each inline TODO to the corresponding issues backlog entry.
Update: Resolved in pull request #218 and merged at commit 80d5c89. The first example is obsolete. The other two are not TODOs and were changed to “Note”.
Consider addressing the following typographical errors:
BaseAccount.sol:
BLSAccount.sol:
BLSAccountFactory.sol:
BLSHelper.sol:BLSSignatureAggregator.sol:
DepositPaymaster.sol:
EIP4337Manager.sol:
EntryPoint.sol:
IAccount.sol:
IAggregatedAccount.sol:
IAggregator.sol:
IEntryPoint.sol:
IPaymaster.sol:
IStakeManager.sol:
SimpleAccount.sol:
TestAggregatedAccount.sol:
TestAggregatedAccountFactory.sol:
TokenPaymaster.sol:
UserOperation.sol:
Update: Resolved in pull request #219 and merged at commit b4ce311.
Throughout the codebase imports on the following lines are unused and could be removed:
console of BLSSignatureAggregator.solEIP4337Manager of EIP4337Fallback.solExec of GnosisAccountFactory.solIAggregator of IAggregatedAccount.solUserOperation of IAggregatedAccount.solOwnable of DepositPaymaster.solBaseAccount of TestAggregatedAccount.solSimpleAccount of TestSignatureAggregator.solconsole in TestSignatureAggregator.solSimpleAccount of TokenPaymaster.solConsider removing unused imports to avoid confusion that could reduce the overall clarity and readability of the codebase.
Update: Resolved in pull request #206 and merged at commit e019bbd.
The ICreate2Deployer.sol import was removed from EntryPoint.sol in pull request #144, but the file still exists in the interfaces directory. None of the contracts import this file.
Consider deleting the unused interface file.
Update: Resolved in pull request #205 and merged at commit 679ac11.
Throughout the codebase, an effort has been made to change the term “wallet” to “account”, e.g. SimpleWallet was renamed SimpleAccount. However, some “wallet” references remain in various comments:
To avoid confusion, consider replacing these instances of “wallet” with “account”.
Update: Resolved in pull request #210 and merged at commit d6a2db7.
One high severity issue was found. Several changes were proposed to improve the code’s overall quality and reduce the attack surface.
While audits help in identifying potential security risks, the Ethereum Foundation is encouraged to also incorporate automated monitoring of on-chain contract activity, and activity within the new mempool, into their operations. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment. In this case, it may also provide useful information about how the system is being used or misused. Consider monitoring the following items: