From 1bd4718bea0dd138f181d425eb8decbcf9035cc4 Mon Sep 17 00:00:00 2001 From: Sahil Vasava Date: Fri, 8 Dec 2023 18:36:06 +0530 Subject: [PATCH 1/3] feat: added delegateCall support in session key --- lib/account-abstraction | 1 + lib/openzeppelin-contracts | 1 + src/common/Structs.sol | 3 +- src/interfaces/IKernel.sol | 2 + src/validator/SessionKeyValidator.sol | 64 +++++++++----- test/foundry/mock/TestCallee.sol | 7 ++ .../validator/SessionKeyValidator.t.sol | 87 ++++++++++++++----- 7 files changed, 118 insertions(+), 47 deletions(-) create mode 160000 lib/account-abstraction create mode 160000 lib/openzeppelin-contracts diff --git a/lib/account-abstraction b/lib/account-abstraction new file mode 160000 index 00000000..12be13e2 --- /dev/null +++ b/lib/account-abstraction @@ -0,0 +1 @@ +Subproject commit 12be13e2e97b763e1ef294602b3f2072bc301443 diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts new file mode 160000 index 00000000..d00acef4 --- /dev/null +++ b/lib/openzeppelin-contracts @@ -0,0 +1 @@ +Subproject commit d00acef4059807535af0bd0dd0ddf619747a044b diff --git a/src/common/Structs.sol b/src/common/Structs.sol index f2986627..ab957155 100644 --- a/src/common/Structs.sol +++ b/src/common/Structs.sol @@ -1,7 +1,7 @@ pragma solidity ^0.8.0; import {IKernelValidator} from "../interfaces/IKernelValidator.sol"; -import {ParamCondition} from "./Enums.sol"; +import {ParamCondition, Operation} from "./Enums.sol"; import {ValidAfter, ValidUntil} from "./Types.sol"; // Defining a struct for execution details @@ -57,6 +57,7 @@ struct Permission { uint256 valueLimit; ParamRule[] rules; ExecutionRule executionRule; + Operation operation; } struct SessionData { diff --git a/src/interfaces/IKernel.sol b/src/interfaces/IKernel.sol index d0029e6e..ec0b98d6 100644 --- a/src/interfaces/IKernel.sol +++ b/src/interfaces/IKernel.sol @@ -80,6 +80,8 @@ interface IKernel { function executeBatch(Call[] memory calls) external payable; + function executeDelegateCall(address to, bytes memory data) external payable; + /// @notice Validates a user operation based on its mode /// @dev This function will validate user operation and be called by EntryPoint /// @param userOp The user operation to be validated diff --git a/src/validator/SessionKeyValidator.sol b/src/validator/SessionKeyValidator.sol index db2d3e02..7df4bb14 100644 --- a/src/validator/SessionKeyValidator.sol +++ b/src/validator/SessionKeyValidator.sol @@ -7,7 +7,7 @@ import {IKernelValidator} from "../interfaces/IKernelValidator.sol"; import {UserOperation} from "I4337/interfaces/UserOperation.sol"; import {SIG_VALIDATION_FAILED} from "../common/Constants.sol"; -import {ParamCondition} from "../common/Enums.sol"; +import {ParamCondition, Operation} from "../common/Enums.sol"; import {ParamRule, SessionData, Permission, Call, ExecutionRule, ExecutionStatus, Nonces} from "../common/Structs.sol"; import "../common/Types.sol"; @@ -154,29 +154,13 @@ contract SessionKeyValidator is IKernelValidator { } bytes calldata callData = userOp.callData; - if (bytes4(callData[0:4]) == Kernel.execute.selector) { + if ( + bytes4(callData[0:4]) == Kernel.execute.selector + || bytes4(callData[0:4]) == Kernel.executeDelegateCall.selector + ) { (Permission calldata permission, bytes32[] calldata merkleProof) = _getPermission(userOp.signature[85:]); - require( - address(bytes20(userOp.callData[16:36])) == permission.target, "SessionKeyValidator: target mismatch" - ); - require( - uint256(bytes32(userOp.callData[36:68])) <= permission.valueLimit, - "SessionKeyValidator: value limit exceeded" - ); - bytes calldata data; - assembly { - let dataOffset := add(add(callData.offset, 0x04), calldataload(add(callData.offset, 0x44))) - let length := calldataload(dataOffset) - data.offset := add(dataOffset, 32) - data.length := length - } - require(verifyPermission(data, permission), "SessionKeyValidator: permission verification failed"); - ValidAfter validAfter = - _updateValidAfter(permission, keccak256(abi.encodePacked(session.nonce, permission.index))); - if (ValidAfter.unwrap(validAfter) < ValidAfter.unwrap(session.validAfter)) { - validAfter = session.validAfter; - } - if (!MerkleProofLib.verify(merkleProof, session.merkleRoot, keccak256(abi.encode(permission)))) { + (ValidAfter validAfter, bool verifyFailed) = _verifyParam(sessionKey, callData, permission, merkleProof); + if (verifyFailed) { return SIG_VALIDATION_FAILED; } return _verifyUserOpHash(sessionKey, validAfter, session.validUntil); @@ -254,6 +238,40 @@ contract SessionKeyValidator is IKernelValidator { } } + function _verifyParam( + address sessionKey, + bytes calldata callData, + Permission calldata _permission, + bytes32[] calldata _merkleProof + ) internal returns (ValidAfter validAfter, bool verifyFailed) { + SessionData storage session = sessionData[sessionKey][msg.sender]; + bool isExecute = bytes4(callData[0:4]) == Kernel.execute.selector; + require(address(bytes20(callData[16:36])) == _permission.target, "SessionKeyValidator: target mismatch"); + if (isExecute) { + require( + uint256(bytes32(callData[36:68])) <= _permission.valueLimit, "SessionKeyValidator: value limit exceeded" + ); + } else { + require(_permission.operation == Operation.DelegateCall, "SessionKeyValidator: operation mismatch"); + } + bytes calldata data; + uint8 dataParamOffset = isExecute ? 0x44 : 0x24; + assembly { + let dataOffset := add(add(callData.offset, 0x04), calldataload(add(callData.offset, dataParamOffset))) + let length := calldataload(dataOffset) + data.offset := add(dataOffset, 32) + data.length := length + } + require(verifyPermission(data, _permission), "SessionKeyValidator: permission verification failed"); + validAfter = _updateValidAfter(_permission, keccak256(abi.encodePacked(session.nonce, _permission.index))); + if (ValidAfter.unwrap(validAfter) < ValidAfter.unwrap(session.validAfter)) { + validAfter = session.validAfter; + } + if (!MerkleProofLib.verify(_merkleProof, session.merkleRoot, keccak256(abi.encode(_permission)))) { + return (validAfter, true); + } + } + function verifyPermission(bytes calldata data, Permission calldata permission) internal pure returns (bool) { if (bytes4(data[0:4]) != permission.sig) return false; for (uint256 i = 0; i < permission.rules.length; i++) { diff --git a/test/foundry/mock/TestCallee.sol b/test/foundry/mock/TestCallee.sol index 2dff256d..05b26543 100644 --- a/test/foundry/mock/TestCallee.sol +++ b/test/foundry/mock/TestCallee.sol @@ -9,5 +9,12 @@ contract TestCallee { result = a + b + msg.value; } + function transferErc20Tester(address token, address to, uint256 amount) external { + (bool success, bytes memory data) = token.call( + abi.encodeWithSignature("transfer(address,uint256)", to, amount) + ); + require(success, string(data)); + } + function notThis() external {} } diff --git a/test/foundry/validator/SessionKeyValidator.t.sol b/test/foundry/validator/SessionKeyValidator.t.sol index bf831ec2..78289995 100644 --- a/test/foundry/validator/SessionKeyValidator.t.sol +++ b/test/foundry/validator/SessionKeyValidator.t.sol @@ -21,18 +21,21 @@ import "src/validator/SessionKeyValidator.sol"; import {KernelECDSATest} from "../KernelECDSA.t.sol"; import "../mock/TestCallee.sol"; +import "../mock/TestERC20.sol"; using ERC4337Utils for IEntryPoint; contract SessionKeyValidatorTest is KernelECDSATest { SessionKeyValidator sessionKeyValidator; TestCallee[] callees; + TestERC20[] erc20s; ExecutionRule execRule; bytes32[] data; address sessionKey; uint256 sessionKeyPriv; TestPaymaster paymaster; TestPaymaster unknownPaymaster; + address recipient; function setUp() public override { super.setUp(); @@ -42,24 +45,47 @@ contract SessionKeyValidatorTest is KernelECDSATest { unknownPaymaster = new TestPaymaster(); entryPoint.depositTo{value: 1e18}(address(unknownPaymaster)); entryPoint.depositTo{value: 1e18}(address(paymaster)); + recipient = makeAddr("recipient"); } - function _setup_permission(uint256 _length) internal returns (Permission[] memory permissions) { + function _setup_permission(uint256 _length, bool isDelegateCall) + internal + returns (Permission[] memory permissions) + { permissions = new Permission[](_length); callees = new TestCallee[](_length); + if (isDelegateCall) { + erc20s = new TestERC20[](_length); + } for (uint8 i = 0; i < _length; i++) { - callees[i] = new TestCallee(); + address target; + bytes4 sig; + if (isDelegateCall) { + erc20s[i] = new TestERC20(); + target = address(callees[i]); + sig = TestCallee.transferErc20Tester.selector; + erc20s[i].mint(address(kernel), 200); + } else { + callees[i] = new TestCallee(); + target = address(callees[i]); + sig = TestCallee.addTester.selector; + } ParamRule[] memory paramRules = new ParamRule[](2); - paramRules[0] = ParamRule({offset: 0, condition: ParamCondition(i % 6), param: bytes32(uint256(100))}); + if (isDelegateCall) { + paramRules[0] = ParamRule({offset: 0, condition: ParamCondition(0), param: bytes32(uint256(uint160(recipient)))}); + } else { + paramRules[0] = ParamRule({offset: 0, condition: ParamCondition(i % 6), param: bytes32(uint256(100))}); + } paramRules[1] = ParamRule({offset: 32, condition: ParamCondition((i + 1) % 6), param: bytes32(uint256(100))}); permissions[i] = Permission({ index: i, - target: address(callees[i]), - sig: TestCallee.addTester.selector, + target: target, + sig: sig, valueLimit: 0, rules: paramRules, - executionRule: execRule + executionRule: execRule, + operation: isDelegateCall ? Operation.DelegateCall : Operation.Call }); } } @@ -154,21 +180,33 @@ contract SessionKeyValidatorTest is KernelECDSATest { uint256 indexToUse, uint8 usingPaymasterMode, bool param1Faulty, - bool param2Faulty + bool param2Faulty, + bool isDelegateCall ) internal view returns (UserOperation memory op) { + bytes4 selector = isDelegateCall ? IKernel.executeDelegateCall.selector : IKernel.execute.selector; op = entryPoint.fillUserOp( address(kernel), - abi.encodeWithSelector( - IKernel.execute.selector, - permissions[indexToUse].target, - 0, - abi.encodeWithSelector( - permissions[indexToUse].sig, - _generateParam(ParamCondition(indexToUse % 6), !param1Faulty), // since EQ - _generateParam(ParamCondition((indexToUse + 1) % 6), !param2Faulty) // since NOT_EQ - ), - Operation.Call - ) + isDelegateCall + ? abi.encodeWithSelector( + selector, + permissions[indexToUse].target, + abi.encodeWithSelector( + permissions[indexToUse].sig, + recipient, + _generateParam(ParamCondition((indexToUse + 1) % 6), !param2Faulty) // since NOT_EQ + ) + ) + : abi.encodeWithSelector( + selector, + permissions[indexToUse].target, + 0, + abi.encodeWithSelector( + permissions[indexToUse].sig, + _generateParam(ParamCondition(indexToUse % 6), !param1Faulty), // since EQ + _generateParam(ParamCondition((indexToUse + 1) % 6), !param2Faulty) // since NOT_EQ + ), + Operation.Call + ) ); if (usingPaymasterMode != 0) { // 0 = no paymaster @@ -187,7 +225,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { sessionData.nonce ); bytes32 digest = getTypedDataHash( - IKernel.execute.selector, + selector, ValidAfter.unwrap(sessionData.validAfter), ValidUntil.unwrap(sessionData.validUntil), address(sessionKeyValidator), @@ -234,6 +272,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { bool param1Faulty; bool param2Faulty; bool wrongProof; + bool isDelegateCall; } struct BatchTestConfig { @@ -248,6 +287,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { config.runs = 0; config.interval = 0; config.validAfter = 0; // TODO: runs not checked with batch + vm.assume(!config.isDelegateCall); vm.assume(config.indexToUse < config.numberOfPermissions && config.numberOfPermissions > 1); vm.assume( config.validAfter < type(uint32).max && config.interval < type(uint32).max && config.runs < type(uint32).max @@ -272,7 +312,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { validAfter: ValidAfter.wrap(config.validAfter), interval: config.interval }); - Permission[] memory permissions = _setup_permission(config.numberOfPermissions); + Permission[] memory permissions = _setup_permission(config.numberOfPermissions, false); _buildHashes(permissions); (uint128 lastNonce,) = sessionKeyValidator.nonces(address(kernel)); SessionData memory sessionData = SessionData({ @@ -368,7 +408,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { config.paymasterMode = config.paymasterMode % 3; config.usingPaymasterMode = config.usingPaymasterMode % 3; bool shouldFail = (config.usingPaymasterMode < config.paymasterMode) || (1000 < config.validAfter) - || config.faultySig || config.param1Faulty || config.param2Faulty || config.wrongProof; + || config.faultySig || (config.param1Faulty && !config.isDelegateCall) || config.param2Faulty || config.wrongProof; config.runs = config.runs % 10; config.earlyRun = config.runs == 0 ? 0 : config.earlyRun % config.runs; if (config.interval == 0 || config.validAfter == 0) { @@ -385,7 +425,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { validAfter: ValidAfter.wrap(config.validAfter), interval: config.interval }); - Permission[] memory permissions = _setup_permission(config.numberOfPermissions); + Permission[] memory permissions = _setup_permission(config.numberOfPermissions, config.isDelegateCall); _buildHashes(permissions); (uint128 lastNonce,) = sessionKeyValidator.nonces(address(kernel)); SessionData memory sessionData = SessionData({ @@ -402,7 +442,8 @@ contract SessionKeyValidatorTest is KernelECDSATest { config.indexToUse, config.usingPaymasterMode, config.param1Faulty, - config.param2Faulty + config.param2Faulty, + config.isDelegateCall ); op.signature = bytes.concat( op.signature, From a1a327bca06b55a3c48443f416d1916dedde4160 Mon Sep 17 00:00:00 2001 From: Sahil Vasava Date: Fri, 8 Dec 2023 18:40:11 +0530 Subject: [PATCH 2/3] fix: remove deps --- lib/account-abstraction | 1 - lib/openzeppelin-contracts | 1 - 2 files changed, 2 deletions(-) delete mode 160000 lib/account-abstraction delete mode 160000 lib/openzeppelin-contracts diff --git a/lib/account-abstraction b/lib/account-abstraction deleted file mode 160000 index 12be13e2..00000000 --- a/lib/account-abstraction +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 12be13e2e97b763e1ef294602b3f2072bc301443 diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts deleted file mode 160000 index d00acef4..00000000 --- a/lib/openzeppelin-contracts +++ /dev/null @@ -1 +0,0 @@ -Subproject commit d00acef4059807535af0bd0dd0ddf619747a044b From e50d2a82123dc263c9bfb389f81ad3779f6da693 Mon Sep 17 00:00:00 2001 From: Sahil Vasava Date: Thu, 14 Dec 2023 01:30:49 +0400 Subject: [PATCH 3/3] feat: added wildcard target permission condition in session key --- src/validator/SessionKeyValidator.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/validator/SessionKeyValidator.sol b/src/validator/SessionKeyValidator.sol index 7df4bb14..8021a452 100644 --- a/src/validator/SessionKeyValidator.sol +++ b/src/validator/SessionKeyValidator.sol @@ -224,7 +224,9 @@ contract SessionKeyValidator is IKernelValidator { for (i = 0; i < calls.length; i++) { Call calldata call = calls[i]; Permission calldata permission = _permissions[i]; - require(call.to == permission.target, "SessionKeyValidator: target mismatch"); + require( + permission.target == address(0) || call.to == permission.target, "SessionKeyValidator: target mismatch" + ); require(uint256(bytes32(call.value)) <= permission.valueLimit, "SessionKeyValidator: value limit exceeded"); require(verifyPermission(call.data, permission), "SessionKeyValidator: permission verification failed"); ValidAfter validAfter = @@ -246,7 +248,10 @@ contract SessionKeyValidator is IKernelValidator { ) internal returns (ValidAfter validAfter, bool verifyFailed) { SessionData storage session = sessionData[sessionKey][msg.sender]; bool isExecute = bytes4(callData[0:4]) == Kernel.execute.selector; - require(address(bytes20(callData[16:36])) == _permission.target, "SessionKeyValidator: target mismatch"); + require( + _permission.target == address(0) || address(bytes20(callData[16:36])) == _permission.target, + "SessionKeyValidator: target mismatch" + ); if (isExecute) { require( uint256(bytes32(callData[36:68])) <= _permission.valueLimit, "SessionKeyValidator: value limit exceeded"