eth-infinitism / account-abstraction Goto Github PK

After processing a deposit, the StakeManager emits a Deposited event, but incorrectly uses the msg.sender instead of the updated account as the account parameter. This may mislead observers and offline processing systems. Consider updating the account parameter to match the deposit functionality.

Client reported: The Ethereum Foundation identified this issue during the audit

The withdrawTo function in StakeManager and the transfer function in SimpleWallet both uses Solidity's built-in transfer function (on line 129 and line 52, respectively) to send ether to a destination address. The use of transfer for this purpose is no longer recommended.
Consider using the call function or OpenZeppelin's sendValue function, and adhere to the checks- effects-interactions pattern when sending value to an external address. This pattern is already implemented in the compensate function of the EntryPoint contract.

The _validatePrepayment function of the EntryPoint contract combines the various gas parameters into a single variable so they can be collectively compared against a hardcoded limit. Although this correctly ensures all values are individually less than the maximum uint120 value, it is still possible for the combination of smaller values to equal the limit, and unexpectedly fail the check. In practice, at least one of them would need to be impractically large to reach this edge case. Nevertheless, consider using an inclusive bound (i.e. <=) so that the combined guard condition can be conceptually simplified to "each component must fit within a uint120 variable".

In the SimpleWallet contract, the owner and nonce state variables are grouped together into an OwnerNonce struct. Since these variables would still pack into a single storage slot without the struct, encapsulating them together in OwnerNonce does not appear to provide any benefit in the current design, but does require the addition of custom owner and nonce getter functions.
Consider removing the OwnerNonce struct and having standalone nonce and owner state variables.

The validatePaymasterUserOp function in the IPaymaster interface takes a requestId parameter which can be calculated by calling the getRequestId function of EntryPoint. None of the paymaster sample contracts (TokenPaymaster, VerifyingPaymaster, DepositPaymaster) use this variable in their implementations of validatePaymasterUserOp, making it unclear as to why it is being included in the interface.

Consider providing a sample paymaster contract that demonstrates the use of the requestId parameter for validation.

When the owner of the DepositPaymaster adds a new supported token-oracle pair, it ensures the token does not already have an oracle. There is no mechanism to change or remove the oracle. We are simply noting this in case it's an oversight.

The withdrawTo function of the StakeManager contract emits an event after sending the funds. This violates the Checks-Effects-Interactions pattern and it introduces the possibility that some events could be emitted out of order if the recipient's fallback function executes another operation. Consider emitting the event before the funds transfer.

On a fresh checkout of the project repository and npm install of the dependencies, the entire test suite fails to run. This has been identified as the result of a mismatch between specific package versions used during development vs. the current versions of those packages.
On a stable production branch, it is recommended to lock the package versions within the package.json file to prevent package updates from breaking code that has been previously verified to work correctly.

In the EntryPoint contract, the paymasterStake variable has the same name as one of the PaymentMode options.

Consider using different names to improve code readability and avoid confusion.

In order to ensure a user operation can be financed, the maximum amount of gas it could consume is calculated. This depends on the individual gas limits specified in the transaction. Since the paymaster may use verificationGas to limit up to three function calls, operations that have a paymaster should multiply verificationGas by 3 when calculating the maximum gas. However, the calculation is inverted. This means that valid user operations could fail in two ways:

• operations that do not rely on paymasters will be initially overcharged, and the wallet may have insufficient funds to proceed. It should be noted that wallets with sufficient funds will still have any unused gas (including the overcharge) refunded after the operation is executed.
• paymasters that are insufficently staked may nevertheless pass validation, which introduces the possibility that they will be unable to fund the operation. In practice, the excess funds will probably simply be deducted from their stake. Even so, this allows users to craft operations that would unexpectedly cause the paymaster to become unstaked.

Consider updating the maximum gas calculation to match the execution behavior.

The IWallet interface specifies a single validateUserOp function that must be implemented by the wallet developer. This function's docstring lists requirements that must be enforced by the implementation such as checking and incrementing the nonce and verifying that the function was called by the EntryPoint contract. The SimpleWallet sample contract contains all the necessary logic in its own validateUserOp implementation, but developers are not required to use this code and could make errors when attempting to refactor it into their own custom implementation.
To help developers create secure wallet implementations that follow the EIP specification, consider removing the IWallet interface and replacing it with an abstract "BaseWallet" contract that implements all of the mandated checks, but leaves the custom behavior such as validateSignature up to derived classes. The existing SimpleWallet contract would then be derived from BaseWallet.

The handleOp function of the EntryPoint contract tracks and records both the pre-operation gas and the gas consumed before the final postOp call. However, in contrast to the equivalent calculations in handleOps, both values use the same preGas value. This means that the gas used during payment validation is accounted for twice, and the wallet or paymaster will be overcharged for the operation.

In the UserOperation contract, the getSender function takes a UserOperation struct as input and returns the sender field of the struct using inline assembly code. This is the only struct member that has its own custom getter function, and even though there are two addresses in the struct ( sender and paymaster), only this one has a getter function. It is recommended to avoid using inline assembly because in general it is less safe and error-prone. In this particular instance, the code works because sender is the first item in the UserOperation struct, but if the sender variable position in the struct were to change, this code would break.
For safety and clarity, consider removal of the getSender function.

We believe some functions are variables could benefit from renaming. These are our suggestions:
• The IOracle, DepositPaymaster and TokenPaymaster contracts all have a getTokenToEthOutputPrice function, but in all cases, it's not a price (because it accounts for the amount bought) and it appears to be described backwards. Something like getTokenValueOfEth would be clearer.
• The last parameter of validateUserOp in the IWallet interface should be missingWalletFunds or additionalFundsRequired to match how it's used in the EntryPoint contract.
• The gasUsedByValidateUserOp variable should be gasUsedByValidateWalletPrepayment
• The PaymentMode options should be paymasterDeposit and walletDeposit, because neither
uses the "stake" for gas payments.
• The sender parameter in the _call function of the SimpleWallet contract should be target.
• The maxPriorityFeePerGas component of the UserOperation struct should be priorityFeePerGas .
• In BasePaymaster, the setEntrypoint and _requireFromEntrypoint functions should capitalize the "p" for consistency with the rest of the code base.

After executing the user operation, the internalHandleOp function of the EntryPoint contract invokes handlePostOp with a zero opIndex, regardless of the actual position of the operation within the branch. In this particular invocation, the opIndex parameter is unused, so setting it to zero was chosen as a simplification and gas optimization. Nevertheless, in the interest of code clarity, robustness and to support local reasoning, consider either refactoring the code to avoid the unnecessary parameter, passing in the correct value, or clearly documenting any misleading parameter assignments.

In the VerifyingPaymaster contract, the validatePaymasterUserOp function contains a check that the ECDSA signature being verified has a length >= 65 bytes. The tryRecover function in the OpenZeppelin ECDSA library does not support signatures longer than 65 bytes. It's possible for an invalid signature length to pass the check in validatePaymasterUserOp and later cause the transaction to revert.
Consider modifying the check in validatePaymasterUserOp to only allow ECDSA signature lengths equal to 65.

In the EntryPoint contract, in addition to the handleOps function documented in the EIP-4337 specification, there is also a handleOp function that just implements the special case of handleOps where there is only a single UserOperation to be processed. This is inconsistent with the specification and increases the attack surface. It also introduces the possibility that the logic will differ between the two implementations, as demonstrated by Duplicate validation gas accounting.
Consider removing the handleOp function from the EntryPoint API, or using the handleOps function as its implementation.

The StakeManager contract holds ETH on behalf of users for two different reasons:

• Paymasters lock a deposit as an anti-sybil mechanism
• Wallets and paymasters preemptively transfer funds to pay the gas costs associated with user operations

However, the StakeManager contract treats these funds equivalently, making the boundary harder to identify, enforce and reason about. For example, paymasters are unable to decrease the amount of funds available to users without first unstaking and waiting for the withdrawal period. Moreover, miners may choose different thresholds for the minimum paymaster stake, which means they can unilaterally

partition a paymaster's deposit between the two amounts on a per-transaction basis. Consequently, unless they explicitly account for this in their validation function, paymasters cannot safely lock capital in the StakeManager without risking that it will be used to pay for user transactions.
Additionally, we believe the Paymasters can spend locked state issue is a consequence of this unclear boundary.
Consider separating the handling of transaction prepayments and paymaster stake to better encapsulate the two concepts.

The addStakeTo function of the StakeManager contract allows an attacker to update the deposit record associated with another account, and manipulate it in two significant ways.
Firstly, the new funds are added to the caller's current balance instead of the current account balance. This effectively allows anyone to delete the deposit from any account. Secondly, the account's new stake delay is directly chosen by the caller and could be unreasonably long.
Consider replacing the addStakeTo function with an addStake function that only allows the caller to update their own deposit record.

The ecrecover2 function of the ECDSA contract invokes the ecrecover precompile but ignores the return value. Since the input and output buffers overlap, whenever the address recovery operation fails, the least significant 20 bytes of the hash parameter are incorrectly returned as the signer.
Typically, we would recommend checking the returned status flag to identify failed address recovery operations. However, in our internal testing, the precompile unexpectedly returned success on all operations (whether the signer was recovered or not). An alternative option would be to use an empty output buffer, so failure to recover an address will return the zero address, which is the expected behavior of ecrecover. Nevertheless, we would recommend understanding the reason for the unexpected test return value rather than bypassing it. Consider updating the code to correctly handle failed address recoveries.

The _validateWalletPrepayment function of the EntryPoint will attempt to deploy the wallet if the operation specifies an initCode. This will fail if the wallet cannot be deployed, or it does not match the sender address. However, operations without an initCode do not guarantee that the wallet has already been deployed. If not, the validateUserOp call will unexpectedly revert before execution enters the try block (so the FailedOp event will not be triggered).
In practice, this should be identified by the bundler when constructing the batch. Nevertheless, in the interest of predictability, consider ensuring _createSenderIfNeeded always ends with a wallet deployed at the expected address.

Apologies if this is not the right place to ask this question, but I had a question about the spec.

According to the spec, if initCode is included in UserOp and the sender address already has a contract deployed, the operation aborts.

I'm not sure if I understand the rationale for this design decision. In my opinion, it seems to be more user-friendly if the EntryPoint proceeds even if there's already a contract deployed. The reason is that this would simplify client code: the client can safely include initCode, without knowing if the contract has already been deployed. With the current spec, the client would have to query the blockchain to check if the sender has already been deployed, which places extra burden on the client.

If the concern is with efficiency (namely that calldata gets larger if initCode is always included), the client is still totally free to query the blockchain and NOT include initCode if the sender has already been deployed, just like today. Basically, it seems to me that allowing the EntryPoint to proceed improves client UX while still giving clients the option to optimize for efficiency (by not including initCode on every call) should they so choose.

The getTokenToEthOutputPrice function of the IOracle interface does not specify the particular token to translate. This means that each IOracle contract can only support one token, which seems like an unnecessary restriction. Consider including a tokenAddress parameter to the function specification.

To support log filtering for the EntryPointChanged event in the SimpleWallet contract, consider adding the indexed keyword to the address parameters in the event.

• The compensate function in EntryPoint takes a beneficiary address as input and sends the amount specified to that address. This happens as the final step of a handleOps or handleOp function call. The code does not check that beneficiary is not 0, which could lead to accidental loss of funds. Consider adding a check to verify that beneficiary is a non-zero value.
• In the EntryPoint constructor, there are no checks to ensure the immutable contract variables are set to non-zero values. If _create2factory, _paymasterStake, or _unstakeDelaySec were accidentally set to 0, the contract would need to be redeployed because there is no mechanism to update these values. Consider adding a non-zero check for each of the constructor parameters.

Client reported: The Ethereum Foundation identified this issue during the audit.

The gas price to charge the user (potentially through the paymaster) for the operation is calculated as the minimum of the transaction gas price and the user-specified gas price (after accounting for any basefee). However, the user should always pay their specified price so the bundler can receive the excess, which provides the incentive to process the user operation in the first place. Consider allowing the user's gas price to exceed the transaction gas price.

The StakeManager contract defines the DepositInfo struct with three values of size uint112, uint32 and uint64. While the intention to pack the contents into a single word can be inferred, the reason for the particular sizes are not obvious. Consider documenting the reason for this design pattern and the corresponding (reasonable) assumptions about the maximum sizes of each type.

In the EntryPoint contract, there are several functions (handleOp, handleOps, _validateWalletPrepayment, _validatePaymasterPrepayment, handlePostOp) where all or nearly all of the function body is enclosed within an unchecked block, which disables overflow and underflow safety checks. This leaves ambiguity about which specific operations within the function require unchecked math, requires additional effort to review, and is potentially dangerous because math operations that should be checked may be inadvertently added within the block.
For safety and clarity, consider restricting the scope of unchecked math blocks to only include the specific lines where it is needed.

The DepositPaymaster contract is non-compliant with the current version of the EIP, because it accesses an external oracle during validation. Consider including a warning at the top of the contract to explain the associated risks and how bundlers should decide whether to support this paymaster.

The majority of the code base supports solidity versions ^0.8.7, but the StakeManager contract supports versions ^0.8 and the ECDSA contract supports versions ^0.8.0.
Consider using consistent solidity versions throughout the code base. Additionally, in the interest of predictability, consider locking the contracts to a specific supported version.

The StakeManager contract downcasts the amount when incrementing or decrementing deposits, and when increasing an account's stake. Although these values are unlikely to overflow, as a matter of good practice, consider adding bounds checks.

Consider addressing the following typographical errors:
• In EntryPoint.sol:
• line 29 and line 30: "simulateOp" should be "simulateValidation" • line 178: "of" should be "if"
• line 297: "done, by" should be "done and"
• In IWallet.sol:
• line 10: "successfuly." should be "successfully."
• In StakeManager.sol:
• line 12: "blocks to" should be "seconds to wait"

• line 83: "time" should be "duration"
• In DepositPaymaster.sol:
• line 68: "on in the same block as withdrawTo()" should be "in the same block as withdrawTokensTo()"
• In SimpleWallet.sol:
• line 88: "(its" should be "(it's"
• In TokenPaymaster.sol:
• line 11: "os" should be "or"
• line 18: "method-ids." should be "method ids."
• line 77: "paymaster" should be "entrypoint"
• In VerifyingPaymaster.sol:
• line 57: "signing" should be "to signing"
• In eip-4337.md:
• line 46: "for to compensate the bundler for" should be "to compensate the bundler" • line 84: "worlflow" should be "workflow"
• line 89: "simulateWalletValidation" should be "simulateValidation"
• line 127: "paymaster" should be "paymaster)"
• line 151: "op" should be "op validation"
• line 165: "valiation" should be "validation" and the line should end in a period.

The StakeManager contract is intended to provide staking functionality to other contracts, but should not be deployed directly. To better signal this intention, consider declaring the contract as abstract.

The StakeManager contract allows paymasters to lock funds for a period of time and they are intentionally prevented from reducing the delay. However, after unstaking their funds and waiting for the withdrawal period, they should be able to stake again with any delay. This is possible if they withdraw their funds first (which clears the saved delay), but this is should not be a necessary requirement.
Consider updating the staking guard condition to allow the delay time to be reduced if the withdrawTime has been reached.

The StakeManager contract specifies a minimum unstake delay for paymasters to withdraw their stake, but this minimum is not enforced when staking.
The Ethereum Foundation has indicated that they intend to remove the minimum entirely, and instead allow it to be a floating parameter, negotiated between miners and paymasters. To this end, they will introduce two unused parameters to the handleOps function call, set by the miner, specifying their minimum acceptable stake and delay values. These are merely signals that won't be enforced by the contract. Note that this solution would also involve removing the EntryPoint contract's paymasterStake parameter and modifying the isPaymasterStaked function to accept a delay variable.
While we acknowledge and endorse this solution, we would still recommend ensuring the stake delay is non-zero, since this parameter is semantically overloaded as a flag to indicate if the paymaster is staked.

In the TokenPaymaster contract, it is recommended to declare the COST_OF_POST variable as constant. This change will eliminate the use of a storage slot for this value.
For clarity, also consider assigning a named constant to the values 16000 and 35000 that appears within the validatePaymasterUserOp functions.

Many functions in the code base lack documentation. This hinders reviewers' understanding of the code's intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts' public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Wallets pay for their operations using the funds deposited with the StakeManager, whether or not those funds are locked. This is usually acceptable because wallets are not required to lock their funds. However, if a paymaster contract is also a wallet, it will be able to spend funds that are supposed to be locked as stake. This means that it can bypass the reputation system by consuming its locked funds after being throttled. Consider ensuring that wallets cannot spend locked funds.

In the TokenPaymaster contract, the validatePaymasterUserOp function performs a check to verify that the sender's token balance is sufficient to pay the tokenPrefund cost for the user operation. In both cases where this check occurs in the function, > is used for the comparison when >= should also be valid. A user could have exactly the required prefund amount and still fail to pass validation.
Consider modifying the balance checks to support the case where the user's token balance exactly matches the tokenPrefund amount.

Several contracts in the samples directory don't explicitly declare the visibility of the state variables and constants:
In the DepositPaymaster contract: - nullOracle - unlockBlock
In the TokenPaymaster contract: - knownWallet
In the SimpleWallet contract: - ownerNonce
For clarity, consider always explicitly declaring the visibility of functions and variables, even when the default visibility type matches the intended type.

We identified the following examples of inconsistent naming:
• In EntryPoint.sol:

• most internal and private functions are prefixed with an underscore, while compensate and handlePostOp are not.
• the UserOpInfo struct has a single parameter that starts with an underscore.
• the _salt parameter of the getSenderAddress function starts with an underscore.
  Additionally, the prefix "internal" in function names may cause confusion. It seems redundant for functions declared with the internal keyword, such as the deposit manipulation functions and

misleading for the external internalHandleOp function. We believe the prefix is intended to be descriptive of the actual function behavior, but nevertheless would recommend a different prefix, perhaps "local", to avoid overloading Solidity keywords.
For clarity and readability, consider using a consistent naming convention.

There are several contracts where inline assembly code is undocumented. Using inline assembly is risky because it bypasses some of the compiler's safety checks, it's harder to read and audit, and more likely to contain errors. For these reasons, the recommended best practice is for every line of assembly to have a corresponding explanatory comment.

Consider adding documentation to the following lines of code:

• ECDSA.sol lines 188-202
• UserOperation.sol line 25
• UserOperation.sol lines 64-71

The samples directory contains a custom version of OpenZeppelin's ECDSA library, which replaces the existing ecrecover function with an alternate ecrecover2 assembly implementation that does not use the GAS opcode. After ecrecover2 has assigned the return value signer, it executes a few more instructions to zero out the first two words of the free memory space, overwriting the signer and v values. From the associated comment it can be inferred that memory space is being zeroed out for the benefit of future callers that might assume the memory has been cleared. However, the current implementation is not clearing all of the modified memory--it only zeroes out the signer and v values, and leaves r and s untouched.
Consider either clearing all of the memory used, or none of it. The Solidity documentation makes it clear that users should not expect the free memory to point to zeroed-out memory, so this final clearing operation is not necessary.

The TokenPaymaster contract imports the obsolete SimpleWalletForTokens contract as an indirect mechanism to import the SimpleWallet contract. Consider replacing the indirect import with a direct one.

Consider making the following changes to eliminate redundant code:

• In UserOperation.sol, the requiredGas function calculates mul by evaluating the expression
  userOp.paymaster != address(0), which is equivalent to userOp.hasPaymaster().
• In UserOperation.sol, the pack function has an unnecessary return statement.
• In DepositPaymaster.sol, the statement to silence an unused variable warning for mode can be removed. The mode variable is used to determine the payment method.

When perform through EntryPoint, the maximum gas limit is: op.callGas
link:EntryPoint.sol#L158

If a contract wallet has been deployed, callGas can be estimated simply by a similar method:
web3.eth.estimateGas({ from: entryPoint, to: op.sender, data: op.calldata });

However, if the contract wallet needs to perform some operations when it deployed for the first time, op.callGas cannot be estimated using current Entrypoint.

I think a similar function should be added to Entrypoint:
function estimateCallDataGas(UserOperation calldata op) public returns (uint256 callGas) { _createSenderIfNeeded(op); uint256 preGas = gasleft(); address(op.getSender()).call(op.callData); uint256 callGas = preGas - gasleft(); require(msg.sender == address(0), "must be called off-chain with from=zero-addr"); }

===================

I'm not sure if I don't know enough about the EntryPoint code or if do have the above problem, if it does, I'd be happy to submit a formal pr

On deployment, the TokenPaymaster assigns the maximum token allowance to its owner. However, there is no mechanism to update the allowance.
In principle, this means the allowance can run out. More plausibly, if ownership is transferred, the old owner will still be able to spend the tokens, and the new owner will not. Consider allowing the owner to refresh their token allowance. Additionally, consider removing the existing allowance when ownership is transferred.

Moreover, the paymaster mints a single token unit to ensure the contract balance and total supply is non-zero for more predictable gas accounting. However, the owner can withdraw the contract's total balance, restoring it to zero. Consider preventing the withdrawal of the last token unit.

To favor explicitness, consider declaring all instance of uint as uint256.

The DepositPaymaster ignores the token transfer return value when adding deposits, withdrawing tokens from the contract and recovering gas costs. Although many tokens revert on failure, the token standard only specifies a boolean return value indicating success or failure. For tokens that return false, such as the 0x Protocol Token, these transfers may fail silently, leading to incorrect internal accounting.

Consider checking the return value of all ERC20 transfers, or using OpenZeppelin's safe transfer functions.

To improve readability and avoid confusion, consider removing the following unused imports:

• In the TokenPaymaster contract, the hardhat console library.
• In the SimpleWallet contract, the hardhat console library.
• In the TestCounter contract, the UserOperation contract and the IWallet interface.
• In the TestOracle contract, the OpenZeppelin ERC20 contract. • In the TestUtil contract, the IWallet interface.