AA-527 handleOps should process validateUserOp returning wrong data size #533
Conversation
wrap validateUserOp with assembly call. This way, we don't rely on EntryPointSimulation to create FailedOp on sender failures with malformed output (e.g. account not deployed)
optimized callValidateUserOp (less parameters), but actually costs more (16 per useop, but still...)
contracts/core/EntryPoint.sol
Outdated
| assembly ("memory-safe"){ | ||
| let success := call(gasLimit, sender, 0, add(callData, 0x20), mload(callData), 0, 32) | ||
| dataSize := mul(returndatasize(), success) | ||
| validationData := mload(0) | ||
| mstore(0x40, saveFreePtr) | ||
| } | ||
| if (dataSize != 32) { |
There was a problem hiding this comment.
| assembly ("memory-safe"){ | |
| let success := call(gasLimit, sender, 0, add(callData, 0x20), mload(callData), 0, 32) | |
| dataSize := mul(returndatasize(), success) | |
| validationData := mload(0) | |
| mstore(0x40, saveFreePtr) | |
| } | |
| if (dataSize != 32) { | |
| bool success; | |
| assembly ("memory-safe"){ | |
| success := call(gasLimit, sender, 0, add(callData, 0x20), mload(callData), 0, 32) | |
| dataSize := returndatasize() | |
| validationData := mload(0) | |
| mstore(0x40, saveFreePtr) | |
| } | |
| if (!successs || dataSize != 32) { |
There was a problem hiding this comment.
Maybe I don't understand the idea behind the mul(returndatasize(), success) but it seems like you could just check for success, which is a lot more reasonable?
There was a problem hiding this comment.
the logic is "ignore datasize in case of revert". I don't think that double-negative is more readable than multiplying "success" code by datasize. the both have the same effect. I also mload the returned data optimistically, and ignore it if datasize is not 32
contracts/core/EntryPoint.sol
Outdated
| @@ -227,7 +227,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT | |||
| //address(1) is special marker of "signature error" | |||
| require( | |||
| address(aggregator) != address(1), | |||
| "AA96 invalid aggregator" | |||
| FailedOp(totalOps, "AA96 invalid aggregator") | |||
There was a problem hiding this comment.
Is totalOps the same as opIndex in this case? If it is not, any tool build around FailedOp will blame the wrong UserOp for this revert.
contracts/core/EntryPoint.sol
Outdated
| assembly ("memory-safe") { | ||
| saveFreePtr := mload(0x40) | ||
| } |
There was a problem hiding this comment.
I looked into it, and there is probably no way to have a clean generic function that wraps this 'save-restore free memory pointer' trick.
Maybe for simplicity we could create inline save() and restore() functions and reuse those instead?
These assembly blocks kind of pollute the code.
drortirosh
changed the title
use if success { ... } instead of mul(success,....)
also, for EntryPointSimulations:
- had to reduce code size (so cleared simulation-unrelated
"supportsInterface")
- fixed maxgap
drortirosh
force-pushed
the
callValidateUserOp-catch-revert
branch
from
1a09a5f to
ad58580
Compare
| if(sender.code.length == 0) { | ||
| revert FailedOp(opIndex, "AA20 account not deployed"); | ||
| } else { | ||
| revert FailedOpWithRevert(opIndex, "AA23 reverted", Exec.getReturnData(REVERT_REASON_MAX_LEN)); |
There was a problem hiding this comment.
Thinking about it a little more.
I don't know if that's important, but technically it is not checked here if the call reverted or not, only that the return data is not of the correct length. However, if the call did revert, you don't even load it's return data so the emitter even will always be empty, right?
Does it matter? I think it may be useful to see the revert message if there is a way to do it, especially if the UserOp got on-chain and reverted. It could be two different events - FailedOpWithRevert and FailedOpWithBadValidationData. What do you think?
Or at least, maybe change the string to AA23 wrong return or something like that?
There was a problem hiding this comment.
I check only the size==32, but the assembly code above makes any differrent return value size (either revert or return data) to be size=0
so checking "size==32" actually means "didn't revert, AND size is 32"
thus no-account causes a size=0 with no revert, but handled by this exception case.
also, assembly { return (0,0) } (or any other assembly-level return with size!=32) would be treated as revert.
There was a problem hiding this comment.
I understand what you did, I am asking if that's actually the right thing to do here.
There was a problem hiding this comment.
what do you mean? success is defined as "call didn't revert AND datasize is 32"
since I have to use assembly, I prefer to do the AND also in assembly, and expose only the result to solidity.
I could reverse the terms: have "bool success", and clear it if datasize!=32... it has the same impact, only that the solidity code ends up doing if (success) { ... }
There was a problem hiding this comment.
There are two distinct failure cases:
- The call reverted
- The call retuned value of wrong size
I know there is no practical difference to us. But it may be beneficial for wallet developers to be able to tell the difference, maybe?
There was a problem hiding this comment.
The only way to return the wrong data size is by NOT inheriting the IAccount interface, and implement it differently (or, by using assembly return)
I think neither modes are something we want to explicitly support.
(btw: if solidity "try/catch" worked correctly, then all these cases would be considered "revert"...)
drortirosh
force-pushed
the
callValidateUserOp-catch-revert
branch
from
24ee87d to
683b2c8
Compare
HandleOps should properly revert with FailedOp if validateUserOp returns wrong data size.
the try/catch is unable to catch wrong return value size, which causes entire handleOps to revert with empty reason
Use _callValidateUserOp instead of sender.validateUserOp
Wrap validateUserOp with assembly call.
Now we can catch FailedOp on broken (or missing) sender.