From 739f928b66d074bbfcd68b73faf254f3d6e0a31c Mon Sep 17 00:00:00 2001 From: vladbochok Date: Wed, 20 Nov 2024 14:59:10 +0800 Subject: [PATCH] refactor: improve code quality --- packages/contracts/src/AAFactory.sol | 5 - packages/contracts/src/SsoAccount.sol | 267 +++++++----------- packages/contracts/src/batch/BatchCaller.sol | 15 +- .../src/handlers/ValidationHandler.sol | 3 - packages/contracts/src/libraries/Errors.sol | 1 + .../contracts/src/managers/HookManager.sol | 5 +- .../src/validators/PasskeyValidator.sol | 2 - .../src/validators/WebAuthValidator.sol | 19 -- 8 files changed, 113 insertions(+), 204 deletions(-) diff --git a/packages/contracts/src/AAFactory.sol b/packages/contracts/src/AAFactory.sol index 990cdcf8..edea6a31 100644 --- a/packages/contracts/src/AAFactory.sol +++ b/packages/contracts/src/AAFactory.sol @@ -7,8 +7,6 @@ import "@matterlabs/zksync-contracts/l2/system-contracts/libraries/SystemContrac import { ISsoAccount } from "./interfaces/ISsoAccount.sol"; import { UpgradeableBeacon } from "./UpgradeableBeacon.sol"; -import "./helpers/Logger.sol"; - contract AAFactory is UpgradeableBeacon { event AccountCreated(address indexed accountAddress, string uniqueAccountId); @@ -43,9 +41,6 @@ contract AAFactory is UpgradeableBeacon { (accountAddress) = abi.decode(returnData, (address)); - Logger.logString("accountAddress"); - Logger.logAddress(accountAddress); - // add session-key/spend-limit module (similar code) ISsoAccount(accountAddress).initialize(initialValidators, initialModules, initialK1Owners); diff --git a/packages/contracts/src/SsoAccount.sol b/packages/contracts/src/SsoAccount.sol index fdaaaf05..cd270451 100644 --- a/packages/contracts/src/SsoAccount.sol +++ b/packages/contracts/src/SsoAccount.sol @@ -24,13 +24,13 @@ import { BatchCaller } from "./batch/BatchCaller.sol"; import { ISsoAccount } from "./interfaces/ISsoAccount.sol"; -import "./helpers/Logger.sol"; - -/** - * @title Main account contract from the Clave wallet infrastructure in ZKsync Era - * @author https://getclave.io - */ - +/// @title SSO Account +/// @author Matter Labs +/// @custom:security-contact security@matterlabs.dev +/// @notice The implementation is inspired by Clave wallet. +/// @notice This contract is a modular and extensible account implementation with support of +/// multi-ownership, custom modules, validation/execution hooks and different signature validation formats. +/// @dev Contract is expected to be used as Beacon proxy implementation. contract SsoAccount is Initializable, HookManager, @@ -43,136 +43,119 @@ contract SsoAccount is // Helper library for the Transaction struct using TransactionHelper for Transaction; - /** - * @notice Constructor for the account implementation - */ constructor() { - // address batchCaller _disableInitializers(); } - /** - * @notice Initializer function for the account contract - * @dev Sets passkey and passkey validator within account storage - * @param initialValidators bytes[] calldata - Validator addresses and init data for validation modules - * @param initialModules bytes[] - Non-validator modules and init data for validation modules - */ + /// @notice Initializer function that sets account initial configuration. Expected to be used in the proxy. + /// @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 _initialModules An array of native module addresses and their initialize data + /// in an ABI encoded format of `abi.encode(moduleAddr,initData))`. + /// @param _initialK1Owners An array of addresses with full control over the account. function initialize( - bytes[] calldata initialValidators, - bytes[] calldata initialModules, - address[] calldata initialK1Owners + bytes[] calldata _initialValidators, + bytes[] calldata _initialModules, + address[] calldata _initialK1Owners ) external initializer { - for (uint256 validatorIndex = 0; validatorIndex < initialValidators.length; validatorIndex++) { - (address validatorAddress, bytes memory validatorData) = abi.decode( - initialValidators[validatorIndex], - (address, bytes) - ); - _addModuleValidator(validatorAddress, validatorData); + for (uint256 i = 0; i < _initialValidators.length; ++i) { + (address validatorAddr, bytes memory validationKey) = abi.decode(initialValidators[i], (address, bytes)); + _addModuleValidator(validatorAddr, validationKey); } - for (uint256 moduleIndex = 0; moduleIndex < initialModules.length; moduleIndex++) { - (address moduleAddress, bytes memory moduleData) = abi.decode(initialModules[moduleIndex], (address, bytes)); - _addNativeModule(moduleAddress, moduleData); + for (uint256 i = 0; i < _initialModules.length; ++i) { + (address moduleAddr, bytes memory initData) = abi.decode(_initialModules[i], (address, bytes)); + _addNativeModule(moduleAddr, initData); } - for (uint256 ownerIndex = 0; ownerIndex < initialK1Owners.length; ownerIndex++) { - _k1AddOwner(initialK1Owners[ownerIndex]); + for (uint256 i = 0; i < _initialK1Owners.length; ++i) { + _k1AddOwner(_initialK1Owners[i]); } } - // Receive function to allow ETHs + /// @dev Account might receive/hold base tokens. receive() external payable {} - /** - * @notice Called by the bootloader to validate that an account agrees to process the transaction - * (and potentially pay for it). - * @dev The developer should strive to preserve as many steps as possible both for valid - * and invalid transactions as this very method is also used during the gas fee estimation - * (without some of the necessary data, e.g. signature). - * @param - bytes32 - Not used - * @param suggestedSignedHash bytes32 - The suggested hash of the transaction that is signed by the signer - * @param transaction Transaction calldata - The transaction itself - * @return magic bytes4 - The magic value that should be equal to the signature of this function - * if the user agrees to proceed with the transaction. - */ + /// @notice Called by the bootloader to validate that an account agrees to process the transaction + /// (and potentially pay for it). + /// @dev The developer should strive to preserve as many steps as possible both for valid + /// and invalid transactions as this very method is also used during the gas fee estimation + /// (without some of the necessary data, e.g. signature). + /// @param _suggestedSignedHash The suggested hash of the transaction that is signed by the signer. + /// @param _transaction The transaction data. + /// @return magic The magic value that should be equal to the signature of this function. + /// if the user agrees to proceed with the transaction. function validateTransaction( bytes32, - bytes32 suggestedSignedHash, - Transaction calldata transaction + bytes32 _suggestedSignedHash, + Transaction calldata _transaction ) external payable override onlyBootloader returns (bytes4 magic) { - // FIXME session txs have their own nonce managers, - // so they have to not alter this nonce - _incrementNonce(transaction.nonce); + // TODO: session txs have their own nonce managers, so they have to not alter this nonce + _incrementNonce(_transaction.nonce); - // The fact there is enough balance for the account - // should be checked explicitly to prevent user paying for fee for a - // transaction that wouldn't be included on Ethereum. - if (transaction.totalRequiredBalance() > address(this).balance) { + // If there is not enough balance for the transaction, the account should reject it + // on the validation step to prevent paying fees for revertable transactions. + if (_transaction.totalRequiredBalance() > address(this).balance) { revert Errors.INSUFFICIENT_FUNDS(); } // While the suggested signed hash is usually provided, it is generally // not recommended to rely on it to be present, since in the future // there may be tx types with no suggested signed hash. - bytes32 signedHash = suggestedSignedHash == bytes32(0) - ? transaction.encodeHash() // TODO: this hash needs to depend on the signature type? - : suggestedSignedHash; + bytes32 signedHash = _suggestedSignedHash == bytes32(0) ? _transaction.encodeHash() : _suggestedSignedHash; - magic = _validateTransaction(signedHash, transaction); + magic = _validateTransaction(signedHash, _transaction); } - /** - * @notice Called by the bootloader to make the account execute the transaction. - * @dev The transaction is considered successful if this function does not revert - * @param - bytes32 - Not used - * @param - bytes32 - Not used - * @param transaction Transaction calldata - The transaction itself - */ + /// @notice Called by the bootloader to make the account execute the transaction. + /// @dev The transaction is considered successful if this function does not revert + /// @param _transaction The transaction data. function executeTransaction( bytes32, bytes32, - Transaction calldata transaction - ) external payable override onlyBootloader { - _executeTransaction(transaction); - } + Transaction calldata _transaction + ) external payable override onlyBootloader runExecutionHooks(_transaction) { + address to = _safeCastToAddress(_transaction.to); + uint128 value = Utils.safeCastToU128(_transaction.value); - /** - * @notice This function allows an EOA to start a transaction for the account. - * @dev There is no point in providing possible signed hash in the `executeTransactionFromOutside` method, - * since it typically should not be trusted. - * @param transaction Transaction calldata - The transaction itself - */ - function executeTransactionFromOutside(Transaction calldata transaction) external payable override { - // Check if msg.sender is authorized - if (!_k1IsOwner(msg.sender)) { - revert Errors.UNAUTHORIZED_OUTSIDE_TRANSACTION(); - } - - // Extract hook data from transaction.signature - bytes[] memory hookData = SignatureDecoder.decodeSignatureOnlyHookData(transaction.signature); + _executeCall(to, value, transaction.data); + } - // Get the hash of the transaction - bytes32 signedHash = transaction.encodeHash(); + /// @notice Executes a call to a given address with a specified value and calldata. + /// @param to The address to which the call is made. + /// @param value The value to send along with the call. + /// @param data The calldata to pass along with the call. + function _executeCall(address _to, uint128 _value, bytes calldata _data) internal { + uint32 gas = Utils.safeCastToU32(gasleft()); - // Run the validation hooks - if (!runValidationHooks(signedHash, transaction, hookData)) { - revert Errors.VALIDATION_HOOK_FAILED(); + if (_to == address(DEPLOYER_SYSTEM_CONTRACT)) { + // Note, that the deployer contract can only be called with a "systemCall" flag. + SystemContractsCaller.systemCallWithPropagatedRevert(gas, _to, _value, _data); + } else { + bool success = EfficientCall.rawCall(gas, _to, _value, _data, false); + if (!success) { + EfficientCall.propagateRevert(); + } } + } - _executeTransaction(transaction); + /// @notice This function allows an EOA to start a transaction for the account. The main purpose of which is + /// to have and entry point for escaping funds when L2 transactions are censored by the chain, and only + /// forced transactions are accepted by the network. + /// @dev It is not implemented yet. + /// @param _transaction The transaction data. + function executeTransactionFromOutside(Transaction calldata _transaction) external payable override { + revert Errors.METHOD_NOT_IMPLEMENTED(); } - /** - * @notice This function allows the account to pay for its own gas and used when there is no paymaster - * @param - bytes32 - not used - * @param - bytes32 - not used - * @param transaction Transaction calldata - Transaction to pay for - * @dev "This method must send at least `tx.gasprice * tx.gasLimit` ETH to the bootloader address." - */ + /// @notice This function allows the account to pay for its own gas and used when there is no paymaster. + /// @param _transaction The transaction data. + /// @dev This method must send at least `tx.gasprice * tx.gasLimit` ETH to the bootloader address. function payForTransaction( bytes32, bytes32, - Transaction calldata transaction + Transaction calldata _transaction ) external payable override onlyBootloader { - bool success = transaction.payToTheBootloader(); + bool success = _transaction.payToTheBootloader(); if (!success) { revert Errors.FEE_PAYMENT_FAILED(); @@ -181,19 +164,15 @@ contract SsoAccount is emit FeePaid(); } - /** - * @notice This function is called by the system if the transaction has a paymaster - and prepares the interaction with the paymaster - * @param - bytes32 - not used - * @param - bytes32 - not used - * @param transaction Transaction - The transaction itself - */ + /// @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. function prepareForPaymaster( bytes32, bytes32, - Transaction calldata transaction + Transaction calldata _transaction ) external payable override onlyBootloader { - transaction.processPaymasterInput(); + _transaction.processPaymasterInput(); } /// @dev type(ISsoAccount).interfaceId indicates SSO accounts @@ -201,80 +180,48 @@ contract SsoAccount is return interfaceId == type(ISsoAccount).interfaceId || super.supportsInterface(interfaceId); } - function _validateTransaction( - bytes32 signedHash, - Transaction calldata transaction - ) internal returns (bytes4 magicValue) { - if (transaction.signature.length == 65) { - (address signer, ) = ECDSA.tryRecover(signedHash, transaction.signature); - Logger.logString("recovered EOA signer"); - Logger.logAddress(signer); - if (signer == address(0)) { - return bytes4(0); - } + /// @notice Validates the provided transaction by validating signature of ECDSA k1 owner. + /// or running validation hooks and signature validation in the provided validator address. + /// @param _signedHash The signed hash of the transaction. + /// @param _transaction The transaction data. + /// @return The magic value if the validation was successful and bytes4(0) otherwise. + function _validateTransaction(bytes32 _signedHash, bytes 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 + // Extract the signature, validator address and hook data from the _transaction.signature (bytes memory signature, address validator, bytes[] memory hookData) = SignatureDecoder.decodeSignature( - transaction.signature + _transaction.signature ); - Logger.logString("validator address"); - Logger.logAddress(validator); - // Run validation hooks - bool hookSuccess = runValidationHooks(signedHash, transaction, hookData); + bool hookSuccess = runValidationHooks(_signedHash, _transaction, hookData); if (!hookSuccess) { - Logger.logString("failed hook validation"); return bytes4(0); } - bool valid = _handleValidation(validator, signedHash, signature); - - magicValue = valid ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); - } - - function _executeTransaction(Transaction calldata transaction) internal runExecutionHooks(transaction) { - address to = _safeCastToAddress(transaction.to); - uint128 value = Utils.safeCastToU128(transaction.value); - bytes calldata data = transaction.data; - - _executeCall(to, value, data, false); - } - - function _executeCall(address to, uint128 value, bytes calldata data, bool allowFailure) internal { - uint32 gas = Utils.safeCastToU32(gasleft()); - - if (to == address(DEPLOYER_SYSTEM_CONTRACT)) { - // Note, that the deployer contract can only be called - // with a "systemCall" flag. - (bool success, bytes memory returnData) = SystemContractsCaller.systemCallWithReturndata(gas, to, value, data); - if (!success && !allowFailure) { - assembly { - let size := mload(returnData) - revert(add(returnData, 0x20), size) - } - } - } else { - bool success = EfficientCall.rawCall(gas, to, value, data, false); - if (!success && !allowFailure) { - EfficientCall.propagateRevert(); - } - } + bool validationSuccess = _handleValidation(validator, _signedHash, signature); + return validationSuccess ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); } - function _incrementNonce(uint256 nonce) internal { + /// @dev Increments the nonce value in Nonce Holder system contract to ensure replay attack protection. + /// @dev Reverts if the Nonce Holder stores different `_nonce` value from the expected one. + /// @param _expectedNonce The nonce value expected for the account to be stored in the Nonce Holder. + function _incrementNonce(uint256 _expectedNonce) internal { SystemContractsCaller.systemCallWithPropagatedRevert( uint32(gasleft()), address(NONCE_HOLDER_SYSTEM_CONTRACT), 0, - abi.encodeCall(INonceHolder.incrementMinNonceIfEquals, (nonce)) + abi.encodeCall(INonceHolder.incrementMinNonceIfEquals, (_expectedNonce)) ); } - function _safeCastToAddress(uint256 value) internal pure returns (address) { - if (value > type(uint160).max) revert(); + /// @dev Safely casts a uint256 to an address. + /// @dev Revert if the value exceeds the maximum size for an address (160 bits). + function _safeCastToAddress(uint256 _value) internal pure returns (address) { + if (_value > type(uint160).max) revert(); return address(uint160(value)); } } diff --git a/packages/contracts/src/batch/BatchCaller.sol b/packages/contracts/src/batch/BatchCaller.sol index 2de4e124..3bb50a2c 100644 --- a/packages/contracts/src/batch/BatchCaller.sol +++ b/packages/contracts/src/batch/BatchCaller.sol @@ -22,23 +22,12 @@ contract BatchCaller { function batchCall(Call[] calldata calls) external payable { require(msg.sender == address(this), "External calls not allowed"); - bool isDelegateCall = SystemContractHelper.getCodeAddress() != address(this); - if (!isDelegateCall) { - revert Errors.ONLY_DELEGATECALL(); - } - // Execute each call uint256 len = calls.length; uint256 totalValue = 0; - Call calldata calli; for (uint256 i = 0; i < len; ) { - calli = calls[i]; - address target = calli.target; - uint256 value = calli.value; - bytes calldata callData = calli.callData; - totalValue += calli.value; - - bool success = EfficientCall.rawCall(gasleft(), target, value, callData, false); + totalValue += calls[i].value; + bool success = EfficientCall.rawCall(gasleft(), calls[i].target, calls[i].value, calls[i].callData, false); if (!calls[i].allowFailure && !success) { revert Errors.CALL_FAILED(); } diff --git a/packages/contracts/src/handlers/ValidationHandler.sol b/packages/contracts/src/handlers/ValidationHandler.sol index 12d1b942..757bb6de 100644 --- a/packages/contracts/src/handlers/ValidationHandler.sol +++ b/packages/contracts/src/handlers/ValidationHandler.sol @@ -9,8 +9,6 @@ import { ValidatorManager } from "../managers/ValidatorManager.sol"; import { IK1Validator, IR1Validator } from "../interfaces/IValidator.sol"; import { IModuleValidator } from "../interfaces/IModuleValidator.sol"; -import "../helpers/Logger.sol"; - /** * @title ValidationHandler * @notice Contract which calls validators for signature validation @@ -47,7 +45,6 @@ abstract contract ValidationHandler is OwnerManager, ValidatorManager { return true; } } else if (_isModuleValidator(validator)) { - Logger.logString("_isModuleValidator"); return IModuleValidator(validator).handleValidation(signedHash, signature); } diff --git a/packages/contracts/src/libraries/Errors.sol b/packages/contracts/src/libraries/Errors.sol index e368e954..03126411 100644 --- a/packages/contracts/src/libraries/Errors.sol +++ b/packages/contracts/src/libraries/Errors.sol @@ -10,6 +10,7 @@ library Errors { error FEE_PAYMENT_FAILED(); error UNAUTHORIZED_OUTSIDE_TRANSACTION(); error VALIDATION_HOOK_FAILED(); + error METHOD_NOT_IMPLEMENTED(); /*////////////////////////////////////////////////////////////// LINKED LIST diff --git a/packages/contracts/src/managers/HookManager.sol b/packages/contracts/src/managers/HookManager.sol index a187703a..985c0119 100644 --- a/packages/contracts/src/managers/HookManager.sol +++ b/packages/contracts/src/managers/HookManager.sol @@ -131,14 +131,15 @@ abstract contract HookManager is IHookManager, Auth { mapping(address => address) storage validationHooks = _validationHooksLinkedList(); address cursor = validationHooks[AddressLinkedList.SENTINEL_ADDRESS]; - uint256 idx = 0; + uint256 idx; // Iterate through hooks while (cursor > AddressLinkedList.SENTINEL_ADDRESS) { // Call it with corresponding hookData bool success = _call( cursor, - abi.encodeWithSelector(IValidationHook.validationHook.selector, signedHash, transaction, hookData[idx++]) + abi.encodeCall(IValidationHook.validationHook, (signedHash, transaction, hookData[idx])) ); + ++idx; if (!success) { return false; diff --git a/packages/contracts/src/validators/PasskeyValidator.sol b/packages/contracts/src/validators/PasskeyValidator.sol index cb3a5b97..5169900e 100644 --- a/packages/contracts/src/validators/PasskeyValidator.sol +++ b/packages/contracts/src/validators/PasskeyValidator.sol @@ -9,8 +9,6 @@ import { VerifierCaller } from "../helpers/VerifierCaller.sol"; import { JsmnSolLib } from "../libraries/JsmnSolLib.sol"; import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; -import "../helpers/Logger.sol"; - /** * @title validator contract for passkey r1 signatures * @author https://getclave.io diff --git a/packages/contracts/src/validators/WebAuthValidator.sol b/packages/contracts/src/validators/WebAuthValidator.sol index dd044e88..883ceb5f 100644 --- a/packages/contracts/src/validators/WebAuthValidator.sol +++ b/packages/contracts/src/validators/WebAuthValidator.sol @@ -4,8 +4,6 @@ pragma solidity ^0.8.24; import { IModuleValidator } from "../interfaces/IModuleValidator.sol"; import "./PasskeyValidator.sol"; -import "../helpers/Logger.sol"; - /** * @title validator contract for passkey r1 signatures * @author https://getclave.io @@ -29,10 +27,6 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator { } function handleValidation(bytes32 signedHash, bytes memory signature) external view returns (bool) { - // Printing this hash makes capturing this for a replay test easier - Logger.logString("signed hash"); - Logger.logBytes32(signedHash); - return webAuthVerify(signedHash, signature); } @@ -42,13 +36,11 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator { ); if (rs[1] > lowSmax) { - Logger.logString("malleability check failed"); return false; } // check if the flags are set if (authenticatorData[32] & AUTH_DATA_MASK != AUTH_DATA_MASK) { - Logger.logString("auth data mask failed"); return false; } @@ -56,8 +48,6 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator { // TODO: test if the parse fails for more than 10 elements, otherwise can have a malicious header (uint returnValue, JsmnSolLib.Token[] memory tokens, uint actualNum) = JsmnSolLib.parse(clientDataJSON, 20); if (returnValue != 0) { - Logger.logString("failed to parse json"); - Logger.logUint(returnValue); return false; } @@ -76,38 +66,30 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator { string memory challengeValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end); // this should only be set once, otherwise this is an error if (validChallenge) { - Logger.logString("duplicate challenge, bad json!"); return false; } // this is the key part to ensure the signature is for the provided transaction bytes memory challengeDataArray = Base64.decode(challengeValue); if (challengeDataArray.length != 32) { // wrong hash size - Logger.logString("invalid hash data length in json challenge field"); return false; } bytes32 challengeData = abi.decode(challengeDataArray, (bytes32)); validChallenge = challengeData == transactionHash; - Logger.logString("validChallenge"); - Logger.logBool(validChallenge); } else if (Strings.equal(keyOrValue, "type")) { JsmnSolLib.Token memory nextT = tokens[index + 1]; string memory typeValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end); // this should only be set once, otherwise this is an error if (validType) { - Logger.logString("duplicate type field, bad json"); return false; } validType = Strings.equal("webauthn.get", typeValue); - Logger.logString("valid type"); - Logger.logBool(validType); } else if (Strings.equal(keyOrValue, "origin")) { JsmnSolLib.Token memory nextT = tokens[index + 1]; string memory originValue = JsmnSolLib.getBytes(clientDataJSON, nextT.start, nextT.end); // this should only be set once, otherwise this is an error if (validOrigin) { - Logger.logString("duplicate origin field, bad json"); return false; } pubKey[0] = lowerKeyHalf[originValue][msg.sender]; @@ -121,7 +103,6 @@ contract WebAuthValidator is PasskeyValidator, IModuleValidator { } if (!validChallenge || !validType) { - Logger.logString("invalid challenge or type"); return false; }