jayden-sudo
commented
1 year ago Maybe it would be better to use a custom structure (userOP) instead of a struct in solidity (solves the above problem while having a better cost advantage on L2) ?
Closed Gooong closed 1 year ago
jayden-sudo
commented
1 year ago Maybe it would be better to use a custom structure (userOP) instead of a struct in solidity (solves the above problem while having a better cost advantage on L2) ?
leekt
commented
1 year ago This looks like a valid issue, which can also be used to attack the sender contract eg. signing userOp with signature.offset < calldata.offset will allow hacker to call arbitrary calldata to SimpleAccount, which can also be attacked with frontrun the userOp if offset is not set correctly
Which also extends to issue addressed in VerifyingPaymaster https://github.com/eth-infinitism/account-abstraction/pull/233#issuecomment-1459155222
We need to fix this
drortirosh
commented
1 year ago Yes, it is a valid issue, that a manually created structures can create different and even non-unique UserOp hash. However:
Gooong
commented
1 year ago I agree that the security risk is limited. For now it causes OpHash mismatching in Event, which confuses off-chain actors.
We need to fix by either fixing pack function or, like @zhangshengjie said, using custom OP structure. Maybe need to mark the current version contract as not recommended for production.
adam-alchemy
commented
1 year ago I believe there is reason for this issue to be escalated.
Malicious bundlers, or non-bundler EOAs calling EntryPoint.handleOps, can modify their representation of a user operation in calldata to change the user op hash in emitted events. This can break offchain systems integrating with the emitted events, since the events are now revealed to be non-deterministic for a given user operation.
Additionally, the bundler will have to deal with non-determinism when reading emitted userOpHashes from the EntryPoint contract. To see if an emitted UserOperationEvent from the EntryPoint corresponds to a user operation in the bundler’s local mempool, a comparison of the hash value is no longer enough, as the calldata to handleOps can be modified to change hash values. Instead, bundlers will have to look up transaction receipts, fetch the calldata sent to handleOps, decode the calldata, then get the “canonical” hashes by re-encoding via a the standard ABI coder and calling EntryPoint.getUserOpHash(...). This is needed to determine whether or not user operations in the local mempool have been mined. Additionally, since calls to EntryPoint.handleOps can happen from within other contract calls, the decoding can be deep in the call stack.
This divergence will also affect the implementation of bundler RPC methods, as a user op hash is used for identification in eth_getUserOperationByHash and eth_getUserOperationReceipt. Bundlers will need to perform expensive searches, parsing, and decoding of calldata to EntryPoint.handleOps(...) to translate the emitted hashes from events into “canonical” hashes from EntryPoint.getUserOpHash(...).
Note that this vulnerability is distinct from the fact that rogue SCWs can reuse user operations. Reused user operations, and more generally, all user operations, should have a deterministic hash. Other applications and services that build on top of ERC-4337 will have to implement their own mitigation unless this is resolved.
GitHub repo with demonstration: https://github.com/alchemyplatform/entrypoint-hash-poc.
This risk introduced to EntryPoint are that a single user operation can be representing by multiple “user op hashes” and that the same “user op hash” can represent multiple user operations.
Consider this account, called ExampleAccount, that has it’s own ExampleAccountFactory. Example account uses a single signer to validate user operations. To grant permission to run a user operation, a hash over all fields in the user operation, except the signature itself, is generated and signed. The validateUserOp method is defined as follows:
_requireFromEntryPoint();
bytes32 hash = keccak256(
abi.encode(
userOp.sender,
userOp.nonce,
userOp.initCode,
userOp.callData,
userOp.callGasLimit,
userOp.verificationGasLimit,
userOp.preVerificationGas,
userOp.maxFeePerGas,
userOp.maxPriorityFeePerGas,
userOp.paymasterAndData,
address(_entryPoint),
block.chainid
)
).toEthSignedMessageHash();
if (owner != hash.recover(userOp.signature)) {
return SIG_VALIDATION_FAILED;
}
_validateAndUpdateNonce(userOp);
_payPrefund(missingAccountFunds);
return 0;This is a relatively standard implementation of signature validation. As one of the goals of account abstraction, the validateUserOp method can contain arbitrary logic (though bounded by limitations to storage access), since this method represents the conditions under which a user operation can originate. For this example account, user operations start from a signature by the owner. More generally, however, user operations can originate from arbitrary conditions: on-chain state, multiple signatures, or app-specific signatures - it’s a feature of account abstraction.
To demonstrate this vulnerability, let’s construct malicious calldata to EntryPoint.handleOps such that the UserOperationEvent emitted by EntryPoint will have an unexpected value. After defining a a sample UserOperation memory uo struct, here is how we can construct the calldata:
bytes memory callData = abi.encodePacked(
entryPoint.handleOps.selector,
uint256(0x40), // Offset of ops
uint256(uint160(account)), // beneficiary
uint256(1), // Len of ops
uint256(0x20), // offset of ops[0]
uint256(uint160(uo.sender)),
uo.nonce,
uint256(0x240), // offset of uo.initCode (encoding assumes a 65-byte long signature, which it is using the provided address.)
uint256(0x180), // offset of uo.callData
uo.callGasLimit,
uo.verificationGasLimit,
uo.preVerificationGas,
uo.maxFeePerGas,
uo.maxPriorityFeePerGas,
uint256(0x160), // offset of uo.paymasterAndData
uint256(0x1c0), // offset of uo.signature
uint256(uo.paymasterAndData.length),
rightPadBytes(uo.paymasterAndData),
uint256(uo.callData.length),
rightPadBytes(uo.callData),
uint256(uo.signature.length),
rightPadBytes(uo.signature),
uint256(uo.initCode.length),
rightPadBytes(uo.initCode)
);rightPadBytes is a helper function written to align bytes types to the nearest full word length. It is defined as follows:
function rightPadBytes(bytes memory input) internal pure returns (bytes memory) {
bytes memory zeroPadding = "";
uint256 zeros = 32 - (input.length % 32);
if (zeros != 32) {
for (uint256 i = 0; i < zeros; ++i) {
zeroPadding = bytes.concat(zeroPadding, hex"00");
}
}
return bytes.concat(input, zeroPadding);
}Now, when calling handleOps, the emitted event and the result of EntryPoint.getUserOpHash() will be different.
address(entryPoint).call(callData);
// Emitted event: UserOperationEvent(userOpHash:
// 0xfd3ec4e5d5568b2b0ba2d87f1445217558d870c0dfa9a48b27f9c31dcda5ae16, ...)
entryPoint.getUserOpHash(uo);
// Returns 0xf8d19bb1ac52479d3bc953d8768a2eef2ce0051d55d5bafb34bcee8ed9c4da09This is because the assembly block in UserOperationLib's pack(...) directly uses the value of userOp.signature.offset to determine where to copy calldata from, rather than following the offsets and lengths specified by the dynamic fields uo.initCode, uo.callData, and uo.paymasterAndData to copy their lengths and values.
function pack(UserOperation calldata userOp) internal pure returns (bytes memory ret) {
//lighter signature scheme. must match UserOp.ts#packUserOp
bytes calldata sig = userOp.signature;
// copy directly the userOp from calldata up to (but not including) the signature.
// this encoding depends on the ABI encoding of calldata, but is much lighter to copy
// than referencing each field separately.
assembly {
let ofs := userOp
let len := sub(sub(sig.offset, ofs), 32)
ret := mload(0x40)
mstore(0x40, add(ret, add(len, 32)))
mstore(ret, len)
calldatacopy(add(ret, 32), ofs, len)
}
}Redeploy EntryPoint with an updated pack method in UserOperationLib:
function pack(UserOperation calldata userOp) internal pure returns (bytes memory) {
return abi.encode(
userOp.sender,
userOp.nonce,
userOp.initCode,
userOp.callData,
userOp.callGasLimit,
userOp.verificationGasLimit,
userOp.preVerificationGas,
userOp.maxFeePerGas,
userOp.maxPriorityFeePerGas,
userOp.paymasterAndData,
);
}This will ensure a 1:1 mapping between how the user operation is used by EntryPoint (i.e. what arguments are sent to the SCW) and how the user operation is used to generate the user op hash. It will be more gas expensive, though additional savings can come from implementing the packing scheme in assembly if desired, weighing the cost of maintainability.
A change to EntryPoint requires a redeployment. Given the severity of impact from this vulnerability on implementing bundlers and other offchain services interfacing with ERC 4337, I recommend addressing the issue sooner rather than later.
drortirosh
commented
1 year ago The validateUserOp is given a userOpHash to the validateUserOp, which is a hash over all fields - including all offsets to dynamic parameters, so that a bundler is prevented from performing any field-reordering as you describe.
As long a a wallet signs this hash, it is protected from a malicious bundler.
You are right that a wallet written using the pack method you describe IS vulnerable - and also wastes gas to re-create the hash)
The only one who can create such reordering is the wallet itself (who signs the userop) - and it requires a custom bundler to send this re-ordered userop on-chain. But then again - there is no attack here, just a convoluted protocol between the userop and this custom bundler.
The hash must contain the sender address, and thus one sender cannot possibly cause it to be calculated as a userOpHash of any other sender.
adam-alchemy
commented
1 year ago Agree with everything you stated. It is not an attack on any accounts or smart contracts, instead a way for malicious bundlers/other EOAs to make it difficult for regular bundlers to provide rpc services if other 4337 accounts make use of custom logic in the validation step.
To address your last point, the issue I'm trying to raise is not that a single user operation's hash can be made to appear like a different user operation, it's that a single user operation can have multiple possible hashes emitted in UserOperationEvent.
The hash provided by the EntryPoint into validateUserOp is a useful convenience, but in it's current form, expects all implementers of IAccount to perform a similar signature validation scheme to the current SimpleAccount example. As 4337 grows, I expect this to not be the case for an increasing number of wallet accounts. Alternative validation schemes, or non-signature-origin user operations, will be susceptible to hash divergence. Consider the Visa pull-payments example: if adapted from starknet to ethereum via 4337, the condition for a user operation to start is merely time passing since the last payment. A 4337 account can adopt this through a case in validateUserOp that performs different checks based on what method selector was set in userOp.callData, and return a valid sigTimeRange based on associated storage, without requiring anything in userOp.signature. This would avoid using the value of userOpHash given from the EntryPoint into validateUserOp.
The user operation generated in this example could then be intercepted and calldata-manipulated by other bundlers to alter the hash, leading to offchain applications that would otherwise listen for emitted events to miss it, and difficulty for bundlers to detect block inclusion based on the "canonical" hash from EntryPoint.
To generalize, I don't think this is an issue with the behaviors of existing accounts - I think this can cause implementation issues for bundlers and other offchain serves in the future. If the 4337 standard expects signature-only origins for user operations and validation exclusively in the pattern of using the hash from EntryPoint, then nothing needs to change. If 4337-compliant account abstraction wants to also support custom stateful validation logic, then I recommend a change to allow for deterministic user op hashes and simpler explorer, bundler, and listener services.
livingrockrises
commented
1 year ago some cases we may not rely on validating userOpHash passed on from EntryPoint, use cases like social recovery module or upgrading simple account to multisig via module would not be possible otherwise.
function _validateSignature(
UserOperation calldata userOp,
bytes32 userOpHash
) internal virtual override returns (uint256 validationData) {
bytes calldata userOpData = userOp.callData;
if (userOpData.length > 0) {
bytes4 methodSig = bytes4(userOpData[:4]);
// If method to be called is executeCall then only check for module transaction
if (methodSig == this.executeCall.selector) {
(address _to, uint _amount, bytes memory _data) = abi.decode(
userOpData[4:],
(address, uint, bytes)
);
if (address(modules[_to]) != address(0)) return 0;
}
}
bytes32 hash = userOpHash.toEthSignedMessageHash();
if (owner != hash.recover(userOp.signature))
return SIG_VALIDATION_FAILED;
return 0;
}For accounts what would help is checking userOp against userOpHash
drortirosh
commented
1 year ago Not checking the userOpHash is dangerous: you expose your account to replay attacks, or gas-attacks (where someone passes huge values for gaslimit/gasprice) to drain your balance.
Even if it validates nothing else, think of validateUserOp at a minimum as "do I agree to pay for this transaction"? even if your recoverUsingSocial() method performs some other checks, at a minimum check you accept this sender to avoid draining by anonymous users.
noam-alchemy
commented
1 year ago Hey @drortirosh - totally hear where your coming from. My concerns here can be summarized succinctly by Hyrum's Law - "With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody."
From our experience running some of these services at scale, these issues can and will arise overtime and to the above points, can unnecessarily complicate certain operations required to support this paradigm. We're really excited about the unlocks this will provide, and want to make sure that it scales with invariants (e.g. validation schemes may vary) to provide a robust foundation for dApps, data indexers, dashboards, explorers, consumers, and whoever else in the future.
Do you think a reasonable next step here is perhaps a community call to poll a few more opinions from the ecosystem? I know a few other folks had opinions on this topic and conversations have scattered between this issue and a few different Telegram chats, so it might be worthwhile to consolidate all perspectives and data points.
We're also very much aware of the overhead of re-auditing and deploying some contracts and are happy to help operationalize any actions that need to be taken here. We appreciate you driving things this far this quickly and want to support you and your team as things continue to grow.
hazim-j
commented
1 year ago @adam-alchemy @noam-alchemy I've been following this conversation over the different channels. Correct me if I'm wrong, but this vulnerability becomes an issue for account implementations that don't validate userOpHash. In which case we can run into the scenario of a bad bundler re-ordering the op so that the userOpHash emitted in UserOperationEvent is different then what was originally returned by the bundler. This then breaks any off-chain component that relies on this event emitting the same hash for indexing purposes. Is this a correct high level take?
Assuming that's the case, are there any valid scenarios where a userOpHash can safely be ignored in validateUserOp without running into replay and gas attacks too?
leekt
commented
1 year ago Based on this behavior, i was able to make a wallet that is signature replay-proof and generates same "userOpHash" for every userOp by setting userOp.signature.offset == 0x20 (nonce slot) and set userOp.nonce large enough
it is not gas-proof yet but i can make it gas-proof
https://github.com/leekt/foundry-4337/compare/PoC/sameUserOpHash
edit) i made it gas-proof
edit) i have made this tx that has different nonce userOp to be resulting in same userOpHash https://goerli.etherscan.io/tx/0xb9a6d1a4c2fd4092f75761c7eee197195a72ab7d2c4fbcd91fb92ed632a976fc#eventlog
livingrockrises
commented
1 year ago Not checking the userOpHash is dangerous: you expose your account to replay attacks, or gas-attacks (where someone passes huge values for gaslimit/gasprice) to drain your balance. Even if it validates nothing else, think of
validateUserOpat a minimum as "do I agree to pay for this transaction"? even if yourrecoverUsingSocial()method performs some other checks, at a minimum check you accept this sender to avoid draining by anonymous users.
That means we need to be doing signature validation for all the ops. Our logic of skipping signature check is based on enabled 'module' as for example social recovery module recoverUsingSocial() performs its own checks.
i) when recovering there won't be original owner key to sign userop even if it's kept at a minimum. ii) same applies for session key module where use case is every op is not meant to be signed by account owner.
Should we pass on userOpHash to module as part of recoverUsingSocial (along with performing it's own checks) it won't still serve the session key use case unless we ignore userOpHash in validateUserOp. essentially the question @hazim-j asked
drortirosh
commented
1 year ago Our logic of skipping signature check is based on enabled 'module' as for example social recovery module recoverUsingSocial()
I don't understand: anonymous users can execute this recover module, at any gas price, at any time? This looks to me like a way to deplete the account. I believe that at a minimum, there should be some criteria to allow only specific entities (guardians?) to make a call at the expense of the account. This way, you are not exposed to griefing attempt by anonymous users, only your guardians can. The recovery module itself can do some other checks whether the recovery is possible at this time or should be ignored - but the first line of defense is prevent anonymous griefing.
livingrockrises
commented
1 year ago Our logic of skipping signature check is based on enabled 'module' as for example social recovery module recoverUsingSocial()
I don't understand: anonymous users can execute this recover module, at any gas price, at any time? This looks to me like a way to deplete the account. I believe that at a minimum, there should be some criteria to allow only specific entities (guardians?) to make a call at the expense of the account. This way, you are not exposed to griefing attempt by anonymous users, only your guardians can. The recovery module itself can do some other checks whether the recovery is possible at this time or should be ignored - but the first line of defense is prevent anonymous griefing.
not anonymous users, only guardian (rather guardian signature) can invoke this module. [ EP -> Account -> Module -> Account -> recovery.. ] guardian signature is indeed validated but when calls comes from EP -> Account which has userOp with target as module (decoded from calldata of execute) then signature check within validateUserOp is skipped.
If the userOpHash still must be signed, should we make recover module verify signature of guardian signed over userOp?
adam-alchemy
commented
1 year ago @hazim-j that is mostly correct, but it's actually slightly larger in scope. There are three conditions under which there can be an unexpected user op hash divergence:
A wallet signs non-standard data. Arguably this is the fault of the wallet, but as shown by @Gooong, we can't rely on all wallets to perform exactly the same format of validation, especially not if they are still able to maintain security properties while following a non-standard encoding. In the linked example, the only "fault" in the encoding is that the wallet signed for an encoding of the user operation with an extra zero word. This does not change the contents of the ABI-encoded struct, only the serialized representation of it. All safety properties of the wallet are preserved. I've annotated the calldata to show exactly where it diverges here.
The wallet signs over all fields of the user operation, but does not validate the encoding format of the user operation. This is shown in my original example, and thank you @leekt for the additional example. It shows another case where a wallet can behave "safely" while interfering with the user op hashes generated by EntryPoint. With the new example, they not only have an unexpected user op hash, but have the exact same hash for all user operations from that wallet.
The validation scheme does not use the userOpHash provided by the EntryPoint in its verification scheme. Once such example is recurring payments, these don't need signatures since they only depend on on-chain contract state. Here's an example implementation (gas-unoptimized):
This example is intended to demonstrate a valid use case for an ERC-4337 account that doesn't always verify a signature over the user op hash provided by
EntryPointDefine the following struct and storage mapping in the account:struct RecurringPaymentInfo { uint256 frequency; address token; uint256 amount; uint256 callGasLimit; uint256 verificationGasLimit; uint256 preVerificationGas; uint256 maxFeePerGas; uint256 maxPriorityFeePerGas; uint256 lastPaidTimestamp; } mapping(address => RecurringPaymentInfo) recurringPaymentRecipients;There is some initialization function that sets these fields for the recurring payment.
This method is also defined on the account:
function sendRecurringPayment(address recipient, address token, uint256 amount) external { _requireFromEntryPoint(); recurringPaymentRecipients[recipient].lastPaidTimestamp = block.timestamp; IERC20(token).transfer(recipient, amount); }The validation method is defined as:
function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds) external override virtual returns (uint256 ret) { _requireFromEntryPoint(); if (bytes4(userOp.callData[:4]) == this.sendRecurringPayment.selector) { (address recipient, address token, uint256 amount) = abi.decode(userOp.callData[4:], (address, address, uint256)); RecurringPaymentInfo storage info = recurringPaymentRecipients[recipient]; require(token == info.token && amount == info.amount && userOp.callGasLimit == info.callGasLimit && userOp.verificationGasLimit == info.verificationGasLimit && userOp.preVerificationGas == info.preVerificationGas && userOp.maxFeePerGas == info.maxFeePerGas && userOp.maxPriorityFeePerGas == info.maxPriorityFeePerGas && userOp.initCode.length == 0 && userOp.callData.length == 0x64 && // Only encodes this call and nothing else userOp.paymasterAndData.length == 0 && userOp.signature.length == 0, "Recurring payment: invalid input data"); ret = _packValidationData(false, uint48(0), uint48(info.lastPaidTimestamp) + uint48(info.frequency)); } else { bytes32 hash = userOpHash.toEthSignedMessageHash(); if (_owner != hash.recover(userOp.signature)) { return SIG_VALIDATION_FAILED; } ret = 0; } _validateAndUpdateNonce(userOp); _payPrefund(missingAccountFunds); }Optionally, the gas values may be validated in a specific paymaster instead, or have some other programmatic validation. This is a simplified example implementation, where the account supports exactly two validation schemes.
This is intended just as an example of what can be done with account abstraction using user operations that are based on validity conditions other than a signature. The "associated storage" access rules during the simulation step still allow for a large amount of flexibility, and lots of programmatic functionality can be unlocked.
In case 1 and 2, the wallet is responsible for the non-standard hash. In case 3, either the wallet or a malicious bundler can generate the non-standard encoding, which results in a non-standard hash.
The intention behind changing the pack method is to remove the encoding format from the hash input. Currently, the user op hash is logically a hash over both the user op's contents + the encoding format, and issues can be resolved for 4337-integrated services by changing the hash to only be over the user op contents. hash(userOp, encoding format) => hash(userOp).
If gas savings are the concern, the pack method can also be implemented in assembly such that it only packs the contents of the user operation struct, without its encoding. For instance, copying the length and contents of userOp.initCode can be done as follows:
bytes calldata initCode = userOp.initCode;let writePtr := add(ret, x) // set the current write pointer to the correct location in memory for the returned byte array
calldatacopy(writePtr, sub(initCode.offset, 32), add(initCode.length, 32))Omitted free memory pointer modification for brevity.
While case 1 may be the fault of the wallet, the design of EntryPoint should not depend on particular wallet behavior. As @noam-alchemy mentioned, Hyrum's Law applies: non-spec observable behaviors (user op hash differences) will eventually be depended upon programmatically as the number of users grow. Addressing it now will make it easier to build future services on top of 4337.
livingrockrises
commented
1 year ago _validateAndUpdateNonce
in your example, _validateAndUpdateNonce also should be modified with the same logic?
adam-alchemy
commented
1 year ago in your example, _validateAndUpdateNonce also should be modified with the same logic?
What _validateAndUpdateNonce(...) does is check that the user operation's nonce is correct and increment the nonce in contract storage, so it does not need to be changed. It will be called by both branching cases of the validation logic.
noam-alchemy
commented
1 year ago Hey @drortirosh - looks like @leekt put together another illustrative example of potential issues that might arise. Phrased one way, the implementation of the wallet impacts the behavior of the protocol whereas we probably want that behavior abstracted away from the wallet implementation. Any new thoughts here? Any way we can help? Maybe we can include this update with a few others being discussed (e.g. re-entrancy guards on the Entrypoint, and some proposals to nonces).
filmakarov
commented
1 year ago Any new thoughts here?
Hey @noam-alchemy , idk if you've seen this proposal that can help addressing the problem of inconsistent userOpHashes https://docs.google.com/document/d/1MywdH_TCkyEjD3QusLZ_kUZg4ZEI00qp97mBze9JI4k/edit#
Hi,
The
packfunction directly use calldata to calculate the OpHash. However, this may result in incorrect userOp hash.https://github.com/eth-infinitism/account-abstraction/blob/556f03fadcaba0d7d04cd901e6eb9601db50a998/contracts/interfaces/UserOperation.sol#L61-L75
The calldata is the raw data of transaction. If the transaction sender encode some extra property in UserOperation struct, the calldata will be corrupted. Below is an example of encoding extra property that is still valid:
Actually, the incorrect hash already exists on-chain:
https://goerli.etherscan.io/tx/0xbee50381e09b1fbf21667255ae0c70f02aa5fe8e8167fe0a5253f7dd9a61d138
In the calldata, UserOperation is encoded in a different way. The OpHash emitted in the event is: 0x019C01CFD2BE5898EA5A4944E7A6FE0E6286179D734F0DDDCBDF85C01EA77450 If we manually trigger
getUserOpHashfunction with Op data, we get OpHash as: 0xf276978a4dd7397e19a8f4b6c686a46ddb2ef605de3604bc8af129f463262bd4Below is the screenshot and data: