From 90fa72ed4386b3a4acb86c021743c5d56fc4f603 Mon Sep 17 00:00:00 2001 From: taek Date: Thu, 16 Nov 2023 00:08:49 +0900 Subject: [PATCH 01/10] Dev (#47) * Added new ERC1271 signature verification logic in Kernel v0.2.3 (#43) * Added 1271 wrapper * Update kernel version to 0.2.3 * use kernel name and version from constants in tests * added delegatecall support (#44) * session key validator fixed for batch scenario * test: fuzz testing for batched options include array * fix: warning removed, forge fmt (#46) --------- Co-authored-by: David Eiber --- src/Kernel.sol | 60 +++- src/common/Constants.sol | 2 +- src/validator/SessionKeyValidator.sol | 47 ++- src/validator/WeightedECDSAValidator.sol | 10 +- test/foundry/KernelLiteECDSA.t.sol | 3 +- test/foundry/KernelTestBase.sol | 33 +- test/foundry/utils/Merkle.sol | 5 +- .../validator/SessionKeyValidator.t.sol | 284 +++++++++++++++--- 8 files changed, 358 insertions(+), 86 deletions(-) diff --git a/src/Kernel.sol b/src/Kernel.sol index 6275f5cb..9687c120 100644 --- a/src/Kernel.sol +++ b/src/Kernel.sol @@ -76,6 +76,22 @@ contract Kernel is EIP712, Compatibility, KernelStorage { } } + /// @notice Executes a function call to an external contract with delegatecall + /// @param to The address of the target contract + /// @param data The call data to be sent + function executeDelegateCall(address to, bytes memory data) external payable { + if (msg.sender != address(entryPoint) && msg.sender != address(this) && !_checkCaller()) { + revert NotAuthorizedCaller(); + } + assembly { + let success := delegatecall(gas(), to, add(data, 0x20), mload(data), 0, 0) + returndatacopy(0, 0, returndatasize()) + switch success + case 0 { revert(0, returndatasize()) } + default { return(0, returndatasize()) } + } + } + /// @notice Executes a function call to an external contract batched /// @param calls The calls to be executed, in order /// @dev operation deprecated param, use executeBatch for batched transaction @@ -235,29 +251,51 @@ contract Kernel is EIP712, Compatibility, KernelStorage { } } - function validateSignature(bytes32 hash, bytes calldata signature) public view returns(ValidationData) { + function validateSignature(bytes32 hash, bytes calldata signature) public view returns (ValidationData) { return _validateSignature(hash, signature); } + function _domainSeparator() internal view override returns (bytes32) { + // Obtain the name and version from the _domainNameAndVersion function. + (string memory _name, string memory _version) = _domainNameAndVersion(); + bytes32 nameHash = keccak256(bytes(_name)); + bytes32 versionHash = keccak256(bytes(_version)); + + // Use the proxy address for the EIP-712 domain separator. + address proxyAddress = address(this); + + // Construct the domain separator with name, version, chainId, and proxy address. + bytes32 typeHash = + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + return keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, proxyAddress)); + } + /// @notice Checks if a signature is valid /// @dev This function checks if a signature is valid based on the hash of the data signed. /// @param hash The hash of the data that was signed /// @param signature The signature to be validated /// @return The magic value 0x1626ba7e if the signature is valid, otherwise returns 0xffffffff. function isValidSignature(bytes32 hash, bytes calldata signature) public view returns (bytes4) { - ValidationData validationData = validateSignature(hash, signature); + // Include the proxy address in the domain separator + bytes32 domainSeparator = _domainSeparator(); + + // Recreate the signed message hash with the correct domain separator + bytes32 signedMessageHash = keccak256(abi.encodePacked("\x19\x01", domainSeparator, hash)); + + ValidationData validationData = validateSignature(signedMessageHash, signature); (ValidAfter validAfter, ValidUntil validUntil, address result) = parseValidationData(validationData); - if (ValidAfter.unwrap(validAfter) > block.timestamp) { - return 0xffffffff; - } - if (ValidUntil.unwrap(validUntil) < block.timestamp) { - return 0xffffffff; - } - if (result != address(0)) { + + // Check if the signature is valid within the specified time frame and the result is successful + if ( + ValidAfter.unwrap(validAfter) <= block.timestamp && ValidUntil.unwrap(validUntil) >= block.timestamp + && result == address(0) + ) { + // If all checks pass, return the ERC1271 magic value for a valid signature + return 0x1626ba7e; + } else { + // If any check fails, return the failure magic value return 0xffffffff; } - - return 0x1626ba7e; } function _checkCaller() internal view returns (bool) { diff --git a/src/common/Constants.sol b/src/common/Constants.sol index e65d4328..e85dc7f3 100644 --- a/src/common/Constants.sol +++ b/src/common/Constants.sol @@ -4,7 +4,7 @@ import {ValidationData} from "./Types.sol"; // constants for kernel metadata string constant KERNEL_NAME = "Kernel"; -string constant KERNEL_VERSION = "0.2.2"; +string constant KERNEL_VERSION = "0.2.3"; // ERC4337 constants uint256 constant SIG_VALIDATION_FAILED_UINT = 1; diff --git a/src/validator/SessionKeyValidator.sol b/src/validator/SessionKeyValidator.sol index 2bbc40be..db2d3e02 100644 --- a/src/validator/SessionKeyValidator.sol +++ b/src/validator/SessionKeyValidator.sol @@ -30,11 +30,9 @@ contract SessionKeyValidator is IKernelValidator { } function invalidateNonce(uint128 nonce) public { - require( - nonce > nonces[msg.sender].invalidNonce && nonce <= nonces[msg.sender].lastNonce, - "SessionKeyValidator: invalid nonce" - ); + require(nonce > nonces[msg.sender].invalidNonce, "SessionKeyValidator: invalid nonce"); nonces[msg.sender].invalidNonce = nonce; + nonces[msg.sender].lastNonce = nonce; } function disable(bytes calldata _data) external payable { @@ -113,7 +111,14 @@ contract SessionKeyValidator is IKernelValidator { function _getPermissions(bytes calldata _sig) internal pure returns (Permission[] calldata permissions) { assembly { permissions.offset := add(add(_sig.offset, 0x20), calldataload(_sig.offset)) - permissions.length := calldataload(permissions.offset) + permissions.length := calldataload(add(_sig.offset, calldataload(_sig.offset))) + } + } + + function _getProofs(bytes calldata _sig) internal pure returns (bytes32[][] calldata proofs) { + assembly { + proofs.length := calldataload(add(_sig.offset, calldataload(add(_sig.offset, 0x20)))) + proofs.offset := add(add(_sig.offset, 0x20), calldataload(add(_sig.offset, 0x20))) } } @@ -152,8 +157,7 @@ contract SessionKeyValidator is IKernelValidator { if (bytes4(callData[0:4]) == Kernel.execute.selector) { (Permission calldata permission, bytes32[] calldata merkleProof) = _getPermission(userOp.signature[85:]); require( - address(bytes20(userOp.callData[16:36])) == permission.target, - "SessionKeyValidator: target mismatch" + address(bytes20(userOp.callData[16:36])) == permission.target, "SessionKeyValidator: target mismatch" ); require( uint256(bytes32(userOp.callData[36:68])) <= permission.valueLimit, @@ -178,10 +182,9 @@ contract SessionKeyValidator is IKernelValidator { return _verifyUserOpHash(sessionKey, validAfter, session.validUntil); } else if (bytes4(callData[0:4]) == Kernel.executeBatch.selector) { Permission[] calldata permissions = _getPermissions(userOp.signature[85:]); - (, bytes32[] memory merkleProof, bool[] memory flags, uint256[] memory index) = - abi.decode(userOp.signature[85:], (Permission[], bytes32[], bool[], uint256[])); - (bytes32[] memory leaves, ValidAfter validAfter) = _verifyParams(sessionKey, callData, permissions, index); - if (!MerkleProofLib.verifyMultiProof(merkleProof, session.merkleRoot, leaves, flags)) { + bytes32[][] calldata merkleProof = _getProofs(userOp.signature[85:]); + (ValidAfter validAfter, bool verifyFailed) = _verifyParams(sessionKey, callData, permissions, merkleProof); + if (verifyFailed) { return SIG_VALIDATION_FAILED; } return _verifyUserOpHash(sessionKey, validAfter, session.validUntil); @@ -224,23 +227,20 @@ contract SessionKeyValidator is IKernelValidator { address sessionKey, bytes calldata callData, Permission[] calldata _permissions, - uint256[] memory index - ) internal returns (bytes32[] memory leaves, ValidAfter maxValidAfter) { + bytes32[][] calldata _merkleProof + ) internal returns (ValidAfter maxValidAfter, bool verifyFailed) { Call[] calldata calls; assembly { calls.offset := add(add(callData.offset, 0x24), calldataload(add(callData.offset, 4))) calls.length := calldataload(add(add(callData.offset, 4), calldataload(add(callData.offset, 4)))) } uint256 i = 0; - leaves = _generateLeaves(index); SessionData storage session = sessionData[sessionKey][msg.sender]; maxValidAfter = session.validAfter; 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(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 = @@ -248,19 +248,10 @@ contract SessionKeyValidator is IKernelValidator { if (ValidAfter.unwrap(validAfter) > ValidAfter.unwrap(maxValidAfter)) { maxValidAfter = validAfter; } - leaves[index[i]] = keccak256(abi.encode(permission)); - } - } - - function _generateLeaves(uint256[] memory _indexes) internal pure returns (bytes32[] memory leaves) { - uint256 maxIndex; - uint256 i = 0; - for (i = 0; i < _indexes.length; i++) { - if (_indexes[i] > maxIndex) { - maxIndex = _indexes[i]; + if (!MerkleProofLib.verify(_merkleProof[i], session.merkleRoot, keccak256(abi.encode(permission)))) { + return (maxValidAfter, true); } } - leaves = new bytes32[](maxIndex + 1); } function verifyPermission(bytes calldata data, Permission calldata permission) internal pure returns (bool) { diff --git a/src/validator/WeightedECDSAValidator.sol b/src/validator/WeightedECDSAValidator.sol index 0f1d9de2..faa4f3bc 100644 --- a/src/validator/WeightedECDSAValidator.sol +++ b/src/validator/WeightedECDSAValidator.sol @@ -127,7 +127,7 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator { proposal.status = ProposalStatus.Rejected; } - function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingFunds) + function validateUserOp(UserOperation calldata userOp, bytes32, uint256) external payable returns (ValidationData validationData) @@ -149,7 +149,9 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator { for (uint256 i = 0; i < sigCount; i++) { // last sig is for userOpHash verification signer = ECDSA.recover( - _hashTypedData(keccak256(abi.encode(keccak256("Approve(bytes32 callDataAndNonceHash)"), callDataAndNonceHash))), + _hashTypedData( + keccak256(abi.encode(keccak256("Approve(bytes32 callDataAndNonceHash)"), callDataAndNonceHash)) + ), sig[i * 65:(i + 1) * 65] ); vote = voteStatus[callDataAndNonceHash][signer][msg.sender]; @@ -173,11 +175,11 @@ contract WeightedECDSAValidator is EIP712, IKernelValidator { } } - function validCaller(address, bytes calldata) external view override returns (bool) { + function validCaller(address, bytes calldata) external pure override returns (bool) { return false; } - function validateSignature(bytes32 hash, bytes calldata signature) external view returns (ValidationData) { + function validateSignature(bytes32, bytes calldata) external pure returns (ValidationData) { return SIG_VALIDATION_FAILED; } } diff --git a/test/foundry/KernelLiteECDSA.t.sol b/test/foundry/KernelLiteECDSA.t.sol index c2864d91..e18f21ff 100644 --- a/test/foundry/KernelLiteECDSA.t.sol +++ b/test/foundry/KernelLiteECDSA.t.sol @@ -102,8 +102,7 @@ contract KernelECDSATest is KernelTestBase { function test_transfer_ownership() external { address newOwner = makeAddr("new owner"); UserOperation memory op = entryPoint.fillUserOp( - address(kernel), - abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner) + address(kernel), abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner) ); op.signature = signUserOp(op); UserOperation[] memory ops = new UserOperation[](1); diff --git a/test/foundry/KernelTestBase.sol b/test/foundry/KernelTestBase.sol index baa2d42d..b4501cdd 100644 --- a/test/foundry/KernelTestBase.sol +++ b/test/foundry/KernelTestBase.sol @@ -14,6 +14,7 @@ import {IKernelValidator} from "src/interfaces/IKernelValidator.sol"; import {Call, ExecutionDetail} from "src/common/Structs.sol"; import {ValidationData, ValidUntil, ValidAfter} from "src/common/Types.sol"; +import {KERNEL_VERSION, KERNEL_NAME} from "src/common/Constants.sol"; import {ERC4337Utils} from "test/foundry/utils/ERC4337Utils.sol"; import {Test} from "forge-std/Test.sol"; @@ -97,7 +98,24 @@ abstract contract KernelTestBase is Test { } } + function test_external_call_execute_delegatecall_success() external { + address[] memory validCallers = getOwners(); + for (uint256 i = 0; i < validCallers.length; i++) { + vm.prank(validCallers[i]); + kernel.executeDelegateCall(validCallers[i], ""); + } + } + function test_external_call_execute_delegatecall_fail() external { + address[] memory validCallers = getOwners(); + for (uint256 i = 0; i < validCallers.length; i++) { + vm.prank(address(uint160(validCallers[i]) + 1)); + vm.expectRevert(); + kernel.executeDelegateCall(validCallers[i], ""); + } + } + + function test_external_call_execute_delegatecall_option_fail() external { address[] memory validCallers = getOwners(); for (uint256 i = 0; i < validCallers.length; i++) { vm.prank(validCallers[i]); @@ -143,8 +161,8 @@ abstract contract KernelTestBase is Test { (bytes1 fields, string memory name, string memory version,, address verifyingContract, bytes32 salt,) = kernel.eip712Domain(); assertEq(fields, bytes1(hex"0f")); - assertEq(name, "Kernel"); - assertEq(version, "0.2.2"); + assertEq(name, KERNEL_NAME); + assertEq(version, KERNEL_VERSION); assertEq(verifyingContract, address(kernel)); assertEq(salt, bytes32(0)); } @@ -177,9 +195,16 @@ abstract contract KernelTestBase is Test { } function test_validate_signature() external { + Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3))); bytes32 hash = keccak256(abi.encodePacked("hello world")); - bytes memory sig = signHash(hash); + bytes32 digest = keccak256( + abi.encodePacked( + "\x19\x01", ERC4337Utils._buildDomainSeparator(KERNEL_NAME, KERNEL_VERSION, address(kernel)), hash + ) + ); + bytes memory sig = signHash(digest); assertEq(kernel.isValidSignature(hash, sig), Kernel.isValidSignature.selector); + assertEq(kernel2.isValidSignature(hash, sig), bytes4(0xffffffff)); } function test_fail_validate_wrongsignature() external { @@ -471,7 +496,7 @@ abstract contract KernelTestBase is Test { return keccak256( abi.encodePacked( "\x19\x01", - ERC4337Utils._buildDomainSeparator("Kernel", "0.2.2", address(kernel)), + ERC4337Utils._buildDomainSeparator(KERNEL_NAME, KERNEL_VERSION, address(kernel)), ERC4337Utils.getStructHash(sig, validUntil, validAfter, validator, executor, enableData) ) ); diff --git a/test/foundry/utils/Merkle.sol b/test/foundry/utils/Merkle.sol index 55b49b5a..26730161 100644 --- a/test/foundry/utils/Merkle.sol +++ b/test/foundry/utils/Merkle.sol @@ -8,7 +8,7 @@ function _getRoot(bytes32[] memory data) pure returns (bytes32) { return data[0]; } -function _getProof(bytes32[] memory data, uint256 nodeIndex) pure returns (bytes32[] memory) { +function _getProof(bytes32[] memory data, uint256 nodeIndex, bool wrongProof) pure returns (bytes32[] memory) { require(data.length > 1); bytes32[] memory result = new bytes32[](64); @@ -33,6 +33,9 @@ function _getProof(bytes32[] memory data, uint256 nodeIndex) pure returns (bytes assembly { mstore(result, pos) } + if (wrongProof) { + result[0] = result[0] ^ bytes32(uint256(0x01)); + } return result; } diff --git a/test/foundry/validator/SessionKeyValidator.t.sol b/test/foundry/validator/SessionKeyValidator.t.sol index 1683e5a4..bf831ec2 100644 --- a/test/foundry/validator/SessionKeyValidator.t.sol +++ b/test/foundry/validator/SessionKeyValidator.t.sol @@ -5,6 +5,7 @@ import "src/Kernel.sol"; import "src/interfaces/IKernel.sol"; import "src/validator/ECDSAValidator.sol"; import "src/factory/KernelFactory.sol"; +import {Call} from "src/common/Structs.sol"; // test artifacts import "../mock/TestValidator.sol"; import "../mock/TestExecutor.sol"; @@ -49,16 +50,9 @@ contract SessionKeyValidatorTest is KernelECDSATest { for (uint8 i = 0; i < _length; i++) { callees[i] = new TestCallee(); ParamRule[] memory paramRules = new ParamRule[](2); - 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)) - }); + 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]), @@ -78,22 +72,82 @@ contract SessionKeyValidatorTest is KernelECDSATest { } } - function _generateParam(ParamCondition condition, bool correct) internal pure returns(uint256 param) { - if(condition == ParamCondition.EQUAL) { + function _generateParam(ParamCondition condition, bool correct) internal pure returns (uint256 param) { + if (condition == ParamCondition.EQUAL) { param = correct ? 100 : 101; - } else if(condition == ParamCondition.GREATER_THAN) { - param = correct ? 101 : 100; - } else if(condition == ParamCondition.LESS_THAN) { + } else if (condition == ParamCondition.GREATER_THAN) { + param = correct ? 101 : 100; + } else if (condition == ParamCondition.LESS_THAN) { param = correct ? 99 : 100; - } else if(condition == ParamCondition.NOT_EQUAL) { + } else if (condition == ParamCondition.NOT_EQUAL) { param = correct ? 101 : 100; - } else if(condition == ParamCondition.GREATER_THAN_OR_EQUAL) { + } else if (condition == ParamCondition.GREATER_THAN_OR_EQUAL) { param = correct ? 100 : 99; - } else if(condition == ParamCondition.LESS_THAN_OR_EQUAL) { + } else if (condition == ParamCondition.LESS_THAN_OR_EQUAL) { param = correct ? 100 : 101; } } + function _buildUserOpBatch( + Permission[] memory permissions, + SessionData memory sessionData, + uint256 indexToUse, + uint8 usingPaymasterMode, + bool param1Faulty, + bool param2Faulty + ) internal view returns (UserOperation memory op) { + Call[] memory calls = new Call[](1); + calls[0] = Call({ + to: permissions[indexToUse].target, + value: 0, + data: abi.encodeWithSelector( + permissions[indexToUse].sig, + _generateParam(ParamCondition(indexToUse % 6), !param1Faulty), + _generateParam(ParamCondition((indexToUse + 1) % 6), !param2Faulty) + ) + }); + + op = entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(IKernel.executeBatch.selector, calls)); + if (usingPaymasterMode != 0) { + // 0 = no paymaster + // 1 = unknown paymaster + // 2 = correct paymaster + op.paymasterAndData = usingPaymasterMode == 1 + ? abi.encodePacked(address(unknownPaymaster)) + : abi.encodePacked(address(paymaster)); + } + bytes memory enableData = abi.encodePacked( + sessionKey, + sessionData.merkleRoot, + sessionData.validAfter, + sessionData.validUntil, + sessionData.paymaster, + sessionData.nonce + ); + bytes32 digest = getTypedDataHash( + IKernel.executeBatch.selector, + ValidAfter.unwrap(sessionData.validAfter), + ValidUntil.unwrap(sessionData.validUntil), + address(sessionKeyValidator), + address(0), + enableData + ); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, digest); + op.signature = abi.encodePacked( + bytes4(0x00000002), + uint48(ValidAfter.unwrap(sessionData.validAfter)), + uint48(ValidUntil.unwrap(sessionData.validUntil)), + address(sessionKeyValidator), + address(0), + uint256(enableData.length), + enableData, + uint256(65), + r, + s, + v + ); + } + function _buildUserOp( Permission[] memory permissions, SessionData memory sessionData, @@ -110,8 +164,8 @@ contract SessionKeyValidatorTest is KernelECDSATest { 0, abi.encodeWithSelector( permissions[indexToUse].sig, - _generateParam(ParamCondition(indexToUse%6), !param1Faulty), // since EQ - _generateParam(ParamCondition((indexToUse+1)%6), !param2Faulty) // since NOT_EQ + _generateParam(ParamCondition(indexToUse % 6), !param1Faulty), // since EQ + _generateParam(ParamCondition((indexToUse + 1) % 6), !param2Faulty) // since NOT_EQ ), Operation.Call ) @@ -179,19 +233,32 @@ contract SessionKeyValidatorTest is KernelECDSATest { bool faultySig; bool param1Faulty; bool param2Faulty; + bool wrongProof; + } + + struct BatchTestConfig { + uint8 count; } - function test_scenario_non_batch( - TestConfig memory config - ) public { + + function test_scenario_batch(TestConfig memory config, BatchTestConfig memory batchConfig) public { vm.warp(1000); + if (batchConfig.count == 0) { + batchConfig.count = 1; + } + config.runs = 0; + config.interval = 0; + config.validAfter = 0; // TODO: runs not checked with batch 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); + vm.assume( + config.validAfter < type(uint32).max && config.interval < type(uint32).max && config.runs < type(uint32).max + ); 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; + bool shouldFail = (config.usingPaymasterMode < config.paymasterMode) || (1000 < config.validAfter) + || config.faultySig || config.param1Faulty || 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) { + if (config.interval == 0 || config.validAfter == 0) { config.earlyRun = 0; } if (config.interval > 0) { @@ -200,7 +267,11 @@ contract SessionKeyValidatorTest is KernelECDSATest { vm.assume(config.validAfter < block.timestamp); } // setup permissions - execRule = ExecutionRule({runs: config.runs, validAfter: ValidAfter.wrap(config.validAfter), interval: config.interval}); + execRule = ExecutionRule({ + runs: config.runs, + validAfter: ValidAfter.wrap(config.validAfter), + interval: config.interval + }); Permission[] memory permissions = _setup_permission(config.numberOfPermissions); _buildHashes(permissions); (uint128 lastNonce,) = sessionKeyValidator.nonces(address(kernel)); @@ -209,16 +280,29 @@ contract SessionKeyValidatorTest is KernelECDSATest { validAfter: ValidAfter.wrap(config.validAfter), validUntil: ValidUntil.wrap(0), paymaster: config.paymasterMode == 2 ? address(paymaster) : address(uint160(config.paymasterMode)), - nonce: uint256(lastNonce) + 1//lastNonce + 1 + nonce: uint256(lastNonce) + 1 //lastNonce + 1 }); // now encode data to op - UserOperation memory op = _buildUserOp(permissions, sessionData, config.indexToUse, config.usingPaymasterMode, config.param1Faulty, config.param2Faulty); + UserOperation memory op = _buildUserOpBatch( + permissions, + sessionData, + config.indexToUse, + config.usingPaymasterMode, + config.param1Faulty, + config.param2Faulty + ); + bytes32[][] memory proofs = new bytes32[][](batchConfig.count); + Permission[] memory usingPermission = new Permission[](batchConfig.count); + for (uint256 i = 0; i < batchConfig.count; i++) { + proofs[i] = _getProof(data, config.indexToUse, config.wrongProof); + usingPermission[i] = permissions[config.indexToUse]; + } op.signature = bytes.concat( op.signature, abi.encodePacked( sessionKey, entryPoint.signUserOpHash(vm, config.faultySig ? sessionKeyPriv + 1 : sessionKeyPriv, op), - abi.encode(permissions[config.indexToUse], _getProof(data, config.indexToUse)) + abi.encode(usingPermission, proofs) ) ); @@ -241,7 +325,114 @@ contract SessionKeyValidatorTest is KernelECDSATest { } if (!shouldFail && config.runs > 0) { for (uint256 i = 1; i < config.runs; i++) { - if(config.earlyRun != i) { + if (config.earlyRun != i) { + vm.warp(config.validAfter + config.interval * i); + } else { + vm.warp(config.validAfter + config.interval * i - 1); + } + op.nonce = op.nonce + 1; + op.signature = _getBatchActionSignature(op, permissions, config.indexToUse); + if (config.earlyRun == i) { + vm.expectRevert(); + } + entryPoint.handleOps(ops, beneficiary); + if (config.earlyRun == i) { + vm.warp(config.validAfter + config.interval * i); + entryPoint.handleOps(ops, beneficiary); + } + (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( + keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) + ); + if (config.validAfter > 0 && config.interval > 0) { + assertEq( + uint256(ValidAfter.unwrap(updatedValidAfter)), uint256(config.validAfter + config.interval * i) + ); + } + if (config.runs > 0) { + assertEq(uint256(r), uint256(i + 1)); + } + } + op.nonce = op.nonce + 1; + op.signature = _getBatchActionSignature(op, permissions, config.indexToUse); + vm.expectRevert(); + entryPoint.handleOps(ops, beneficiary); + } + } + + function test_scenario_non_batch(TestConfig memory config) public { + vm.warp(1000); + 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 + ); + 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.runs = config.runs % 10; + config.earlyRun = config.runs == 0 ? 0 : config.earlyRun % config.runs; + if (config.interval == 0 || config.validAfter == 0) { + config.earlyRun = 0; + } + if (config.interval > 0) { + vm.assume(config.validAfter > 0 && config.validAfter < block.timestamp); + } else { + vm.assume(config.validAfter < block.timestamp); + } + // setup permissions + execRule = ExecutionRule({ + runs: config.runs, + validAfter: ValidAfter.wrap(config.validAfter), + interval: config.interval + }); + Permission[] memory permissions = _setup_permission(config.numberOfPermissions); + _buildHashes(permissions); + (uint128 lastNonce,) = sessionKeyValidator.nonces(address(kernel)); + SessionData memory sessionData = SessionData({ + merkleRoot: _getRoot(data), + validAfter: ValidAfter.wrap(config.validAfter), + validUntil: ValidUntil.wrap(0), + paymaster: config.paymasterMode == 2 ? address(paymaster) : address(uint160(config.paymasterMode)), + nonce: uint256(lastNonce) + 1 //lastNonce + 1 + }); + // now encode data to op + UserOperation memory op = _buildUserOp( + permissions, + sessionData, + config.indexToUse, + config.usingPaymasterMode, + config.param1Faulty, + config.param2Faulty + ); + op.signature = bytes.concat( + op.signature, + abi.encodePacked( + sessionKey, + entryPoint.signUserOpHash(vm, config.faultySig ? sessionKeyPriv + 1 : sessionKeyPriv, op), + abi.encode(permissions[config.indexToUse], _getProof(data, config.indexToUse, config.wrongProof)) + ) + ); + + UserOperation[] memory ops = new UserOperation[](1); + ops[0] = op; + if (shouldFail) { + vm.expectRevert(); + } + entryPoint.handleOps(ops, beneficiary); + if (config.interval > 0 && config.validAfter > 0 && !shouldFail) { + (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( + keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) + ); + assertEq(uint256(ValidAfter.unwrap(updatedValidAfter)), uint256(config.validAfter)); + if (config.runs > 0) { + assertEq(uint256(r), uint256(1)); + } else { + assertEq(uint256(r), uint256(0)); + } + } + if (!shouldFail && config.runs > 0) { + for (uint256 i = 1; i < config.runs; i++) { + if (config.earlyRun != i) { vm.warp(config.validAfter + config.interval * i); } else { vm.warp(config.validAfter + config.interval * i - 1); @@ -260,9 +451,11 @@ contract SessionKeyValidatorTest is KernelECDSATest { keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) ); if (config.validAfter > 0 && config.interval > 0) { - assertEq(uint256(ValidAfter.unwrap(updatedValidAfter)), uint256(config.validAfter + config.interval * i)); + assertEq( + uint256(ValidAfter.unwrap(updatedValidAfter)), uint256(config.validAfter + config.interval * i) + ); } - if(config.runs > 0) { + if (config.runs > 0) { assertEq(uint256(r), uint256(i + 1)); } } @@ -273,13 +466,34 @@ contract SessionKeyValidatorTest is KernelECDSATest { } } - function _getSingleActionSignature(UserOperation memory _op, Permission[] memory permissions, uint8 indexToUse) internal view returns(bytes memory) { + function _getBatchActionSignature(UserOperation memory _op, Permission[] memory permissions, uint8 indexToUse) + internal + view + returns (bytes memory) + { + Permission[] memory _permissions = new Permission[](1); + _permissions[0] = permissions[indexToUse]; + bytes32[][] memory _proofs = new bytes32[][](1); + _proofs[0] = _getProof(data, indexToUse, false); + return abi.encodePacked( + bytes4(0x00000001), + abi.encodePacked( + sessionKey, entryPoint.signUserOpHash(vm, sessionKeyPriv, _op), abi.encode(_permissions, _proofs) + ) + ); + } + + function _getSingleActionSignature(UserOperation memory _op, Permission[] memory permissions, uint8 indexToUse) + internal + view + returns (bytes memory) + { return abi.encodePacked( bytes4(0x00000001), abi.encodePacked( sessionKey, entryPoint.signUserOpHash(vm, sessionKeyPriv, _op), - abi.encode(permissions[indexToUse], _getProof(data, indexToUse)) + abi.encode(permissions[indexToUse], _getProof(data, indexToUse, false)) ) ); } From e09c0e61750332eaeea6f115e5c80f5cd0036c07 Mon Sep 17 00:00:00 2001 From: David Eiber Date: Sat, 18 Nov 2023 04:41:15 -0500 Subject: [PATCH 02/10] Initial commit --- .gitmodules | 3 + foundry.toml | 3 +- lib/p256-verifier | 1 + nok.py | 48 +++++ pkc.py | 35 ++++ remappings.txt | 1 + signature.py | 43 +++++ src/validator/P256Validator.sol | 70 +++++++ test/foundry/validator/P256Validator.t.sol | 208 +++++++++++++++++++++ 9 files changed, 411 insertions(+), 1 deletion(-) create mode 160000 lib/p256-verifier create mode 100644 nok.py create mode 100644 pkc.py create mode 100644 signature.py create mode 100644 src/validator/P256Validator.sol create mode 100644 test/foundry/validator/P256Validator.t.sol diff --git a/.gitmodules b/.gitmodules index 744ac54b..467f62e8 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,3 +7,6 @@ [submodule "lib/I4337"] path = lib/I4337 url = https://github.com/leekt/I4337 +[submodule "lib/p256-verifier"] + path = lib/p256-verifier + url = https://github.com/daimo-eth/p256-verifier diff --git a/foundry.toml b/foundry.toml index 77dc9e49..7fe5e1d8 100644 --- a/foundry.toml +++ b/foundry.toml @@ -2,12 +2,13 @@ src = 'src' out = 'out' libs = ['lib'] -solc_version = '0.8.19' +solc_version = '0.8.21' bytecode_hash = "none" cbor_metadata = false optimize = true via-ir = true runs = 1000000 +ffi = true [fuzz] runs = 1024 diff --git a/lib/p256-verifier b/lib/p256-verifier new file mode 160000 index 00000000..fcaa173c --- /dev/null +++ b/lib/p256-verifier @@ -0,0 +1 @@ +Subproject commit fcaa173c36e6a0fd55370f74f949998356869b2c diff --git a/nok.py b/nok.py new file mode 100644 index 00000000..d13f1a43 --- /dev/null +++ b/nok.py @@ -0,0 +1,48 @@ +import os +import sys +import binascii +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.backends import default_backend +from cryptography.hazmat.primitives.asymmetric import utils + +def hex_to_bin(hex_string): + return binascii.unhexlify(hex_string) + +if len(sys.argv) != 3: + print("Not all arguments supplied. Please provide the hash to be signed and private key.") + sys.exit(1) + +hash_to_be_signed = sys.argv[1] +private_key = sys.argv[2] + +if not all(c in '0123456789abcdefABCDEF' for c in hash_to_be_signed) or not all(c in '0123456789abcdefABCDEF' for c in private_key): + print("Invalid input. The hash and private key must be valid hexadecimal strings.") + sys.exit(1) + +private_key = ec.derive_private_key(int(private_key, 16), ec.SECP256R1(), default_backend()) +public_key = private_key.public_key() +public_key_bytes = public_key.public_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PublicFormat.SubjectPublicKeyInfo +) + +public_key_hex = public_key_bytes.decode() +public_key_coordinates = public_key.public_numbers() +x = public_key_coordinates.x +y = public_key_coordinates.y +x_hex = hex(x) +y_hex = hex(y) + +# sign with secp256r1 +hash_to_be_signed_bytes = hex_to_bin(hash_to_be_signed) +# use prehash to avoid double hashing + +signature = private_key.sign(hash_to_be_signed_bytes, ec.ECDSA(utils.Prehashed(hashes.SHA256()))) +r, s = utils.decode_dss_signature(signature) +r_hex = hex(r) +s_hex = hex(s) + +print(f"Public Key Coordinates: x = {x_hex}, y = {y_hex}") +print(f"Signature: r = {r_hex}, s = {s_hex}") \ No newline at end of file diff --git a/pkc.py b/pkc.py new file mode 100644 index 00000000..5fefdcfb --- /dev/null +++ b/pkc.py @@ -0,0 +1,35 @@ +# pkc.py +import os +import sys +import binascii +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.hazmat.backends import default_backend +from eth_abi import encode + +def hex_to_bin(hex_string): + return binascii.unhexlify(hex_string) + +if len(sys.argv) != 2: + print("Not all arguments supplied. Please provide the private key.") + sys.exit(1) + +private_key = sys.argv[1] + +if not private_key.isdigit() or int(private_key) <= 0: + print("Invalid input. The private key must be a valid large positive number.") + sys.exit(1) + +if not private_key.isdigit(): + print("Invalid input. The private key must be a valid large number.") + sys.exit(1) + +private_key = ec.derive_private_key(int(private_key), ec.SECP256R1(), default_backend()) +public_key = private_key.public_key() +public_key_coordinates = public_key.public_numbers() +x = public_key_coordinates.x +y = public_key_coordinates.y + +encoded_coordinates = encode(['uint256', 'uint256'], [x, y]) +encoded_coordinates_hex = binascii.hexlify(encoded_coordinates).decode() +print(encoded_coordinates_hex) \ No newline at end of file diff --git a/remappings.txt b/remappings.txt index e55a454b..b28f7be1 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,3 +1,4 @@ ds-test/=lib/forge-std/lib/ds-test/src/ forge-std/=lib/forge-std/src/ solady/=lib/solady/src/ +p256-verifier/=lib/p256-verifier/src/ \ No newline at end of file diff --git a/signature.py b/signature.py new file mode 100644 index 00000000..24d26c83 --- /dev/null +++ b/signature.py @@ -0,0 +1,43 @@ +import os +import sys +import binascii +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.backends import default_backend +from cryptography.hazmat.primitives.asymmetric import utils +from eth_abi import encode + +def hex_to_bin(hex_string): + return binascii.unhexlify(hex_string) + +if len(sys.argv) != 3: + print("Not all arguments supplied. Please provide the hash to be signed and private key.") + sys.exit(1) + +hash_to_be_signed = sys.argv[1] +private_key = sys.argv[2] + +# Remove '0x' prefix if present +if hash_to_be_signed.startswith('0x'): + hash_to_be_signed = hash_to_be_signed[2:] + +if not all(c in '0123456789abcdefABCDEF' for c in hash_to_be_signed): + print("Invalid input. The hash must be a valid hexadecimal string.") + sys.exit(1) + +# Check if private key is a positive integer +if not private_key.isdigit() or int(private_key) <= 0: + print("Invalid input. The private key must be a positive integer.") + sys.exit(1) + +private_key = ec.derive_private_key(int(private_key), ec.SECP256R1(), default_backend()) +hash_to_be_signed_bytes = hex_to_bin(hash_to_be_signed) +signature = private_key.sign(hash_to_be_signed_bytes, ec.ECDSA(utils.Prehashed(hashes.SHA256()))) +r, s = utils.decode_dss_signature(signature) + +encoded_sig = encode(['uint256', 'uint256'], [r, s]) + +# print encoded_sig as hex string +encoded_sig_hex = binascii.b2a_hex(encoded_sig).decode() +print(encoded_sig_hex) \ No newline at end of file diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol new file mode 100644 index 00000000..5d50213c --- /dev/null +++ b/src/validator/P256Validator.sol @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import {UserOperation} from "I4337/interfaces/UserOperation.sol"; +import {ECDSA} from "solady/utils/ECDSA.sol"; +import {IKernelValidator} from "../interfaces/IKernelValidator.sol"; +import {ValidationData} from "../common/Types.sol"; +import {SIG_VALIDATION_FAILED} from "../common/Constants.sol"; +import {P256} from "p256-verifier/P256.sol"; + +contract P256Validator is IKernelValidator { + uint256 constant n = + 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551; + + event P256PublicKeysChanged(address indexed kernel, PublicKey indexed oldKeys, PublicKey indexed newKeys); + + struct PublicKey { + uint256 x; + uint256 y; + } + + error BadKey(); + + mapping(address => PublicKey) public p256PublicKey; + + function enable(bytes calldata _data) external payable override { + PublicKey memory key = abi.decode(_data, (PublicKey)); + //throw custom error if key[0] or key[1] is 0, or if key[0] or key[1] is greater than n + if (key.x == 0 || key.y == 0 || key.x > n || key.y > n) { + revert BadKey(); + } + PublicKey memory oldKey = p256PublicKey[msg.sender]; + p256PublicKey[msg.sender] = key; + emit P256PublicKeysChanged(msg.sender, oldKey, key); + } + + 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) { + (uint256 r, uint256 s) = abi.decode(_userOp.signature, (uint256, uint256)); + PublicKey memory key = p256PublicKey[_userOp.sender]; + bytes32 hash = ECDSA.toEthSignedMessageHash(_userOpHash); + if (P256.verifySignatureAllowMalleability(hash, r, s, key.x, key.y)) { + return ValidationData.wrap(0); + } + if (!P256.verifySignatureAllowMalleability(_userOpHash, r, s, key.x, key.y)) { + return SIG_VALIDATION_FAILED; + } + } + + 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.verifySignatureAllowMalleability(hash, r, s, key.x, key.y)) { + return ValidationData.wrap(0); + } + bytes32 ethHash = ECDSA.toEthSignedMessageHash(hash); + if (!P256.verifySignatureAllowMalleability(ethHash, r, s, key.x, key.y)) { + return SIG_VALIDATION_FAILED; + } + return ValidationData.wrap(0); + } + + function validCaller(address _caller, bytes calldata) external view override returns (bool) { + revert NotImplemented(); + } +} diff --git a/test/foundry/validator/P256Validator.t.sol b/test/foundry/validator/P256Validator.t.sol new file mode 100644 index 00000000..73efee63 --- /dev/null +++ b/test/foundry/validator/P256Validator.t.sol @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import {IEntryPoint} from "I4337/interfaces/IEntryPoint.sol"; +import "src/Kernel.sol"; +import "src/lite/KernelLiteECDSA.sol"; +// test artifacts +// test utils +import "forge-std/Test.sol"; +import {ENTRYPOINT_0_6_ADDRESS, ENTRYPOINT_0_6_BYTECODE} from "I4337/artifacts/EntryPoint_0_6.sol"; +import {CREATOR_0_6_BYTECODE, CREATOR_0_6_ADDRESS} from "I4337/artifacts/EntryPoint_0_6.sol"; +import {ERC4337Utils} from "../utils/ERC4337Utils.sol"; +import {KernelTestBase} from "../KernelTestBase.sol"; +import {KernelFactory} from "src/factory/KernelFactory.sol"; +import {TestExecutor} from "../mock/TestExecutor.sol"; +import {TestValidator} from "../mock/TestValidator.sol"; +import {P256Validator} from "src/validator/P256Validator.sol"; +import {P256Verifier} from "p256-verifier/P256Verifier.sol"; +import {P256} from "p256-verifier/P256.sol"; +import {LibString} from "solady/utils/LibString.sol"; + + +using ERC4337Utils for IEntryPoint; +using LibString for uint256; + +// contract P256ValidatorTest is KernelTestBase { + + +///make sure R and S aren't 0. Sidechain attacks. +contract P256ValidatorTest is Test { + event UserOperationEvent( + bytes32 indexed userOpHash, + address indexed sender, + address indexed paymaster, + uint256 nonce, + bool success, + uint256 actualGasCost, + uint256 actualGasUsed + ); + + P256Validator p256Validator; + uint256 x; + uint256 y; + + P256Verifier p256Verifier; + + Kernel kernel; + Kernel kernelImpl; + KernelFactory factory; + IEntryPoint entryPoint; + IKernelValidator defaultValidator; + address owner; + uint256 ownerKey; + address payable beneficiary; + address factoryOwner; + + bytes4 executionSig; + ExecutionDetail executionDetail; + + function setUp() public { + p256Validator = new P256Validator(); + + p256Verifier = new P256Verifier(); + vm.etch(0xc2b78104907F722DABAc4C69f826a522B2754De4, address(p256Verifier).code); + + _initialize(); + + + + (x, y) = generatePublicKey(ownerKey); + + + + _setAddress(); + + + + (uint256 x2, uint256 y2) = p256Validator.p256PublicKey(address(kernel)); + assertEq(x, x2); + assertEq(y, y2); + } + + function _initialize() internal { + (owner, ownerKey) = makeAddrAndKey("owner"); + (factoryOwner,) = makeAddrAndKey("factoryOwner"); + beneficiary = payable(address(makeAddr("beneficiary"))); + vm.etch(ENTRYPOINT_0_6_ADDRESS, ENTRYPOINT_0_6_BYTECODE); + entryPoint = IEntryPoint(payable(ENTRYPOINT_0_6_ADDRESS)); + vm.etch(CREATOR_0_6_ADDRESS, CREATOR_0_6_BYTECODE); + kernelImpl = new Kernel(entryPoint); + factory = new KernelFactory(factoryOwner, entryPoint); + vm.startPrank(factoryOwner); + factory.setImplementation(address(kernelImpl), true); + vm.stopPrank(); + } + + function _setAddress() internal { + kernel = Kernel(payable(address(factory.createAccount(address(kernelImpl), getInitializeData(), 0)))); + vm.deal(address(kernel), 1e30); + } + + function generatePublicKey(uint256 privateKey) internal returns (uint256, uint256) { + string[] memory inputs = new string[](3); + inputs[0] = "python"; + inputs[1] = "pkc.py"; + inputs[2] = privateKey.toString(); + bytes memory output = vm.ffi(inputs); + + return abi.decode(output, (uint256, uint256)); + } + + function generateSignature(uint256 privateKey, bytes32 hash) internal returns (uint256, uint256) { + string[] memory inputs = new string[](4); + inputs[0] = "python"; + inputs[1] = "signature.py"; + inputs[2] = uint256(hash).toHexString(32); + inputs[3] = privateKey.toString(); + bytes memory output = vm.ffi(inputs); + + return abi.decode(output, (uint256, uint256)); + } + + function test_utils(uint256 privateKey, bytes32 hash) external { + // vm.assume(uint256(privateKey).toHexString().length == 64); + vm.assume(hash != 0); + vm.assume(privateKey != 0); + (uint256 x, uint256 y) = generatePublicKeys(privateKey); + (uint256 r, uint256 s) = generateSignature(privateKey, hash); + + vm.assume(x != 0); + vm.assume(y != 0); + vm.assume(r != 0); + vm.assume(s != 0); + assertEq(P256.verifySignatureAllowMalleability(hash, r, s, x, y), true); + } + + function test_validate_signature() external { + 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 + ) + ); + + + (uint256 x, uint256 y) = generatePublicKey(ownerKey); + (uint256 r, uint256 s) = generateSignature(ownerKey, digest); + + assertEq(kernel.isValidSignature(hash, abi.encode(r, s)), Kernel.isValidSignature.selector); + assertEq(kernel2.isValidSignature(hash, abi.encode(r, s)), bytes4(0xffffffff)); + } + + function test_sudo() external { + UserOperation memory op = + entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); + op.signature = signUserOp(op); + UserOperation[] memory ops = new UserOperation[](1); + ops[0] = op; + entryPoint.handleOps(ops, beneficiary); + } + + function getInitializeData() internal view returns (bytes memory) { + return abi.encodeWithSelector( + KernelStorage.initialize.selector, + p256Validator, + abi.encode(x, y) + ); + } + + function test_set_default_validator() external { + TestValidator newValidator = new TestValidator(); + bytes memory empty; + UserOperation memory op = entryPoint.fillUserOp( + address(kernel), + abi.encodeWithSelector(KernelStorage.setDefaultValidator.selector, address(newValidator), empty) + ); + op.signature = signUserOp(op); + UserOperation[] memory ops = new UserOperation[](1); + ops[0] = op; + vm.expectEmit(true, true, true, false, address(entryPoint)); + emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0); + entryPoint.handleOps(ops, beneficiary); + } + + function signUserOp(UserOperation memory op) internal returns (bytes memory) { + bytes32 hash = entryPoint.getUserOpHash(op); + (uint256 r, uint256 s) = generateSignature(ownerKey, hash); + return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); + } + + function getWrongSignature(UserOperation memory op) internal returns (bytes memory) { + bytes32 hash = entryPoint.getUserOpHash(op); + (uint256 r, uint256 s) = generateSignature(ownerKey + 1, hash); + return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); + } + + function signHash(bytes32 hash) internal returns (bytes memory) { + (uint256 r, uint256 s) = generateSignature(ownerKey, ECDSA.toEthSignedMessageHash(hash)); + return abi.encode(r, s); + } + + function getWrongSignature(bytes32 hash) internal returns (bytes memory) { + (uint256 r, uint256 s) = generateSignature(ownerKey + 1, ECDSA.toEthSignedMessageHash(hash)); + return abi.encode(r, s); + } +} From 6a410d1e338bf69779f009dceb1944ac4cebaacf Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 10:00:57 -0500 Subject: [PATCH 03/10] Update P256Validator.t.sol --- test/foundry/validator/P256Validator.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/foundry/validator/P256Validator.t.sol b/test/foundry/validator/P256Validator.t.sol index 73efee63..d83840c6 100644 --- a/test/foundry/validator/P256Validator.t.sol +++ b/test/foundry/validator/P256Validator.t.sol @@ -124,7 +124,7 @@ contract P256ValidatorTest is Test { // vm.assume(uint256(privateKey).toHexString().length == 64); vm.assume(hash != 0); vm.assume(privateKey != 0); - (uint256 x, uint256 y) = generatePublicKeys(privateKey); + (uint256 x, uint256 y) = generatePublicKey(privateKey); (uint256 r, uint256 s) = generateSignature(privateKey, hash); vm.assume(x != 0); From f18737d280affae26896ac7a9686c4e9fa8a27fe Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 10:01:14 -0500 Subject: [PATCH 04/10] forge install: FreshCryptoLib --- .gitmodules | 3 +++ lib/FreshCryptoLib | 1 + 2 files changed, 4 insertions(+) create mode 160000 lib/FreshCryptoLib diff --git a/.gitmodules b/.gitmodules index 467f62e8..d2bb2da1 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,3 +10,6 @@ [submodule "lib/p256-verifier"] path = lib/p256-verifier url = https://github.com/daimo-eth/p256-verifier +[submodule "lib/FreshCryptoLib"] + path = lib/FreshCryptoLib + url = https://github.com/rdubois-crypto/FreshCryptoLib diff --git a/lib/FreshCryptoLib b/lib/FreshCryptoLib new file mode 160000 index 00000000..e2830cb5 --- /dev/null +++ b/lib/FreshCryptoLib @@ -0,0 +1 @@ +Subproject commit e2830cb5d7b0f6ae35b5800287c0f5c92388070b From b69fda345486de6f81d55370ae5d2662c246c3e5 Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 13:12:03 -0500 Subject: [PATCH 05/10] Refactor with helper functions --- test/foundry/KernelECDSA.t.sol | 16 +-- test/foundry/KernelLiteECDSA.t.sol | 17 +-- test/foundry/KernelTestBase.sol | 127 +++++++----------- .../validator/KillSwitchValidator.t.sol | 18 +-- .../validator/SessionKeyValidator.t.sol | 25 ++-- 5 files changed, 73 insertions(+), 130 deletions(-) diff --git a/test/foundry/KernelECDSA.t.sol b/test/foundry/KernelECDSA.t.sol index b22c2c4c..8d68abce 100644 --- a/test/foundry/KernelECDSA.t.sol +++ b/test/foundry/KernelECDSA.t.sol @@ -68,8 +68,7 @@ contract KernelECDSATest is KernelTestBase { } function test_default_validator_enable() external override { - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.execute.selector, address(defaultValidator), @@ -78,17 +77,13 @@ contract KernelECDSATest is KernelTestBase { Operation.Call ) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); (address owner) = ECDSAValidator(address(defaultValidator)).ecdsaValidatorStorage(address(kernel)); assertEq(owner, address(0xdeadbeef), "owner should be 0xdeadbeef"); } function test_default_validator_disable() external override { - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.execute.selector, address(defaultValidator), @@ -97,10 +92,7 @@ contract KernelECDSATest is KernelTestBase { Operation.Call ) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); (address owner) = ECDSAValidator(address(defaultValidator)).ecdsaValidatorStorage(address(kernel)); assertEq(owner, address(0), "owner should be 0"); } diff --git a/test/foundry/KernelLiteECDSA.t.sol b/test/foundry/KernelLiteECDSA.t.sol index e18f21ff..3496986c 100644 --- a/test/foundry/KernelLiteECDSA.t.sol +++ b/test/foundry/KernelLiteECDSA.t.sol @@ -57,16 +57,12 @@ contract KernelECDSATest is KernelTestBase { function test_set_default_validator() external override { TestValidator newValidator = new TestValidator(); bytes memory empty; - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector(KernelStorage.setDefaultValidator.selector, address(newValidator), empty) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; vm.expectEmit(true, true, true, false, address(entryPoint)); emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0); - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); } function signUserOp(UserOperation memory op) internal view override returns (bytes memory) { @@ -101,14 +97,9 @@ contract KernelECDSATest is KernelTestBase { function test_transfer_ownership() external { address newOwner = makeAddr("new owner"); - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), abi.encodeWithSelector(KernelLiteECDSA.transferOwnership.selector, newOwner) - ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; + 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); - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); } } diff --git a/test/foundry/KernelTestBase.sol b/test/foundry/KernelTestBase.sol index b4501cdd..1951cba4 100644 --- a/test/foundry/KernelTestBase.sol +++ b/test/foundry/KernelTestBase.sol @@ -90,7 +90,7 @@ abstract contract KernelTestBase is Test { function test_default_validator_disable() external virtual; - function test_external_call_execute_success() external { + function test_external_call_execute_success() external virtual { address[] memory validCallers = getOwners(); for (uint256 i = 0; i < validCallers.length; i++) { vm.prank(validCallers[i]); @@ -98,7 +98,7 @@ abstract contract KernelTestBase is Test { } } - function test_external_call_execute_delegatecall_success() external { + function test_external_call_execute_delegatecall_success() external virtual { address[] memory validCallers = getOwners(); for (uint256 i = 0; i < validCallers.length; i++) { vm.prank(validCallers[i]); @@ -106,7 +106,7 @@ abstract contract KernelTestBase is Test { } } - function test_external_call_execute_delegatecall_fail() external { + function test_external_call_execute_delegatecall_fail() external virtual { address[] memory validCallers = getOwners(); for (uint256 i = 0; i < validCallers.length; i++) { vm.prank(address(uint160(validCallers[i]) + 1)); @@ -133,7 +133,7 @@ abstract contract KernelTestBase is Test { } } - function test_external_call_batch_execute_success() external { + function test_external_call_batch_execute_success() external virtual { Call[] memory calls = new Call[](1); calls[0] = Call(owner, 0, ""); vm.prank(owner); @@ -174,7 +174,7 @@ abstract contract KernelTestBase is Test { kernel.upgradeTo(address(0xdeadbeef)); } - function test_external_call_default() external { + function test_external_call_default() external virtual { vm.startPrank(owner); (bool success,) = address(kernel).call(abi.encodePacked("Hello world")); assertEq(success, true); @@ -194,7 +194,7 @@ abstract contract KernelTestBase is Test { assertEq(proxy, address(kernel)); } - function test_validate_signature() external { + function test_validate_signature() external virtual { Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3))); bytes32 hash = keccak256(abi.encodePacked("hello world")); bytes32 digest = keccak256( @@ -207,7 +207,7 @@ abstract contract KernelTestBase is Test { assertEq(kernel2.isValidSignature(hash, sig), bytes4(0xffffffff)); } - function test_fail_validate_wrongsignature() external { + function test_fail_validate_wrongsignature() external virtual { bytes32 hash = keccak256(abi.encodePacked("hello world")); bytes memory sig = getWrongSignature(hash); assertEq(kernel.isValidSignature(hash, sig), bytes4(0xffffffff)); @@ -262,35 +262,25 @@ abstract contract KernelTestBase is Test { function test_set_default_validator() external virtual { TestValidator newDefaultValidator = new TestValidator(); bytes memory empty; - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector(IKernel.setDefaultValidator.selector, address(newDefaultValidator), empty) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); assertEq(address(IKernel(address(kernel)).getDefaultValidator()), address(newDefaultValidator)); } function test_disable_mode() external { vm.warp(1000); bytes memory empty; - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) - ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + 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); } function test_set_execution() external { TestValidator newValidator = new TestValidator(); - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.setExecution.selector, bytes4(0xdeadbeef), @@ -301,10 +291,7 @@ abstract contract KernelTestBase is Test { bytes("") ) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); ExecutionDetail memory execution = IKernel(address(kernel)).getExecution(bytes4(0xdeadbeef)); assertEq(execution.executor, address(0xdead)); assertEq(address(execution.validator), address(newValidator)); @@ -312,10 +299,9 @@ abstract contract KernelTestBase is Test { assertEq(uint256(ValidAfter.unwrap(execution.validAfter)), uint256(0)); } - function test_external_call_execution() external { + function test_external_call_execution() external virtual { TestValidator newValidator = new TestValidator(); - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.setExecution.selector, bytes4(0xdeadbeef), @@ -326,10 +312,7 @@ abstract contract KernelTestBase is Test { bytes("") ) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); ExecutionDetail memory execution = IKernel(address(kernel)).getExecution(bytes4(0xdeadbeef)); assertEq(execution.executor, address(0xdead)); assertEq(address(execution.validator), address(newValidator)); @@ -353,39 +336,28 @@ abstract contract KernelTestBase is Test { function test_revert_when_mode_disabled() external { vm.warp(1000); bytes memory empty; - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) + UserOperation memory op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001), address(0), empty) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperationWithSig(op); // try to run with mode 0x00000001 - op = entryPoint.fillUserOp( - address(kernel), abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001)) - ); + op = buildUserOperation(abi.encodeWithSelector(IKernel.disableMode.selector, bytes4(0x00000001))); op.signature = abi.encodePacked(bytes4(0x00000001), entryPoint.signUserOpHash(vm, ownerKey, op)); - ops[0] = op; - vm.expectRevert( abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, string.concat("AA23 reverted (or OOG)")) ); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } // validate user op function test_validateUserOp_fail_not_entryPoint() external { - UserOperation memory op = - entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); - op.signature = signUserOp(op); + UserOperation memory op = buildUserOperation(abi.encodeWithSelector(TestExecutor.doNothing.selector)); vm.expectRevert(IKernel.NotEntryPoint.selector); kernel.validateUserOp(op, bytes32(hex"deadbeef"), uint256(100)); } function test_validateUserOp_fail_invalid_mode() external { - UserOperation memory op = - entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); + UserOperation memory op = buildUserOperation(abi.encodeWithSelector(TestExecutor.doNothing.selector)); op.signature = hex"00000003"; vm.prank(address(entryPoint)); ValidationData res = kernel.validateUserOp(op, bytes32(hex"deadbeef"), uint256(100)); @@ -393,36 +365,27 @@ abstract contract KernelTestBase is Test { } function test_sudo() external { - UserOperation memory op = - entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + UserOperation memory op = buildUserOperation(abi.encodeWithSelector(TestExecutor.doNothing.selector)); + performUserOperationWithSig(op); } function test_sudo_wrongSig() external { - UserOperation memory op = - entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); + UserOperation memory op = buildUserOperation(abi.encodeWithSelector(TestExecutor.doNothing.selector)); op.signature = getWrongSignature(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; vm.expectRevert(); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } // mode 2 tests function test_mode_2() public { - UserOperation memory op = entryPoint.fillUserOp(address(kernel), abi.encodePacked(executionSig)); + UserOperation memory op = buildUserOperation(abi.encodePacked(executionSig)); op.signature = buildEnableSignature( op, executionSig, uint48(0), uint48(0), executionDetail.validator, executionDetail.executor ); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; vm.expectEmit(true, true, true, false, address(entryPoint)); emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } function buildEnableSignature( @@ -453,8 +416,7 @@ abstract contract KernelTestBase is Test { } function test_enable_then_mode_1() public { - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), + UserOperation memory op = buildUserOperation( abi.encodeWithSelector( IKernel.setExecution.selector, executionSig, @@ -465,18 +427,10 @@ abstract contract KernelTestBase is Test { getEnableData() ) ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - - entryPoint.handleOps(ops, beneficiary); - // vm.expectEmit(true, false, false, false); - // emit TestValidator.TestValidateUserOp(opHash); - op = entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(executionSig)); - // registered + performUserOperationWithSig(op); + op = buildUserOperation(abi.encodeWithSelector(executionSig)); op.signature = abi.encodePacked(bytes4(0x00000001), getValidatorSignature(op)); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } function _setAddress() internal { @@ -501,4 +455,21 @@ abstract contract KernelTestBase is Test { ) ); } + + function buildUserOperation(bytes memory callData) internal view returns (UserOperation memory op) { + return entryPoint.fillUserOp(address(kernel), callData); + } + + function performUserOperationWithSig(UserOperation memory op) internal { + op.signature = signUserOp(op); + UserOperation[] memory ops = new UserOperation[](1); + ops[0] = op; + entryPoint.handleOps(ops, beneficiary); + } + + function performUserOperation(UserOperation memory op) internal { + UserOperation[] memory ops = new UserOperation[](1); + ops[0] = op; + entryPoint.handleOps(ops, beneficiary); + } } diff --git a/test/foundry/validator/KillSwitchValidator.t.sol b/test/foundry/validator/KillSwitchValidator.t.sol index 47d12f11..6b080d64 100644 --- a/test/foundry/validator/KillSwitchValidator.t.sol +++ b/test/foundry/validator/KillSwitchValidator.t.sol @@ -68,16 +68,12 @@ contract KillSwitchValidatorTest is KernelECDSATest { } function test_force_unblock() external { - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), 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)); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); - op = entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(KillSwitchAction.toggleKillSwitch.selector)); + op = buildUserOperation(abi.encodeWithSelector(KillSwitchAction.toggleKillSwitch.selector)); bytes memory enableData = abi.encodePacked(guardian); { bytes32 digest = getTypedDataHash( @@ -110,15 +106,13 @@ contract KillSwitchValidatorTest is KernelECDSATest { op.signature = bytes.concat(op.signature, bytes6(uint48(pausedUntil)), sig); } - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); assertEq(kernel.getDisabledMode(), bytes4(0xffffffff)); assertEq(address(kernel.getDefaultValidator()), address(killSwitch)); - op = entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(KillSwitchAction.toggleKillSwitch.selector)); + op = buildUserOperation(abi.encodeWithSelector(KillSwitchAction.toggleKillSwitch.selector)); op.signature = bytes.concat(bytes4(0), entryPoint.signUserOpHash(vm, guardianKey, op)); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); assertEq(kernel.getDisabledMode(), bytes4(0)); } } diff --git a/test/foundry/validator/SessionKeyValidator.t.sol b/test/foundry/validator/SessionKeyValidator.t.sol index bf831ec2..3c7927c5 100644 --- a/test/foundry/validator/SessionKeyValidator.t.sol +++ b/test/foundry/validator/SessionKeyValidator.t.sol @@ -107,7 +107,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { ) }); - op = entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(IKernel.executeBatch.selector, calls)); + op = buildUserOperation(abi.encodeWithSelector(IKernel.executeBatch.selector, calls)); if (usingPaymasterMode != 0) { // 0 = no paymaster // 1 = unknown paymaster @@ -156,8 +156,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { bool param1Faulty, bool param2Faulty ) internal view returns (UserOperation memory op) { - op = entryPoint.fillUserOp( - address(kernel), + op = buildUserOperation( abi.encodeWithSelector( IKernel.execute.selector, permissions[indexToUse].target, @@ -306,12 +305,10 @@ contract SessionKeyValidatorTest is KernelECDSATest { ) ); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; if (shouldFail) { vm.expectRevert(); } - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); if (config.interval > 0 && config.validAfter > 0 && !shouldFail) { (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) @@ -335,10 +332,10 @@ contract SessionKeyValidatorTest is KernelECDSATest { if (config.earlyRun == i) { vm.expectRevert(); } - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); if (config.earlyRun == i) { vm.warp(config.validAfter + config.interval * i); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) @@ -355,7 +352,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { op.nonce = op.nonce + 1; op.signature = _getBatchActionSignature(op, permissions, config.indexToUse); vm.expectRevert(); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } } @@ -413,12 +410,10 @@ contract SessionKeyValidatorTest is KernelECDSATest { ) ); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; if (shouldFail) { vm.expectRevert(); } - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); if (config.interval > 0 && config.validAfter > 0 && !shouldFail) { (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) @@ -442,10 +437,10 @@ contract SessionKeyValidatorTest is KernelECDSATest { if (config.earlyRun == i) { vm.expectRevert(); } - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); if (config.earlyRun == i) { vm.warp(config.validAfter + config.interval * i); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } (ValidAfter updatedValidAfter, uint48 r) = sessionKeyValidator.executionStatus( keccak256(abi.encodePacked(sessionData.nonce, uint32(config.indexToUse))), address(kernel) @@ -462,7 +457,7 @@ contract SessionKeyValidatorTest is KernelECDSATest { op.nonce = op.nonce + 1; op.signature = _getSingleActionSignature(op, permissions, config.indexToUse); vm.expectRevert(); - entryPoint.handleOps(ops, beneficiary); + performUserOperation(op); } } From 871b07800e9de0c5a03e71911c5f2c2517079686 Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 13:12:19 -0500 Subject: [PATCH 06/10] Add P256Validator, tests, deps --- remappings.txt | 3 +- src/validator/P256Validator.sol | 6 +- test/foundry/validator/P256Validator.t.sol | 210 ++++++++++----------- 3 files changed, 99 insertions(+), 120 deletions(-) diff --git a/remappings.txt b/remappings.txt index b28f7be1..82c83131 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,4 +1,5 @@ ds-test/=lib/forge-std/lib/ds-test/src/ forge-std/=lib/forge-std/src/ solady/=lib/solady/src/ -p256-verifier/=lib/p256-verifier/src/ \ No newline at end of file +p256-verifier/=lib/p256-verifier/src/ +FreshCryptoLib/=lib/FreshCryptoLib/solidity/src/ \ No newline at end of file diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol index 5d50213c..b163ab7a 100644 --- a/src/validator/P256Validator.sol +++ b/src/validator/P256Validator.sol @@ -10,9 +10,6 @@ import {SIG_VALIDATION_FAILED} from "../common/Constants.sol"; import {P256} from "p256-verifier/P256.sol"; contract P256Validator is IKernelValidator { - uint256 constant n = - 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551; - event P256PublicKeysChanged(address indexed kernel, PublicKey indexed oldKeys, PublicKey indexed newKeys); struct PublicKey { @@ -26,8 +23,7 @@ contract P256Validator is IKernelValidator { function enable(bytes calldata _data) external payable override { PublicKey memory key = abi.decode(_data, (PublicKey)); - //throw custom error if key[0] or key[1] is 0, or if key[0] or key[1] is greater than n - if (key.x == 0 || key.y == 0 || key.x > n || key.y > n) { + if (key.x == 0 || key.y == 0) { revert BadKey(); } PublicKey memory oldKey = p256PublicKey[msg.sender]; diff --git a/test/foundry/validator/P256Validator.t.sol b/test/foundry/validator/P256Validator.t.sol index d83840c6..fbb13991 100644 --- a/test/foundry/validator/P256Validator.t.sol +++ b/test/foundry/validator/P256Validator.t.sol @@ -3,138 +3,143 @@ pragma solidity ^0.8.0; import {IEntryPoint} from "I4337/interfaces/IEntryPoint.sol"; import "src/Kernel.sol"; -import "src/lite/KernelLiteECDSA.sol"; // test artifacts // test utils import "forge-std/Test.sol"; -import {ENTRYPOINT_0_6_ADDRESS, ENTRYPOINT_0_6_BYTECODE} from "I4337/artifacts/EntryPoint_0_6.sol"; -import {CREATOR_0_6_BYTECODE, CREATOR_0_6_ADDRESS} from "I4337/artifacts/EntryPoint_0_6.sol"; import {ERC4337Utils} from "../utils/ERC4337Utils.sol"; import {KernelTestBase} from "../KernelTestBase.sol"; -import {KernelFactory} from "src/factory/KernelFactory.sol"; import {TestExecutor} from "../mock/TestExecutor.sol"; import {TestValidator} from "../mock/TestValidator.sol"; import {P256Validator} from "src/validator/P256Validator.sol"; import {P256Verifier} from "p256-verifier/P256Verifier.sol"; import {P256} from "p256-verifier/P256.sol"; -import {LibString} from "solady/utils/LibString.sol"; +import {FCL_ecdsa_utils} from "FreshCryptoLib/FCL_ecdsa_utils.sol"; +import {IKernel} from "src/interfaces/IKernel.sol"; using ERC4337Utils for IEntryPoint; -using LibString for uint256; - -// contract P256ValidatorTest is KernelTestBase { - - -///make sure R and S aren't 0. Sidechain attacks. -contract P256ValidatorTest is Test { - event UserOperationEvent( - bytes32 indexed userOpHash, - address indexed sender, - address indexed paymaster, - uint256 nonce, - bool success, - uint256 actualGasCost, - uint256 actualGasUsed - ); +contract P256ValidatorTest is KernelTestBase { + P256Verifier p256Verifier; P256Validator p256Validator; uint256 x; uint256 y; - P256Verifier p256Verifier; - - Kernel kernel; - Kernel kernelImpl; - KernelFactory factory; - IEntryPoint entryPoint; - IKernelValidator defaultValidator; - address owner; - uint256 ownerKey; - address payable beneficiary; - address factoryOwner; - - bytes4 executionSig; - ExecutionDetail executionDetail; - function setUp() public { p256Validator = new P256Validator(); - p256Verifier = new P256Verifier(); + vm.etch(0xc2b78104907F722DABAc4C69f826a522B2754De4, address(p256Verifier).code); _initialize(); + (x, y) = generatePublicKey(ownerKey); + _setAddress(); + _setExecutionDetail(); + } - + function _setExecutionDetail() internal virtual override { + executionDetail.executor = address(new TestExecutor()); + executionSig = TestExecutor.doNothing.selector; + executionDetail.validator = new TestValidator(); + } - (x, y) = generatePublicKey(ownerKey); + function getValidatorSignature(UserOperation memory _op) internal view virtual override returns (bytes memory) { + bytes32 hash = entryPoint.getUserOpHash(_op); + (uint256 r, uint256 s) = generateSignature(ownerKey, ECDSA.toEthSignedMessageHash(hash)); + return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); + } + function getOwners() internal virtual override returns (address[] memory _owners){ + _owners = new address[](1); + _owners[0] = address(0); + return _owners; + } + function getEnableData() internal view virtual override returns (bytes memory) { + return ""; + } - _setAddress(); - + function getInitializeData() internal view override returns (bytes memory) { + return abi.encodeWithSelector(KernelStorage.initialize.selector, p256Validator, abi.encode(x, y)); + } + function test_default_validator_enable() external override{ + UserOperation memory op = buildUserOperation( + abi.encodeWithSelector( + IKernel.execute.selector, + address(p256Validator), + 0, + abi.encodeWithSelector(P256Validator.enable.selector, abi.encode(x, y)), + Operation.Call + ) + ); + performUserOperationWithSig(op); + (uint256 x2, uint256 y2) = P256Validator(address(p256Validator)).p256PublicKey(address(kernel)); + verifyPublicKey(x2, y2, x, y); + } - (uint256 x2, uint256 y2) = p256Validator.p256PublicKey(address(kernel)); - assertEq(x, x2); - assertEq(y, y2); + function test_default_validator_disable() external override { + UserOperation memory op = buildUserOperation( + abi.encodeWithSelector( + IKernel.execute.selector, + address(p256Validator), + 0, + abi.encodeWithSelector(P256Validator.disable.selector, ""), + Operation.Call + ) + ); + performUserOperationWithSig(op); + (uint256 x2, uint256 y2) = P256Validator(address(p256Validator)).p256PublicKey(address(kernel)); + verifyPublicKey(x2, y2, 0, 0); } - function _initialize() internal { - (owner, ownerKey) = makeAddrAndKey("owner"); - (factoryOwner,) = makeAddrAndKey("factoryOwner"); - beneficiary = payable(address(makeAddr("beneficiary"))); - vm.etch(ENTRYPOINT_0_6_ADDRESS, ENTRYPOINT_0_6_BYTECODE); - entryPoint = IEntryPoint(payable(ENTRYPOINT_0_6_ADDRESS)); - vm.etch(CREATOR_0_6_ADDRESS, CREATOR_0_6_BYTECODE); - kernelImpl = new Kernel(entryPoint); - factory = new KernelFactory(factoryOwner, entryPoint); - vm.startPrank(factoryOwner); - factory.setImplementation(address(kernelImpl), true); - vm.stopPrank(); + function test_external_call_batch_execute_success() external override { + vm.skip(true); } - function _setAddress() internal { - kernel = Kernel(payable(address(factory.createAccount(address(kernelImpl), getInitializeData(), 0)))); - vm.deal(address(kernel), 1e30); + function test_external_call_execute_success() external override { + vm.skip(true); + } + + function test_external_call_execute_delegatecall_success() external override { + vm.skip(true); } - function generatePublicKey(uint256 privateKey) internal returns (uint256, uint256) { - string[] memory inputs = new string[](3); - inputs[0] = "python"; - inputs[1] = "pkc.py"; - inputs[2] = privateKey.toString(); - bytes memory output = vm.ffi(inputs); + function test_external_call_execute_delegatecall_fail() external override { + vm.skip(true); + } - return abi.decode(output, (uint256, uint256)); + function test_external_call_default() external override { + vm.skip(true); } - function generateSignature(uint256 privateKey, bytes32 hash) internal returns (uint256, uint256) { - string[] memory inputs = new string[](4); - inputs[0] = "python"; - inputs[1] = "signature.py"; - inputs[2] = uint256(hash).toHexString(32); - inputs[3] = privateKey.toString(); - bytes memory output = vm.ffi(inputs); + function test_external_call_execution() external override { + vm.skip(true); + } - return abi.decode(output, (uint256, uint256)); + function generatePublicKey(uint256 privateKey) internal view returns (uint256, uint256) { + return FCL_ecdsa_utils.ecdsa_derivKpub(privateKey); + } + + function generateSignature(uint256 privateKey, bytes32 hash) internal view returns (uint256, uint256) { + uint256 k = type(uint256).max - 1; + return FCL_ecdsa_utils.ecdsa_sign(hash, k, privateKey); } function test_utils(uint256 privateKey, bytes32 hash) external { - // vm.assume(uint256(privateKey).toHexString().length == 64); vm.assume(hash != 0); vm.assume(privateKey != 0); - (uint256 x, uint256 y) = generatePublicKey(privateKey); + (uint256 x1, uint256 y1) = generatePublicKey(privateKey); (uint256 r, uint256 s) = generateSignature(privateKey, hash); vm.assume(x != 0); vm.assume(y != 0); vm.assume(r != 0); vm.assume(s != 0); - assertEq(P256.verifySignatureAllowMalleability(hash, r, s, x, y), true); + assertEq(P256.verifySignatureAllowMalleability(hash, r, s, x1, y1), true); } - function test_validate_signature() external { + function test_validate_signature() external override { Kernel kernel2 = Kernel(payable(factory.createAccount(address(kernelImpl), getInitializeData(), 3))); bytes32 hash = keccak256(abi.encodePacked("hello world")); @@ -144,65 +149,42 @@ contract P256ValidatorTest is Test { ) ); - - (uint256 x, uint256 y) = generatePublicKey(ownerKey); (uint256 r, uint256 s) = generateSignature(ownerKey, digest); assertEq(kernel.isValidSignature(hash, abi.encode(r, s)), Kernel.isValidSignature.selector); assertEq(kernel2.isValidSignature(hash, abi.encode(r, s)), bytes4(0xffffffff)); } - function test_sudo() external { - UserOperation memory op = - entryPoint.fillUserOp(address(kernel), abi.encodeWithSelector(TestExecutor.doNothing.selector)); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - entryPoint.handleOps(ops, beneficiary); - } - - function getInitializeData() internal view returns (bytes memory) { - return abi.encodeWithSelector( - KernelStorage.initialize.selector, - p256Validator, - abi.encode(x, y) - ); - } - - function test_set_default_validator() external { - TestValidator newValidator = new TestValidator(); - bytes memory empty; - UserOperation memory op = entryPoint.fillUserOp( - address(kernel), - abi.encodeWithSelector(KernelStorage.setDefaultValidator.selector, address(newValidator), empty) - ); - op.signature = signUserOp(op); - UserOperation[] memory ops = new UserOperation[](1); - ops[0] = op; - vm.expectEmit(true, true, true, false, address(entryPoint)); - emit UserOperationEvent(entryPoint.getUserOpHash(op), address(kernel), address(0), op.nonce, false, 0, 0); - entryPoint.handleOps(ops, beneficiary); + function test_fail_validate_wrongsignature() external override { + bytes32 hash = keccak256(abi.encodePacked("hello world")); + bytes memory sig = getWrongSignature(hash); + assertEq(kernel.isValidSignature(hash, sig), bytes4(0xffffffff)); } - function signUserOp(UserOperation memory op) internal returns (bytes memory) { + function signUserOp(UserOperation memory op) internal view override returns (bytes memory) { bytes32 hash = entryPoint.getUserOpHash(op); (uint256 r, uint256 s) = generateSignature(ownerKey, hash); return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); } - function getWrongSignature(UserOperation memory op) internal returns (bytes memory) { + function getWrongSignature(UserOperation memory op) internal view override returns (bytes memory) { bytes32 hash = entryPoint.getUserOpHash(op); (uint256 r, uint256 s) = generateSignature(ownerKey + 1, hash); return abi.encodePacked(bytes4(0x00000000), abi.encode(r, s)); } - function signHash(bytes32 hash) internal returns (bytes memory) { + function signHash(bytes32 hash) internal view override returns (bytes memory) { (uint256 r, uint256 s) = generateSignature(ownerKey, ECDSA.toEthSignedMessageHash(hash)); return abi.encode(r, s); } - function getWrongSignature(bytes32 hash) internal returns (bytes memory) { + function getWrongSignature(bytes32 hash) internal view override returns (bytes memory) { (uint256 r, uint256 s) = generateSignature(ownerKey + 1, ECDSA.toEthSignedMessageHash(hash)); return abi.encode(r, s); } + + function verifyPublicKey(uint256 actualX, uint256 actualY, uint256 expectedX, uint256 expectedY) internal { + assertEq(actualX, expectedX, "Public key X component mismatch"); + assertEq(actualY, expectedY, "Public key Y component mismatch"); + } } From c0c44a62a1298dcd678749d72f39ae29f4522f6a Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 13:14:56 -0500 Subject: [PATCH 07/10] Remove py scripts used for initial testing --- nok.py | 48 ------------------------------------------------ pkc.py | 35 ----------------------------------- signature.py | 43 ------------------------------------------- 3 files changed, 126 deletions(-) delete mode 100644 nok.py delete mode 100644 pkc.py delete mode 100644 signature.py diff --git a/nok.py b/nok.py deleted file mode 100644 index d13f1a43..00000000 --- a/nok.py +++ /dev/null @@ -1,48 +0,0 @@ -import os -import sys -import binascii -from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import ec -from cryptography.hazmat.primitives import hashes -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives.asymmetric import utils - -def hex_to_bin(hex_string): - return binascii.unhexlify(hex_string) - -if len(sys.argv) != 3: - print("Not all arguments supplied. Please provide the hash to be signed and private key.") - sys.exit(1) - -hash_to_be_signed = sys.argv[1] -private_key = sys.argv[2] - -if not all(c in '0123456789abcdefABCDEF' for c in hash_to_be_signed) or not all(c in '0123456789abcdefABCDEF' for c in private_key): - print("Invalid input. The hash and private key must be valid hexadecimal strings.") - sys.exit(1) - -private_key = ec.derive_private_key(int(private_key, 16), ec.SECP256R1(), default_backend()) -public_key = private_key.public_key() -public_key_bytes = public_key.public_bytes( - encoding=serialization.Encoding.PEM, - format=serialization.PublicFormat.SubjectPublicKeyInfo -) - -public_key_hex = public_key_bytes.decode() -public_key_coordinates = public_key.public_numbers() -x = public_key_coordinates.x -y = public_key_coordinates.y -x_hex = hex(x) -y_hex = hex(y) - -# sign with secp256r1 -hash_to_be_signed_bytes = hex_to_bin(hash_to_be_signed) -# use prehash to avoid double hashing - -signature = private_key.sign(hash_to_be_signed_bytes, ec.ECDSA(utils.Prehashed(hashes.SHA256()))) -r, s = utils.decode_dss_signature(signature) -r_hex = hex(r) -s_hex = hex(s) - -print(f"Public Key Coordinates: x = {x_hex}, y = {y_hex}") -print(f"Signature: r = {r_hex}, s = {s_hex}") \ No newline at end of file diff --git a/pkc.py b/pkc.py deleted file mode 100644 index 5fefdcfb..00000000 --- a/pkc.py +++ /dev/null @@ -1,35 +0,0 @@ -# pkc.py -import os -import sys -import binascii -from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import ec -from cryptography.hazmat.backends import default_backend -from eth_abi import encode - -def hex_to_bin(hex_string): - return binascii.unhexlify(hex_string) - -if len(sys.argv) != 2: - print("Not all arguments supplied. Please provide the private key.") - sys.exit(1) - -private_key = sys.argv[1] - -if not private_key.isdigit() or int(private_key) <= 0: - print("Invalid input. The private key must be a valid large positive number.") - sys.exit(1) - -if not private_key.isdigit(): - print("Invalid input. The private key must be a valid large number.") - sys.exit(1) - -private_key = ec.derive_private_key(int(private_key), ec.SECP256R1(), default_backend()) -public_key = private_key.public_key() -public_key_coordinates = public_key.public_numbers() -x = public_key_coordinates.x -y = public_key_coordinates.y - -encoded_coordinates = encode(['uint256', 'uint256'], [x, y]) -encoded_coordinates_hex = binascii.hexlify(encoded_coordinates).decode() -print(encoded_coordinates_hex) \ No newline at end of file diff --git a/signature.py b/signature.py deleted file mode 100644 index 24d26c83..00000000 --- a/signature.py +++ /dev/null @@ -1,43 +0,0 @@ -import os -import sys -import binascii -from cryptography.hazmat.primitives import serialization -from cryptography.hazmat.primitives.asymmetric import ec -from cryptography.hazmat.primitives import hashes -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives.asymmetric import utils -from eth_abi import encode - -def hex_to_bin(hex_string): - return binascii.unhexlify(hex_string) - -if len(sys.argv) != 3: - print("Not all arguments supplied. Please provide the hash to be signed and private key.") - sys.exit(1) - -hash_to_be_signed = sys.argv[1] -private_key = sys.argv[2] - -# Remove '0x' prefix if present -if hash_to_be_signed.startswith('0x'): - hash_to_be_signed = hash_to_be_signed[2:] - -if not all(c in '0123456789abcdefABCDEF' for c in hash_to_be_signed): - print("Invalid input. The hash must be a valid hexadecimal string.") - sys.exit(1) - -# Check if private key is a positive integer -if not private_key.isdigit() or int(private_key) <= 0: - print("Invalid input. The private key must be a positive integer.") - sys.exit(1) - -private_key = ec.derive_private_key(int(private_key), ec.SECP256R1(), default_backend()) -hash_to_be_signed_bytes = hex_to_bin(hash_to_be_signed) -signature = private_key.sign(hash_to_be_signed_bytes, ec.ECDSA(utils.Prehashed(hashes.SHA256()))) -r, s = utils.decode_dss_signature(signature) - -encoded_sig = encode(['uint256', 'uint256'], [r, s]) - -# print encoded_sig as hex string -encoded_sig_hex = binascii.b2a_hex(encoded_sig).decode() -print(encoded_sig_hex) \ No newline at end of file From c79d1fbe552e342352a884ae822c04ff4221e65a Mon Sep 17 00:00:00 2001 From: David Eiber Date: Tue, 21 Nov 2023 13:15:44 -0500 Subject: [PATCH 08/10] Update foundry.toml --- foundry.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/foundry.toml b/foundry.toml index 7fe5e1d8..d4dbafef 100644 --- a/foundry.toml +++ b/foundry.toml @@ -8,7 +8,6 @@ cbor_metadata = false optimize = true via-ir = true runs = 1000000 -ffi = true [fuzz] runs = 1024 From f913ba0f0cfcc149cfcabff262a16d3de4697197 Mon Sep 17 00:00:00 2001 From: David Eiber Date: Mon, 27 Nov 2023 11:42:43 -0500 Subject: [PATCH 09/10] update deps --- lib/forge-std | 2 +- lib/solady | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/forge-std b/lib/forge-std index 705263c9..37a37ab7 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 705263c95892a906d7af65f0f73ce8a4a0c80b80 +Subproject commit 37a37ab73364d6644bfe11edf88a07880f99bd56 diff --git a/lib/solady b/lib/solady index 6f724bd0..cde0a5fb 160000 --- a/lib/solady +++ b/lib/solady @@ -1 +1 @@ -Subproject commit 6f724bd0f654b1199ee4cd909d206878c405bbcb +Subproject commit cde0a5fb594da8655ba6bfcdc2e40a7c870c0cc0 From fa69cc4edff23927cbd5faef9e283ca6c719df8e Mon Sep 17 00:00:00 2001 From: David Eiber Date: Mon, 27 Nov 2023 11:43:04 -0500 Subject: [PATCH 10/10] add non-malleable signatures --- src/validator/P256Validator.sol | 8 +++--- test/foundry/validator/P256Validator.t.sol | 33 +++++++++++++++++----- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/validator/P256Validator.sol b/src/validator/P256Validator.sol index b163ab7a..02f122fd 100644 --- a/src/validator/P256Validator.sol +++ b/src/validator/P256Validator.sol @@ -39,10 +39,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.verifySignatureAllowMalleability(hash, r, s, key.x, key.y)) { + if (P256.verifySignature(hash, r, s, key.x, key.y)) { return ValidationData.wrap(0); } - if (!P256.verifySignatureAllowMalleability(_userOpHash, r, s, key.x, key.y)) { + if (!P256.verifySignature(_userOpHash, r, s, key.x, key.y)) { return SIG_VALIDATION_FAILED; } } @@ -50,11 +50,11 @@ contract P256Validator is IKernelValidator { 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.verifySignatureAllowMalleability(hash, r, s, key.x, key.y)) { + if (P256.verifySignature(hash, r, s, key.x, key.y)) { return ValidationData.wrap(0); } bytes32 ethHash = ECDSA.toEthSignedMessageHash(hash); - if (!P256.verifySignatureAllowMalleability(ethHash, r, s, key.x, key.y)) { + if (!P256.verifySignature(ethHash, r, s, key.x, key.y)) { return SIG_VALIDATION_FAILED; } return ValidationData.wrap(0); diff --git a/test/foundry/validator/P256Validator.t.sol b/test/foundry/validator/P256Validator.t.sol index fbb13991..25336449 100644 --- a/test/foundry/validator/P256Validator.t.sol +++ b/test/foundry/validator/P256Validator.t.sol @@ -22,6 +22,12 @@ using ERC4337Utils for IEntryPoint; contract P256ValidatorTest is KernelTestBase { P256Verifier p256Verifier; P256Validator p256Validator; + + // Curve order (number of points) + uint256 constant n = + 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551; + + uint256 x; uint256 y; @@ -121,9 +127,22 @@ contract P256ValidatorTest is KernelTestBase { return FCL_ecdsa_utils.ecdsa_derivKpub(privateKey); } - function generateSignature(uint256 privateKey, bytes32 hash) internal view returns (uint256, uint256) { - uint256 k = type(uint256).max - 1; - return FCL_ecdsa_utils.ecdsa_sign(hash, k, privateKey); + function generateSignature(uint256 privateKey, bytes32 hash) internal view returns (uint256 r, uint256 s) { + // Securely generate a random k value for each signature + uint256 k = uint256(keccak256(abi.encodePacked(hash, block.timestamp, block.difficulty, privateKey))) % n; + while (k == 0) { + k = uint256(keccak256(abi.encodePacked(k))) % n; + } + + // Generate the signature using the k value and the private key + (r, s) = FCL_ecdsa_utils.ecdsa_sign(hash, k, privateKey); + + // Ensure that s is in the lower half of the range [1, n-1] + if (r == 0 || s == 0 || s > P256.P256_N_DIV_2) { + s = n - s; // If s is in the upper half, use n - s instead + } + + return (r, s); } function test_utils(uint256 privateKey, bytes32 hash) external { @@ -132,11 +151,11 @@ contract P256ValidatorTest is KernelTestBase { (uint256 x1, uint256 y1) = generatePublicKey(privateKey); (uint256 r, uint256 s) = generateSignature(privateKey, hash); - vm.assume(x != 0); - vm.assume(y != 0); + vm.assume(x1 != 0); + vm.assume(y1 != 0); vm.assume(r != 0); - vm.assume(s != 0); - assertEq(P256.verifySignatureAllowMalleability(hash, r, s, x1, y1), true); + vm.assume(s < P256.P256_N_DIV_2); + assertEq(P256.verifySignature(hash, r, s, x1, y1), true); } function test_validate_signature() external override {