From 9b661e3b3a9b978e7a3134d63f493f5c905c3f12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Tue, 12 Nov 2024 11:41:31 -0500 Subject: [PATCH 01/12] [TRST-M-1][M-05]: Require specific deployment salt and initializer selector for account deployment --- contracts/AccountFactory.sol | 26 +++++++++++++++++++++++--- contracts/ClaveImplementation.sol | 5 +++++ contracts/libraries/Errors.sol | 2 ++ test/deployments/deployer.test.ts | 18 +++++++++++++++++- test/utils/deployer.ts | 15 ++++++++++----- 5 files changed, 57 insertions(+), 9 deletions(-) diff --git a/contracts/AccountFactory.sol b/contracts/AccountFactory.sol index 0842b9e..d9501f3 100644 --- a/contracts/AccountFactory.sol +++ b/contracts/AccountFactory.sol @@ -13,8 +13,12 @@ import {IClaveRegistry} from './interfaces/IClaveRegistry.sol'; * @author https://getclave.io */ contract AccountFactory is Ownable { - // Addresses of the implementation and registry contract + // 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; @@ -85,7 +91,20 @@ contract AccountFactory is Ownable { bytes32 salt, bytes memory initializer ) external 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()), @@ -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..0711e94 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -68,6 +68,11 @@ contract ClaveImplementation is __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) { diff --git a/contracts/libraries/Errors.sol b/contracts/libraries/Errors.sol index 1c8f4dc..ee9c314 100644 --- a/contracts/libraries/Errors.sol +++ b/contracts/libraries/Errors.sol @@ -100,6 +100,8 @@ library Errors { error DEPLOYMENT_FAILED(); error INITIALIZATION_FAILED(); + error INVALID_INITIALIZER(); + error INVALID_SALT(); /*////////////////////////////////////////////////////////////// PAYMASTER diff --git a/test/deployments/deployer.test.ts b/test/deployments/deployer.test.ts index 3c9f136..7f0dc87 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 { 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,21 @@ 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, 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, undefined, '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, undefined, '0xabababab')) + .to.be.revertedWithCustomError(factory, "INVALID_INITIALIZER"); + }); }); }); diff --git a/test/utils/deployer.ts b/test/utils/deployer.ts index acda762..73db252 100644 --- a/test/utils/deployer.ts +++ b/test/utils/deployer.ts @@ -3,7 +3,6 @@ * 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 type { HardhatRuntimeEnvironment } from 'hardhat/types'; import type { Wallet } 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, @@ -133,9 +132,12 @@ export class ClaveDeployer { wallet: HDNodeWallet, factory: Contract, validator: Contract, + salt?: string, + initializer?: string, ): Promise { - - const salt = keccak256(wallet.address); + if (!salt) { + salt = keccak256(wallet.address); + } const call: CallStruct = { target: ZeroAddress, allowFailure: false, @@ -144,7 +146,9 @@ export class ClaveDeployer { }; const abiCoder = AbiCoder.defaultAbiCoder(); - const initializer = + + if (!initializer) { + initializer = '0xb4e581f5' + abiCoder .encode( @@ -167,6 +171,7 @@ export class ClaveDeployer { ], ) .slice(2); + } const deployPromise = await Promise.all([ // Deploy account From a68b93c6e518e6c546c1c3933a915ae43971e0b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 14:57:46 -0500 Subject: [PATCH 02/12] [M-04] make initial call payable --- contracts/AccountFactory.sol | 4 +-- contracts/ClaveImplementation.sol | 2 +- test/deployments/deployer.test.ts | 27 +++++++++++++++++--- test/utils/deployer.ts | 41 ++++++++++++++++++++----------- 4 files changed, 52 insertions(+), 22 deletions(-) diff --git a/contracts/AccountFactory.sol b/contracts/AccountFactory.sol index d9501f3..3118c4b 100644 --- a/contracts/AccountFactory.sol +++ b/contracts/AccountFactory.sol @@ -90,7 +90,7 @@ 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(); @@ -137,7 +137,7 @@ contract AccountFactory is Ownable { initializeSuccess := call( gas(), accountAddress, - 0, + callvalue(), add(initializer, 0x20), mload(initializer), 0, diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 0711e94..78f5e03 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -64,7 +64,7 @@ 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); diff --git a/test/deployments/deployer.test.ts b/test/deployments/deployer.test.ts index 7f0dc87..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 { hexlify, parseEther, randomBytes } 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'; @@ -107,18 +107,37 @@ describe('Clave Contracts - Deployer class tests', () => { it('should not deploy an account with an invalid salt', async () => { const salt = randomBytes(32); - await expect(deployer.account(wallet, factory, eoaValidator, hexlify(salt))) + 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, undefined, '0x')) + 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, undefined, '0xabababab')) + 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 73db252..e92d1ff 100644 --- a/test/utils/deployer.ts +++ b/test/utils/deployer.ts @@ -3,7 +3,7 @@ * Unauthorized copying of this file, via any medium is strictly prohibited * Proprietary and confidential */ -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'; @@ -129,21 +129,32 @@ export class ClaveDeployer { } public async account( - wallet: HDNodeWallet, + wallet: BaseWallet, factory: Contract, validator: Contract, - salt?: string, - initializer?: string, + overrideValues: { + salt?: string, + initializer?: string, + callValue?: BigNumberish, + initialCall?: CallStruct, + } = {}, ): Promise { + let { salt, initializer, callValue, initialCall } = overrideValues; + if (!salt) { salt = keccak256(wallet.address); } - const call: CallStruct = { - target: ZeroAddress, - allowFailure: false, - value: 0, - callData: '0x', - }; + if (callValue === undefined) { + callValue = 0; + } + if (!initialCall) { + initialCall = { + target: ZeroAddress, + allowFailure: false, + value: 0, + callData: '0x', + }; + } const abiCoder = AbiCoder.defaultAbiCoder(); @@ -163,10 +174,10 @@ export class ClaveDeployer { await validator.getAddress(), [], [ - call.target, - call.allowFailure, - call.value, - call.callData, + initialCall.target, + initialCall.allowFailure, + initialCall.value, + initialCall.callData, ], ], ) @@ -176,7 +187,7 @@ export class ClaveDeployer { 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 From c0a75191ed3ee79ac34d893b94228e9c720002d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 15:09:19 -0500 Subject: [PATCH 03/12] [M-02] - clean up hook contet when removing hook --- contracts/managers/HookManager.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contracts/managers/HookManager.sol b/contracts/managers/HookManager.sol index 5633822..a24769c 100644 --- a/contracts/managers/HookManager.sol +++ b/contracts/managers/HookManager.sol @@ -171,6 +171,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(), From b5bc11d7499863313db39941598bfed26dc51fce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 15:12:56 -0500 Subject: [PATCH 04/12] [L-05] Use Ownable2Step --- contracts/AccountFactory.sol | 4 ++-- contracts/ClaveRegistry.sol | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/AccountFactory.sol b/contracts/AccountFactory.sol index 3118c4b..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,7 +12,7 @@ import {IClaveRegistry} from './interfaces/IClaveRegistry.sol'; * @title Factory contract to create Clave accounts in zkSync Era * @author https://getclave.io */ -contract AccountFactory is Ownable { +contract AccountFactory is Ownable2Step { // Address of the account implementation address public implementationAddress; // Selector of the account initializer function 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; From ef0eb5418074618f8db4394961f168b248beeff8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 15:26:01 -0500 Subject: [PATCH 05/12] [L-04] - Return false instead of reverting for invalid validator address --- contracts/handlers/ValidationHandler.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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) { From d17f40bdff3e13209f73a365b87651136254e7bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 15:33:46 -0500 Subject: [PATCH 06/12] [L-03] return false from `runValidationHooks` if hook data length is lower than the number of hooks being executed --- contracts/managers/HookManager.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contracts/managers/HookManager.sol b/contracts/managers/HookManager.sol index a24769c..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, From 549399fa5ae2545cb1cee93e15129d64dfbc0268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Thu, 14 Nov 2024 15:35:20 -0500 Subject: [PATCH 07/12] [L-01] add fallback function --- contracts/ClaveImplementation.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 78f5e03..0348466 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -96,8 +96,13 @@ 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 {} /** * @notice Called by the bootloader to validate that an account agrees to process the transaction From 9449a146de44d1330baab09f72d1b6aa8c2c75f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Wed, 20 Nov 2024 08:48:26 -0500 Subject: [PATCH 08/12] [H-01] increment nonce on executeTransactionFromOutside --- contracts/ClaveImplementation.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 0348466..e1d6ffc 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -182,6 +182,7 @@ contract ClaveImplementation is revert Errors.VALIDATION_HOOK_FAILED(); } + _incrementNonce(transaction.nonce); _executeTransaction(transaction); } From f8fefcbd3c9383bcf36474c65003a4e93e0ce22d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Wed, 20 Nov 2024 08:58:41 -0500 Subject: [PATCH 09/12] [M-03] remove early return and check hookSuccess at the end of validation --- contracts/ClaveImplementation.sol | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index e1d6ffc..833af52 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -246,13 +246,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( From fed2e77775a3bc4c89bac339214f94f7c39b0e6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Wed, 20 Nov 2024 09:15:16 -0500 Subject: [PATCH 10/12] [TRST-L-01] Ensure at least one K1 validator and owner are always present --- contracts/managers/OwnerManager.sol | 11 +---------- contracts/managers/ValidatorManager.sol | 13 +------------ 2 files changed, 2 insertions(+), 22 deletions(-) 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(); } From a39991ec29dbc1fe408d9cd3ec9407524e5011e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Wed, 20 Nov 2024 11:17:57 -0500 Subject: [PATCH 11/12] [M-03] remove early return --- contracts/ClaveImplementation.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 833af52..704f4be 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -234,11 +234,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); From bbc9b2e09c4dc213681c99534246b9d43545bd67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Coffee=E2=98=95=EF=B8=8F?= Date: Wed, 20 Nov 2024 11:43:35 -0500 Subject: [PATCH 12/12] [L-01] add assertion that fallback is never called from bootloader and inline ignore delegateCall --- contracts/ClaveImplementation.sol | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/contracts/ClaveImplementation.sol b/contracts/ClaveImplementation.sol index 704f4be..aeb892d 100644 --- a/contracts/ClaveImplementation.sol +++ b/contracts/ClaveImplementation.sol @@ -102,7 +102,21 @@ contract ClaveImplementation is // Fallback function to allow ETH to be sent to the account // with arbitrary calldata to mirror the behavior of an EOA - fallback() external payable {} + 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