From 748de31d5463217483c842c67cca2be97950bec5 Mon Sep 17 00:00:00 2001 From: Nicolas Villanueva <1890113+MexicanAce@users.noreply.github.com> Date: Tue, 10 Dec 2024 14:48:26 +0000 Subject: [PATCH 1/4] fix: multi validator account creation (#217) --- src/validators/SessionKeyValidator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validators/SessionKeyValidator.sol b/src/validators/SessionKeyValidator.sol index df51f530..ecd3138a 100644 --- a/src/validators/SessionKeyValidator.sol +++ b/src/validators/SessionKeyValidator.sol @@ -68,7 +68,7 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { function init(bytes calldata data) external { // to prevent duplicate inits, since this can be hook plus a validator - if (!_isHookAndModuleInitialized(msg.sender) && data.length != 0) { + if (_isHookAndModuleInitialized(msg.sender) && data.length != 0) { require(_addValidationKey(data), "init failed"); } } From d03cbdc039ffab23ee69bcc3c7e7c38f4933820c Mon Sep 17 00:00:00 2001 From: Nicolas Villanueva <1890113+MexicanAce@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:38:39 +0000 Subject: [PATCH 2/4] Revert "fix: multi validator account creation" (#218) Revert "fix: multi validator account creation (#217)" This reverts commit 748de31d5463217483c842c67cca2be97950bec5. --- src/validators/SessionKeyValidator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validators/SessionKeyValidator.sol b/src/validators/SessionKeyValidator.sol index ecd3138a..df51f530 100644 --- a/src/validators/SessionKeyValidator.sol +++ b/src/validators/SessionKeyValidator.sol @@ -68,7 +68,7 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { function init(bytes calldata data) external { // to prevent duplicate inits, since this can be hook plus a validator - if (_isHookAndModuleInitialized(msg.sender) && data.length != 0) { + if (!_isHookAndModuleInitialized(msg.sender) && data.length != 0) { require(_addValidationKey(data), "init failed"); } } From 4984c66b13ff806c3a7731fa405dc37b84313322 Mon Sep 17 00:00:00 2001 From: Nicolas Villanueva <1890113+MexicanAce@users.noreply.github.com> Date: Wed, 11 Dec 2024 18:22:39 +0000 Subject: [PATCH 3/4] chore: add sepolia timestamp asserter address (#220) * chore: add sepolia timestamp asserter address * Update src/helpers/TimestampAsserterLocator.sol --- src/helpers/TimestampAsserterLocator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers/TimestampAsserterLocator.sol b/src/helpers/TimestampAsserterLocator.sol index 68698a8e..f01a666b 100644 --- a/src/helpers/TimestampAsserterLocator.sol +++ b/src/helpers/TimestampAsserterLocator.sol @@ -9,7 +9,7 @@ library TimestampAsserterLocator { return ITimestampAsserter(address(0x00000000000000000000000000000000808012)); } if (block.chainid == 300) { - revert("Timestamp asserter is not deployed on ZKsync Sepolia testnet yet"); + return ITimestampAsserter(address(0xa64EC71Ee812ac62923c85cf0796aA58573c4Cf3)); } if (block.chainid == 324) { revert("Timestamp asserter is not deployed on ZKsync mainnet yet"); From 4628172a77c50604c02100932b99670f9ec46337 Mon Sep 17 00:00:00 2001 From: cpb8010 Date: Wed, 11 Dec 2024 11:42:21 -0800 Subject: [PATCH 4/4] Session Validation Module Interface Update (#208) * chore: clean up unused code R1 validation and keys were unreachable after changing the init interface, K1 validation was unused since it was validated directly without a module (simple signature instead of modular signature) Fallback modules were entirely unused. Modules and exec modules could have been used, but were not and weren't supported by the factory to be installed at init. * fix: interface size dropping unused arg * fix: single module interface Moving to a single module to elimiate duplicate code * fix: replace modules with hooks This is much more direct path to installing instead of using the install to re-enter the account * fix: run all tests removing only locally tests pass * fix: revert this to older implemention This broke locally, but now fails in CI, maybe the converse is also true * chore: cleanup test code This wasn't necessary to pass in ci, but now fails locally :( * fix: await on test transaction failure * fix: have to await the reverted within expect This is just a test setup issue combined with a era-test-node change * fix: no validation hook Remove clave interface for validation, only need one interface * feat: move to two interfaces for module validation Would love to have a single interface, but 1271 makes the extra transaction argument difficult, so an easy workaround is just having two Having them named the same is also slightly confusing when one is only called from isValidSignature * feat: remove old validator interface So now there are only validation modules, k1 owners, and execution hooks. And only validation modules and k1 owners can be installed on creation. * feat: re-add execution hook also rename validation functions. Still trying to understand how the data is different without hook interface. * fix: gas estimation during session validation Couldn't find a better way to do this and I don't know how this wasn't a problem before? * fix: more merge conflicts * chore: cleanup accidental changes To reduce noise in diff * feat: re-allow validation type hooks These aren't being used, but are added for some unique flavor in case someone can find a use case. Removing the hook data concept as it's difficult to manage if the signer isn't aware of all of the hooks on the account. * fix: validation hook missing lines * fix: removed extra hash for gas estimates This turned out to just cost more gas with no benefit to estimation accuracy. * fix: cleanup * chore: format * fix: remove invalid test * fix: more fixes * fix: gas estimation * fix: add session during init --------- Co-authored-by: Lyova Potyomkin --- src/AAFactory.sol | 3 +- src/SsoAccount.sol | 38 ++++------- src/handlers/ERC1271Handler.sol | 7 +- src/handlers/ValidationHandler.sol | 9 ++- src/interfaces/IHook.sol | 5 +- src/interfaces/IHookManager.sol | 2 +- src/interfaces/IModuleValidator.sol | 9 ++- src/interfaces/ISsoAccount.sol | 6 +- src/libraries/SignatureDecoder.sol | 4 +- src/managers/HookManager.sol | 17 +---- src/validators/SessionKeyValidator.sol | 90 +++++++++----------------- src/validators/WebAuthValidator.sol | 13 +++- test/BasicTest.ts | 1 - test/SessionKeyTest.ts | 62 +++++++++--------- 14 files changed, 117 insertions(+), 149 deletions(-) diff --git a/src/AAFactory.sol b/src/AAFactory.sol index fe57503b..1cb9ccda 100644 --- a/src/AAFactory.sol +++ b/src/AAFactory.sol @@ -41,7 +41,6 @@ contract AAFactory is UpgradeableBeacon { bytes32 _salt, string calldata _uniqueAccountId, bytes[] calldata _initialValidators, - bytes[] calldata _initialHooks, address[] calldata _initialK1Owners ) external returns (address accountAddress) { require(accountMappings[_uniqueAccountId] == address(0), "Account already exists"); @@ -64,7 +63,7 @@ contract AAFactory is UpgradeableBeacon { (accountAddress) = abi.decode(returnData, (address)); // Initialize the newly deployed account with validators, hooks and K1 owners. - ISsoAccount(accountAddress).initialize(_initialValidators, _initialHooks, _initialK1Owners); + ISsoAccount(accountAddress).initialize(_initialValidators, _initialK1Owners); accountMappings[_uniqueAccountId] = accountAddress; diff --git a/src/SsoAccount.sol b/src/SsoAccount.sol index f4d21f4a..382e1c1e 100644 --- a/src/SsoAccount.sol +++ b/src/SsoAccount.sol @@ -41,22 +41,12 @@ contract SsoAccount is Initializable, HookManager, ERC1271Handler, TokenCallback /// @dev Sets passkey and passkey validator within account storage /// @param _initialValidators An array of module validator addresses and initial validation keys /// in an ABI encoded format of `abi.encode(validatorAddr,validationKey))`. - /// @param _initialValidationHooks An array of hook module validator addresses and initial validation keys - /// in an ABI encoded format of `abi.encode(validatorAddr,validationKey))`. /// @param _initialK1Owners An array of addresses with full control over the account. - function initialize( - bytes[] calldata _initialValidators, - bytes[] calldata _initialValidationHooks, - address[] calldata _initialK1Owners - ) external initializer { + function initialize(bytes[] calldata _initialValidators, address[] calldata _initialK1Owners) external initializer { for (uint256 i = 0; i < _initialValidators.length; ++i) { (address validatorAddr, bytes memory validationKey) = abi.decode(_initialValidators[i], (address, bytes)); _addModuleValidator(validatorAddr, validationKey); } - for (uint256 i = 0; i < _initialValidationHooks.length; ++i) { - (address validatorAddr, bytes memory validationKey) = abi.decode(_initialValidationHooks[i], (address, bytes)); - _installHook(validatorAddr, validationKey, true); - } for (uint256 i = 0; i < _initialK1Owners.length; ++i) { _k1AddOwner(_initialK1Owners[i]); } @@ -153,7 +143,6 @@ contract SsoAccount is Initializable, HookManager, ERC1271Handler, TokenCallback emit FeePaid(); } - /// @notice This function is called by the system if the transaction has a paymaster /// and prepares the interaction with the paymaster. /// @param _transaction The transaction data. @@ -176,23 +165,24 @@ contract SsoAccount is Initializable, HookManager, ERC1271Handler, TokenCallback /// @param _transaction The transaction data. /// @return The magic value if the validation was successful and bytes4(0) otherwise. function _validateTransaction(bytes32 _signedHash, Transaction calldata _transaction) internal returns (bytes4) { - if (_transaction.signature.length == 65) { - (address signer, ) = ECDSA.tryRecover(_signedHash, _transaction.signature); - return _k1IsOwner(signer) ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); - } - - // Extract the signature, validator address and hook data from the _transaction.signature - (bytes memory signature, address validator, bytes[] memory hookData) = SignatureDecoder.decodeSignature( - _transaction.signature - ); - // Run validation hooks - bool hookSuccess = runValidationHooks(_signedHash, _transaction, hookData); + bool hookSuccess = runValidationHooks(_signedHash, _transaction); if (!hookSuccess) { return bytes4(0); } - bool validationSuccess = _handleValidation(validator, _signedHash, signature); + if (_transaction.signature.length == 65) { + (address signer, ECDSA.RecoverError error) = ECDSA.tryRecover(_signedHash, _transaction.signature); + return + signer == address(0) || error != ECDSA.RecoverError.NoError || !_k1IsOwner(signer) + ? bytes4(0) + : ACCOUNT_VALIDATION_SUCCESS_MAGIC; + } + + // Extract the signature, validator address and hook data from the _transaction.signature + (bytes memory signature, address validator, ) = SignatureDecoder.decodeSignature(_transaction.signature); + + bool validationSuccess = _handleValidation(validator, _signedHash, signature, _transaction); return validationSuccess ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); } diff --git a/src/handlers/ERC1271Handler.sol b/src/handlers/ERC1271Handler.sol index 41bb5885..a8b4593f 100644 --- a/src/handlers/ERC1271Handler.sol +++ b/src/handlers/ERC1271Handler.sol @@ -2,8 +2,10 @@ pragma solidity ^0.8.24; import { IERC1271Upgradeable } from "@openzeppelin/contracts-upgradeable/interfaces/IERC1271Upgradeable.sol"; +import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol"; import { SignatureDecoder } from "../libraries/SignatureDecoder.sol"; +import { IModuleValidator } from "../interfaces/IModuleValidator.sol"; import { ValidationHandler } from "./ValidationHandler.sol"; import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; @@ -34,9 +36,10 @@ abstract contract ERC1271Handler is IERC1271Upgradeable, EIP712("Sso1271", "1.0. bytes32 eip712Hash = _hashTypedDataV4(_ssoMessageHash(SsoMessage(signedHash))); - bool valid = _handleValidation(validator, eip712Hash, signature); + bool isValid = _isModuleValidator(validator) && + IModuleValidator(validator).validateSignature(eip712Hash, signature); - magicValue = valid ? _ERC1271_MAGIC : bytes4(0); + magicValue = isValid ? _ERC1271_MAGIC : bytes4(0); } /** diff --git a/src/handlers/ValidationHandler.sol b/src/handlers/ValidationHandler.sol index 89b9d5b9..276a7245 100644 --- a/src/handlers/ValidationHandler.sol +++ b/src/handlers/ValidationHandler.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.24; +import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol"; + import { SignatureDecoder } from "../libraries/SignatureDecoder.sol"; import { BytesLinkedList } from "../libraries/LinkedList.sol"; import { OwnerManager } from "../managers/OwnerManager.sol"; @@ -17,10 +19,11 @@ abstract contract ValidationHandler is OwnerManager, ValidatorManager { function _handleValidation( address validator, bytes32 signedHash, - bytes memory signature - ) internal view returns (bool) { + bytes memory signature, + Transaction calldata transaction + ) internal returns (bool) { if (_isModuleValidator(validator)) { - return IModuleValidator(validator).handleValidation(signedHash, signature); + return IModuleValidator(validator).validateTransaction(signedHash, signature, transaction); } return false; diff --git a/src/interfaces/IHook.sol b/src/interfaces/IHook.sol index c8be5785..564a2526 100644 --- a/src/interfaces/IHook.sol +++ b/src/interfaces/IHook.sol @@ -5,8 +5,11 @@ import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/li import { IInitable } from "../interfaces/IInitable.sol"; import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +// Validation hooks are a non-standard way to always perform validation, +// They can't expect any specific transaction data or signature, but can be used to enforce +// additional restrictions on the account during the validation phase interface IValidationHook is IInitable, IERC165 { - function validationHook(bytes32 signedHash, Transaction calldata transaction, bytes calldata hookData) external; + function validationHook(bytes32 signedHash, Transaction calldata transaction) external; } interface IExecutionHook is IInitable, IERC165 { diff --git a/src/interfaces/IHookManager.sol b/src/interfaces/IHookManager.sol index 35952fa4..ff3485a7 100644 --- a/src/interfaces/IHookManager.sol +++ b/src/interfaces/IHookManager.sol @@ -20,7 +20,7 @@ interface IHookManager { /** * @notice Add a hook to the list of hooks and call it's init function - * @dev Can only be called by self or a module + * @dev Can only be called by self * @param hookAndData bytes calldata - Address of the hook and data to initialize it with * @param isValidation bool - True if the hook is a validation hook, false otherwise */ diff --git a/src/interfaces/IModuleValidator.sol b/src/interfaces/IModuleValidator.sol index 836b0c22..da749f79 100644 --- a/src/interfaces/IModuleValidator.sol +++ b/src/interfaces/IModuleValidator.sol @@ -3,13 +3,20 @@ pragma solidity ^0.8.24; import { IInitable } from "../interfaces/IInitable.sol"; import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol"; /** * @title Modular validator interface for native AA * @dev Add signature to module or validate existing signatures for acccount */ interface IModuleValidator is IInitable, IERC165 { - function handleValidation(bytes32 signedHash, bytes memory signature) external view returns (bool); + function validateTransaction( + bytes32 signedHash, + bytes memory signature, + Transaction calldata transaction + ) external returns (bool); + + function validateSignature(bytes32 signedHash, bytes memory signature) external view returns (bool); function addValidationKey(bytes memory key) external returns (bool); } diff --git a/src/interfaces/ISsoAccount.sol b/src/interfaces/ISsoAccount.sol index 3705cb17..36ae812a 100644 --- a/src/interfaces/ISsoAccount.sol +++ b/src/interfaces/ISsoAccount.sol @@ -28,9 +28,5 @@ interface ISsoAccount is { event FeePaid(); - function initialize( - bytes[] calldata initialValidators, - bytes[] calldata initialHooks, - address[] calldata k1Owners - ) external; + function initialize(bytes[] calldata initialValidators, address[] calldata k1Owners) external; } diff --git a/src/libraries/SignatureDecoder.sol b/src/libraries/SignatureDecoder.sol index ea48fdae..bc52026f 100644 --- a/src/libraries/SignatureDecoder.sol +++ b/src/libraries/SignatureDecoder.sol @@ -7,8 +7,8 @@ library SignatureDecoder { // Decode transaction.signature into signature, validator and hook data function decodeSignature( bytes calldata txSignature - ) internal pure returns (bytes memory signature, address validator, bytes[] memory hookData) { - (signature, validator, hookData) = abi.decode(txSignature, (bytes, address, bytes[])); + ) internal pure returns (bytes memory signature, address validator, bytes memory validatorData) { + (signature, validator, validatorData) = abi.decode(txSignature, (bytes, address, bytes)); } // Decode signature into signature and validator diff --git a/src/managers/HookManager.sol b/src/managers/HookManager.sol index 4e723f1b..44db44b0 100644 --- a/src/managers/HookManager.sol +++ b/src/managers/HookManager.sol @@ -109,23 +109,13 @@ abstract contract HookManager is IHookManager, Auth { } // Runs the validation hooks that are enabled by the account and returns true if none reverts - function runValidationHooks( - bytes32 signedHash, - Transaction calldata transaction, - bytes[] memory hookData - ) internal returns (bool) { + function runValidationHooks(bytes32 signedHash, Transaction calldata transaction) internal returns (bool) { mapping(address => address) storage validationHooks = _validationHooksLinkedList(); address cursor = validationHooks[AddressLinkedList.SENTINEL_ADDRESS]; - uint256 idx; // Iterate through hooks while (cursor > AddressLinkedList.SENTINEL_ADDRESS) { - // Call it with corresponding hookData - bool success = _call( - cursor, - abi.encodeCall(IValidationHook.validationHook, (signedHash, transaction, hookData[idx])) - ); - ++idx; + bool success = _call(cursor, abi.encodeCall(IValidationHook.validationHook, (signedHash, transaction))); if (!success) { return false; @@ -134,9 +124,6 @@ abstract contract HookManager is IHookManager, Auth { cursor = validationHooks[cursor]; } - // Ensure that hookData is not tampered with - if (hookData.length != idx) return false; - return true; } diff --git a/src/validators/SessionKeyValidator.sol b/src/validators/SessionKeyValidator.sol index df51f530..2ad93f59 100644 --- a/src/validators/SessionKeyValidator.sol +++ b/src/validators/SessionKeyValidator.sol @@ -4,17 +4,16 @@ pragma solidity ^0.8.24; import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -import { IValidationHook } from "../interfaces/IHook.sol"; import { IModuleValidator } from "../interfaces/IModuleValidator.sol"; import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol"; import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import { IHookManager } from "../interfaces/IHookManager.sol"; import { IValidatorManager } from "../interfaces/IValidatorManager.sol"; import { SessionLib } from "../libraries/SessionLib.sol"; +import { SignatureDecoder } from "../libraries/SignatureDecoder.sol"; -contract SessionKeyValidator is IValidationHook, IModuleValidator { +contract SessionKeyValidator is IModuleValidator { using SessionLib for SessionLib.SessionStorage; using EnumerableSet for EnumerableSet.Bytes32Set; @@ -40,15 +39,9 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { return sessions[sessionHash].status[account]; } - function handleValidation(bytes32 signedHash, bytes memory signature) external view returns (bool) { - // This only succeeds if the validationHook has previously succeeded for this hash. - uint256 slot = uint256(signedHash); - uint256 hookResult; - assembly { - hookResult := tload(slot) - } - require(hookResult == 1, "Can't call this function without calling validationHook"); - return true; + // This module should not be used to validate signatures + function validateSignature(bytes32 signedHash, bytes memory signature) external view returns (bool) { + return false; } function addValidationKey(bytes memory sessionData) external returns (bool) { @@ -57,8 +50,8 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { function createSession(SessionLib.SessionSpec memory sessionSpec) public { bytes32 sessionHash = keccak256(abi.encode(sessionSpec)); - require(_isHookInitialized(msg.sender), "Account not initialized"); - require(sessionSpec.signer != address(0), "Invalid signer"); + require(isInitialized(msg.sender), "Account not initialized"); + require(sessionSpec.signer != address(0), "Invalid signer (create)"); require(sessions[sessionHash].status[msg.sender] == SessionLib.Status.NotInitialized, "Session already exists"); require(sessionSpec.feeLimit.limitType != SessionLib.LimitType.Unlimited, "Unlimited fee allowance is not safe"); sessionCounter[msg.sender]++; @@ -67,9 +60,8 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { } function init(bytes calldata data) external { - // to prevent duplicate inits, since this can be hook plus a validator - if (!_isHookAndModuleInitialized(msg.sender) && data.length != 0) { - require(_addValidationKey(data), "init failed"); + if (data.length != 0) { + require(_addValidationKey(data), "Init failed"); } } @@ -80,17 +72,12 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { } function disable() external { - // Here we have to revoke all keys, so that if the module - // is installed again later, there will be no active sessions from the past. - // Problem: if there are too many keys, this will run out of gas. - // Solution: before uninstalling, require that all keys are revoked manually. - require(sessionCounter[msg.sender] == 0, "Revoke all keys first"); - - // Check module and hook independently so it's not stuck in a bad state - if (_isHookInitialized(msg.sender)) { - IHookManager(msg.sender).removeHook(address(this), true); - } - if (_isModuleInitialized(msg.sender)) { + if (isInitialized(msg.sender)) { + // Here we have to revoke all keys, so that if the module + // is installed again later, there will be no active sessions from the past. + // Problem: if there are too many keys, this will run out of gas. + // Solution: before uninstalling, require that all keys are revoked manually. + require(sessionCounter[msg.sender] == 0, "Revoke all keys first"); IValidatorManager(msg.sender).removeModuleValidator(address(this)); } } @@ -98,9 +85,7 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { function supportsInterface(bytes4 interfaceId) external pure override returns (bool) { return interfaceId != 0xffffffff && - (interfaceId == type(IERC165).interfaceId || - interfaceId == type(IValidationHook).interfaceId || - interfaceId == type(IModuleValidator).interfaceId); + (interfaceId == type(IERC165).interfaceId || interfaceId == type(IModuleValidator).interfaceId); } // TODO: make the session owner able revoke its own key, in case it was leaked, to prevent further misuse? @@ -122,42 +107,31 @@ contract SessionKeyValidator is IValidationHook, IModuleValidator { * @param smartAccount The smart account to check * @return true if validator is registered for the account, false otherwise */ - function isInitialized(address smartAccount) external view returns (bool) { - return _isHookAndModuleInitialized(smartAccount); - } - - function _isHookAndModuleInitialized(address smartAccount) internal view returns (bool) { - return _isHookInitialized(smartAccount) && _isModuleInitialized(smartAccount); - } - - function _isHookInitialized(address smartAccount) internal view returns (bool) { - return IHookManager(smartAccount).isHook(address(this)); - } - - function _isModuleInitialized(address smartAccount) internal view returns (bool) { + function isInitialized(address smartAccount) public view returns (bool) { return IValidatorManager(smartAccount).isModuleValidator(address(this)); } - function validationHook(bytes32 signedHash, Transaction calldata transaction, bytes calldata hookData) external { - (bytes memory signature, address validator, ) = abi.decode(transaction.signature, (bytes, address, bytes[])); - if (validator != address(this)) { - // This transaction is not meant to be validated by this module - return; - } + function validateTransaction( + bytes32 signedHash, + bytes memory _signature, + Transaction calldata transaction + ) external returns (bool) { + (bytes memory transactionSignature, address validator, bytes memory validatorData) = SignatureDecoder + .decodeSignature(transaction.signature); (SessionLib.SessionSpec memory spec, uint64[] memory periodIds) = abi.decode( - hookData, + validatorData, // this is passed by the signature builder (SessionLib.SessionSpec, uint64[]) ); - (address recoveredAddress, ) = ECDSA.tryRecover(signedHash, signature); - require(recoveredAddress == spec.signer, "Invalid signer"); + require(spec.signer != address(0), "Invalid signer (empty)"); bytes32 sessionHash = keccak256(abi.encode(spec)); + // this generally throws instead of returning false sessions[sessionHash].validate(transaction, spec, periodIds); - - // Set the validation result to 1 for this hash, so that isValidSignature succeeds - uint256 slot = uint256(signedHash); - assembly { - tstore(slot, 1) + (address recoveredAddress, ECDSA.RecoverError recoverError) = ECDSA.tryRecover(signedHash, transactionSignature); + if (recoverError != ECDSA.RecoverError.NoError || recoveredAddress == address(0)) { + return false; } + require(recoveredAddress == spec.signer, "Invalid signer (mismatch)"); + return true; } /** diff --git a/src/validators/WebAuthValidator.sol b/src/validators/WebAuthValidator.sol index 02c3ed14..c2e24c09 100644 --- a/src/validators/WebAuthValidator.sol +++ b/src/validators/WebAuthValidator.sol @@ -1,6 +1,9 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.24; +import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol"; +import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; + import { IModuleValidator } from "../interfaces/IModuleValidator.sol"; import { VerifierCaller } from "../helpers/VerifierCaller.sol"; import { JsmnSolLib } from "../libraries/JsmnSolLib.sol"; @@ -50,7 +53,15 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator { return initialLowerHalf == 0 && initialUpperHalf == 0; } - function handleValidation(bytes32 signedHash, bytes memory signature) external view returns (bool) { + function validateSignature(bytes32 signedHash, bytes memory signature) external view returns (bool) { + return webAuthVerify(signedHash, signature); + } + + function validateTransaction( + bytes32 signedHash, + bytes memory signature, + Transaction calldata _transaction + ) external view returns (bool) { return webAuthVerify(signedHash, signature); } diff --git a/test/BasicTest.ts b/test/BasicTest.ts index 39dd125b..70f4a8a3 100644 --- a/test/BasicTest.ts +++ b/test/BasicTest.ts @@ -40,7 +40,6 @@ describe("Basic tests", function () { randomBytes(32), "id", [], - [], [fixtures.wallet.address], ); const deployTxReceipt = await deployTx.wait(); diff --git a/test/SessionKeyTest.ts b/test/SessionKeyTest.ts index 815405d5..44393eec 100644 --- a/test/SessionKeyTest.ts +++ b/test/SessionKeyTest.ts @@ -148,14 +148,14 @@ class SessionTester { this.sessionOwner = new Wallet(Wallet.createRandom().privateKey, provider); this.sessionAccount = new SmartAccount({ payloadSigner: async (hash) => abiCoder.encode( - ["bytes", "address", "bytes[]"], + ["bytes", "address", "bytes"], [ this.sessionOwner.signingKey.sign(hash).serialized, sessionKeyModuleAddress, - [abiCoder.encode( + abiCoder.encode( [sessionSpecAbi, "uint64[]"], [this.session, await this.periodIds(this.aaTransaction.to!, this.aaTransaction.data?.slice(0, 10))], - )], // this array supplies data for hooks + ), ], ), address: this.proxyAccountAddress, @@ -178,7 +178,7 @@ class SessionTester { const aaTx = { ...await this.aaTxTemplate(), to: await sessionKeyModuleContract.getAddress(), - data: sessionKeyModuleContract.interface.encodeFunctionData("createSession", [this.session]), + data: sessionKeyModuleContract.interface.encodeFunctionData("createSession", [this.session]) }; aaTx.gasLimit = await provider.estimateGas(aaTx); @@ -250,12 +250,11 @@ class SessionTester { async sendTxSuccess(txRequest: ethers.TransactionLike = {}) { this.aaTransaction = { - ...await this.aaTxTemplate(), + ...await this.aaTxTemplate(await this.periodIds(txRequest.to!, txRequest.data?.slice(0, 10))), ...txRequest, }; - const estimatedGas = await provider.estimateGas(this.aaTransaction); - this.aaTransaction.gasLimit = BigInt(Math.ceil(Number(estimatedGas) * 1.1)); - logInfo(`\`sessionTx\` gas estimated: ${await provider.estimateGas(this.aaTransaction)}`); + this.aaTransaction.gasLimit = await provider.estimateGas(this.aaTransaction); + logInfo(`\`sessionTx\` gas estimated: ${this.aaTransaction.gasLimit}`); const signedTransaction = await this.sessionAccount.signTransaction(this.aaTransaction); const tx = await provider.broadcastTransaction(signedTransaction); @@ -265,7 +264,7 @@ class SessionTester { async sendTxFail(tx: ethers.TransactionLike = {}) { this.aaTransaction = { - ...await this.aaTxTemplate(), + ...await this.aaTxTemplate(await this.periodIds(tx.to!, tx.data?.slice(0, 10))), gasLimit: 100_000_000n, ...tx, }; @@ -277,21 +276,21 @@ class SessionTester { getLimit(limit?: PartialLimit): SessionLib.UsageLimitStruct { return limit == null ? { - limitType: LimitType.Unlimited, - limit: 0, - period: 0, - } + limitType: LimitType.Unlimited, + limit: 0, + period: 0, + } : limit.period == null ? { - limitType: LimitType.Lifetime, - limit: limit.limit, - period: 0, - } + limitType: LimitType.Lifetime, + limit: limit.limit, + period: 0, + } : { - limitType: LimitType.Allowance, - limit: limit.limit, - period: limit.period, - }; + limitType: LimitType.Allowance, + limit: limit.limit, + period: limit.period, + }; } getSession(session: PartialSession): SessionLib.SessionSpecStruct { @@ -320,8 +319,7 @@ class SessionTester { }; } - async aaTxTemplate() { - const numberOfConstraints = this.session.callPolicies.map((policy) => policy.constraints.length); + async aaTxTemplate(periodIds?: number[]) { return { type: 113, from: this.proxyAccountAddress, @@ -332,17 +330,17 @@ class SessionTester { gasPrice: await provider.getGasPrice(), customData: { gasPerPubdata: utils.DEFAULT_GAS_PER_PUBDATA_LIMIT, - customSignature: abiCoder.encode( - ["bytes", "address", "bytes[]"], + customSignature: periodIds ? abiCoder.encode( + ["bytes", "address", "bytes"], [ ethers.zeroPadValue("0x1b", 65), await fixtures.getSessionKeyModuleAddress(), - [abiCoder.encode( + abiCoder.encode( [sessionSpecAbi, "uint64[]"], - [this.session, new Array(2 + Math.max(0, ...numberOfConstraints)).fill(0)], - )], + [this.session, periodIds], + ), ], - ), + ) : undefined, }, gasLimit: 0n, }; @@ -390,7 +388,6 @@ describe("SessionKeyModule tests", function () { randomBytes(32), "session-key-test-id", [sessionKeyPayload], - [sessionKeyPayload], [fixtures.wallet.address], ); @@ -403,7 +400,7 @@ describe("SessionKeyModule tests", function () { const account = SsoAccount__factory.connect(proxyAccountAddress, provider); assert(await account.k1IsOwner(fixtures.wallet.address)); - assert(await account.isHook(sessionKeyModuleAddress), "session key module should be a hook"); + assert(!await account.isHook(sessionKeyModuleAddress), "session key module should not be an execution hook"); assert(await account.isModuleValidator(sessionKeyModuleAddress), "session key module should be a validator"); }); @@ -436,6 +433,7 @@ describe("SessionKeyModule tests", function () { value: parseEther("0.02"), }); }); + }); describe("ERC20 transfer limit tests", function () { @@ -601,7 +599,5 @@ describe("SessionKeyModule tests", function () { }); // TODO: module uninstall tests - // TODO: session expiresAt tests // TODO: session fee limit tests - // TODO: allowance tests });