Audit Fixes (#13)

* [TRST-M-1][M-05]: Require specific deployment salt and initializer selector for account deployment

* [M-04] make initial call payable

* [M-02] - clean up hook contet when removing hook

* [L-05] Use Ownable2Step

* [L-04] - Return false instead of reverting for invalid validator address

* [L-03] return false from `runValidationHooks` if hook data length is lower than the number of hooks being executed

* [L-01] add fallback function

* [H-01] increment nonce on executeTransactionFromOutside

* [M-03] remove early return and check hookSuccess at the end of validation

* [TRST-L-01] Ensure at least one K1 validator and owner are always present

* [M-03] remove early return

* [L-01] add assertion that fallback is never called from bootloader and inline ignore delegateCall

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);
     }

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,25 +248,17 @@ 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);
 
         // 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(

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;

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) {

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

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(),

contracts/managers/OwnerManager.sol

@@ -91,23 +91,14 @@ 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);
     }
 
     function _k1RemoveOwner(address addr) internal {
         _k1OwnersLinkedList().remove(addr);
 
         // The wallet should have at least one owner
-        if (
-            _k1OwnersLinkedList().isEmpty() && _r1OwnersLinkedList().isEmpty()
-        ) {
+        if (_k1OwnersLinkedList().isEmpty()) {
             revert Errors.EMPTY_OWNERS();
         }
 

contracts/managers/ValidatorManager.sol

@@ -85,25 +85,14 @@ 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);
     }
 
     function _k1RemoveValidator(address validator) internal {
         _k1ValidatorsLinkedList().remove(validator);
 
         // At least one validator must be present
-        if (
-            _r1ValidatorsLinkedList().isEmpty() &&
-            _k1ValidatorsLinkedList().isEmpty()
-        ) {
+        if (_k1ValidatorsLinkedList().isEmpty()) {
             revert Errors.EMPTY_VALIDATORS();
         }
 

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'));
+
+        });
     });
 });