diff --git a/contracts/AccountFactory.sol b/contracts/AccountFactory.sol index 0842b9e..8f8cc38 100644 --- a/contracts/AccountFactory.sol +++ b/contracts/AccountFactory.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.17; import {DEPLOYER_SYSTEM_CONTRACT, IContractDeployer} from '@matterlabs/zksync-contracts/l2/system-contracts/Constants.sol'; import {SystemContractsCaller} from '@matterlabs/zksync-contracts/l2/system-contracts/libraries/SystemContractsCaller.sol'; -import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; +import {Ownable, Ownable2Step} from '@openzeppelin/contracts/access/Ownable2Step.sol'; import {Errors} from './libraries/Errors.sol'; import {IClaveRegistry} from './interfaces/IClaveRegistry.sol'; @@ -12,9 +12,13 @@ import {IClaveRegistry} from './interfaces/IClaveRegistry.sol'; * @title Factory contract to create Clave accounts in zkSync Era * @author https://getclave.io */ -contract AccountFactory is Ownable { - // Addresses of the implementation and registry contract +contract AccountFactory is Ownable2Step { + // Address of the account implementation address public implementationAddress; + // Selector of the account initializer function + bytes4 public initializerSelector; + + // Account registry contract address address public registry; // Account creation bytecode hash @@ -63,12 +67,14 @@ contract AccountFactory is Ownable { */ constructor( address _implementation, + bytes4 _initializerSelector, address _registry, bytes32 _proxyBytecodeHash, address _deployer, address _owner ) Ownable(_owner) { implementationAddress = _implementation; + initializerSelector = _initializerSelector; registry = _registry; proxyBytecodeHash = _proxyBytecodeHash; deployer = _deployer; @@ -84,8 +90,21 @@ contract AccountFactory is Ownable { function deployAccount( bytes32 salt, bytes memory initializer - ) external returns (address accountAddress) { - + ) external payable returns (address accountAddress) { + // Check that the initializer is not empty + if (initializer.length < 4) { + revert Errors.INVALID_INITIALIZER(); + } + // Check that the initializer selector is correct + { + bytes4 selector; + assembly ('memory-safe') { + selector := mload(add(initializer, 0x20)) + } + if (selector != initializerSelector) { + revert Errors.INVALID_INITIALIZER(); + } + } // Deploy the implementation contract (bool success, bytes memory returnData) = SystemContractsCaller.systemCallWithReturndata( uint32(gasleft()), @@ -118,7 +137,7 @@ contract AccountFactory is Ownable { initializeSuccess := call( gas(), accountAddress, - 0, + callvalue(), add(initializer, 0x20), mload(initializer), 0, @@ -161,8 +180,9 @@ contract AccountFactory is Ownable { * @notice Changes the implementation contract address * @param newImplementation address - Address of the new implementation contract */ - function changeImplementation(address newImplementation) external onlyOwner { + function changeImplementation(address newImplementation, bytes4 newInitializerSelector) external onlyOwner { implementationAddress = newImplementation; + initializerSelector = newInitializerSelector; emit ImplementationChanged(newImplementation); } diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 6d0bc00..aeb892d 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -64,10 +64,15 @@ contract ClaveImplementation is address initialK1Validator, bytes[] calldata modules, Call calldata initCall - ) public initializer { + ) public payable initializer { __ERC1271Handler_init(); // check that this account is being deployed by the initial signer or the factory authorized deployer AccountFactory factory = AccountFactory(msg.sender); + // require a specific salt for the acccount based on the initial k1 owner + address expectedAddress = factory.getAddressForSalt(keccak256(abi.encodePacked(initialK1Owner))); + if (address(this) != expectedAddress) { + revert Errors.INVALID_SALT(); + } address thisDeployer = factory.accountToDeployer(address(this)); if (thisDeployer != factory.deployer()) { if (initialK1Owner != thisDeployer) { @@ -91,8 +96,27 @@ contract ClaveImplementation is } } - // Receive function to allow ETHs + // Receive function to allow ETH to be sent to the account + // with no additional calldata receive() external payable {} + + // Fallback function to allow ETH to be sent to the account + // with arbitrary calldata to mirror the behavior of an EOA + fallback() external payable { + // Simulate the behavior of the EOA if it is called via `delegatecall`. + address codeAddress = SystemContractHelper.getCodeAddress(); + if (codeAddress != address(this)) { + // If the function was delegate called, behave like an EOA. + assembly { + return(0, 0) + } + } + + // fallback of default account shouldn't be called by bootloader under any circumstances + assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS); + + // If the contract is called directly, behave like an EOA + } /** * @notice Called by the bootloader to validate that an account agrees to process the transaction @@ -172,6 +196,7 @@ contract ClaveImplementation is revert Errors.VALIDATION_HOOK_FAILED(); } + _incrementNonce(transaction.nonce); _executeTransaction(transaction); } @@ -223,11 +248,6 @@ contract ClaveImplementation is bytes32 signedHash, Transaction calldata transaction ) internal returns (bytes4 magicValue) { - if (transaction.signature.length == 65) { - // This is a gas estimation - return 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); @@ -235,13 +255,10 @@ contract ClaveImplementation is // Run validation hooks bool hookSuccess = runValidationHooks(signedHash, transaction, hookData); - if (!hookSuccess) { - return bytes4(0); - } - + // Handle validation bool valid = _handleValidation(validator, signedHash, signature); - magicValue = valid ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); + magicValue = (hookSuccess && valid) ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0); } function _executeTransaction( diff --git a/contracts/ClaveRegistry.sol b/contracts/ClaveRegistry.sol index 00dadae..e1545dd 100644 --- a/contracts/ClaveRegistry.sol +++ b/contracts/ClaveRegistry.sol @@ -1,12 +1,12 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.17; -import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol'; +import {Ownable, Ownable2Step} from '@openzeppelin/contracts/access/Ownable2Step.sol'; import {Errors} from './libraries/Errors.sol'; import {IClaveRegistry} from './interfaces/IClaveRegistry.sol'; -contract ClaveRegistry is Ownable, IClaveRegistry { +contract ClaveRegistry is Ownable2Step, IClaveRegistry { mapping(address => bool) public isFactory; // Mapping of Clave accounts mapping(address => bool) public isClave; diff --git a/contracts/handlers/ValidationHandler.sol b/contracts/handlers/ValidationHandler.sol index f0bf186..07cc3d5 100644 --- a/contracts/handlers/ValidationHandler.sol +++ b/contracts/handlers/ValidationHandler.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.17; import {SignatureDecoder} from '../libraries/SignatureDecoder.sol'; -import {BytesLinkedList} from '../libraries/LinkedList.sol'; +import {BytesLinkedList, AddressLinkedList} from '../libraries/LinkedList.sol'; import {OwnerManager} from '../managers/OwnerManager.sol'; import {ValidatorManager} from '../managers/ValidatorManager.sol'; @@ -19,7 +19,11 @@ abstract contract ValidationHandler is OwnerManager, ValidatorManager { bytes32 signedHash, bytes memory signature ) internal view returns (bool) { - if (_r1IsValidator(validator)) { + if (validator <= AddressLinkedList.SENTINEL_ADDRESS) { + // address less than or equal to sentinel address can't be used in linked list + // implementation so this scenario is never valid + return false; + } else if (_r1IsValidator(validator)) { mapping(bytes => bytes) storage owners = OwnerManager._r1OwnersLinkedList(); bytes memory cursor = owners[BytesLinkedList.SENTINEL_BYTES]; while (cursor.length > BytesLinkedList.SENTINEL_LENGTH) { diff --git a/contracts/libraries/Errors.sol b/contracts/libraries/Errors.sol index c2e4f31..c998e45 100644 --- a/contracts/libraries/Errors.sol +++ b/contracts/libraries/Errors.sol @@ -100,6 +100,8 @@ library Errors { error DEPLOYMENT_FAILED(); // 0x0f02d218 error INITIALIZATION_FAILED(); // 0x5b101091 + error INVALID_INITIALIZER(); + error INVALID_SALT(); /*////////////////////////////////////////////////////////////// PAYMASTER diff --git a/contracts/managers/HookManager.sol b/contracts/managers/HookManager.sol index 5633822..6c9d333 100644 --- a/contracts/managers/HookManager.sol +++ b/contracts/managers/HookManager.sol @@ -85,6 +85,11 @@ abstract contract HookManager is IHookManager, Auth { uint256 idx = 0; // Iterate through hooks while (cursor > AddressLinkedList.SENTINEL_ADDRESS) { + // hookData array is out of bounds for the number of hooks + if (idx >= hookData.length) { + return false; + } + // Call it with corresponding hookData bool success = _call( cursor, @@ -171,6 +176,11 @@ abstract contract HookManager is IHookManager, Auth { } else { _executionHooksLinkedList().remove(hook); } + + // if the hook removal occured during execution of hooks, the hook + // context will not be cleaned up during post execution so we need + // to delete it manually + _deleteContext(hook); (bool success, ) = hook.excessivelySafeCall( gasleft(), diff --git a/contracts/managers/OwnerManager.sol b/contracts/managers/OwnerManager.sol index 92bf27a..c5e7d2d 100644 --- a/contracts/managers/OwnerManager.sol +++ b/contracts/managers/OwnerManager.sol @@ -91,13 +91,6 @@ abstract contract OwnerManager is IOwnerManager, Auth { function _r1RemoveOwner(bytes calldata pubKey) internal { _r1OwnersLinkedList().remove(pubKey); - // The wallet should have at least one owner - if ( - _k1OwnersLinkedList().isEmpty() && _r1OwnersLinkedList().isEmpty() - ) { - revert Errors.EMPTY_OWNERS(); - } - emit R1RemoveOwner(pubKey); } @@ -105,9 +98,7 @@ abstract contract OwnerManager is IOwnerManager, Auth { _k1OwnersLinkedList().remove(addr); // The wallet should have at least one owner - if ( - _k1OwnersLinkedList().isEmpty() && _r1OwnersLinkedList().isEmpty() - ) { + if (_k1OwnersLinkedList().isEmpty()) { revert Errors.EMPTY_OWNERS(); } diff --git a/contracts/managers/ValidatorManager.sol b/contracts/managers/ValidatorManager.sol index 91f51a6..60b97f9 100644 --- a/contracts/managers/ValidatorManager.sol +++ b/contracts/managers/ValidatorManager.sol @@ -85,14 +85,6 @@ abstract contract ValidatorManager is IValidatorManager, Auth { function _r1RemoveValidator(address validator) internal { _r1ValidatorsLinkedList().remove(validator); - // At least one validator must be present - if ( - _r1ValidatorsLinkedList().isEmpty() && - _k1ValidatorsLinkedList().isEmpty() - ) { - revert Errors.EMPTY_VALIDATORS(); - } - emit R1RemoveValidator(validator); } @@ -100,10 +92,7 @@ abstract contract ValidatorManager is IValidatorManager, Auth { _k1ValidatorsLinkedList().remove(validator); // At least one validator must be present - if ( - _r1ValidatorsLinkedList().isEmpty() && - _k1ValidatorsLinkedList().isEmpty() - ) { + if (_k1ValidatorsLinkedList().isEmpty()) { revert Errors.EMPTY_VALIDATORS(); } diff --git a/test/deployments/deployer.test.ts b/test/deployments/deployer.test.ts index 3c9f136..f21ac24 100644 --- a/test/deployments/deployer.test.ts +++ b/test/deployments/deployer.test.ts @@ -6,7 +6,7 @@ import { expect } from 'chai'; import type { ec } from 'elliptic'; import type { BytesLike, HDNodeWallet } from 'ethers'; -import { parseEther } from 'ethers'; +import { getAddress, hexlify, parseEther, randomBytes } from 'ethers'; import * as hre from 'hardhat'; import type { Contract, Wallet } from 'zksync-ethers'; import { Provider } from 'zksync-ethers'; @@ -104,5 +104,40 @@ describe('Clave Contracts - Deployer class tests', () => { expect(await registry.isClave(accountAddress)).to.be.true; expect(await registry.isClave(factoryAddress)).not.to.be.true; }); + + it('should not deploy an account with an invalid salt', async () => { + const salt = randomBytes(32); + await expect(deployer.account(wallet, factory, eoaValidator, { salt: hexlify(salt) })) + .to.be.revertedWithCustomError(factory, "INITIALIZATION_FAILED"); + }); + + it('should not deploy an account with an empty initializer', async () => { + await expect(deployer.account(wallet, factory, eoaValidator, { initializer: '0x' })) + .to.be.revertedWithCustomError(factory, "INVALID_INITIALIZER"); + }); + + it('should not deploy an account with an invalid initializer selector', async () => { + await expect(deployer.account(wallet, factory, eoaValidator, { initializer: '0xabababab' })) + .to.be.revertedWithCustomError(factory, "INVALID_INITIALIZER"); + }); + + it('should deploy an account with a payable initial call', async () => { + const target = getAddress('0x0000000000000000000000000000000000abcdef'); + const initialCall = { + target, + allowFailure: false, + value: parseEther('0.9'), + callData: '0x', + }; + const otherWallet = getWallet(hre, LOCAL_RICH_WALLETS[1].privateKey); + const newAccount = await deployer.account(otherWallet, factory, eoaValidator, { + initialCall, + callValue: parseEther('1') + }); + + expect(await provider.getBalance(target)).to.eq(parseEther('0.9')); + expect(await provider.getBalance(await newAccount.getAddress())).to.eq(parseEther('0.1')); + + }); }); }); diff --git a/test/utils/deployer.ts b/test/utils/deployer.ts index acda762..e92d1ff 100644 --- a/test/utils/deployer.ts +++ b/test/utils/deployer.ts @@ -3,8 +3,7 @@ * Unauthorized copying of this file, via any medium is strictly prohibited * Proprietary and confidential */ -import type { ec } from 'elliptic'; -import { AbiCoder, HDNodeWallet, ZeroAddress, keccak256, parseEther } from 'ethers'; +import { AbiCoder, BaseWallet, BigNumberish, HDNodeWallet, ZeroAddress, keccak256, parseEther } from 'ethers'; import type { HardhatRuntimeEnvironment } from 'hardhat/types'; import type { Wallet } from 'zksync-ethers'; import { Contract, utils } from 'zksync-ethers'; @@ -12,7 +11,6 @@ import { Contract, utils } from 'zksync-ethers'; import { deployContract, getWallet } from '../../deploy/utils'; import type { CallStruct } from '../../typechain-types/contracts/batch/BatchCaller'; import { CONTRACT_NAMES, PAYMASTERS, type VALIDATORS } from './names'; -import { encodePublicKeyK1 } from './p256'; // This class helps deploy Clave contracts for the tests export class ClaveDeployer { @@ -88,6 +86,7 @@ export class ClaveDeployer { CONTRACT_NAMES.FACTORY, [ await implementation.getAddress(), + '0xb4e581f5', await registry.getAddress(), bytecodeHash, this.deployerWallet.address, @@ -130,21 +129,37 @@ export class ClaveDeployer { } public async account( - wallet: HDNodeWallet, + wallet: BaseWallet, factory: Contract, validator: Contract, + overrideValues: { + salt?: string, + initializer?: string, + callValue?: BigNumberish, + initialCall?: CallStruct, + } = {}, ): Promise { - - const salt = keccak256(wallet.address); - const call: CallStruct = { - target: ZeroAddress, - allowFailure: false, - value: 0, - callData: '0x', - }; + let { salt, initializer, callValue, initialCall } = overrideValues; + + if (!salt) { + salt = keccak256(wallet.address); + } + if (callValue === undefined) { + callValue = 0; + } + if (!initialCall) { + initialCall = { + target: ZeroAddress, + allowFailure: false, + value: 0, + callData: '0x', + }; + } const abiCoder = AbiCoder.defaultAbiCoder(); - const initializer = + + if (!initializer) { + initializer = '0xb4e581f5' + abiCoder .encode( @@ -159,19 +174,20 @@ export class ClaveDeployer { await validator.getAddress(), [], [ - call.target, - call.allowFailure, - call.value, - call.callData, + initialCall.target, + initialCall.allowFailure, + initialCall.value, + initialCall.callData, ], ], ) .slice(2); + } const deployPromise = await Promise.all([ // Deploy account (async (): Promise => { - const deployTx = await factory.deployAccount(salt, initializer); + const deployTx = await factory.deployAccount(salt, initializer, { value: callValue }); await deployTx.wait(); })(), // Calculate new account address