Address audit feedback

config/devnet_config.json

@@ -1,13 +1,13 @@
 {
   "allow_root_override": true,
   "vault_verifier": "0x3498311FC4D1D1AEa7c2BE9996fa9660e88B1EbE",
+  "vault_root_provider": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
   "operators": {
     "pauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "unpauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "disburser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "defaultAdmin": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "accountRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
-    "vaultRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "tokenMappingManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa"
   },
   "lookup_tables": [

config/tenderly_eth_mainnet_config.json

@@ -1,13 +1,13 @@
 {
   "allow_root_override": true,
   "vault_verifier": "0x0000000000000000000000000000000000000000",
+  "vault_root_provider": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
   "operators": {
     "pauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "unpauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "disburser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "defaultAdmin": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "accountRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
-    "vaultRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "tokenMappingManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa"
   },
   "lookup_tables": [

config/tenderly_zkevm_mainnet_config.json

@@ -1,13 +1,13 @@
 {
   "allow_root_override": true,
   "vault_verifier": "0x0000000000000000000000000000000000000000",
+  "vault_root_provider": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
   "operators": {
     "pauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "unpauser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "disburser": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "defaultAdmin": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "accountRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
-    "vaultRootManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa",
     "tokenMappingManager": "0xBc6E5103D599FaEC8f4D2911efAEb98814cb5Aaa"
   },
   "lookup_tables": [

script/DeployL2Contracts.s.sol

@@ -13,6 +13,7 @@ import {ProcessorAccessControl} from "@src/withdrawals/ProcessorAccessControl.so
 contract DeployL2Contracts is Script {
     bool private allowRootOverride;
     address private vaultVerifier;
+    address private vaultRootProvider;
     address[63] private lookupTables;
     VaultWithdrawalProcessor private withdrawalProcessor;
     VaultWithdrawalProcessor.RoleOperators private operators;
@@ -23,6 +24,7 @@ contract DeployL2Contracts is Script {
         allowRootOverride = vm.parseJsonBool(config, "$.allow_root_override");
 
         vaultVerifier = vm.parseJsonAddress(config, "$.vault_verifier");
+        vaultRootProvider = vm.parseJsonAddress(config, "$.vault_root_provider");
 
         operators = abi.decode(vm.parseJson(config, "$.operators"), (ProcessorAccessControl.RoleOperators));
 
@@ -48,7 +50,8 @@ contract DeployL2Contracts is Script {
             vaultVerifier = address(new VaultEscapeProofVerifier(lookupTables));
         }
 
-        withdrawalProcessor = new VaultWithdrawalProcessor(vaultVerifier, operators, allowRootOverride);
+        withdrawalProcessor =
+            new VaultWithdrawalProcessor(vaultVerifier, vaultRootProvider, operators, allowRootOverride);
         withdrawalProcessor.registerTokenMappings(assetMappings);
 
         _logDeploymentDetails();

src/bridge/starkex/StarkExchangeMigration.sol

@@ -34,6 +34,10 @@ contract StarkExchangeMigration is IStarkExchangeMigration, LegacyStarkExchangeB
     /// @dev Reference to native ETH, based on the value used to represent ETH on the zkEVM bridge.
     address public constant NATIVE_ETH = address(0xeee);
 
+    constructor() {
+        _disableInitializers();
+    }
+
     /**
      * @notice Initializes the contract with migration configuration
      * @param data Encoded initialization data containing addresses

src/verifiers/vaults/VaultEscapeProofVerifier.sol

@@ -337,22 +337,11 @@ contract VaultEscapeProofVerifier is IVaultProofVerifier {
         return escapeProof[escapeProof.length - 2] >> 4;
     }
 
-    function _validateProofStructure(uint256[] calldata escapeProof) private pure returns (bool) {
-        uint256 proofLength = escapeProof.length;
-
-        // The minimal supported proof length is for a tree height of 31 in a 68 word representation as follows:
+    function _validateProofStructure(uint256[] calldata escapeProof) private pure {
+        // The only supported proof length is for a tree height of 31 in a 68 word representation as follows:
         // 1. 2 word pairs representing the vault contents + one hash of the 1st pair.
-        // 2. 31  word pairs representing the authentication path.
+        // 2. 31 word pairs representing the authentication path.
         // 3. 1 word pair representing the root and the leaf index.
-        require(proofLength >= VAULT_PROOF_LENGTH, InvalidVaultProof("Proof too short."));
-
-        // The contract supports verification paths of lengths up to 97 in a 200 word representation as described above.
-        // This limitation is imposed in order to avoid potential attacks.
-        require(proofLength < 200, InvalidVaultProof("Proof too long."));
-
-        // Ensure proofs are always a series of word pairs.
-        require((proofLength & 1) == 0, InvalidVaultProof("Proof length must be even."));
-
-        return true;
+        require(escapeProof.length == VAULT_PROOF_LENGTH, InvalidVaultProof("Invalid proof length."));
     }
 }

src/withdrawals/ProcessorAccessControl.sol

@@ -17,15 +17,12 @@ abstract contract ProcessorAccessControl is AccessControlEnumerable, Pausable {
     bytes32 public constant DISBURSER_ROLE = keccak256("DISBURSER_ROLE");
     /// @notice Role for setting the account root
     bytes32 public constant ACCOUNT_ROOT_PROVIDER_ROLE = keccak256("ACCOUNT_ROOT_PROVIDER_ROLE");
-    /// @notice Role for setting the vault root
-    bytes32 public constant VAULT_ROOT_PROVIDER_ROLE = keccak256("VAULT_ROOT_PROVIDER_ROLE");
     /// @notice Role for managing token mappings
     bytes32 public constant TOKEN_MAPPING_MANAGER = keccak256("TOKEN_MAPPING_MANAGER");
 
     /// @notice Struct encapsulating addresses that can perform privileged operations
     struct RoleOperators {
         address accountRootProvider;
-        address vaultRootProvider;
         address tokenMappingManager;
         address disburser;
         address pauser;
@@ -51,7 +48,6 @@ abstract contract ProcessorAccessControl is AccessControlEnumerable, Pausable {
 
     function _validateOperators(RoleOperators memory operators) internal pure {
         require(operators.accountRootProvider != address(0), InvalidOperatorAddress());
-        require(operators.vaultRootProvider != address(0), InvalidOperatorAddress());
         require(operators.tokenMappingManager != address(0), InvalidOperatorAddress());
         require(operators.disburser != address(0), InvalidOperatorAddress());
         require(operators.pauser != address(0), InvalidOperatorAddress());
@@ -61,7 +57,6 @@ abstract contract ProcessorAccessControl is AccessControlEnumerable, Pausable {
 
     function _grantRoleOperators(RoleOperators memory operators) internal {
         _grantRole(ACCOUNT_ROOT_PROVIDER_ROLE, operators.accountRootProvider);
-        _grantRole(VAULT_ROOT_PROVIDER_ROLE, operators.vaultRootProvider);
         _grantRole(TOKEN_MAPPING_MANAGER, operators.tokenMappingManager);
         _grantRole(DISBURSER_ROLE, operators.disburser);
         _grantRole(PAUSER_ROLE, operators.pauser);

src/withdrawals/VaultWithdrawalProcessor.sol

@@ -30,6 +30,9 @@ contract VaultWithdrawalProcessor is
     /// @notice Thrown if attempting to set the root override value to the existing value
     error NoChangeInOverrideValue();
 
+    /// @notice Thrown when a caller other than the authorised vault root provider attempts to set the vault root
+    error UnauthorizedVaultRootProvider();
+
     /// @notice Thrown if dequantization would cause an overflow
     /// @param quantizedBalance The quantized balance that caused the overflow
     /// @param quantum The quantum value that caused the overflow
@@ -40,26 +43,36 @@ contract VaultWithdrawalProcessor is
     /// @dev Upper bound for valid Stark keys (2^251 + 17 * 2^192 + 1)
     uint256 internal constant STARK_KEY_UPPER_BOUND = 0x800000000000011000000000000000000000000000000000000000000000001;
 
-    uint256 public constant VAULT_PROOF_LENGTH = 68;
     uint256 public constant ACCOUNT_PROOF_LENGTH = 27;
 
     /// @notice The vault proof verifier contract
     IVaultProofVerifier public immutable VAULT_PROOF_VERIFIER;
 
+    /// @notice The address authorised to set the vault root (expected to be a VaultRootReceiverAdapter instance)
+    address public immutable VAULT_ROOT_PROVIDER;
+
     /// @notice Flag indicating whether the vault root can be overridden after initial setting
     bool public rootOverrideAllowed = false;
 
     /**
      * @notice Constructs the VaultWithdrawalProcessor contract
      * @param _vaultProofVerifier The address of the vault proof verifier contract
+     * @param _vaultRootProvider The address authorised to set the vault root (e.g. a VaultRootReceiverAdapter instance)
      * @param _operators The list of addresses to be granted specific roles
      * @param _rootOverrideAllowed Whether the vault and account roots can be overridden after initial setting
      */
-    constructor(address _vaultProofVerifier, RoleOperators memory _operators, bool _rootOverrideAllowed) {
+    constructor(
+        address _vaultProofVerifier,
+        address _vaultRootProvider,
+        RoleOperators memory _operators,
+        bool _rootOverrideAllowed
+    ) {
         require(_vaultProofVerifier != address(0), ZeroAddress());
+        require(_vaultRootProvider != address(0), ZeroAddress());
         _validateOperators(_operators);
 
         VAULT_PROOF_VERIFIER = IVaultProofVerifier(_vaultProofVerifier);
+        VAULT_ROOT_PROVIDER = _vaultRootProvider;
         _grantRoleOperators(_operators);
         rootOverrideAllowed = _rootOverrideAllowed;
     }
@@ -78,11 +91,9 @@ contract VaultWithdrawalProcessor is
 
         require(receiver != address(0), ZeroAddress());
         require(accountProof.length == ACCOUNT_PROOF_LENGTH, InvalidAccountProof("Invalid account proof length"));
-        require(
-            vaultProof.length == VAULT_PROOF_LENGTH, IVaultProofVerifier.InvalidVaultProof("Invalid vault proof length")
-        );
 
         // Extract the vault and vault root information from the submitted proof
+        // Note: the verifier validates the proof length strictly (must be exactly 68)
         (IVaultProofVerifier.Vault memory vault, uint256 _vaultRoot) =
             VAULT_PROOF_VERIFIER.extractVaultAndRootFromProof(vaultProof);
 
@@ -118,11 +129,12 @@ contract VaultWithdrawalProcessor is
 
     /**
      * @notice Sets the vault root hash for proof verification
-     * @dev Only the vault root provider can call this function
+     * @dev Only the immutable vault root provider address can call this function
      * @dev The vault root can only be set once unless rootOverrideAllowed is true
      * @param newRoot The new vault root hash
      */
-    function setVaultRoot(uint256 newRoot) external override onlyRole(VAULT_ROOT_PROVIDER_ROLE) {
+    function setVaultRoot(uint256 newRoot) external override {
+        require(msg.sender == VAULT_ROOT_PROVIDER, UnauthorizedVaultRootProvider());
         _setVaultRoot(newRoot, rootOverrideAllowed);
     }
 

test/integration/proofs/withdrawals/VaultWithdrawalProcessor.t.sol

@@ -63,11 +63,10 @@ contract VaultWithdrawalProcessorIntegrationTest is
             disburser: address(this),
             defaultAdmin: address(this),
             accountRootProvider: address(this),
-            vaultRootProvider: address(rootReceiver),
             tokenMappingManager: address(this)
         });
 
-        vaultProcessor = new VaultWithdrawalProcessor(address(vaultVerifier), operators, true);
+        vaultProcessor = new VaultWithdrawalProcessor(address(vaultVerifier), address(rootReceiver), operators, true);
 
         vaultProcessor.setAccountRoot(accountsRoot);
         vaultProcessor.registerTokenMappings(fixAssets);

test/unit/bridge/starkex/StarkExchangeMigration.t.sol

@@ -6,6 +6,8 @@ import "@src/bridge/starkex/StarkExchangeMigration.sol";
 import "@src/bridge/starkex/IStarkExchangeMigration.sol";
 import "@src/bridge/zkEVM/IRootERC20Bridge.sol";
 import "@src/bridge/messaging/VaultRootSenderAdapter.sol";
+import "@openzeppelin/contracts/proxy/utils/Initializable.sol";
+import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 
@@ -103,37 +105,43 @@ contract StarkExchangeMigrationTest is Test {
     uint256 private constant BRIDGE_FEE = 0.001 ether;
 
     function setUp() public {
-        starkExBridge = new StarkExchangeMigration();
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
         mockBridge = new MockRootERC20Bridge();
         mockSenderAdapter = new MockVaultRootSenderAdapter();
         testToken = new MockERC20("Test Token", "TEST", 18);
 
         bytes memory initData =
             abi.encode(MIGRATION_MANAGER, address(mockBridge), address(mockSenderAdapter), WITHDRAWAL_PROCESSOR);
-        starkExBridge.initialize(initData);
+        bytes memory initCallData = abi.encodeWithSelector(StarkExchangeMigration.initialize.selector, initData);
+        ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), initCallData);
+        starkExBridge = StarkExchangeMigration(address(proxy));
 
         vm.store(address(starkExBridge), bytes32(uint256(13)), bytes32(TEST_VAULT_ROOT));
         vm.deal(MIGRATION_MANAGER, 10 ether);
     }
 
-    function test_Initialize_ValidParameters() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
-
+    function test_Constructor_DisablesInitializers_InitializeRevertsWhenCalledOnImplementation() public {
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
         bytes memory initData =
             abi.encode(MIGRATION_MANAGER, address(mockBridge), address(mockSenderAdapter), WITHDRAWAL_PROCESSOR);
 
-        newMigration.initialize(initData);
+        vm.expectRevert(Initializable.InvalidInitialization.selector);
+        implementation.initialize(initData);
+    }
 
-        assertEq(newMigration.migrationManager(), MIGRATION_MANAGER, "Migration manager should be set");
-        assertEq(newMigration.zkEVMBridge(), address(mockBridge), "zkEVM bridge should be set");
+    function test_Initialize_ValidParameters() public view {
+        assertEq(starkExBridge.migrationManager(), MIGRATION_MANAGER, "Migration manager should be set");
+        assertEq(starkExBridge.zkEVMBridge(), address(mockBridge), "zkEVM bridge should be set");
         assertEq(
-            address(newMigration.rootSenderAdapter()), address(mockSenderAdapter), "Root sender adapter should be set"
+            address(starkExBridge.rootSenderAdapter()), address(mockSenderAdapter), "Root sender adapter should be set"
         );
-        assertEq(newMigration.zkEVMWithdrawalProcessor(), WITHDRAWAL_PROCESSOR, "Withdrawal processor should be set");
+        assertEq(starkExBridge.zkEVMWithdrawalProcessor(), WITHDRAWAL_PROCESSOR, "Withdrawal processor should be set");
     }
 
     function test_RevertIf_Initialize_InvalidMigrationManager() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
+        ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), "");
+        StarkExchangeMigration newMigration = StarkExchangeMigration(address(proxy));
 
         bytes memory initData = abi.encode(
             address(0), // Invalid migration manager
@@ -147,7 +155,9 @@ contract StarkExchangeMigrationTest is Test {
     }
 
     function test_RevertIf_Initialize_InvalidZkEVMBridge() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
+        ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), "");
+        StarkExchangeMigration newMigration = StarkExchangeMigration(address(proxy));
 
         bytes memory initData = abi.encode(
             MIGRATION_MANAGER,
@@ -161,7 +171,9 @@ contract StarkExchangeMigrationTest is Test {
     }
 
     function test_RevertIf_Initialize_InvalidRootSenderAdapter() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
+        ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), "");
+        StarkExchangeMigration newMigration = StarkExchangeMigration(address(proxy));
 
         bytes memory initData = abi.encode(
             MIGRATION_MANAGER,
@@ -175,7 +187,9 @@ contract StarkExchangeMigrationTest is Test {
     }
 
     function test_RevertIf_Initialize_InvalidWithdrawalProcessor() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
+        StarkExchangeMigration implementation = new StarkExchangeMigration();
+        ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), "");
+        StarkExchangeMigration newMigration = StarkExchangeMigration(address(proxy));
 
         bytes memory initData = abi.encode(
             MIGRATION_MANAGER,
@@ -189,15 +203,11 @@ contract StarkExchangeMigrationTest is Test {
     }
 
     function test_RevertIf_Initialize_Twice() public {
-        StarkExchangeMigration newMigration = new StarkExchangeMigration();
-
         bytes memory initData =
             abi.encode(MIGRATION_MANAGER, address(mockBridge), address(mockSenderAdapter), WITHDRAWAL_PROCESSOR);
 
-        newMigration.initialize(initData);
-
-        vm.expectRevert();
-        newMigration.initialize(initData);
+        vm.expectRevert(Initializable.InvalidInitialization.selector);
+        starkExBridge.initialize(initData);
     }
 
     function test_MigrateVaultRoot() public {