From 535cf522597fd5ee50538c19e6c113ba334a9dc8 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 13:09:08 -0800 Subject: [PATCH 01/23] add forge deps --- .gitmodules | 9 +++++++++ contracts/lib/account-abstraction | 1 + contracts/lib/safe-contracts | 1 + contracts/lib/safe-modules | 1 + contracts/remappings.txt | 4 +++- contracts/src/PBH4337Module.sol | 13 +++++++++++++ 6 files changed, 28 insertions(+), 1 deletion(-) create mode 160000 contracts/lib/account-abstraction create mode 160000 contracts/lib/safe-contracts create mode 160000 contracts/lib/safe-modules create mode 100644 contracts/src/PBH4337Module.sol diff --git a/.gitmodules b/.gitmodules index e08c1fb3..91122bb0 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,3 +7,12 @@ [submodule "contracts/lib/world-id-contracts"] path = contracts/lib/world-id-contracts url = https://github.com/worldcoin/world-id-contracts +[submodule "contracts/lib/safe-modules"] + path = contracts/lib/safe-modules + url = https://github.com/safe-global/safe-modules +[submodule "contracts/lib/account-abstraction"] + path = contracts/lib/account-abstraction + url = https://github.com/eth-infinitism/account-abstraction +[submodule "contracts/lib/safe-contracts"] + path = contracts/lib/safe-contracts + url = https://github.com/safe-global/safe-contracts diff --git a/contracts/lib/account-abstraction b/contracts/lib/account-abstraction new file mode 160000 index 00000000..7af70c89 --- /dev/null +++ b/contracts/lib/account-abstraction @@ -0,0 +1 @@ +Subproject commit 7af70c8993a6f42973f520ae0752386a5032abe7 diff --git a/contracts/lib/safe-contracts b/contracts/lib/safe-contracts new file mode 160000 index 00000000..192c7dc6 --- /dev/null +++ b/contracts/lib/safe-contracts @@ -0,0 +1 @@ +Subproject commit 192c7dc67290940fcbc75165522bb86a37187069 diff --git a/contracts/lib/safe-modules b/contracts/lib/safe-modules new file mode 160000 index 00000000..edf7af00 --- /dev/null +++ b/contracts/lib/safe-modules @@ -0,0 +1 @@ +Subproject commit edf7af00279b3af223b012d24064da62247ee843 diff --git a/contracts/remappings.txt b/contracts/remappings.txt index 025934e2..b66a5c4a 100644 --- a/contracts/remappings.txt +++ b/contracts/remappings.txt @@ -1,6 +1,8 @@ @world-id-contracts/=lib/world-id-contracts/src/ -@account-abstraction/=lib/account-abstraction/contracts/ @BokkyPooBahsDateTimeLibrary/=lib/BokkyPooBahsDateTimeLibrary/contracts/ @helpers/=src/helpers/ openzeppelin-contracts/=lib/world-id-contracts/lib/openzeppelin-contracts/contracts/ +@4337=lib/safe-modules/modules/4337/contracts/ +@account-abstraction/contracts/=lib/account-abstraction/contracts/ +@safe-global/safe-contracts/contracts/=lib/safe-contracts/contracts/ \ No newline at end of file diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol new file mode 100644 index 00000000..ed09ca3f --- /dev/null +++ b/contracts/src/PBH4337Module.sol @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; +` +import {Safe4337Module} from "@4337/Safe4337Module.sol"; + +contract PBHSafe4337Module is Safe4337Module { + + address public immutable PBH_SIGNATURE_AGGREGATOR; + + constructor(address _safe, address _pbhSignatureAggregator) Safe4337Module(_safe) { + PBH_SIGNATURE_AGGREGATOR = _pbhSignatureAggregator; + } +} \ No newline at end of file From babfe1942dc2321c7e05277592aed0755a1471d5 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 13:10:33 -0800 Subject: [PATCH 02/23] bump version --- contracts/src/PBH4337Module.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index ed09ca3f..353807d8 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; -` +pragma solidity ^0.8.20; + import {Safe4337Module} from "@4337/Safe4337Module.sol"; contract PBHSafe4337Module is Safe4337Module { From a7c4956640d65f56849555d85bf2546b93ab6c87 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 13:26:05 -0800 Subject: [PATCH 03/23] set required solidity version in PBHExternalNullifier.sol --- contracts/foundry.toml | 3 +-- contracts/src/helpers/PBHExternalNullifier.sol | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/foundry.toml b/contracts/foundry.toml index 047810a7..1de45d5a 100644 --- a/contracts/foundry.toml +++ b/contracts/foundry.toml @@ -1,5 +1,4 @@ [profile.default] src = "src" out = "out" -libs = ["lib"] -via_ir = true +libs = ["lib"] \ No newline at end of file diff --git a/contracts/src/helpers/PBHExternalNullifier.sol b/contracts/src/helpers/PBHExternalNullifier.sol index e142076a..80b615ea 100644 --- a/contracts/src/helpers/PBHExternalNullifier.sol +++ b/contracts/src/helpers/PBHExternalNullifier.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; From b079686cf75e295ffde8115c01bbcc4e5c707908 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 13:26:55 -0800 Subject: [PATCH 04/23] fmt --- contracts/src/PBH4337Module.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 353807d8..5ef24d9a 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -4,10 +4,9 @@ pragma solidity ^0.8.20; import {Safe4337Module} from "@4337/Safe4337Module.sol"; contract PBHSafe4337Module is Safe4337Module { - address public immutable PBH_SIGNATURE_AGGREGATOR; constructor(address _safe, address _pbhSignatureAggregator) Safe4337Module(_safe) { PBH_SIGNATURE_AGGREGATOR = _pbhSignatureAggregator; } -} \ No newline at end of file +} From 7f74d0f08bfdfd9a0eb4d030543c2dedc4c18c97 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 15:50:44 -0800 Subject: [PATCH 05/23] return PBH_SIGNATURE_AGGREGATOR inside _validateSignatures --- contracts/src/PBH4337Module.sol | 84 ++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 5ef24d9a..1ff92275 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -2,11 +2,93 @@ pragma solidity ^0.8.20; import {Safe4337Module} from "@4337/Safe4337Module.sol"; +import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; +import {ValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; +import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; +import {ISafe} from "@4337/interfaces/Safe.sol"; contract PBHSafe4337Module is Safe4337Module { + uint256 constant ECDSA_SIGNATURE_LENGTH = 65; + uint256 constant TIMESTAMP_BYTES = 12; // 6 bytes each for validAfter and validUntil + + // TODO: Import from Parent Safe4337Module + bytes32 private constant SAFE_OP_TYPEHASH = 0xc03dfc11d8b10bf9cf703d558958c8c42777f785d998c62060d85a4f0ef6ea7f; + address public immutable PBH_SIGNATURE_AGGREGATOR; + uint192 public immutable PBH_NONCE_KEY; - constructor(address _safe, address _pbhSignatureAggregator) Safe4337Module(_safe) { + constructor(address _safe, address _pbhSignatureAggregator, uint192 _pbhNonceKey) Safe4337Module(_safe) { PBH_SIGNATURE_AGGREGATOR = _pbhSignatureAggregator; + PBH_NONCE_KEY = _pbhNonceKey; + } + + // TODO: Fork the Safe4337Module dependency and add 'override' to _validateSignatures. It is manually updated currently + /** + * @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data + * of `validAfter || validUntil || authorizer`: + * - `authorizer`: 20-byte address, 0 for valid signature or 1 to mark signature failure (this module does not make use of signature aggregators). + * - `validUntil`: 6-byte timestamp value, or zero for "infinite". The user operation is valid only up to this time. + * - `validAfter`: 6-byte timestamp. The user operation is valid only after this time. + * @param userOp User operation struct. + * @return validationData An integer indicating the result of the validation. + */ + function _validateSignatures(PackedUserOperation calldata userOp) + internal + view + override + returns (uint256 validationData) + { + // Check if the userOp has the specified PBH key + // https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/core/NonceManager.sol#L38 + uint192 key = uint192(userOp.nonce >> 64); + bool isPBH = false; + + // This does NOT validate the proof + // It removes the first 12 bytes from the signature as it represents the validAfter and validUntil values + // operationData is not determined by the signature + (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = + _getSafeOp(userOp); + + // If it is a PBH transaction, we need to handle two cases with the signature: + // 1. The bundler simulates the call with the proof appended + // 2. UserOp execution without proof appended + if (key == PBH_NONCE_KEY) { + isPBH = true; + } + + // Base signature length calculation: + // TIMESTAMP_BYTES (12) + (threshold * ECDSA_SIGNATURE_LENGTH) + uint256 expectedLength = + TIMESTAMP_BYTES + (ISafe(payable(userOp.sender)).getThreshold() * ECDSA_SIGNATURE_LENGTH); + + // If the signature length is greater than the expected length, then we know that the bundler appended the proof + // We need to remove the proof from the signature before validation + if (key == PBH_NONCE_KEY && userOp.signature.length > expectedLength) { + signatures = userOp.signature[0:expectedLength]; + } + + // The `checkSignatures` function in the Safe contract does not force a fixed size on signature length. + // A malicious bundler can pad the Safe operation `signatures` with additional bytes, causing the account to pay + // more gas than needed for user operation validation (capped by `verificationGasLimit`). + // `_checkSignaturesLength` ensures that there are no additional bytes in the `signature` than are required. + bool validSignature = _checkSignaturesLength(signatures, ISafe(payable(userOp.sender)).getThreshold()); + + try ISafe(payable(userOp.sender)).checkSignatures(keccak256(operationData), operationData, signatures) {} + catch { + validSignature = false; + } + + address authorizer; + + // If the signature is valid and the userOp is a PBH userOp, return the PBH signature aggregator as the authorizer + // Else return 0 for valid signature and 1 for invalid signature + if (isPBH && validSignature) { + authorizer = PBH_SIGNATURE_AGGREGATOR; + } else { + authorizer = validSignature ? address(0) : address(1); + } + + // The timestamps are validated by the entry point, therefore we will not check them again. + validationData = _packValidationData(ValidationData(authorizer, validUntil, validAfter)); } } From d82e2d8e9cba5558eab05d2146eb156fe03f493f Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 15:51:07 -0800 Subject: [PATCH 06/23] remove unused const --- contracts/src/PBH4337Module.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 1ff92275..e553b21c 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -11,9 +11,6 @@ contract PBHSafe4337Module is Safe4337Module { uint256 constant ECDSA_SIGNATURE_LENGTH = 65; uint256 constant TIMESTAMP_BYTES = 12; // 6 bytes each for validAfter and validUntil - // TODO: Import from Parent Safe4337Module - bytes32 private constant SAFE_OP_TYPEHASH = 0xc03dfc11d8b10bf9cf703d558958c8c42777f785d998c62060d85a4f0ef6ea7f; - address public immutable PBH_SIGNATURE_AGGREGATOR; uint192 public immutable PBH_NONCE_KEY; From 93d2d40a8e7334a6e7eac49be83cb9c977ce3b7c Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 15:53:16 -0800 Subject: [PATCH 07/23] comment --- contracts/src/PBH4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index e553b21c..74c3bd02 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -19,7 +19,7 @@ contract PBHSafe4337Module is Safe4337Module { PBH_NONCE_KEY = _pbhNonceKey; } - // TODO: Fork the Safe4337Module dependency and add 'override' to _validateSignatures. It is manually updated currently + // TODO: Fork the Safe4337Module dependency and add 'override' to _validateSignatures. It is manually updated currently and CI will fail /** * @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data * of `validAfter || validUntil || authorizer`: From 97ea0e26a76054330addd4f38abd520b7b06c3f7 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Wed, 18 Dec 2024 16:21:19 -0800 Subject: [PATCH 08/23] init pbh module with entrypoint address --- contracts/src/PBH4337Module.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 74c3bd02..b6b4266c 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -14,7 +14,7 @@ contract PBHSafe4337Module is Safe4337Module { address public immutable PBH_SIGNATURE_AGGREGATOR; uint192 public immutable PBH_NONCE_KEY; - constructor(address _safe, address _pbhSignatureAggregator, uint192 _pbhNonceKey) Safe4337Module(_safe) { + constructor(address entryPoint, address _pbhSignatureAggregator, uint192 _pbhNonceKey) Safe4337Module(entryPoint) { PBH_SIGNATURE_AGGREGATOR = _pbhSignatureAggregator; PBH_NONCE_KEY = _pbhNonceKey; } @@ -61,6 +61,7 @@ contract PBHSafe4337Module is Safe4337Module { // If the signature length is greater than the expected length, then we know that the bundler appended the proof // We need to remove the proof from the signature before validation if (key == PBH_NONCE_KEY && userOp.signature.length > expectedLength) { + // TODO: Check for valid proof size signatures = userOp.signature[0:expectedLength]; } From e1427c5afe943b35c9be68518a354f273fba6819 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Fri, 20 Dec 2024 01:27:23 -0800 Subject: [PATCH 09/23] wip --- contracts/src/PBH4337Module.sol | 5 +- contracts/test/Mock4337Module.sol | 25 ++++ contracts/test/PBH4337Module.t.sol | 211 +++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+), 1 deletion(-) create mode 100644 contracts/test/Mock4337Module.sol create mode 100644 contracts/test/PBH4337Module.t.sol diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index b6b4266c..968f02bf 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -6,6 +6,7 @@ import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/Pac import {ValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; import {ISafe} from "@4337/interfaces/Safe.sol"; +import "forge-std/console.sol"; contract PBHSafe4337Module is Safe4337Module { uint256 constant ECDSA_SIGNATURE_LENGTH = 65; @@ -50,9 +51,9 @@ contract PBHSafe4337Module is Safe4337Module { // 1. The bundler simulates the call with the proof appended // 2. UserOp execution without proof appended if (key == PBH_NONCE_KEY) { + console.log("PBH transaction detected"); isPBH = true; } - // Base signature length calculation: // TIMESTAMP_BYTES (12) + (threshold * ECDSA_SIGNATURE_LENGTH) uint256 expectedLength = @@ -62,6 +63,8 @@ contract PBHSafe4337Module is Safe4337Module { // We need to remove the proof from the signature before validation if (key == PBH_NONCE_KEY && userOp.signature.length > expectedLength) { // TODO: Check for valid proof size + console.log("Removing proof"); + signatures = userOp.signature[0:expectedLength]; } diff --git a/contracts/test/Mock4337Module.sol b/contracts/test/Mock4337Module.sol new file mode 100644 index 00000000..1101a7ac --- /dev/null +++ b/contracts/test/Mock4337Module.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {PBHSafe4337Module} from "../src/PBH4337Module.sol"; +import {Safe4337Module} from "@4337/Safe4337Module.sol"; +import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; +import {ValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; +import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; +import {ISafe} from "@4337/interfaces/Safe.sol"; +import "forge-std/console.sol"; + +contract Mock4337Module is PBHSafe4337Module { + constructor(address entryPoint, address _pbhSignatureAggregator, uint192 _pbhNonceKey) + PBHSafe4337Module(entryPoint, _pbhSignatureAggregator, _pbhNonceKey) + {} + + function validateSignaturesExternal(PackedUserOperation calldata userOp) + external + view + returns (uint256 validationData) + { + console.log("validateSignaturesExternal"); + return _validateSignatures(userOp); + } +} diff --git a/contracts/test/PBH4337Module.t.sol b/contracts/test/PBH4337Module.t.sol new file mode 100644 index 00000000..6285db1e --- /dev/null +++ b/contracts/test/PBH4337Module.t.sol @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {Vm} from "forge-std/Vm.sol"; +import {Test} from "forge-std/Test.sol"; + +import "forge-std/console.sol"; +import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; + +import {Safe} from "@safe-global/safe-contracts/contracts/Safe.sol"; +import {SafeProxyFactory} from "@safe-global/safe-contracts/contracts/proxies/SafeProxyFactory.sol"; +import {SafeProxy} from "@safe-global/safe-contracts/contracts/proxies/SafeProxy.sol"; +import {Enum} from "@safe-global/safe-contracts/contracts/common/Enum.sol"; +import {SafeModuleSetup} from "@4337/SafeModuleSetup.sol"; +import {PBHSafe4337Module} from "../src/PBH4337Module.sol"; +import {Mock4337Module} from "./Mock4337Module.sol"; +import {Safe4337Module} from "@4337/Safe4337Module.sol"; + +contract PBHSafe4337ModuleTest is Test { + Mock4337Module public module; + Safe public singleton; + Safe public safe; + SafeProxyFactory public factory; + SafeModuleSetup public moduleSetup; + + address public owner; + uint256 public ownerKey; + + address public constant PBH_SIGNATURE_AGGREGATOR = address(0x123); + uint192 public constant PBH_NONCE_KEY = 1123123123; + + function setUp() public { + // Create single EOA owner + ownerKey = 0x1; + owner = vm.addr(ownerKey); + + module = new Mock4337Module(owner, PBH_SIGNATURE_AGGREGATOR, PBH_NONCE_KEY); + + // module = new PBHSafe4337Module( + // // TODO: Find transaction broadcaster in forge test + // owner, + // PBH_SIGNATURE_AGGREGATOR, + // PBH_NONCE_KEY + // ); + + // Deploy SafeModuleSetup + moduleSetup = new SafeModuleSetup(); + + // Deploy Safe singleton and factory + singleton = new Safe(); + factory = new SafeProxyFactory(); + + // Prepare module initialization + address[] memory modules = new address[](1); + modules[0] = address(module); + + // Encode the moduleSetup.enableModules call + bytes memory moduleSetupCall = abi.encodeCall(SafeModuleSetup.enableModules, (modules)); + + // Create owners array with single owner + address[] memory owners = new address[](1); + owners[0] = owner; + + // Encode initialization data for proxy + bytes memory initData = abi.encodeCall( + Safe.setup, + ( + owners, + 1, // threshold + address(moduleSetup), // to + moduleSetupCall, // data + address(0), // fallbackHandler + address(0), // paymentToken + 0, // payment + payable(address(0)) // paymentReceiver + ) + ); + + // Deploy and initialize Safe proxy + SafeProxy proxy = factory.createProxyWithNonce( + address(singleton), + initData, + 0 // salt nonce + ); + + // Cast proxy to Safe for easier interaction + safe = Safe(payable(address(proxy))); + + console.log("Is module enabled:", safe.isModuleEnabled(address(module))); + } + + function testValidateSignaturesWithUserOp() public { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(safe), + nonce: uint256(PBH_NONCE_KEY) << 64, // Keep the nonce key format + initCode: "", // Empty for already deployed safe + callData: abi.encodeCall( // Actual call the Safe will make + Safe.execTransaction, + ( + address(0xdead), // target address + 1 ether, // value + bytes(""), // data + Enum.Operation.Call, + 0, // safeTxGas + 0, // baseGas + 0, // gasPrice + address(0), // gasToken + payable(address(0)), // refundReceiver + bytes("") // signatures - empty for user op + ) + ), + accountGasLimits: bytes32( + abi.encode( // Pack verification and call gas limits + uint128(100000), // verification gas limit + uint128(300000) // call gas limit + ) + ), + preVerificationGas: 21000, // Base cost + gasFees: bytes32( + abi.encode( // Pack max priority fee and max fee + uint128(1 gwei), // maxPriorityFeePerGas + uint128(100 gwei) // maxFeePerGas + ) + ), + paymasterAndData: "", // No paymaster + signature: "" // Will be filled after signing + }); + + bytes32 operationHash = module.getOperationHash(userOp); + + // // First create the userOp in memory + // PackedUserOperation memory userOp = PackedUserOperation({ + // sender: address(safe), + // nonce: uint256(PBH_NONCE_KEY) << 64, + // initCode: "", + // callData: "0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf", + // accountGasLimits: bytes32(0), + // preVerificationGas: 0, + // gasFees: bytes32(0), + // paymasterAndData: "", + // signature: "" + // }); + + // // Encode it to bytes + // bytes memory encodedUserOp = abi.encode(userOp); + + // // Make the call using a raw call to handle the calldata requirement + // (bool success, bytes memory returnData) = address(module).call( + // abi.encodeCall(Safe4337Module.getOperationHash, (abi.decode(encodedUserOp, (PackedUserOperation)))) + // ); + // require(success, "getOperationHash failed"); + + // bytes32 operationHash = abi.decode(returnData, (bytes32)); + + // // Sign the operation hash + // (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, operationHash); + + // // Pack the signature with just time bounds and r,s,v + // bytes memory signature = abi.encodePacked( + // uint48(0), // uint48 (6 bytes) + // uint48(0), // uint48 (6 bytes) + // r, + // s, + // v // The raw signature components + // ); + // // Update userOp with signature + // userOp.signature = signature; + + // // Encode the function call using abi.encodeCall + // // bytes memory callData = abi.encodeCall( + // // Mock4337Module.validateSignaturesExternal, + // // (userOp) + // // ); + + // // module.validateSignaturesExternal(userOp); + + // // bytes memory callData = abi.encodeCall( + // // // Safe4337Module.getOperationHash, + // // Mock4337Module.validateSignaturesExternal2, + // // (userOp) + // // ); + + // // // Make the raw call + // // (bool success2, bytes memory returnData) = address(safe).call(callData); + // // require(success2, "validateSignaturesExternal call failed"); + + // // console.log("Success:", success2); + // // console.log("Return data length:", returnData.length); + // // console.logBytes(returnData); + + // // // // Decode the return value + // // // uint256 validationData = abi.decode(returnData, (uint256)); + // // // console.log("validationData", returnData); + + // // uint256 validationData = uint256(returnData); + + // // // // Extract validation components + // // address authorizer = address(uint160(returnData)); + // // uint48 validUntil = uint48(validationData >> 160); + // // uint48 validAfter = uint48(validationData >> 208); + + // // console.log("authorizer", authorizer); + // // console.log("validUntil", validUntil); + // // console.log("validAfter", validAfter); + + // // // Verify signature was valid (authorizer should be address(0) for valid non-PBH signature) + // // assertEq(authorizer, PBH_SIGNATURE_AGGREGATOR, "PBH Aggregator address not returned"); + // // assertEq(validUntil, 0, "ValidUntil should be 0"); + // // assertEq(validAfter, 0, "ValidAfter should be 0"); + } +} From 8bc065082ac042c94401e1e54239889126fe3e13 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 21:55:17 -0800 Subject: [PATCH 10/23] test working --- contracts/src/PBH4337Module.sol | 4 +- contracts/test/Mock4337Module.sol | 1 - contracts/test/PBH4337Module.t.sol | 133 ++++++----------------------- 3 files changed, 31 insertions(+), 107 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 968f02bf..5c0b5cc9 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -47,11 +47,13 @@ contract PBHSafe4337Module is Safe4337Module { (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); + // console.log("operationData during validation", operationData); + // console.log("signatures", signatures); + // If it is a PBH transaction, we need to handle two cases with the signature: // 1. The bundler simulates the call with the proof appended // 2. UserOp execution without proof appended if (key == PBH_NONCE_KEY) { - console.log("PBH transaction detected"); isPBH = true; } // Base signature length calculation: diff --git a/contracts/test/Mock4337Module.sol b/contracts/test/Mock4337Module.sol index 1101a7ac..28b28d43 100644 --- a/contracts/test/Mock4337Module.sol +++ b/contracts/test/Mock4337Module.sol @@ -19,7 +19,6 @@ contract Mock4337Module is PBHSafe4337Module { view returns (uint256 validationData) { - console.log("validateSignaturesExternal"); return _validateSignatures(userOp); } } diff --git a/contracts/test/PBH4337Module.t.sol b/contracts/test/PBH4337Module.t.sol index 6285db1e..79ec68dc 100644 --- a/contracts/test/PBH4337Module.t.sol +++ b/contracts/test/PBH4337Module.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.20; import {Vm} from "forge-std/Vm.sol"; import {Test} from "forge-std/Test.sol"; - import "forge-std/console.sol"; import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; @@ -34,14 +33,9 @@ contract PBHSafe4337ModuleTest is Test { ownerKey = 0x1; owner = vm.addr(ownerKey); - module = new Mock4337Module(owner, PBH_SIGNATURE_AGGREGATOR, PBH_NONCE_KEY); + console.log("expected owner", owner); - // module = new PBHSafe4337Module( - // // TODO: Find transaction broadcaster in forge test - // owner, - // PBH_SIGNATURE_AGGREGATOR, - // PBH_NONCE_KEY - // ); + module = new Mock4337Module(owner, PBH_SIGNATURE_AGGREGATOR, PBH_NONCE_KEY); // Deploy SafeModuleSetup moduleSetup = new SafeModuleSetup(); @@ -85,30 +79,16 @@ contract PBHSafe4337ModuleTest is Test { // Cast proxy to Safe for easier interaction safe = Safe(payable(address(proxy))); - - console.log("Is module enabled:", safe.isModuleEnabled(address(module))); } function testValidateSignaturesWithUserOp() public { + bytes memory signatureBefore = abi.encodePacked(uint48(0), uint48(0)); + PackedUserOperation memory userOp = PackedUserOperation({ sender: address(safe), nonce: uint256(PBH_NONCE_KEY) << 64, // Keep the nonce key format initCode: "", // Empty for already deployed safe - callData: abi.encodeCall( // Actual call the Safe will make - Safe.execTransaction, - ( - address(0xdead), // target address - 1 ether, // value - bytes(""), // data - Enum.Operation.Call, - 0, // safeTxGas - 0, // baseGas - 0, // gasPrice - address(0), // gasToken - payable(address(0)), // refundReceiver - bytes("") // signatures - empty for user op - ) - ), + callData: "", accountGasLimits: bytes32( abi.encode( // Pack verification and call gas limits uint128(100000), // verification gas limit @@ -123,89 +103,32 @@ contract PBHSafe4337ModuleTest is Test { ) ), paymasterAndData: "", // No paymaster - signature: "" // Will be filled after signing + signature: signatureBefore }); bytes32 operationHash = module.getOperationHash(userOp); - // // First create the userOp in memory - // PackedUserOperation memory userOp = PackedUserOperation({ - // sender: address(safe), - // nonce: uint256(PBH_NONCE_KEY) << 64, - // initCode: "", - // callData: "0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf", - // accountGasLimits: bytes32(0), - // preVerificationGas: 0, - // gasFees: bytes32(0), - // paymasterAndData: "", - // signature: "" - // }); - - // // Encode it to bytes - // bytes memory encodedUserOp = abi.encode(userOp); - - // // Make the call using a raw call to handle the calldata requirement - // (bool success, bytes memory returnData) = address(module).call( - // abi.encodeCall(Safe4337Module.getOperationHash, (abi.decode(encodedUserOp, (PackedUserOperation)))) - // ); - // require(success, "getOperationHash failed"); - - // bytes32 operationHash = abi.decode(returnData, (bytes32)); - - // // Sign the operation hash - // (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, operationHash); - - // // Pack the signature with just time bounds and r,s,v - // bytes memory signature = abi.encodePacked( - // uint48(0), // uint48 (6 bytes) - // uint48(0), // uint48 (6 bytes) - // r, - // s, - // v // The raw signature components - // ); - // // Update userOp with signature - // userOp.signature = signature; - - // // Encode the function call using abi.encodeCall - // // bytes memory callData = abi.encodeCall( - // // Mock4337Module.validateSignaturesExternal, - // // (userOp) - // // ); - - // // module.validateSignaturesExternal(userOp); - - // // bytes memory callData = abi.encodeCall( - // // // Safe4337Module.getOperationHash, - // // Mock4337Module.validateSignaturesExternal2, - // // (userOp) - // // ); - - // // // Make the raw call - // // (bool success2, bytes memory returnData) = address(safe).call(callData); - // // require(success2, "validateSignaturesExternal call failed"); - - // // console.log("Success:", success2); - // // console.log("Return data length:", returnData.length); - // // console.logBytes(returnData); - - // // // // Decode the return value - // // // uint256 validationData = abi.decode(returnData, (uint256)); - // // // console.log("validationData", returnData); - - // // uint256 validationData = uint256(returnData); - - // // // // Extract validation components - // // address authorizer = address(uint160(returnData)); - // // uint48 validUntil = uint48(validationData >> 160); - // // uint48 validAfter = uint48(validationData >> 208); - - // // console.log("authorizer", authorizer); - // // console.log("validUntil", validUntil); - // // console.log("validAfter", validAfter); - - // // // Verify signature was valid (authorizer should be address(0) for valid non-PBH signature) - // // assertEq(authorizer, PBH_SIGNATURE_AGGREGATOR, "PBH Aggregator address not returned"); - // // assertEq(validUntil, 0, "ValidUntil should be 0"); - // // assertEq(validAfter, 0, "ValidAfter should be 0"); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, operationHash); + + bytes memory signature = abi.encodePacked( + uint48(0), + uint48(0), + r, + s, + v // The raw signature components + ); + userOp.signature = signature; + + uint256 validationData = module.validateSignaturesExternal(userOp); + + // // Extract validation components + address authorizer = address(uint160(validationData)); + uint48 validUntil = uint48(validationData >> 160); + uint48 validAfter = uint48(validationData >> 208); + + // Verify signature was valid (authorizer should be address(0) for valid non-PBH signature) + assertEq(authorizer, PBH_SIGNATURE_AGGREGATOR, "PBH Aggregator address not returned"); + assertEq(validUntil, 0, "ValidUntil should be 0"); + assertEq(validAfter, 0, "ValidAfter should be 0"); } } From 017ff92be198728a31bb331aa0ee0df660676597 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 21:56:27 -0800 Subject: [PATCH 11/23] cleanup --- contracts/src/PBH4337Module.sol | 4 ---- contracts/test/PBH4337Module.t.sol | 2 -- 2 files changed, 6 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 5c0b5cc9..5bfbb191 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -6,7 +6,6 @@ import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/Pac import {ValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; import {_packValidationData} from "@account-abstraction/contracts/core/Helpers.sol"; import {ISafe} from "@4337/interfaces/Safe.sol"; -import "forge-std/console.sol"; contract PBHSafe4337Module is Safe4337Module { uint256 constant ECDSA_SIGNATURE_LENGTH = 65; @@ -47,9 +46,6 @@ contract PBHSafe4337Module is Safe4337Module { (bytes memory operationData, uint48 validAfter, uint48 validUntil, bytes calldata signatures) = _getSafeOp(userOp); - // console.log("operationData during validation", operationData); - // console.log("signatures", signatures); - // If it is a PBH transaction, we need to handle two cases with the signature: // 1. The bundler simulates the call with the proof appended // 2. UserOp execution without proof appended diff --git a/contracts/test/PBH4337Module.t.sol b/contracts/test/PBH4337Module.t.sol index 79ec68dc..622ffe9b 100644 --- a/contracts/test/PBH4337Module.t.sol +++ b/contracts/test/PBH4337Module.t.sol @@ -33,8 +33,6 @@ contract PBHSafe4337ModuleTest is Test { ownerKey = 0x1; owner = vm.addr(ownerKey); - console.log("expected owner", owner); - module = new Mock4337Module(owner, PBH_SIGNATURE_AGGREGATOR, PBH_NONCE_KEY); // Deploy SafeModuleSetup From f76ab19f8fe95ab6528aa45cbf5177ab7c56bf7e Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 22:01:22 -0800 Subject: [PATCH 12/23] check proof size --- contracts/src/PBH4337Module.sol | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 5bfbb191..69b46593 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -38,7 +38,6 @@ contract PBHSafe4337Module is Safe4337Module { // Check if the userOp has the specified PBH key // https://github.com/eth-infinitism/account-abstraction/blob/develop/contracts/core/NonceManager.sol#L38 uint192 key = uint192(userOp.nonce >> 64); - bool isPBH = false; // This does NOT validate the proof // It removes the first 12 bytes from the signature as it represents the validAfter and validUntil values @@ -49,9 +48,8 @@ contract PBHSafe4337Module is Safe4337Module { // If it is a PBH transaction, we need to handle two cases with the signature: // 1. The bundler simulates the call with the proof appended // 2. UserOp execution without proof appended - if (key == PBH_NONCE_KEY) { - isPBH = true; - } + bool isPBH = (key == PBH_NONCE_KEY) ? true : false; + // Base signature length calculation: // TIMESTAMP_BYTES (12) + (threshold * ECDSA_SIGNATURE_LENGTH) uint256 expectedLength = @@ -59,10 +57,10 @@ contract PBHSafe4337Module is Safe4337Module { // If the signature length is greater than the expected length, then we know that the bundler appended the proof // We need to remove the proof from the signature before validation - if (key == PBH_NONCE_KEY && userOp.signature.length > expectedLength) { - // TODO: Check for valid proof size - console.log("Removing proof"); - + if (isPBH && userOp.signature.length > expectedLength) { + // 352 bytes is the size of the encoded proof + require(userOp.signature.length - expectedLength == 352, "Invalid proof size in signature"); + // Remove the proof from the signature signatures = userOp.signature[0:expectedLength]; } From 92bf022cfef6e8bab6869a6e16a15eb161e47eb6 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 22:06:03 -0800 Subject: [PATCH 13/23] add test for invalid signature --- contracts/test/PBH4337Module.t.sol | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/contracts/test/PBH4337Module.t.sol b/contracts/test/PBH4337Module.t.sol index 622ffe9b..eaaf0f50 100644 --- a/contracts/test/PBH4337Module.t.sol +++ b/contracts/test/PBH4337Module.t.sol @@ -129,4 +129,52 @@ contract PBHSafe4337ModuleTest is Test { assertEq(validUntil, 0, "ValidUntil should be 0"); assertEq(validAfter, 0, "ValidAfter should be 0"); } + + function testInvalidSignatures() public { + bytes memory signatureBefore = abi.encodePacked(uint48(0), uint48(0)); + + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(safe), + nonce: uint256(PBH_NONCE_KEY) << 64, // Keep the nonce key format + initCode: "", // Empty for already deployed safe + callData: "", + accountGasLimits: bytes32( + abi.encode( // Pack verification and call gas limits + uint128(100000), // verification gas limit + uint128(300000) // call gas limit + ) + ), + preVerificationGas: 21000, // Base cost + gasFees: bytes32( + abi.encode( // Pack max priority fee and max fee + uint128(1 gwei), // maxPriorityFeePerGas + uint128(100 gwei) // maxFeePerGas + ) + ), + paymasterAndData: "", // No paymaster + signature: signatureBefore + }); + + bytes32 operationHash = module.getOperationHash(userOp); + + // Create an invalid signature + (uint8 v, bytes32 r, bytes32 s) = vm.sign(ownerKey, keccak256("invalid")); + + bytes memory signature = abi.encodePacked( + uint48(0), + uint48(0), + r, + s, + v // The raw signature components + ); + userOp.signature = signature; + + uint256 validationData = module.validateSignaturesExternal(userOp); + + // // Extract validation components + address authorizer = address(uint160(validationData)); + + // Verify signature was invalid (authorizer should be 1 for invalid signature) + assertEq(authorizer, address(1), "PBH Aggregator address not returned"); + } } From 750f8473301142da176a7c371243553cc3d23deb Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 22:06:25 -0800 Subject: [PATCH 14/23] fix test naming --- contracts/test/PBH4337Module.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/test/PBH4337Module.t.sol b/contracts/test/PBH4337Module.t.sol index eaaf0f50..31a56ef9 100644 --- a/contracts/test/PBH4337Module.t.sol +++ b/contracts/test/PBH4337Module.t.sol @@ -79,7 +79,7 @@ contract PBHSafe4337ModuleTest is Test { safe = Safe(payable(address(proxy))); } - function testValidateSignaturesWithUserOp() public { + function testValidSignature() public { bytes memory signatureBefore = abi.encodePacked(uint48(0), uint48(0)); PackedUserOperation memory userOp = PackedUserOperation({ @@ -130,7 +130,7 @@ contract PBHSafe4337ModuleTest is Test { assertEq(validAfter, 0, "ValidAfter should be 0"); } - function testInvalidSignatures() public { + function testInvalidSignature() public { bytes memory signatureBefore = abi.encodePacked(uint48(0), uint48(0)); PackedUserOperation memory userOp = PackedUserOperation({ From 0901b7a70ce40d6cf5ab46559740834a13856fd0 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 22:15:06 -0800 Subject: [PATCH 15/23] use forked worldcoin safe-modules --- .gitmodules | 8 ++++---- contracts/foundry.toml | 1 - contracts/lib/safe-modules | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.gitmodules b/.gitmodules index 8422ba99..ddf6254e 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,9 +7,6 @@ [submodule "contracts/lib/world-id-contracts"] path = contracts/lib/world-id-contracts url = https://github.com/worldcoin/world-id-contracts -[submodule "contracts/lib/safe-modules"] - path = contracts/lib/safe-modules - url = https://github.com/safe-global/safe-modules [submodule "contracts/lib/safe-contracts"] path = contracts/lib/safe-contracts url = https://github.com/safe-global/safe-contracts @@ -21,4 +18,7 @@ url = https://github.com/eth-infinitism/account-abstraction [submodule "contracts/lib/openzeppelin-contracts-upgradeable"] path = contracts/lib/openzeppelin-contracts-upgradeable - url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable \ No newline at end of file + url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable +[submodule "contracts/lib/safe-modules"] + path = contracts/lib/safe-modules + url = https://github.com/worldcoin/safe-modules diff --git a/contracts/foundry.toml b/contracts/foundry.toml index b9a2e451..401fd978 100644 --- a/contracts/foundry.toml +++ b/contracts/foundry.toml @@ -3,7 +3,6 @@ evm_version = 'cancun' src = "src" out = "out" libs = ["lib"] -via_ir = true optimizer = true optimizer_runs = 200 diff --git a/contracts/lib/safe-modules b/contracts/lib/safe-modules index edf7af00..60dc191a 160000 --- a/contracts/lib/safe-modules +++ b/contracts/lib/safe-modules @@ -1 +1 @@ -Subproject commit edf7af00279b3af223b012d24064da62247ee843 +Subproject commit 60dc191a1dba2ffdb0d5c60bff1262e3288b425f From d1b08ca6aa2935d9360f675572ec51d13c80815b Mon Sep 17 00:00:00 2001 From: karankurbur Date: Sun, 22 Dec 2024 22:31:05 -0800 Subject: [PATCH 16/23] bump all files solidity version to 0.8.28 --- contracts/lib/safe-modules | 2 +- contracts/src/PBH4337Module.sol | 2 +- contracts/src/PBHEntryPoint.sol | 2 +- contracts/src/PBHEntryPointImplV1.sol | 2 +- contracts/src/PBHSignatureAggregator.sol | 2 +- contracts/src/PBHVerifier.sol | 2 +- contracts/src/helpers/ByteHasher.sol | 2 +- contracts/src/interfaces/IPBHEntryPoint.sol | 2 +- contracts/src/interfaces/IPBHVerifier.sol | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/lib/safe-modules b/contracts/lib/safe-modules index 60dc191a..9abf69ea 160000 --- a/contracts/lib/safe-modules +++ b/contracts/lib/safe-modules @@ -1 +1 @@ -Subproject commit 60dc191a1dba2ffdb0d5c60bff1262e3288b425f +Subproject commit 9abf69ea1df673c1010aeb9bbbc6aa14124ba425 diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 69b46593..f64810be 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {Safe4337Module} from "@4337/Safe4337Module.sol"; import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; diff --git a/contracts/src/PBHEntryPoint.sol b/contracts/src/PBHEntryPoint.sol index 5e4e4383..3c07f6de 100644 --- a/contracts/src/PBHEntryPoint.sol +++ b/contracts/src/PBHEntryPoint.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; diff --git a/contracts/src/PBHEntryPointImplV1.sol b/contracts/src/PBHEntryPointImplV1.sol index e471b195..3dad9f51 100644 --- a/contracts/src/PBHEntryPointImplV1.sol +++ b/contracts/src/PBHEntryPointImplV1.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {PBHVerifier} from "./PBHVerifier.sol"; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; diff --git a/contracts/src/PBHSignatureAggregator.sol b/contracts/src/PBHSignatureAggregator.sol index fadb60bc..77c7ea30 100644 --- a/contracts/src/PBHSignatureAggregator.sol +++ b/contracts/src/PBHSignatureAggregator.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; import {IPBHEntryPoint} from "./interfaces/IPBHEntryPoint.sol"; diff --git a/contracts/src/PBHVerifier.sol b/contracts/src/PBHVerifier.sol index eebe0eb0..90a596d8 100644 --- a/contracts/src/PBHVerifier.sol +++ b/contracts/src/PBHVerifier.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {ByteHasher} from "./helpers/ByteHasher.sol"; import {PBHExternalNullifier} from "./helpers/PBHExternalNullifier.sol"; diff --git a/contracts/src/helpers/ByteHasher.sol b/contracts/src/helpers/ByteHasher.sol index c655a1d9..960d282d 100644 --- a/contracts/src/helpers/ByteHasher.sol +++ b/contracts/src/helpers/ByteHasher.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; library ByteHasher { /// @dev Creates a keccak256 hash of a bytestring. diff --git a/contracts/src/interfaces/IPBHEntryPoint.sol b/contracts/src/interfaces/IPBHEntryPoint.sol index ca8a05e5..2de80e90 100644 --- a/contracts/src/interfaces/IPBHEntryPoint.sol +++ b/contracts/src/interfaces/IPBHEntryPoint.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; diff --git a/contracts/src/interfaces/IPBHVerifier.sol b/contracts/src/interfaces/IPBHVerifier.sol index 11b8551f..3cac8964 100644 --- a/contracts/src/interfaces/IPBHVerifier.sol +++ b/contracts/src/interfaces/IPBHVerifier.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.28; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; From 31675bcfb72965b40a95dd3c7ba751055ff64f47 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Mon, 23 Dec 2024 08:07:00 -0800 Subject: [PATCH 17/23] Update contracts/src/PBH4337Module.sol Co-authored-by: 0xKitsune <77890308+0xKitsune@users.noreply.github.com> --- contracts/src/PBH4337Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index f64810be..430bbce3 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -48,7 +48,7 @@ contract PBHSafe4337Module is Safe4337Module { // If it is a PBH transaction, we need to handle two cases with the signature: // 1. The bundler simulates the call with the proof appended // 2. UserOp execution without proof appended - bool isPBH = (key == PBH_NONCE_KEY) ? true : false; + bool isPBH = (key == PBH_NONCE_KEY); // Base signature length calculation: // TIMESTAMP_BYTES (12) + (threshold * ECDSA_SIGNATURE_LENGTH) From ee124f8d7b2e9cd7f91c6d98600d5bd2540d72ca Mon Sep 17 00:00:00 2001 From: 0xKitsune <0xkitsune@protonmail.com> Date: Mon, 23 Dec 2024 12:06:36 -0500 Subject: [PATCH 18/23] combine entry point and pbh verifier logic --- contracts/src/PBHEntryPointImplV1.sol | 157 ++++++++++++++++++++++++-- 1 file changed, 145 insertions(+), 12 deletions(-) diff --git a/contracts/src/PBHEntryPointImplV1.sol b/contracts/src/PBHEntryPointImplV1.sol index 3dad9f51..3f1ba953 100644 --- a/contracts/src/PBHEntryPointImplV1.sol +++ b/contracts/src/PBHEntryPointImplV1.sol @@ -5,12 +5,17 @@ import {PBHVerifier} from "./PBHVerifier.sol"; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; import {IPBHEntryPoint} from "./interfaces/IPBHEntryPoint.sol"; +import {ByteHasher} from "./helpers/ByteHasher.sol"; +import {PBHExternalNullifier} from "./helpers/PBHExternalNullifier.sol"; +import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; +import {IPBHVerifier} from "./interfaces/IPBHVerifier.sol"; +import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; /// @title PBH Entry Point Implementation V1 /// @dev This contract is an implementation of the PBH Entry Point. /// It is used to verify the signature of a Priority User Operation, and Relaying Priority Bundles to the EIP-4337 Entry Point. /// @author Worldcoin -contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { +contract PBHEntryPointImplV1 is IPBHEntryPoint, IPBHVerifier, WorldIDImpl { /////////////////////////////////////////////////////////////////////////////// /// A NOTE ON IMPLEMENTATION CONTRACTS /// /////////////////////////////////////////////////////////////////////////////// @@ -48,14 +53,46 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { // these variables takes place. If reordering happens, a storage clash will occur (effectively a // memory safety error). + using ByteHasher for bytes; + + /////////////////////////////////////////////////////////////////////////////// + /// ERRORS /// + ////////////////////////////////////////////////////////////////////////////// + + /// @notice Thrown when attempting to reuse a nullifier + error InvalidNullifier(); + /////////////////////////////////////////////////////////////////////////////// /// Events /// ////////////////////////////////////////////////////////////////////////////// event PBHEntryPointImplInitialized( - IWorldIDGroups indexed worldId, IEntryPoint indexed entryPoint, uint8 indexed numPbhPerMonth + IWorldIDGroups indexed worldId, + IEntryPoint indexed entryPoint, + uint8 indexed numPbhPerMonth + ); + + /// @notice Emitted once for each successful PBH verification. + /// + /// @param sender The sender of this particular transaction or UserOp. + /// @param nonce Transaction/UserOp nonce. + /// @param payload The zero-knowledge proof that demonstrates the claimer is registered with World ID. + event PBH( + address indexed sender, + uint256 indexed nonce, + PBHPayload payload ); + /// @notice Emitted when the World ID address is set. + /// + /// @param worldId The World ID instance that will be used for verifying proofs. + event WorldIdSet(address indexed worldId); + + /// @notice Emitted when the number of PBH transactions allowed per month is set. + /// + /// @param numPbhPerMonth The number of allowed PBH transactions per month. + event NumPbhPerMonthSet(uint8 indexed numPbhPerMonth); + /////////////////////////////////////////////////////////////////////////////// /// Vars /// ////////////////////////////////////////////////////////////////////////////// @@ -65,6 +102,22 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { /// The PBHVerifier is always the proxy to the EntryPoint for PBH Bundles. bytes32 internal _hashedOps; + /// @dev The World ID group ID (always 1) + uint256 internal constant _GROUP_ID = 1; + + /// @dev The World ID instance that will be used for verifying proofs + IWorldIDGroups public worldId; + + /// @dev The EntryPoint where Aggregated PBH Bundles will be proxied to. + IEntryPoint public entryPoint; + + /// @notice The number of PBH transactions that may be used by a single + /// World ID in a given month. + uint8 public numPbhPerMonth; + + /// @dev Whether a nullifier hash has been used already. Used to guarantee an action is only performed once by a single person + mapping(uint256 => bool) public nullifierHashes; + /////////////////////////////////////////////////////////////////////////////// /// INITIALIZATION /// /////////////////////////////////////////////////////////////////////////////// @@ -92,10 +145,11 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { /// @param _numPbhPerMonth The number of allowed PBH transactions per month. /// /// @custom:reverts string If called more than once at the same initialisation number. - function initialize(IWorldIDGroups _worldId, IEntryPoint _entryPoint, uint8 _numPbhPerMonth) - external - reinitializer(1) - { + function initialize( + IWorldIDGroups _worldId, + IEntryPoint _entryPoint, + uint8 _numPbhPerMonth + ) external reinitializer(1) { // First, ensure that all of the parent contracts are initialised. __delegateInit(); @@ -105,7 +159,11 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { // Say that the contract is initialized. __setInitialized(); - emit PBHEntryPointImplInitialized(_worldId, _entryPoint, _numPbhPerMonth); + emit PBHEntryPointImplInitialized( + _worldId, + _entryPoint, + _numPbhPerMonth + ); } /// @notice Responsible for initialising all of the supertypes of this contract. @@ -130,14 +188,21 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { address payable beneficiary ) external virtual onlyProxy onlyInitialized { for (uint256 i = 0; i < opsPerAggregator.length; ++i) { - bytes32 hashedOps = keccak256(abi.encode(opsPerAggregator[i].userOps)); + bytes32 hashedOps = keccak256( + abi.encode(opsPerAggregator[i].userOps) + ); assembly ("memory-safe") { - if gt(tload(hashedOps), 0) { revert(0, 0) } + if gt(tload(hashedOps), 0) { + revert(0, 0) + } tstore(hashedOps, hashedOps) } - PBHPayload[] memory pbhPayloads = abi.decode(opsPerAggregator[i].signature, (PBHPayload[])); + PBHPayload[] memory pbhPayloads = abi.decode( + opsPerAggregator[i].signature, + (PBHPayload[]) + ); for (uint256 j = 0; j < pbhPayloads.length; ++j) { verifyPbh( opsPerAggregator[i].userOps[j].sender, @@ -153,9 +218,77 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, PBHVerifier { /// @notice Validates the hashed operations is the same as the hash transiently stored. /// @param hashedOps The hashed operations to validate. - function validateSignaturesCallback(bytes32 hashedOps) external view virtual onlyProxy onlyInitialized { + function validateSignaturesCallback( + bytes32 hashedOps + ) external view virtual onlyProxy onlyInitialized { assembly ("memory-safe") { - if iszero(eq(tload(hashedOps), hashedOps)) { revert(0, 0) } + if iszero(eq(tload(hashedOps), hashedOps)) { + revert(0, 0) + } + } + } + + /// @param sender The sender of this particular transaction or UserOp. + /// @param nonce Transaction/UserOp nonce. + /// @param callData Transaction/UserOp call data. + /// @param pbhPayload The PBH payload containing the proof data. + function verifyPbh( + address sender, + uint256 nonce, + bytes calldata callData, + PBHPayload memory pbhPayload + ) public virtual onlyInitialized onlyProxy { + // First, we make sure this nullifier has not been used before. + if (nullifierHashes[pbhPayload.nullifierHash]) + revert InvalidNullifier(); + + // Verify the external nullifier + PBHExternalNullifier.verify( + pbhPayload.pbhExternalNullifier, + numPbhPerMonth + ); + + // If worldId address is set, proceed with on chain verification, + // otherwise assume verification has been done off chain by the builder. + if (address(worldId) != address(0)) { + // We now generate the signal hash from the sender, nonce, and calldata + uint256 signalHash = abi + .encodePacked(sender, nonce, callData) + .hashToField(); + + // We now verify the provided proof is valid and the user is verified by World ID + worldId.verifyProof( + pbhPayload.root, + _GROUP_ID, + signalHash, + pbhPayload.nullifierHash, + pbhPayload.pbhExternalNullifier, + pbhPayload.proof + ); } + + // We now record the user has done this, so they can't do it again (proof of uniqueness) + nullifierHashes[pbhPayload.nullifierHash] = true; + + emit PBH(sender, nonce, pbhPayload); + } + + /// @notice Sets the number of PBH transactions allowed per month. + /// @param _numPbhPerMonth The number of allowed PBH transactions per month. + function setNumPbhPerMonth( + uint8 _numPbhPerMonth + ) external virtual onlyOwner onlyProxy onlyInitialized { + numPbhPerMonth = _numPbhPerMonth; + emit NumPbhPerMonthSet(_numPbhPerMonth); + } + + /// @dev If the World ID address is set to 0, then it is assumed that verification will take place off chain. + /// @notice Sets the World ID instance that will be used for verifying proofs. + /// @param _worldId The World ID instance that will be used for verifying proofs. + function setWorldId( + address _worldId + ) external virtual onlyOwner onlyProxy onlyInitialized { + worldId = IWorldIDGroups(_worldId); + emit WorldIdSet(_worldId); } } From d7400df72990883c7d748eeeb611f584ce1372e8 Mon Sep 17 00:00:00 2001 From: 0xKitsune <0xkitsune@protonmail.com> Date: Mon, 23 Dec 2024 12:09:07 -0500 Subject: [PATCH 19/23] remove ipbhverifier --- contracts/src/PBHEntryPointImplV1.sol | 1 - contracts/src/PBHVerifier.sol | 165 ------------------ contracts/src/interfaces/IPBHEntryPoint.sol | 35 +++- contracts/src/interfaces/IPBHVerifier.sol | 24 --- .../PBHEntryPointOwnershipManagement.t.sol | 108 +++++++++--- contracts/test/PBHVerifier.t.sol | 59 +++++-- 6 files changed, 160 insertions(+), 232 deletions(-) delete mode 100644 contracts/src/PBHVerifier.sol delete mode 100644 contracts/src/interfaces/IPBHVerifier.sol diff --git a/contracts/src/PBHEntryPointImplV1.sol b/contracts/src/PBHEntryPointImplV1.sol index 3f1ba953..f426b8b0 100644 --- a/contracts/src/PBHEntryPointImplV1.sol +++ b/contracts/src/PBHEntryPointImplV1.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.28; -import {PBHVerifier} from "./PBHVerifier.sol"; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; import {IPBHEntryPoint} from "./interfaces/IPBHEntryPoint.sol"; diff --git a/contracts/src/PBHVerifier.sol b/contracts/src/PBHVerifier.sol deleted file mode 100644 index 90a596d8..00000000 --- a/contracts/src/PBHVerifier.sol +++ /dev/null @@ -1,165 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.28; - -import {ByteHasher} from "./helpers/ByteHasher.sol"; -import {PBHExternalNullifier} from "./helpers/PBHExternalNullifier.sol"; -import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; -import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; -import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; -import {IPBHVerifier} from "./interfaces/IPBHVerifier.sol"; -import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; - -/// @title PBH Verifier Implementation Version 1 -/// @author Worldcoin -/// @notice An implementation of a priority blockspace for humans (PBH) transaction verifier. -/// @dev This is the implementation delegated to by a proxy. -contract PBHVerifier is IPBHVerifier, WorldIDImpl { - /////////////////////////////////////////////////////////////////////////////// - /// A NOTE ON IMPLEMENTATION CONTRACTS /// - /////////////////////////////////////////////////////////////////////////////// - - // This contract is designed explicitly to operate from behind a proxy contract. As a result, - // there are a few important implementation considerations: - // - // - All updates made after deploying a given version of the implementation should inherit from - // the latest version of the implementation. This prevents storage clashes. - // - All functions that are less access-restricted than `private` should be marked `virtual` in - // order to enable the fixing of bugs in the existing interface. - // - Any function that reads from or modifies state (i.e. is not marked `pure`) must be - // annotated with the `onlyProxy` and `onlyInitialized` modifiers. This ensures that it can - // only be called when it has access to the data in the proxy, otherwise results are likely to - // be nonsensical. - // - This contract deals with important data for the PBH system. Ensure that all newly-added - // functionality is carefully access controlled using `onlyOwner`, or a more granular access - // mechanism. - // - Do not assign any contract-level variables at the definition site unless they are - // `constant`. - // - // Additionally, the following notes apply: - // - // - Initialisation and ownership management are not protected behind `onlyProxy` intentionally. - // This ensures that the contract can safely be disposed of after it is no longer used. - // - Carefully consider what data recovery options are presented as new functionality is added. - // Care must be taken to ensure that a migration plan can exist for cases where upgrades - // cannot recover from an issue or vulnerability. - - /////////////////////////////////////////////////////////////////////////////// - /// !!!!! DATA: DO NOT REORDER !!!!! /// - /////////////////////////////////////////////////////////////////////////////// - - // To ensure compatibility between upgrades, it is exceedingly important that no reordering of - // these variables takes place. If reordering happens, a storage clash will occur (effectively a - // memory safety error). - - using ByteHasher for bytes; - - /////////////////////////////////////////////////////////////////////////////// - /// ERRORS /// - ////////////////////////////////////////////////////////////////////////////// - - /// @notice Thrown when attempting to reuse a nullifier - error InvalidNullifier(); - - /////////////////////////////////////////////////////////////////////////////// - /// Events /// - ////////////////////////////////////////////////////////////////////////////// - - /// @notice Emitted once for each successful PBH verification. - /// - /// @param sender The sender of this particular transaction or UserOp. - /// @param nonce Transaction/UserOp nonce. - /// @param payload The zero-knowledge proof that demonstrates the claimer is registered with World ID. - event PBH(address indexed sender, uint256 indexed nonce, PBHPayload payload); - - /// @notice Emitted when the World ID address is set. - /// - /// @param worldId The World ID instance that will be used for verifying proofs. - event WorldIdSet(address indexed worldId); - - /// @notice Emitted when the number of PBH transactions allowed per month is set. - /// - /// @param numPbhPerMonth The number of allowed PBH transactions per month. - event NumPbhPerMonthSet(uint8 indexed numPbhPerMonth); - - /////////////////////////////////////////////////////////////////////////////// - /// Vars /// - ////////////////////////////////////////////////////////////////////////////// - - /// @dev The World ID group ID (always 1) - uint256 internal constant _GROUP_ID = 1; - - /// @dev The World ID instance that will be used for verifying proofs - IWorldIDGroups public worldId; - - /// @dev The EntryPoint where Aggregated PBH Bundles will be proxied to. - IEntryPoint public entryPoint; - - /// @notice The number of PBH transactions that may be used by a single - /// World ID in a given month. - uint8 public numPbhPerMonth; - - /////////////////////////////////////////////////////////////////////////////// - /// Mappings /// - ////////////////////////////////////////////////////////////////////////////// - - /// @dev Whether a nullifier hash has been used already. Used to guarantee an action is only performed once by a single person - mapping(uint256 => bool) public nullifierHashes; - - /////////////////////////////////////////////////////////////////////////////// - /// Functions /// - ////////////////////////////////////////////////////////////////////////////// - - /// @param sender The sender of this particular transaction or UserOp. - /// @param nonce Transaction/UserOp nonce. - /// @param callData Transaction/UserOp call data. - /// @param pbhPayload The PBH payload containing the proof data. - function verifyPbh(address sender, uint256 nonce, bytes calldata callData, PBHPayload memory pbhPayload) - public - virtual - onlyInitialized - onlyProxy - { - // First, we make sure this nullifier has not been used before. - if (nullifierHashes[pbhPayload.nullifierHash]) revert InvalidNullifier(); - - // Verify the external nullifier - PBHExternalNullifier.verify(pbhPayload.pbhExternalNullifier, numPbhPerMonth); - - // If worldId address is set, proceed with on chain verification, - // otherwise assume verification has been done off chain by the builder. - if (address(worldId) != address(0)) { - // We now generate the signal hash from the sender, nonce, and calldata - uint256 signalHash = abi.encodePacked(sender, nonce, callData).hashToField(); - - // We now verify the provided proof is valid and the user is verified by World ID - worldId.verifyProof( - pbhPayload.root, - _GROUP_ID, - signalHash, - pbhPayload.nullifierHash, - pbhPayload.pbhExternalNullifier, - pbhPayload.proof - ); - } - - // We now record the user has done this, so they can't do it again (proof of uniqueness) - nullifierHashes[pbhPayload.nullifierHash] = true; - - emit PBH(sender, nonce, pbhPayload); - } - - /// @notice Sets the number of PBH transactions allowed per month. - /// @param _numPbhPerMonth The number of allowed PBH transactions per month. - function setNumPbhPerMonth(uint8 _numPbhPerMonth) external virtual onlyOwner onlyProxy onlyInitialized { - numPbhPerMonth = _numPbhPerMonth; - emit NumPbhPerMonthSet(_numPbhPerMonth); - } - - /// @dev If the World ID address is set to 0, then it is assumed that verification will take place off chain. - /// @notice Sets the World ID instance that will be used for verifying proofs. - /// @param _worldId The World ID instance that will be used for verifying proofs. - function setWorldId(address _worldId) external virtual onlyOwner onlyProxy onlyInitialized { - worldId = IWorldIDGroups(_worldId); - emit WorldIdSet(_worldId); - } -} diff --git a/contracts/src/interfaces/IPBHEntryPoint.sol b/contracts/src/interfaces/IPBHEntryPoint.sol index 2de80e90..6904b217 100644 --- a/contracts/src/interfaces/IPBHEntryPoint.sol +++ b/contracts/src/interfaces/IPBHEntryPoint.sol @@ -3,13 +3,42 @@ pragma solidity ^0.8.28; import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; -import {IPBHVerifier} from "./IPBHVerifier.sol"; -interface IPBHEntryPoint is IPBHVerifier { +interface IPBHEntryPoint { + /// @notice The Packed World ID Proof data. + /// @param root The root of the Merkle tree. + /// @param pbhExternalNullifier The external nullifier for the PBH User Operation. + /// @param nullifierHash The nullifier hash for the PBH User Operation. + /// @param proof The Semaphore proof. + struct PBHPayload { + uint256 root; + uint256 pbhExternalNullifier; + uint256 nullifierHash; + uint256[8] proof; + } + function handleAggregatedOps( IEntryPoint.UserOpsPerAggregator[] calldata opsPerAggregator, address payable beneficiary ) external; - function initialize(IWorldIDGroups worldId, IEntryPoint entryPoint, uint8 _numPbhPerMonth) external; + + function initialize( + IWorldIDGroups worldId, + IEntryPoint entryPoint, + uint8 _numPbhPerMonth + ) external; function validateSignaturesCallback(bytes32 hashedOps) external view; + + function nullifierHashes(uint256) external view returns (bool); + + function verifyPbh( + address sender, + uint256 nonce, + bytes memory callData, + PBHPayload memory pbhPayload + ) external; + + function setNumPbhPerMonth(uint8 _numPbhPerMonth) external; + + function setWorldId(address _worldId) external; } diff --git a/contracts/src/interfaces/IPBHVerifier.sol b/contracts/src/interfaces/IPBHVerifier.sol deleted file mode 100644 index 3cac8964..00000000 --- a/contracts/src/interfaces/IPBHVerifier.sol +++ /dev/null @@ -1,24 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.28; - -import {IWorldIDGroups} from "@world-id-contracts/interfaces/IWorldIDGroups.sol"; -import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; - -interface IPBHVerifier { - /// @notice The Packed World ID Proof data. - /// @param root The root of the Merkle tree. - /// @param pbhExternalNullifier The external nullifier for the PBH User Operation. - /// @param nullifierHash The nullifier hash for the PBH User Operation. - /// @param proof The Semaphore proof. - struct PBHPayload { - uint256 root; - uint256 pbhExternalNullifier; - uint256 nullifierHash; - uint256[8] proof; - } - - function nullifierHashes(uint256) external view returns (bool); - function verifyPbh(address sender, uint256 nonce, bytes memory callData, PBHPayload memory pbhPayload) external; - function setNumPbhPerMonth(uint8 _numPbhPerMonth) external; - function setWorldId(address _worldId) external; -} diff --git a/contracts/test/PBHEntryPointOwnershipManagement.t.sol b/contracts/test/PBHEntryPointOwnershipManagement.t.sol index 10301ad5..d114c010 100644 --- a/contracts/test/PBHEntryPointOwnershipManagement.t.sol +++ b/contracts/test/PBHEntryPointOwnershipManagement.t.sol @@ -10,8 +10,6 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Own import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; -import {PBHVerifier} from "../src/PBHVerifier.sol"; - /// @title World ID PBHEntryPoint Routing Tests /// @notice Contains tests for the WorldID pbhVerifier /// @author Worldcoin @@ -26,7 +24,10 @@ contract PBHVerifierRouting is Setup { } /// @notice Taken from OwnableUpgradable.sol - event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + event OwnershipTransferred( + address indexed previousOwner, + address indexed newOwner + ); /// @notice Checks that it is possible to get the owner, and that the owner is correctly /// initialised. @@ -43,32 +44,75 @@ contract PBHVerifierRouting is Setup { function testTransferOwner(address newOwner) public { // Setup vm.assume(newOwner != nullAddress); - bytes memory transferCallData = abi.encodeCall(Ownable2StepUpgradeable.transferOwnership, (newOwner)); - bytes memory ownerCallData = abi.encodeCall(OwnableUpgradeable.owner, ()); - bytes memory pendingOwnerCallData = abi.encodeCall(Ownable2StepUpgradeable.pendingOwner, ()); - bytes memory acceptOwnerCallData = abi.encodeCall(Ownable2StepUpgradeable.acceptOwnership, ()); + bytes memory transferCallData = abi.encodeCall( + Ownable2StepUpgradeable.transferOwnership, + (newOwner) + ); + bytes memory ownerCallData = abi.encodeCall( + OwnableUpgradeable.owner, + () + ); + bytes memory pendingOwnerCallData = abi.encodeCall( + Ownable2StepUpgradeable.pendingOwner, + () + ); + bytes memory acceptOwnerCallData = abi.encodeCall( + Ownable2StepUpgradeable.acceptOwnership, + () + ); // Test - assertCallSucceedsOn(pbhEntryPointAddress, transferCallData, new bytes(0x0)); - assertCallSucceedsOn(pbhEntryPointAddress, pendingOwnerCallData, abi.encode(newOwner)); - assertCallSucceedsOn(pbhEntryPointAddress, ownerCallData, abi.encode(thisAddress)); + assertCallSucceedsOn( + pbhEntryPointAddress, + transferCallData, + new bytes(0x0) + ); + assertCallSucceedsOn( + pbhEntryPointAddress, + pendingOwnerCallData, + abi.encode(newOwner) + ); + assertCallSucceedsOn( + pbhEntryPointAddress, + ownerCallData, + abi.encode(thisAddress) + ); vm.expectEmit(true, true, true, true); emit OwnershipTransferred(thisAddress, newOwner); vm.prank(newOwner); - assertCallSucceedsOn(pbhEntryPointAddress, acceptOwnerCallData, new bytes(0x0)); - assertCallSucceedsOn(pbhEntryPointAddress, ownerCallData, abi.encode(newOwner)); + assertCallSucceedsOn( + pbhEntryPointAddress, + acceptOwnerCallData, + new bytes(0x0) + ); + assertCallSucceedsOn( + pbhEntryPointAddress, + ownerCallData, + abi.encode(newOwner) + ); } /// @notice Tests that only the pending owner can accept the ownership transfer. - function testCannotAcceptOwnershipAsNonPendingOwner(address newOwner, address notNewOwner) public { + function testCannotAcceptOwnershipAsNonPendingOwner( + address newOwner, + address notNewOwner + ) public { // Setup vm.assume(newOwner != nullAddress); vm.assume(notNewOwner != newOwner); - bytes memory callData = abi.encodeCall(Ownable2StepUpgradeable.transferOwnership, (newOwner)); - bytes memory acceptCallData = abi.encodeCall(Ownable2StepUpgradeable.acceptOwnership, ()); - bytes memory expectedError = encodeStringRevert("Ownable2Step: caller is not the new owner"); + bytes memory callData = abi.encodeCall( + Ownable2StepUpgradeable.transferOwnership, + (newOwner) + ); + bytes memory acceptCallData = abi.encodeCall( + Ownable2StepUpgradeable.acceptOwnership, + () + ); + bytes memory expectedError = encodeStringRevert( + "Ownable2Step: caller is not the new owner" + ); assertCallSucceedsOn(pbhEntryPointAddress, callData); vm.prank(notNewOwner); @@ -77,11 +121,19 @@ contract PBHVerifierRouting is Setup { } /// @notice Ensures that it is impossible to transfer ownership without being the owner. - function testCannotTransferOwnerIfNotOwner(address naughty, address newOwner) public { + function testCannotTransferOwnerIfNotOwner( + address naughty, + address newOwner + ) public { // Setup vm.assume(naughty != thisAddress && newOwner != nullAddress); - bytes memory callData = abi.encodeCall(OwnableUpgradeable.transferOwnership, (newOwner)); - bytes memory expectedReturn = encodeStringRevert("Ownable: caller is not the owner"); + bytes memory callData = abi.encodeCall( + OwnableUpgradeable.transferOwnership, + (newOwner) + ); + bytes memory expectedReturn = encodeStringRevert( + "Ownable: caller is not the owner" + ); vm.prank(naughty); // Test @@ -91,8 +143,13 @@ contract PBHVerifierRouting is Setup { /// @notice Tests that it is impossible to renounce ownership, even as the owner. function testCannotRenounceOwnershipAsOwner() public { // Setup - bytes memory renounceData = abi.encodeCall(OwnableUpgradeable.renounceOwnership, ()); - bytes memory errorData = abi.encodeWithSelector(WorldIDImpl.CannotRenounceOwnership.selector); + bytes memory renounceData = abi.encodeCall( + OwnableUpgradeable.renounceOwnership, + () + ); + bytes memory errorData = abi.encodeWithSelector( + WorldIDImpl.CannotRenounceOwnership.selector + ); // Test assertCallFailsOn(pbhEntryPointAddress, renounceData, errorData); @@ -102,8 +159,13 @@ contract PBHVerifierRouting is Setup { function testCannotRenounceOwnershipIfNotOwner(address naughty) public { // Setup vm.assume(naughty != thisAddress && naughty != nullAddress); - bytes memory callData = abi.encodeCall(OwnableUpgradeable.renounceOwnership, ()); - bytes memory returnData = encodeStringRevert("Ownable: caller is not the owner"); + bytes memory callData = abi.encodeCall( + OwnableUpgradeable.renounceOwnership, + () + ); + bytes memory returnData = encodeStringRevert( + "Ownable: caller is not the owner" + ); vm.prank(naughty); // Test diff --git a/contracts/test/PBHVerifier.t.sol b/contracts/test/PBHVerifier.t.sol index 8a2901bb..d9b27037 100644 --- a/contracts/test/PBHVerifier.t.sol +++ b/contracts/test/PBHVerifier.t.sol @@ -8,8 +8,6 @@ import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; import {ByteHasher} from "@helpers/ByteHasher.sol"; import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; import "@helpers/PBHExternalNullifier.sol"; -import {PBHVerifier} from "../src/PBHVerifier.sol"; -import {IPBHVerifier} from "../src/interfaces/IPBHVerifier.sol"; import {Setup} from "./Setup.sol"; /// @title PBHVerifer Verify Tests @@ -18,26 +16,41 @@ import {Setup} from "./Setup.sol"; contract PBHVerifierTest is Setup { using ByteHasher for bytes; - event PBH(address indexed sender, uint256 indexed nonce, IPBHVerifier.PBHPayload payload); + event PBH( + address indexed sender, + uint256 indexed nonce, + IPBHVerifier.PBHPayload payload + ); event NumPbhPerMonthSet(uint8 indexed numPbhPerMonth); event WorldIdSet(address indexed worldId); /// @notice Test payload for the PBHVerifier - IPBHVerifier.PBHPayload public testPayload = IPBHVerifier.PBHPayload({ - root: 1, - pbhExternalNullifier: getValidPBHExternalNullifier(), - nullifierHash: 1, - proof: [uint256(0), 0, 0, 0, 0, 0, 0, 0] - }); + IPBHVerifier.PBHPayload public testPayload = + IPBHVerifier.PBHPayload({ + root: 1, + pbhExternalNullifier: getValidPBHExternalNullifier(), + nullifierHash: 1, + proof: [uint256(0), 0, 0, 0, 0, 0, 0, 0] + }); uint256 internal nonce = 1; address internal sender = address(0x123); bytes internal testCallData = hex"deadbeef"; function getValidPBHExternalNullifier() public view returns (uint256) { - uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp)); - uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(block.timestamp)); - return PBHExternalNullifier.encode(PBHExternalNullifier.V1, 0, month, year); + uint8 month = uint8( + BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp) + ); + uint16 year = uint16( + BokkyPooBahsDateTimeLibrary.getYear(block.timestamp) + ); + return + PBHExternalNullifier.encode( + PBHExternalNullifier.V1, + 0, + month, + year + ); } /// @notice Test that a valid proof is verified correctly. @@ -65,11 +78,20 @@ contract PBHVerifierTest is Setup { /// @notice Test that setNumPBHPerMonth works as expected function testSetNumPBHPerMonth() public { MockWorldIDGroups(address(worldIDGroups)).setVerifyProofSuccess(true); - uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp)); - uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(block.timestamp)); + uint8 month = uint8( + BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp) + ); + uint16 year = uint16( + BokkyPooBahsDateTimeLibrary.getYear(block.timestamp) + ); // Value starts at 30, make sure 30 reverts. - testPayload.pbhExternalNullifier = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 30, month, year); + testPayload.pbhExternalNullifier = PBHExternalNullifier.encode( + PBHExternalNullifier.V1, + 30, + month, + year + ); testPayload.nullifierHash = 0; vm.expectRevert(PBHExternalNullifier.InvalidPbhNonce.selector); pbhEntryPoint.verifyPbh(sender, nonce, testCallData, testPayload); @@ -86,7 +108,12 @@ contract PBHVerifierTest is Setup { pbhEntryPoint.setNumPbhPerMonth(40); // Try again, it should work - testPayload.pbhExternalNullifier = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 30, month, year); + testPayload.pbhExternalNullifier = PBHExternalNullifier.encode( + PBHExternalNullifier.V1, + 30, + month, + year + ); testPayload.nullifierHash = 1; vm.expectEmit(true, true, true, true); From f1a48e10968e680143bad2bac4675d22b58f860f Mon Sep 17 00:00:00 2001 From: 0xKitsune <0xkitsune@protonmail.com> Date: Mon, 23 Dec 2024 12:16:57 -0500 Subject: [PATCH 20/23] fix imports --- contracts/src/PBHEntryPointImplV1.sol | 3 +- contracts/src/PBHSignatureAggregator.sol | 34 +++++----- contracts/test/PBHSignatureAggregator.t.sol | 74 ++++++++++++++++----- contracts/test/PBHVerifier.t.sol | 11 +-- 4 files changed, 83 insertions(+), 39 deletions(-) diff --git a/contracts/src/PBHEntryPointImplV1.sol b/contracts/src/PBHEntryPointImplV1.sol index f426b8b0..69c5e6dc 100644 --- a/contracts/src/PBHEntryPointImplV1.sol +++ b/contracts/src/PBHEntryPointImplV1.sol @@ -7,14 +7,13 @@ import {IPBHEntryPoint} from "./interfaces/IPBHEntryPoint.sol"; import {ByteHasher} from "./helpers/ByteHasher.sol"; import {PBHExternalNullifier} from "./helpers/PBHExternalNullifier.sol"; import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; -import {IPBHVerifier} from "./interfaces/IPBHVerifier.sol"; import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; /// @title PBH Entry Point Implementation V1 /// @dev This contract is an implementation of the PBH Entry Point. /// It is used to verify the signature of a Priority User Operation, and Relaying Priority Bundles to the EIP-4337 Entry Point. /// @author Worldcoin -contract PBHEntryPointImplV1 is IPBHEntryPoint, IPBHVerifier, WorldIDImpl { +contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /////////////////////////////////////////////////////////////////////////////// /// A NOTE ON IMPLEMENTATION CONTRACTS /// /////////////////////////////////////////////////////////////////////////////// diff --git a/contracts/src/PBHSignatureAggregator.sol b/contracts/src/PBHSignatureAggregator.sol index 77c7ea30..9555fb6d 100644 --- a/contracts/src/PBHSignatureAggregator.sol +++ b/contracts/src/PBHSignatureAggregator.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.28; import "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; import {IPBHEntryPoint} from "./interfaces/IPBHEntryPoint.sol"; -import {IPBHVerifier} from "./interfaces/IPBHVerifier.sol"; import {IAggregator} from "@account-abstraction/contracts/interfaces/IAggregator.sol"; /// @title PBH Signature Aggregator @@ -29,10 +28,14 @@ contract PBHSignatureAggregator is IAggregator { * Revert if the aggregated signature does not match the given list of operations. * @param userOps - Array of UserOperations to validate the signature for. */ - function validateSignatures(PackedUserOperation[] calldata userOps, bytes calldata) external view { + function validateSignatures( + PackedUserOperation[] calldata userOps, + bytes calldata + ) external view { bytes memory encoded = abi.encode(userOps); - try pbhEntryPoint.validateSignaturesCallback(keccak256(encoded)) {} - catch { + try + pbhEntryPoint.validateSignaturesCallback(keccak256(encoded)) + {} catch { revert InvalidUserOperations(); } } @@ -46,11 +49,9 @@ contract PBHSignatureAggregator is IAggregator { * @return sigForUserOp - The value to put into the signature field of the userOp when calling handleOps. * (usually empty, unless account and aggregator support some kind of "multisig". */ - function validateUserOpSignature(PackedUserOperation calldata userOp) - external - pure - returns (bytes memory sigForUserOp) - {} + function validateUserOpSignature( + PackedUserOperation calldata userOp + ) external pure returns (bytes memory sigForUserOp) {} /** * Aggregate multiple signatures into a single value. @@ -59,17 +60,18 @@ contract PBHSignatureAggregator is IAggregator { * @param userOps - Array of UserOperations to collect the signatures from. * @return aggregatedSignature - The aggregated signature. */ - function aggregateSignatures(PackedUserOperation[] calldata userOps) - external - pure - returns (bytes memory aggregatedSignature) - { - IPBHVerifier.PBHPayload[] memory pbhPayloads = new IPBHVerifier.PBHPayload[](userOps.length); + function aggregateSignatures( + PackedUserOperation[] calldata userOps + ) external pure returns (bytes memory aggregatedSignature) { + IPBHEntryPoint.PBHPayload[] + memory pbhPayloads = new IPBHEntryPoint.PBHPayload[]( + userOps.length + ); for (uint256 i = 0; i < userOps.length; ++i) { // Bytes (0:65) - UserOp Signature // Bytes (65:65 + 352) - Packed Proof Data bytes memory proofData = userOps[i].signature[65:]; - pbhPayloads[i] = abi.decode(proofData, (IPBHVerifier.PBHPayload)); + pbhPayloads[i] = abi.decode(proofData, (IPBHEntryPoint.PBHPayload)); } aggregatedSignature = abi.encode(pbhPayloads); } diff --git a/contracts/test/PBHSignatureAggregator.t.sol b/contracts/test/PBHSignatureAggregator.t.sol index d155caee..cc3beb35 100644 --- a/contracts/test/PBHSignatureAggregator.t.sol +++ b/contracts/test/PBHSignatureAggregator.t.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.21; import {Setup} from "./Setup.sol"; -import {IPBHVerifier} from "../src/interfaces/IPBHVerifier.sol"; import {console} from "@forge-std/console.sol"; import {TestUtils} from "./TestUtils.sol"; import {IEntryPoint} from "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; +import {IPBHEntryPoint} from "../src/interfaces/IPBHEntryPoint.sol"; import "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; import "../src/helpers/PBHExternalNullifier.sol"; @@ -19,17 +19,22 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { uint256 timestamp = block.timestamp; uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(timestamp)); uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(timestamp)); - uint256 encoded = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 0, month, year); + uint256 encoded = PBHExternalNullifier.encode( + PBHExternalNullifier.V1, + 0, + month, + year + ); worldIDGroups.setVerifyProofSuccess(true); - IPBHVerifier.PBHPayload memory proof0 = IPBHVerifier.PBHPayload({ + IPBHEntryPoint.PBHPayload memory proof0 = IPBHEntryPoint.PBHPayload({ root: 1, pbhExternalNullifier: encoded, nullifierHash: 0, proof: [uint256(0), 0, 0, 0, 0, 0, 0, 0] }); - IPBHVerifier.PBHPayload memory proof1 = IPBHVerifier.PBHPayload({ + IPBHEntryPoint.PBHPayload memory proof1 = IPBHEntryPoint.PBHPayload({ root: 2, pbhExternalNullifier: encoded, nullifierHash: 1, @@ -40,21 +45,36 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { proofs[0] = abi.encode(proof0); proofs[1] = abi.encode(proof1); - PackedUserOperation[] memory uoTestFixture = createUOTestData(address(safe), proofs); - bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures(uoTestFixture); + PackedUserOperation[] memory uoTestFixture = createUOTestData( + address(safe), + proofs + ); + bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures( + uoTestFixture + ); - IEntryPoint.UserOpsPerAggregator[] memory userOpsPerAggregator = new IEntryPoint.UserOpsPerAggregator[](1); + IEntryPoint.UserOpsPerAggregator[] + memory userOpsPerAggregator = new IEntryPoint.UserOpsPerAggregator[]( + 1 + ); userOpsPerAggregator[0] = IEntryPoint.UserOpsPerAggregator({ aggregator: pbhAggregator, userOps: uoTestFixture, signature: aggregatedSignature }); - pbhEntryPoint.handleAggregatedOps(userOpsPerAggregator, payable(address(this))); + pbhEntryPoint.handleAggregatedOps( + userOpsPerAggregator, + payable(address(this)) + ); } - function testAggregateSignatures(uint256 root, uint256 pbhExternalNullifier, uint256 nullifierHash) public { - IPBHVerifier.PBHPayload memory proof = IPBHVerifier.PBHPayload({ + function testAggregateSignatures( + uint256 root, + uint256 pbhExternalNullifier, + uint256 nullifierHash + ) public { + IPBHEntryPoint.PBHPayload memory proof = IPBHEntryPoint.PBHPayload({ root: root, pbhExternalNullifier: pbhExternalNullifier, nullifierHash: nullifierHash, @@ -64,21 +84,41 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { bytes[] memory proofs = new bytes[](2); proofs[0] = abi.encode(proof); proofs[1] = abi.encode(proof); - PackedUserOperation[] memory uoTestFixture = createUOTestData(address(safe), proofs); - bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures(uoTestFixture); - IPBHVerifier.PBHPayload[] memory decodedProofs = abi.decode(aggregatedSignature, (IPBHVerifier.PBHPayload[])); + PackedUserOperation[] memory uoTestFixture = createUOTestData( + address(safe), + proofs + ); + bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures( + uoTestFixture + ); + IPBHEntryPoint.PBHPayload[] memory decodedProofs = abi.decode( + aggregatedSignature, + (IPBHEntryPoint.PBHPayload[]) + ); assertEq(decodedProofs.length, 2, "Decoded proof length should be 1"); assertEq(decodedProofs[0].root, proof.root, "Root should match"); assertEq( - decodedProofs[0].pbhExternalNullifier, proof.pbhExternalNullifier, "PBH External Nullifier should match" + decodedProofs[0].pbhExternalNullifier, + proof.pbhExternalNullifier, + "PBH External Nullifier should match" + ); + assertEq( + decodedProofs[0].nullifierHash, + proof.nullifierHash, + "Nullifier Hash should match" ); - assertEq(decodedProofs[0].nullifierHash, proof.nullifierHash, "Nullifier Hash should match"); assertEq(decodedProofs[1].root, proof.root, "Root should match"); assertEq( - decodedProofs[1].pbhExternalNullifier, proof.pbhExternalNullifier, "PBH External Nullifier should match" + decodedProofs[1].pbhExternalNullifier, + proof.pbhExternalNullifier, + "PBH External Nullifier should match" + ); + assertEq( + decodedProofs[1].nullifierHash, + proof.nullifierHash, + "Nullifier Hash should match" ); - assertEq(decodedProofs[1].nullifierHash, proof.nullifierHash, "Nullifier Hash should match"); } receive() external payable {} diff --git a/contracts/test/PBHVerifier.t.sol b/contracts/test/PBHVerifier.t.sol index d9b27037..94d974e6 100644 --- a/contracts/test/PBHVerifier.t.sol +++ b/contracts/test/PBHVerifier.t.sol @@ -6,6 +6,9 @@ import {MockWorldIDGroups} from "./mocks/MockWorldIDGroups.sol"; import {CheckInitialized} from "@world-id-contracts/utils/CheckInitialized.sol"; import {WorldIDImpl} from "@world-id-contracts/abstract/WorldIDImpl.sol"; import {ByteHasher} from "@helpers/ByteHasher.sol"; +import {IPBHEntryPoint} from "../src/interfaces/IPBHEntryPoint.sol"; +import {PBHEntryPointImplV1} from "../src/PBHEntryPointImplV1.sol"; + import "@BokkyPooBahsDateTimeLibrary/BokkyPooBahsDateTimeLibrary.sol"; import "@helpers/PBHExternalNullifier.sol"; import {Setup} from "./Setup.sol"; @@ -19,14 +22,14 @@ contract PBHVerifierTest is Setup { event PBH( address indexed sender, uint256 indexed nonce, - IPBHVerifier.PBHPayload payload + IPBHEntryPoint.PBHPayload payload ); event NumPbhPerMonthSet(uint8 indexed numPbhPerMonth); event WorldIdSet(address indexed worldId); /// @notice Test payload for the PBHVerifier - IPBHVerifier.PBHPayload public testPayload = - IPBHVerifier.PBHPayload({ + IPBHEntryPoint.PBHPayload public testPayload = + IPBHEntryPoint.PBHPayload({ root: 1, pbhExternalNullifier: getValidPBHExternalNullifier(), nullifierHash: 1, @@ -71,7 +74,7 @@ contract PBHVerifierTest is Setup { assertTrue(used, "Nullifier hash should be marked as used"); // Now try to use the same nullifier hash again - vm.expectRevert(PBHVerifier.InvalidNullifier.selector); + vm.expectRevert(PBHEntryPointImplV1.InvalidNullifier.selector); pbhEntryPoint.verifyPbh(sender, nonce, testCallData, testPayload); } From 2096812937c5289b672bb8cb80186bd7231afd5e Mon Sep 17 00:00:00 2001 From: 0xKitsune <0xkitsune@protonmail.com> Date: Mon, 23 Dec 2024 12:17:49 -0500 Subject: [PATCH 21/23] fmt --- contracts/src/interfaces/IPBHEntryPoint.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/src/interfaces/IPBHEntryPoint.sol b/contracts/src/interfaces/IPBHEntryPoint.sol index 6904b217..f2acf8d2 100644 --- a/contracts/src/interfaces/IPBHEntryPoint.sol +++ b/contracts/src/interfaces/IPBHEntryPoint.sol @@ -27,6 +27,7 @@ interface IPBHEntryPoint { IEntryPoint entryPoint, uint8 _numPbhPerMonth ) external; + function validateSignaturesCallback(bytes32 hashedOps) external view; function nullifierHashes(uint256) external view returns (bool); From e6e9f1db3560d2e08075ab13088f709cb0a6aded Mon Sep 17 00:00:00 2001 From: 0xKitsune <0xkitsune@protonmail.com> Date: Mon, 23 Dec 2024 12:27:25 -0500 Subject: [PATCH 22/23] fmt --- contracts/src/PBHEntryPointImplV1.sol | 78 ++++--------- contracts/src/PBHSignatureAggregator.sol | 31 +++-- contracts/src/interfaces/IPBHEntryPoint.sol | 13 +-- .../PBHEntryPointOwnershipManagement.t.sol | 106 ++++-------------- contracts/test/PBHSignatureAggregator.t.sol | 67 +++-------- contracts/test/PBHVerifier.t.sol | 57 +++------- 6 files changed, 89 insertions(+), 263 deletions(-) diff --git a/contracts/src/PBHEntryPointImplV1.sol b/contracts/src/PBHEntryPointImplV1.sol index 69c5e6dc..7ee4cbc7 100644 --- a/contracts/src/PBHEntryPointImplV1.sol +++ b/contracts/src/PBHEntryPointImplV1.sol @@ -65,9 +65,7 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { ////////////////////////////////////////////////////////////////////////////// event PBHEntryPointImplInitialized( - IWorldIDGroups indexed worldId, - IEntryPoint indexed entryPoint, - uint8 indexed numPbhPerMonth + IWorldIDGroups indexed worldId, IEntryPoint indexed entryPoint, uint8 indexed numPbhPerMonth ); /// @notice Emitted once for each successful PBH verification. @@ -75,11 +73,7 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @param sender The sender of this particular transaction or UserOp. /// @param nonce Transaction/UserOp nonce. /// @param payload The zero-knowledge proof that demonstrates the claimer is registered with World ID. - event PBH( - address indexed sender, - uint256 indexed nonce, - PBHPayload payload - ); + event PBH(address indexed sender, uint256 indexed nonce, PBHPayload payload); /// @notice Emitted when the World ID address is set. /// @@ -143,11 +137,10 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @param _numPbhPerMonth The number of allowed PBH transactions per month. /// /// @custom:reverts string If called more than once at the same initialisation number. - function initialize( - IWorldIDGroups _worldId, - IEntryPoint _entryPoint, - uint8 _numPbhPerMonth - ) external reinitializer(1) { + function initialize(IWorldIDGroups _worldId, IEntryPoint _entryPoint, uint8 _numPbhPerMonth) + external + reinitializer(1) + { // First, ensure that all of the parent contracts are initialised. __delegateInit(); @@ -157,11 +150,7 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { // Say that the contract is initialized. __setInitialized(); - emit PBHEntryPointImplInitialized( - _worldId, - _entryPoint, - _numPbhPerMonth - ); + emit PBHEntryPointImplInitialized(_worldId, _entryPoint, _numPbhPerMonth); } /// @notice Responsible for initialising all of the supertypes of this contract. @@ -186,21 +175,14 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { address payable beneficiary ) external virtual onlyProxy onlyInitialized { for (uint256 i = 0; i < opsPerAggregator.length; ++i) { - bytes32 hashedOps = keccak256( - abi.encode(opsPerAggregator[i].userOps) - ); + bytes32 hashedOps = keccak256(abi.encode(opsPerAggregator[i].userOps)); assembly ("memory-safe") { - if gt(tload(hashedOps), 0) { - revert(0, 0) - } + if gt(tload(hashedOps), 0) { revert(0, 0) } tstore(hashedOps, hashedOps) } - PBHPayload[] memory pbhPayloads = abi.decode( - opsPerAggregator[i].signature, - (PBHPayload[]) - ); + PBHPayload[] memory pbhPayloads = abi.decode(opsPerAggregator[i].signature, (PBHPayload[])); for (uint256 j = 0; j < pbhPayloads.length; ++j) { verifyPbh( opsPerAggregator[i].userOps[j].sender, @@ -216,13 +198,9 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @notice Validates the hashed operations is the same as the hash transiently stored. /// @param hashedOps The hashed operations to validate. - function validateSignaturesCallback( - bytes32 hashedOps - ) external view virtual onlyProxy onlyInitialized { + function validateSignaturesCallback(bytes32 hashedOps) external view virtual onlyProxy onlyInitialized { assembly ("memory-safe") { - if iszero(eq(tload(hashedOps), hashedOps)) { - revert(0, 0) - } + if iszero(eq(tload(hashedOps), hashedOps)) { revert(0, 0) } } } @@ -230,29 +208,25 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @param nonce Transaction/UserOp nonce. /// @param callData Transaction/UserOp call data. /// @param pbhPayload The PBH payload containing the proof data. - function verifyPbh( - address sender, - uint256 nonce, - bytes calldata callData, - PBHPayload memory pbhPayload - ) public virtual onlyInitialized onlyProxy { + function verifyPbh(address sender, uint256 nonce, bytes calldata callData, PBHPayload memory pbhPayload) + public + virtual + onlyInitialized + onlyProxy + { // First, we make sure this nullifier has not been used before. - if (nullifierHashes[pbhPayload.nullifierHash]) + if (nullifierHashes[pbhPayload.nullifierHash]) { revert InvalidNullifier(); + } // Verify the external nullifier - PBHExternalNullifier.verify( - pbhPayload.pbhExternalNullifier, - numPbhPerMonth - ); + PBHExternalNullifier.verify(pbhPayload.pbhExternalNullifier, numPbhPerMonth); // If worldId address is set, proceed with on chain verification, // otherwise assume verification has been done off chain by the builder. if (address(worldId) != address(0)) { // We now generate the signal hash from the sender, nonce, and calldata - uint256 signalHash = abi - .encodePacked(sender, nonce, callData) - .hashToField(); + uint256 signalHash = abi.encodePacked(sender, nonce, callData).hashToField(); // We now verify the provided proof is valid and the user is verified by World ID worldId.verifyProof( @@ -273,9 +247,7 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @notice Sets the number of PBH transactions allowed per month. /// @param _numPbhPerMonth The number of allowed PBH transactions per month. - function setNumPbhPerMonth( - uint8 _numPbhPerMonth - ) external virtual onlyOwner onlyProxy onlyInitialized { + function setNumPbhPerMonth(uint8 _numPbhPerMonth) external virtual onlyOwner onlyProxy onlyInitialized { numPbhPerMonth = _numPbhPerMonth; emit NumPbhPerMonthSet(_numPbhPerMonth); } @@ -283,9 +255,7 @@ contract PBHEntryPointImplV1 is IPBHEntryPoint, WorldIDImpl { /// @dev If the World ID address is set to 0, then it is assumed that verification will take place off chain. /// @notice Sets the World ID instance that will be used for verifying proofs. /// @param _worldId The World ID instance that will be used for verifying proofs. - function setWorldId( - address _worldId - ) external virtual onlyOwner onlyProxy onlyInitialized { + function setWorldId(address _worldId) external virtual onlyOwner onlyProxy onlyInitialized { worldId = IWorldIDGroups(_worldId); emit WorldIdSet(_worldId); } diff --git a/contracts/src/PBHSignatureAggregator.sol b/contracts/src/PBHSignatureAggregator.sol index 9555fb6d..e4d2255d 100644 --- a/contracts/src/PBHSignatureAggregator.sol +++ b/contracts/src/PBHSignatureAggregator.sol @@ -28,14 +28,10 @@ contract PBHSignatureAggregator is IAggregator { * Revert if the aggregated signature does not match the given list of operations. * @param userOps - Array of UserOperations to validate the signature for. */ - function validateSignatures( - PackedUserOperation[] calldata userOps, - bytes calldata - ) external view { + function validateSignatures(PackedUserOperation[] calldata userOps, bytes calldata) external view { bytes memory encoded = abi.encode(userOps); - try - pbhEntryPoint.validateSignaturesCallback(keccak256(encoded)) - {} catch { + try pbhEntryPoint.validateSignaturesCallback(keccak256(encoded)) {} + catch { revert InvalidUserOperations(); } } @@ -49,9 +45,11 @@ contract PBHSignatureAggregator is IAggregator { * @return sigForUserOp - The value to put into the signature field of the userOp when calling handleOps. * (usually empty, unless account and aggregator support some kind of "multisig". */ - function validateUserOpSignature( - PackedUserOperation calldata userOp - ) external pure returns (bytes memory sigForUserOp) {} + function validateUserOpSignature(PackedUserOperation calldata userOp) + external + pure + returns (bytes memory sigForUserOp) + {} /** * Aggregate multiple signatures into a single value. @@ -60,13 +58,12 @@ contract PBHSignatureAggregator is IAggregator { * @param userOps - Array of UserOperations to collect the signatures from. * @return aggregatedSignature - The aggregated signature. */ - function aggregateSignatures( - PackedUserOperation[] calldata userOps - ) external pure returns (bytes memory aggregatedSignature) { - IPBHEntryPoint.PBHPayload[] - memory pbhPayloads = new IPBHEntryPoint.PBHPayload[]( - userOps.length - ); + function aggregateSignatures(PackedUserOperation[] calldata userOps) + external + pure + returns (bytes memory aggregatedSignature) + { + IPBHEntryPoint.PBHPayload[] memory pbhPayloads = new IPBHEntryPoint.PBHPayload[](userOps.length); for (uint256 i = 0; i < userOps.length; ++i) { // Bytes (0:65) - UserOp Signature // Bytes (65:65 + 352) - Packed Proof Data diff --git a/contracts/src/interfaces/IPBHEntryPoint.sol b/contracts/src/interfaces/IPBHEntryPoint.sol index f2acf8d2..f5fb8890 100644 --- a/contracts/src/interfaces/IPBHEntryPoint.sol +++ b/contracts/src/interfaces/IPBHEntryPoint.sol @@ -22,22 +22,13 @@ interface IPBHEntryPoint { address payable beneficiary ) external; - function initialize( - IWorldIDGroups worldId, - IEntryPoint entryPoint, - uint8 _numPbhPerMonth - ) external; + function initialize(IWorldIDGroups worldId, IEntryPoint entryPoint, uint8 _numPbhPerMonth) external; function validateSignaturesCallback(bytes32 hashedOps) external view; function nullifierHashes(uint256) external view returns (bool); - function verifyPbh( - address sender, - uint256 nonce, - bytes memory callData, - PBHPayload memory pbhPayload - ) external; + function verifyPbh(address sender, uint256 nonce, bytes memory callData, PBHPayload memory pbhPayload) external; function setNumPbhPerMonth(uint8 _numPbhPerMonth) external; diff --git a/contracts/test/PBHEntryPointOwnershipManagement.t.sol b/contracts/test/PBHEntryPointOwnershipManagement.t.sol index d114c010..183225c6 100644 --- a/contracts/test/PBHEntryPointOwnershipManagement.t.sol +++ b/contracts/test/PBHEntryPointOwnershipManagement.t.sol @@ -24,10 +24,7 @@ contract PBHVerifierRouting is Setup { } /// @notice Taken from OwnableUpgradable.sol - event OwnershipTransferred( - address indexed previousOwner, - address indexed newOwner - ); + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); /// @notice Checks that it is possible to get the owner, and that the owner is correctly /// initialised. @@ -44,75 +41,32 @@ contract PBHVerifierRouting is Setup { function testTransferOwner(address newOwner) public { // Setup vm.assume(newOwner != nullAddress); - bytes memory transferCallData = abi.encodeCall( - Ownable2StepUpgradeable.transferOwnership, - (newOwner) - ); - bytes memory ownerCallData = abi.encodeCall( - OwnableUpgradeable.owner, - () - ); - bytes memory pendingOwnerCallData = abi.encodeCall( - Ownable2StepUpgradeable.pendingOwner, - () - ); - bytes memory acceptOwnerCallData = abi.encodeCall( - Ownable2StepUpgradeable.acceptOwnership, - () - ); + bytes memory transferCallData = abi.encodeCall(Ownable2StepUpgradeable.transferOwnership, (newOwner)); + bytes memory ownerCallData = abi.encodeCall(OwnableUpgradeable.owner, ()); + bytes memory pendingOwnerCallData = abi.encodeCall(Ownable2StepUpgradeable.pendingOwner, ()); + bytes memory acceptOwnerCallData = abi.encodeCall(Ownable2StepUpgradeable.acceptOwnership, ()); // Test - assertCallSucceedsOn( - pbhEntryPointAddress, - transferCallData, - new bytes(0x0) - ); - assertCallSucceedsOn( - pbhEntryPointAddress, - pendingOwnerCallData, - abi.encode(newOwner) - ); - assertCallSucceedsOn( - pbhEntryPointAddress, - ownerCallData, - abi.encode(thisAddress) - ); + assertCallSucceedsOn(pbhEntryPointAddress, transferCallData, new bytes(0x0)); + assertCallSucceedsOn(pbhEntryPointAddress, pendingOwnerCallData, abi.encode(newOwner)); + assertCallSucceedsOn(pbhEntryPointAddress, ownerCallData, abi.encode(thisAddress)); vm.expectEmit(true, true, true, true); emit OwnershipTransferred(thisAddress, newOwner); vm.prank(newOwner); - assertCallSucceedsOn( - pbhEntryPointAddress, - acceptOwnerCallData, - new bytes(0x0) - ); - assertCallSucceedsOn( - pbhEntryPointAddress, - ownerCallData, - abi.encode(newOwner) - ); + assertCallSucceedsOn(pbhEntryPointAddress, acceptOwnerCallData, new bytes(0x0)); + assertCallSucceedsOn(pbhEntryPointAddress, ownerCallData, abi.encode(newOwner)); } /// @notice Tests that only the pending owner can accept the ownership transfer. - function testCannotAcceptOwnershipAsNonPendingOwner( - address newOwner, - address notNewOwner - ) public { + function testCannotAcceptOwnershipAsNonPendingOwner(address newOwner, address notNewOwner) public { // Setup vm.assume(newOwner != nullAddress); vm.assume(notNewOwner != newOwner); - bytes memory callData = abi.encodeCall( - Ownable2StepUpgradeable.transferOwnership, - (newOwner) - ); - bytes memory acceptCallData = abi.encodeCall( - Ownable2StepUpgradeable.acceptOwnership, - () - ); - bytes memory expectedError = encodeStringRevert( - "Ownable2Step: caller is not the new owner" - ); + bytes memory callData = abi.encodeCall(Ownable2StepUpgradeable.transferOwnership, (newOwner)); + bytes memory acceptCallData = abi.encodeCall(Ownable2StepUpgradeable.acceptOwnership, ()); + bytes memory expectedError = encodeStringRevert("Ownable2Step: caller is not the new owner"); assertCallSucceedsOn(pbhEntryPointAddress, callData); vm.prank(notNewOwner); @@ -121,19 +75,11 @@ contract PBHVerifierRouting is Setup { } /// @notice Ensures that it is impossible to transfer ownership without being the owner. - function testCannotTransferOwnerIfNotOwner( - address naughty, - address newOwner - ) public { + function testCannotTransferOwnerIfNotOwner(address naughty, address newOwner) public { // Setup vm.assume(naughty != thisAddress && newOwner != nullAddress); - bytes memory callData = abi.encodeCall( - OwnableUpgradeable.transferOwnership, - (newOwner) - ); - bytes memory expectedReturn = encodeStringRevert( - "Ownable: caller is not the owner" - ); + bytes memory callData = abi.encodeCall(OwnableUpgradeable.transferOwnership, (newOwner)); + bytes memory expectedReturn = encodeStringRevert("Ownable: caller is not the owner"); vm.prank(naughty); // Test @@ -143,13 +89,8 @@ contract PBHVerifierRouting is Setup { /// @notice Tests that it is impossible to renounce ownership, even as the owner. function testCannotRenounceOwnershipAsOwner() public { // Setup - bytes memory renounceData = abi.encodeCall( - OwnableUpgradeable.renounceOwnership, - () - ); - bytes memory errorData = abi.encodeWithSelector( - WorldIDImpl.CannotRenounceOwnership.selector - ); + bytes memory renounceData = abi.encodeCall(OwnableUpgradeable.renounceOwnership, ()); + bytes memory errorData = abi.encodeWithSelector(WorldIDImpl.CannotRenounceOwnership.selector); // Test assertCallFailsOn(pbhEntryPointAddress, renounceData, errorData); @@ -159,13 +100,8 @@ contract PBHVerifierRouting is Setup { function testCannotRenounceOwnershipIfNotOwner(address naughty) public { // Setup vm.assume(naughty != thisAddress && naughty != nullAddress); - bytes memory callData = abi.encodeCall( - OwnableUpgradeable.renounceOwnership, - () - ); - bytes memory returnData = encodeStringRevert( - "Ownable: caller is not the owner" - ); + bytes memory callData = abi.encodeCall(OwnableUpgradeable.renounceOwnership, ()); + bytes memory returnData = encodeStringRevert("Ownable: caller is not the owner"); vm.prank(naughty); // Test diff --git a/contracts/test/PBHSignatureAggregator.t.sol b/contracts/test/PBHSignatureAggregator.t.sol index cc3beb35..03099185 100644 --- a/contracts/test/PBHSignatureAggregator.t.sol +++ b/contracts/test/PBHSignatureAggregator.t.sol @@ -19,12 +19,7 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { uint256 timestamp = block.timestamp; uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(timestamp)); uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(timestamp)); - uint256 encoded = PBHExternalNullifier.encode( - PBHExternalNullifier.V1, - 0, - month, - year - ); + uint256 encoded = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 0, month, year); worldIDGroups.setVerifyProofSuccess(true); IPBHEntryPoint.PBHPayload memory proof0 = IPBHEntryPoint.PBHPayload({ @@ -45,35 +40,20 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { proofs[0] = abi.encode(proof0); proofs[1] = abi.encode(proof1); - PackedUserOperation[] memory uoTestFixture = createUOTestData( - address(safe), - proofs - ); - bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures( - uoTestFixture - ); + PackedUserOperation[] memory uoTestFixture = createUOTestData(address(safe), proofs); + bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures(uoTestFixture); - IEntryPoint.UserOpsPerAggregator[] - memory userOpsPerAggregator = new IEntryPoint.UserOpsPerAggregator[]( - 1 - ); + IEntryPoint.UserOpsPerAggregator[] memory userOpsPerAggregator = new IEntryPoint.UserOpsPerAggregator[](1); userOpsPerAggregator[0] = IEntryPoint.UserOpsPerAggregator({ aggregator: pbhAggregator, userOps: uoTestFixture, signature: aggregatedSignature }); - pbhEntryPoint.handleAggregatedOps( - userOpsPerAggregator, - payable(address(this)) - ); + pbhEntryPoint.handleAggregatedOps(userOpsPerAggregator, payable(address(this))); } - function testAggregateSignatures( - uint256 root, - uint256 pbhExternalNullifier, - uint256 nullifierHash - ) public { + function testAggregateSignatures(uint256 root, uint256 pbhExternalNullifier, uint256 nullifierHash) public { IPBHEntryPoint.PBHPayload memory proof = IPBHEntryPoint.PBHPayload({ root: root, pbhExternalNullifier: pbhExternalNullifier, @@ -84,41 +64,22 @@ contract PBHSignatureAggregatorTest is TestUtils, Setup { bytes[] memory proofs = new bytes[](2); proofs[0] = abi.encode(proof); proofs[1] = abi.encode(proof); - PackedUserOperation[] memory uoTestFixture = createUOTestData( - address(safe), - proofs - ); - bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures( - uoTestFixture - ); - IPBHEntryPoint.PBHPayload[] memory decodedProofs = abi.decode( - aggregatedSignature, - (IPBHEntryPoint.PBHPayload[]) - ); + PackedUserOperation[] memory uoTestFixture = createUOTestData(address(safe), proofs); + bytes memory aggregatedSignature = pbhAggregator.aggregateSignatures(uoTestFixture); + IPBHEntryPoint.PBHPayload[] memory decodedProofs = + abi.decode(aggregatedSignature, (IPBHEntryPoint.PBHPayload[])); assertEq(decodedProofs.length, 2, "Decoded proof length should be 1"); assertEq(decodedProofs[0].root, proof.root, "Root should match"); assertEq( - decodedProofs[0].pbhExternalNullifier, - proof.pbhExternalNullifier, - "PBH External Nullifier should match" - ); - assertEq( - decodedProofs[0].nullifierHash, - proof.nullifierHash, - "Nullifier Hash should match" + decodedProofs[0].pbhExternalNullifier, proof.pbhExternalNullifier, "PBH External Nullifier should match" ); + assertEq(decodedProofs[0].nullifierHash, proof.nullifierHash, "Nullifier Hash should match"); assertEq(decodedProofs[1].root, proof.root, "Root should match"); assertEq( - decodedProofs[1].pbhExternalNullifier, - proof.pbhExternalNullifier, - "PBH External Nullifier should match" - ); - assertEq( - decodedProofs[1].nullifierHash, - proof.nullifierHash, - "Nullifier Hash should match" + decodedProofs[1].pbhExternalNullifier, proof.pbhExternalNullifier, "PBH External Nullifier should match" ); + assertEq(decodedProofs[1].nullifierHash, proof.nullifierHash, "Nullifier Hash should match"); } receive() external payable {} diff --git a/contracts/test/PBHVerifier.t.sol b/contracts/test/PBHVerifier.t.sol index 94d974e6..e8388f05 100644 --- a/contracts/test/PBHVerifier.t.sol +++ b/contracts/test/PBHVerifier.t.sol @@ -19,41 +19,26 @@ import {Setup} from "./Setup.sol"; contract PBHVerifierTest is Setup { using ByteHasher for bytes; - event PBH( - address indexed sender, - uint256 indexed nonce, - IPBHEntryPoint.PBHPayload payload - ); + event PBH(address indexed sender, uint256 indexed nonce, IPBHEntryPoint.PBHPayload payload); event NumPbhPerMonthSet(uint8 indexed numPbhPerMonth); event WorldIdSet(address indexed worldId); /// @notice Test payload for the PBHVerifier - IPBHEntryPoint.PBHPayload public testPayload = - IPBHEntryPoint.PBHPayload({ - root: 1, - pbhExternalNullifier: getValidPBHExternalNullifier(), - nullifierHash: 1, - proof: [uint256(0), 0, 0, 0, 0, 0, 0, 0] - }); + IPBHEntryPoint.PBHPayload public testPayload = IPBHEntryPoint.PBHPayload({ + root: 1, + pbhExternalNullifier: getValidPBHExternalNullifier(), + nullifierHash: 1, + proof: [uint256(0), 0, 0, 0, 0, 0, 0, 0] + }); uint256 internal nonce = 1; address internal sender = address(0x123); bytes internal testCallData = hex"deadbeef"; function getValidPBHExternalNullifier() public view returns (uint256) { - uint8 month = uint8( - BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp) - ); - uint16 year = uint16( - BokkyPooBahsDateTimeLibrary.getYear(block.timestamp) - ); - return - PBHExternalNullifier.encode( - PBHExternalNullifier.V1, - 0, - month, - year - ); + uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp)); + uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(block.timestamp)); + return PBHExternalNullifier.encode(PBHExternalNullifier.V1, 0, month, year); } /// @notice Test that a valid proof is verified correctly. @@ -81,20 +66,11 @@ contract PBHVerifierTest is Setup { /// @notice Test that setNumPBHPerMonth works as expected function testSetNumPBHPerMonth() public { MockWorldIDGroups(address(worldIDGroups)).setVerifyProofSuccess(true); - uint8 month = uint8( - BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp) - ); - uint16 year = uint16( - BokkyPooBahsDateTimeLibrary.getYear(block.timestamp) - ); + uint8 month = uint8(BokkyPooBahsDateTimeLibrary.getMonth(block.timestamp)); + uint16 year = uint16(BokkyPooBahsDateTimeLibrary.getYear(block.timestamp)); // Value starts at 30, make sure 30 reverts. - testPayload.pbhExternalNullifier = PBHExternalNullifier.encode( - PBHExternalNullifier.V1, - 30, - month, - year - ); + testPayload.pbhExternalNullifier = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 30, month, year); testPayload.nullifierHash = 0; vm.expectRevert(PBHExternalNullifier.InvalidPbhNonce.selector); pbhEntryPoint.verifyPbh(sender, nonce, testCallData, testPayload); @@ -111,12 +87,7 @@ contract PBHVerifierTest is Setup { pbhEntryPoint.setNumPbhPerMonth(40); // Try again, it should work - testPayload.pbhExternalNullifier = PBHExternalNullifier.encode( - PBHExternalNullifier.V1, - 30, - month, - year - ); + testPayload.pbhExternalNullifier = PBHExternalNullifier.encode(PBHExternalNullifier.V1, 30, month, year); testPayload.nullifierHash = 1; vm.expectEmit(true, true, true, true); From 6277f07abc71c5f3a8f6cfb50af3c0d9685913e4 Mon Sep 17 00:00:00 2001 From: karankurbur Date: Mon, 23 Dec 2024 09:48:46 -0800 Subject: [PATCH 23/23] use custom error on invalid proof size --- contracts/src/PBH4337Module.sol | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/contracts/src/PBH4337Module.sol b/contracts/src/PBH4337Module.sol index 430bbce3..898ea2ec 100644 --- a/contracts/src/PBH4337Module.sol +++ b/contracts/src/PBH4337Module.sol @@ -14,6 +14,8 @@ contract PBHSafe4337Module is Safe4337Module { address public immutable PBH_SIGNATURE_AGGREGATOR; uint192 public immutable PBH_NONCE_KEY; + error InvalidProofSize(); + constructor(address entryPoint, address _pbhSignatureAggregator, uint192 _pbhNonceKey) Safe4337Module(entryPoint) { PBH_SIGNATURE_AGGREGATOR = _pbhSignatureAggregator; PBH_NONCE_KEY = _pbhNonceKey; @@ -58,8 +60,9 @@ contract PBHSafe4337Module is Safe4337Module { // If the signature length is greater than the expected length, then we know that the bundler appended the proof // We need to remove the proof from the signature before validation if (isPBH && userOp.signature.length > expectedLength) { - // 352 bytes is the size of the encoded proof - require(userOp.signature.length - expectedLength == 352, "Invalid proof size in signature"); + if (userOp.signature.length - expectedLength != 352) { + revert InvalidProofSize(); + } // Remove the proof from the signature signatures = userOp.signature[0:expectedLength]; }