fix: [QS-01] revert on presence of validation hooks in deferred validation, and run validation-associated execution hooks #287
Conversation
Summary by OctaneNew ContractsNo new contracts were added in this PR. Updated Contracts
🔗 Commit Hash: 0810747 |
|
Contract sizes: | Contract | Runtime Size (B) | Initcode Size (B) | Runtime Margin (B) | Initcode Margin (B) |
|-------------------------------|------------------|-------------------|--------------------|---------------------|
| AccountFactory | 5,921 | 6,386 | 18,655 | 42,766 |
| AllowlistModule | 9,553 | 9,580 | 15,023 | 39,572 |
| ExecutionInstallDelegate | 5,714 | 5,760 | 18,862 | 43,392 |
| ModularAccount | 22,254 | 29,083 | 2,322 | 20,069 |
| NativeFunctionDelegate | 560 | 587 | 24,016 | 48,565 |
| NativeTokenLimitModule | 4,449 | 4,476 | 20,127 | 44,676 |
| PaymasterGuardModule | 1,845 | 1,872 | 22,731 | 47,280 |
| SemiModularAccount7702 | 23,157 | 29,979 | 1,419 | 19,173 |
| SemiModularAccountBytecode | 23,639 | 30,468 | 937 | 18,684 |
| SemiModularAccountStorageOnly | 24,133 | 30,962 | 443 | 18,190 |
| SingleSignerValidationModule | 3,646 | 3,673 | 20,930 | 45,479 |
| TimeRangeModule | 2,003 | 2,030 | 22,573 | 47,122 |
| WebAuthnValidationModule | 7,854 | 7,881 | 16,722 | 41,271 |Code coverage:
|
Overview
Detailed findings
|
src/account/ModularAccountBase.sol
Outdated
| @@ -413,12 +414,18 @@ abstract contract ModularAccountBase is | |||
| /// [(33 + deferredActionSigLength + encodedDataLength):] : bytes, userOpSignature. This is the | |||
| /// signature passed to the inner validation. | |||
| if (hasDeferredAction) { | |||
| // Because this bypasses UO validation hooks, we require that the validation used does not include any | |||
| // validation hooks. | |||
| if (!getAccountStorage().validationStorage[validationFunction].validationHooks.isEmpty()) { | |||
There was a problem hiding this comment.
We should document this during validation installation.
If you want to use a validation to approve deferred actions in the future, it should not have validation hooks.
There was a problem hiding this comment.
Where do you think this fits? As a comment in installValidation()? Or perhaps in the documentation for integrators/SDK?
There was a problem hiding this comment.
In the documentation notion doc for now to later transfer to the doc site is enough probably.
There was a problem hiding this comment.
Added a section in the documentation, feel free to give it a peek!
Zer0dot
force-pushed
the
zer0dot/fix-qs-01
branch
from
4f42d88 to
85a7a12
Compare
Zer0dot
changed the base branch from
develop
to
adam/fix-execute-user-op-with-def-action
Zer0dot
force-pushed
the
zer0dot/fix-qs-01
branch
from
85a7a12 to
a317085
Compare
adamegyed
force-pushed
the
adam/fix-execute-user-op-with-def-action
branch
2 times, most recently
from
67305ea to
3973328
Compare
Zer0dot
force-pushed
the
zer0dot/fix-qs-01
branch
from
a317085 to
ce8fe1c
Compare
adamegyed
force-pushed
the
adam/fix-execute-user-op-with-def-action
branch
from
3973328 to
6180cf4
Compare
Zer0dot
force-pushed
the
zer0dot/fix-qs-01
branch
from
4c7bda2 to
7b36322
Compare
src/account/ModularAccountBase.sol
Outdated
| ExecutionLib.callBubbleOnRevertTransient(address(this), 0, encodedData[63:]); | ||
|
|
||
| // Do the cached post hooks | ||
| ExecutionLib.doCachedPostHooks(postHookData); |
There was a problem hiding this comment.
Does it make sense to move this section into the _validateDeferredActionAndSetNonce function? It seems like the functionality is leaking out from the function, as it runs the pre-exec hooks there. Or otherwise reorganize to do the pre-exec hooks + self-call + post exec hooks together?
(Side note: this is a case where we can manually clear the memory after running with freezeFMP / restoreFMP, because this action isn't reused for the user op validation later.)
There was a problem hiding this comment.
Should be good to go now, it's all in a _handleDeferredAction() function!
src/account/ModularAccountBase.sol
Outdated
|
|
||
| // Clear the memory after performing signature validation | ||
| MemSnapshot memSnapshot = MemManagementLib.freezeFMP(); | ||
| if (_isValidSignature(defActionSigValidation.moduleEntity(), typedDataHash, sig) != _1271_MAGIC_VALUE) { | ||
| if (_isValidSignature(defActionValidationModuleEntity, typedDataHash, sig) != _1271_MAGIC_VALUE) { |
There was a problem hiding this comment.
Given that we assert there are no pre-validation hooks at all, we can also assert there are no pre-signature validation hooks, which means we will never have any per-hook data. Given that, I think we should be good to replace this call to _isValidSignature with _exec1271Validation, and omit the signature segment decoding. Wdyt?
src/account/ModularAccountBase.sol
Outdated
| @@ -711,6 +711,21 @@ abstract contract ModularAccountBase is | |||
| return ExecutionLib.invokeUserOpCallBuffer(callBuffer, userOpValidationFunction, signatureSegment); | |||
| } | |||
|
|
|||
| function _runValidationAssociatedExecHooks(ModuleEntity validationFunction, bytes calldata callData) | |||
There was a problem hiding this comment.
Oh, this may be a good place to wrap all of {pre exec hooks, the transient self-call, and the post-exec hooks} while wrapping with the memory deallocator.
There was a problem hiding this comment.
I went ahead and wrapped the entire call to _handleDeferredAction() which is a new function that basically does everything except the initial decoding in _validateUserOp(). It looks good to me, but let me know if there's any memory-related issue you spot.
src/account/ModularAccountBase.sol
Outdated
| @@ -413,13 +414,12 @@ abstract contract ModularAccountBase is | |||
| /// [(33 + deferredActionSigLength + encodedDataLength):] : bytes, userOpSignature. This is the | |||
| /// signature passed to the inner validation. | |||
| if (hasDeferredAction) { | |||
| // Use outer validation to validate the UO, and the inner validation as 1271 validation for the | |||
| // deferred action. | |||
| // Use outer validation as a 1271 validation, then use the inner validation to validate the UO. | |||
There was a problem hiding this comment.
This comment shouldn't be switched, right?
| // Check that the passed nonce isn't already invalidated. | ||
| if (getAccountStorage().deferredActionNonceUsed[nonce]) { | ||
| revert DeferredActionNonceInvalid(); | ||
| } |
There was a problem hiding this comment.
This is correct to do in the workflow, but it isn't described by the function name - can we refactor this out to the parent function, or update the function naming?
There was a problem hiding this comment.
Function name changed, it now handles the entire signature flow
src/account/ModularAccountBase.sol
Outdated
|
|
||
| // Because this bypasses UO validation hooks, we require that the validation used does not include any | ||
| // validation hooks. | ||
| if (!getAccountStorage().validationStorage[defActionValidationModuleEntity].validationHooks.isEmpty()) { |
There was a problem hiding this comment.
We should use validationStorage[defActionValidationModuleEntity].validationHookCount != 0, to prevent the cold sload here.
Motivation
Currently, it's possible to skip userOp validation hooks if the validation function being used is also a signature validation through deferred installation. Furthermore, we're not running userOp validation-associated execution hooks, when we could be.
Solution
Revert if the validation being used includes validation hooks, and run the validation-associated pre and post hooks tied to that validation.