From a15c86652780273a393a515832c3c22993930c74 Mon Sep 17 00:00:00 2001 From: KONFeature Date: Wed, 6 Dec 2023 19:26:49 +0100 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Optimise=20gas=20usage?= =?UTF-8?q?=20when=20enabling=20p256=20validator,=20add=20a=20few=20commen?= =?UTF-8?q?ts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reducing the number of indexed variable inside a log highly decrease his gas usage, in the p256 validator, we only matter about the kernel account as index (it's 375 gas per topic, so per indexed props, so reducing the index on both key reduce the enabling gas cost by 375 * 4 -> 1500 gas) - Add a few reflexion todo comment, do you rly need to send the previous key in the event? Since it's cost with a `sload`& also in the event itself --- src/Kernel.sol | 2 +- src/validator/P256Validator.sol | 42 +++++++++++++++---- test/foundry/KernelLiteECDSA.t.sol | 3 +- test/foundry/KernelTestBase.sol | 7 +++- .../validator/KillSwitchValidator.t.sol | 3 +- test/foundry/validator/P256Validator.t.sol | 15 +++---- 6 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/Kernel.sol b/src/Kernel.sol index 74f2425b..25a94b18 100644 --- a/src/Kernel.sol +++ b/src/Kernel.sol @@ -29,7 +29,7 @@ contract Kernel is EIP712, Compatibility, KernelStorage { /// @dev Selector of the `DisabledMode()` error, to be used in assembly, 'bytes4(keccak256(bytes("DisabledMode()")))', same as DisabledMode.selector() uint256 private constant _DISABLED_MODE_SELECTOR = 0xfc2f51c5; - /// @dev Current kernel name and version, todo: Need to expose getter for this variables? + /// @dev Current kernel name and version, todo: Need to expose getter for this variables? string public constant name = KERNEL_NAME; string public constant version = KERNEL_VERSION; diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol index 02f122fd..22e26f44 100644 --- a/src/validator/P256Validator.sol +++ b/src/validator/P256Validator.sol @@ -9,45 +9,73 @@ import {ValidationData} from "../common/Types.sol"; import {SIG_VALIDATION_FAILED} from "../common/Constants.sol"; import {P256} from "p256-verifier/P256.sol"; +/// @title P256Validator +/// @notice This validator uses the P256 curve to validate signatures. contract P256Validator is IKernelValidator { - event P256PublicKeysChanged(address indexed kernel, PublicKey indexed oldKeys, PublicKey indexed newKeys); + /// @notice Emitted when a bad key is provided. + error BadKey(); + + /// @notice Emitted when the public key of a kernel is changed. + event P256PublicKeysChanged(address indexed kernel, PublicKey oldKeys, PublicKey newKeys); + /// @notice The P256 public key of a kernel. struct PublicKey { uint256 x; uint256 y; } - error BadKey(); - - mapping(address => PublicKey) public p256PublicKey; + /// @notice The P256 public keys of a kernel. + mapping(address kernel => PublicKey PublicKey) public p256PublicKey; + /// @notice Enable this validator for a kernel account. + /// @dev The kernel account need to be the `msg.sender`. + /// @dev The public key is encoded as `abi.encode(PublicKey)` inside the data, so (uint256,uint256). function enable(bytes calldata _data) external payable override { PublicKey memory key = abi.decode(_data, (PublicKey)); if (key.x == 0 || key.y == 0) { revert BadKey(); } + // TODO: Should evaluate if the previous key is rly needed for this event, since it will prevent a `sload` during the enabling of this validator for a kernel account + // TODO: It will also prevent to store too much data inside the memory before sending the event, and so with only the two uint256 of the key, the compiler would use the two initial slots of the memstack and not assign 4 mem slots just for the events + // Copy the previous key for the event (so a sload) PublicKey memory oldKey = p256PublicKey[msg.sender]; + // Update the key (so a sstore) p256PublicKey[msg.sender] = key; + // And emit the event emit P256PublicKeysChanged(msg.sender, oldKey, key); } + /// @notice Disable this validator for a kernel account. + /// @dev The kernel account need to be the `msg.sender`. function disable(bytes calldata) external payable override { delete p256PublicKey[msg.sender]; } - function validateUserOp(UserOperation calldata _userOp, bytes32 _userOpHash, uint256) external payable override returns (ValidationData validationData) { + /// @notice Validate a user operation. + function validateUserOp(UserOperation calldata _userOp, bytes32 _userOpHash, uint256) + external + payable + override + returns (ValidationData validationData) + { (uint256 r, uint256 s) = abi.decode(_userOp.signature, (uint256, uint256)); PublicKey memory key = p256PublicKey[_userOp.sender]; bytes32 hash = ECDSA.toEthSignedMessageHash(_userOpHash); if (P256.verifySignature(hash, r, s, key.x, key.y)) { return ValidationData.wrap(0); - } + } if (!P256.verifySignature(_userOpHash, r, s, key.x, key.y)) { return SIG_VALIDATION_FAILED; } } - function validateSignature(bytes32 hash, bytes calldata signature) external view override returns (ValidationData) { + /// @notice Validate a signature. + function validateSignature(bytes32 hash, bytes calldata signature) + external + view + override + returns (ValidationData) + { (uint256 r, uint256 s) = abi.decode(signature, (uint256, uint256)); PublicKey memory key = p256PublicKey[msg.sender]; if (P256.verifySignature(hash, r, s, key.x, key.y)) { diff --git a/test/foundry/KernelLiteECDSA.t.sol b/test/foundry/KernelLiteECDSA.t.sol index 3496986c..192555c3 100644 --- a/test/foundry/KernelLiteECDSA.t.sol +++ b/test/foundry/KernelLiteECDSA.t.sol @@ -97,7 +97,8 @@ contract KernelECDSATest is KernelTestBase { function test_transfer_ownership() external { address newOwner = makeAddr("new owner"); - UserOperation memory op = buildUserOperation(abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner)); + UserOperation memory op = + buildUserOperation(abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner)); vm.expectEmit(true, true, true, false, address(entryPoint)); emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0); performUserOperationWithSig(op); diff --git a/test/foundry/KernelTestBase.sol b/test/foundry/KernelTestBase.sol index 1951cba4..366761c8 100644 --- a/test/foundry/KernelTestBase.sol +++ b/test/foundry/KernelTestBase.sol @@ -272,7 +272,9 @@ abstract contract KernelTestBase is Test { function test_disable_mode() external { vm.warp(1000); bytes memory empty; - UserOperation memory op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty)); + UserOperation memory op = buildUserOperation( + abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) + ); performUserOperationWithSig(op); assertEq(uint256(bytes32(IKernel(address(kernel)).getDisabledMode())), 1 << 224); assertEq(uint256(IKernel(address(kernel)).getLastDisabledTime()), block.timestamp); @@ -336,7 +338,8 @@ abstract contract KernelTestBase is Test { function test_revert_when_mode_disabled() external { vm.warp(1000); bytes memory empty; - UserOperation memory op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) + UserOperation memory op = buildUserOperation( + abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) ); performUserOperationWithSig(op); diff --git a/test/foundry/validator/KillSwitchValidator.t.sol b/test/foundry/validator/KillSwitchValidator.t.sol index 6b080d64..8a3861fb 100644 --- a/test/foundry/validator/KillSwitchValidator.t.sol +++ b/test/foundry/validator/KillSwitchValidator.t.sol @@ -68,7 +68,8 @@ contract KillSwitchValidatorTest is KernelECDSATest { } function test_force_unblock() external { - UserOperation memory op = buildUserOperation(abi.encodeWithSelector(Kernel.execute.selector, owner, 0, "", Operation.Call)); + UserOperation memory op = + buildUserOperation(abi.encodeWithSelector(Kernel.execute.selector, owner, 0, "", Operation.Call)); op.signature = bytes.concat(bytes4(0), entryPoint.signUserOpHash(vm, ownerKey, op)); performUserOperation(op); diff --git a/test/foundry/validator/P256Validator.t.sol b/test/foundry/validator/P256Validator.t.sol index 25336449..6ffc012f 100644 --- a/test/foundry/validator/P256Validator.t.sol +++ b/test/foundry/validator/P256Validator.t.sol @@ -16,7 +16,6 @@ import {P256} from "p256-verifier/P256.sol"; import {FCL_ecdsa_utils} from "FreshCryptoLib/FCL_ecdsa_utils.sol"; import {IKernel} from "src/interfaces/IKernel.sol"; - using ERC4337Utils for IEntryPoint; contract P256ValidatorTest is KernelTestBase { @@ -24,9 +23,7 @@ contract P256ValidatorTest is KernelTestBase { P256Validator p256Validator; // Curve order (number of points) - uint256 constant n = - 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551; - + uint256 constant n = 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551; uint256 x; uint256 y; @@ -55,7 +52,7 @@ contract P256ValidatorTest is KernelTestBase { return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); } - function getOwners() internal virtual override returns (address[] memory _owners){ + function getOwners() internal virtual override returns (address[] memory _owners) { _owners = new address[](1); _owners[0] = address(0); return _owners; @@ -69,7 +66,7 @@ contract P256ValidatorTest is KernelTestBase { return abi.encodeWithSelector(KernelStorage.initialize.selector, p256Validator, abi.encode(x, y)); } - function test_default_validator_enable() external override{ + function test_default_validator_enable() external override { UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.execute.selector, @@ -106,7 +103,7 @@ contract P256ValidatorTest is KernelTestBase { function test_external_call_execute_success() external override { vm.skip(true); } - + function test_external_call_execute_delegatecall_success() external override { vm.skip(true); } @@ -150,7 +147,7 @@ contract P256ValidatorTest is KernelTestBase { vm.assume(privateKey != 0); (uint256 x1, uint256 y1) = generatePublicKey(privateKey); (uint256 r, uint256 s) = generateSignature(privateKey, hash); - + vm.assume(x1 != 0); vm.assume(y1 != 0); vm.assume(r != 0); @@ -161,7 +158,7 @@ contract P256ValidatorTest is KernelTestBase { function test_validate_signature() external override { Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3))); bytes32 hash = keccak256(abi.encodePacked("hello world")); - + bytes32 digest = keccak256( abi.encodePacked( "\x19\x01", ERC4337Utils._buildDomainSeparator(KERNEL_NAME, KERNEL_VERSION, address(kernel)), hash From 835fee1c348412fcebc0aec0cec3b7ad0eca7898 Mon Sep 17 00:00:00 2001 From: KONFeature Date: Thu, 7 Dec 2023 17:38:29 +0100 Subject: [PATCH 2/3] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Remove=20the=20oldKeys?= =?UTF-8?q?=20for=20the=20event=20signature?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/validator/P256Validator.sol | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol index 22e26f44..c27dcfd2 100644 --- a/src/validator/P256Validator.sol +++ b/src/validator/P256Validator.sol @@ -16,7 +16,7 @@ contract P256Validator is IKernelValidator { error BadKey(); /// @notice Emitted when the public key of a kernel is changed. - event P256PublicKeysChanged(address indexed kernel, PublicKey oldKeys, PublicKey newKeys); + event P256PublicKeysChanged(address indexed kernel, PublicKey newKeys); /// @notice The P256 public key of a kernel. struct PublicKey { @@ -35,14 +35,10 @@ contract P256Validator is IKernelValidator { if (key.x == 0 || key.y == 0) { revert BadKey(); } - // TODO: Should evaluate if the previous key is rly needed for this event, since it will prevent a `sload` during the enabling of this validator for a kernel account - // TODO: It will also prevent to store too much data inside the memory before sending the event, and so with only the two uint256 of the key, the compiler would use the two initial slots of the memstack and not assign 4 mem slots just for the events - // Copy the previous key for the event (so a sload) - PublicKey memory oldKey = p256PublicKey[msg.sender]; // Update the key (so a sstore) p256PublicKey[msg.sender] = key; // And emit the event - emit P256PublicKeysChanged(msg.sender, oldKey, key); + emit P256PublicKeysChanged(msg.sender, key); } /// @notice Disable this validator for a kernel account. From 34d811df205d6edc9481e395d71143a00acaa18d Mon Sep 17 00:00:00 2001 From: KONFeature Date: Fri, 8 Dec 2023 15:02:44 +0100 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Only=20compare=20to=20?= =?UTF-8?q?raw=20msg=20signing=20instead=20of=20eth=20signed=20message=20f?= =?UTF-8?q?or=20p256?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/validator/P256Validator.sol | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol index c27dcfd2..fbbe7c1a 100644 --- a/src/validator/P256Validator.sol +++ b/src/validator/P256Validator.sol @@ -56,13 +56,10 @@ contract P256Validator is IKernelValidator { { (uint256 r, uint256 s) = abi.decode(_userOp.signature, (uint256, uint256)); PublicKey memory key = p256PublicKey[_userOp.sender]; - bytes32 hash = ECDSA.toEthSignedMessageHash(_userOpHash); - if (P256.verifySignature(hash, r, s, key.x, key.y)) { + if (P256.verifySignature(_userOpHash, r, s, key.x, key.y)) { return ValidationData.wrap(0); } - if (!P256.verifySignature(_userOpHash, r, s, key.x, key.y)) { - return SIG_VALIDATION_FAILED; - } + return SIG_VALIDATION_FAILED; } /// @notice Validate a signature. @@ -77,11 +74,7 @@ contract P256Validator is IKernelValidator { if (P256.verifySignature(hash, r, s, key.x, key.y)) { return ValidationData.wrap(0); } - bytes32 ethHash = ECDSA.toEthSignedMessageHash(hash); - if (!P256.verifySignature(ethHash, r, s, key.x, key.y)) { - return SIG_VALIDATION_FAILED; - } - return ValidationData.wrap(0); + return SIG_VALIDATION_FAILED; } function validCaller(address _caller, bytes calldata) external view override returns (bool) {