From dcac37f481456aee5f237e9c0b14718f1dafe860 Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 4 Nov 2024 15:15:37 -0500 Subject: [PATCH] fix: validation applicability check in deferred action --- gas-snapshots/ModularAccount.json | 2 +- gas-snapshots/SemiModularAccount.json | 2 +- src/account/ModularAccountBase.sol | 4 +- test/account/DeferredAction.t.sol | 117 +++++++++++++++++++++++++- 4 files changed, 119 insertions(+), 6 deletions(-) diff --git a/gas-snapshots/ModularAccount.json b/gas-snapshots/ModularAccount.json index 5482e505..f12df802 100644 --- a/gas-snapshots/ModularAccount.json +++ b/gas-snapshots/ModularAccount.json @@ -12,5 +12,5 @@ "UserOp_NativeTransfer": "161531", "UserOp_UseSessionKey_Case1_Counter": "195236", "UserOp_UseSessionKey_Case1_Token": "226220", - "UserOp_deferredValidation": "257078" + "UserOp_deferredValidation": "257289" } \ No newline at end of file diff --git a/gas-snapshots/SemiModularAccount.json b/gas-snapshots/SemiModularAccount.json index b56ba52d..bbe70d94 100644 --- a/gas-snapshots/SemiModularAccount.json +++ b/gas-snapshots/SemiModularAccount.json @@ -12,5 +12,5 @@ "UserOp_NativeTransfer": "156773", "UserOp_UseSessionKey_Case1_Counter": "195464", "UserOp_UseSessionKey_Case1_Token": "226460", - "UserOp_deferredValidation": "253524" + "UserOp_deferredValidation": "253735" } \ No newline at end of file diff --git a/src/account/ModularAccountBase.sol b/src/account/ModularAccountBase.sol index 47246a06..7cebd2ad 100644 --- a/src/account/ModularAccountBase.sol +++ b/src/account/ModularAccountBase.sol @@ -491,8 +491,8 @@ abstract contract ModularAccountBase is ValidationConfig uoValidation = ValidationConfig.wrap(bytes25(encodedData[38:63])); // Check if the outer validation applies to the function call - _checkIfValidationAppliesSelector( - bytes4(encodedData[63:67]), + _checkIfValidationAppliesCallData( + encodedData[63:], sigValidation, isGlobalValidation ? ValidationCheckingType.GLOBAL : ValidationCheckingType.SELECTOR ); diff --git a/test/account/DeferredAction.t.sol b/test/account/DeferredAction.t.sol index 40fb2b51..215f4a76 100644 --- a/test/account/DeferredAction.t.sol +++ b/test/account/DeferredAction.t.sol @@ -21,7 +21,7 @@ import { ExecutionManifest, ManifestExecutionFunction } from "@erc6900/reference-implementation/interfaces/IExecutionModule.sol"; -import {IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; +import {Call, IModularAccount} from "@erc6900/reference-implementation/interfaces/IModularAccount.sol"; import {ValidationConfigLib} from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; @@ -125,7 +125,7 @@ contract DeferredActionTest is AccountTestBase { // Install a new execution function that the validation does not have the privilege to call. // Assert that user op validation reverts if the deferred action tries to call it. - function test_deferredAction_noPrivilegeEscalation() public { + function test_deferredAction_validationApplicabilityCheck() public { bytes4 newFunctionSelector = bytes4(0xabcdabcd); ExecutionManifest memory m; @@ -186,4 +186,117 @@ contract DeferredActionTest is AccountTestBase { ); entryPoint.handleOps(userOps, beneficiary); } + + function test_deferredAction_privilegeEscalationPrevented_executeSingle() public { + // Should not be allowed to call `execute` on the account itself + // Attempt to call it via a deferred action in a user op + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + callData: abi.encodeCall(IModularAccount.execute, (address(0), 0 wei, "")), + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); + bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); + + uint256 deferredInstallNonce = 0; + uint48 deferredInstallDeadline = 0; + + bytes memory deferredAction = abi.encodeCall( + IModularAccount.execute, (address(account1), 0 wei, abi.encodeCall(account1.accountId, ())) + ); + + userOp.signature = _buildFullDeferredInstallSig( + deferredInstallNonce, + deferredInstallDeadline, + deferredAction, + // Use the same validation for the deferred action and the user op + ValidationConfigLib.pack(_signerValidation, true, false, false), + account1, + owner1Key, + uoSig + ); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + vm.prank(beneficiary); + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(ModularAccountBase.SelfCallRecursionDepthExceeded.selector) + ) + ); + entryPoint.handleOps(userOps, beneficiary); + } + + function test_deferredAction_privilegeEscalationPrevented_executeBatch() public { + // Should not be allowed to call `executeBatch` with an internal call to `execute`/`executeBatch`. + // Attempt to call it via a deferred action in a user op + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + callData: abi.encodeCall(IModularAccount.execute, (address(0), 0 wei, "")), + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, MessageHashUtils.toEthSignedMessageHash(userOpHash)); + bytes memory uoSig = _packFinalSignature(abi.encodePacked(EOA_TYPE_SIGNATURE, r, s, v)); + + uint256 deferredInstallNonce = 0; + uint48 deferredInstallDeadline = 0; + + Call[] memory innerCalls = new Call[](1); + innerCalls[0] = + Call({target: address(account1), value: 0, data: abi.encodeCall(IModularAccount.accountId, ())}); + + Call[] memory outerCalls = new Call[](1); + outerCalls[0] = Call({ + target: address(account1), + value: 0, + data: abi.encodeCall(IModularAccount.executeBatch, (innerCalls)) + }); + + bytes memory deferredAction = abi.encodeCall(IModularAccount.executeBatch, (outerCalls)); + + userOp.signature = _buildFullDeferredInstallSig( + deferredInstallNonce, + deferredInstallDeadline, + deferredAction, + // Use the same validation for the deferred action and the user op + ValidationConfigLib.pack(_signerValidation, true, false, false), + account1, + owner1Key, + uoSig + ); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + vm.prank(beneficiary); + vm.expectRevert( + abi.encodeWithSelector( + IEntryPoint.FailedOpWithRevert.selector, + 0, + "AA23 reverted", + abi.encodeWithSelector(ModularAccountBase.SelfCallRecursionDepthExceeded.selector) + ) + ); + entryPoint.handleOps(userOps, beneficiary); + } }