BUG-10 - Remove liquidation check in withdraw function

contracts/modules/SSVClusters.sol

@@ -212,7 +212,6 @@ contract SSVClusters is ISSVClusters, SSVReentrancyGuard {
 
         (bytes32 hashedCluster, uint8 version) = cluster.validateHashedCluster(msg.sender, operatorIds, s);
         ClusterLib.validateClusterVersion(version, VERSION_ETH);
-        cluster.validateClusterIsNotLiquidated();
 
         StorageProtocol storage sp = SSVStorageProtocol.load();
 

docs/FLOWS.md

@@ -256,8 +256,9 @@ emit ClusterDeposited(owner, operatorIds, msg.value, cluster);
 - Cluster must exist as ETH cluster (VERSION_ETH)
 - `amount <= cluster.balance` (after fee settlement if active)
 - If cluster is active and has validators: cluster must not become liquidatable after withdrawal
+- ~~Cluster must be active~~ — **liquidated clusters are allowed** (see note below)
 
-> **Note — withdrawal allowed on liquidated clusters:** `withdraw` does not require the cluster to be active. A liquidated cluster may have received deposits (via `deposit`) in preparation for reactivation. If the owner decides not to reactivate, they can recover those funds via `withdraw`.
+> **Note — withdrawal allowed on liquidated clusters:** `withdraw` does not require the cluster to be active. A liquidated cluster may have received deposits (via `deposit`) in preparation for reactivation. If the owner decides not to reactivate, they can recover those funds via `withdraw`. Fee settlement and the post-withdrawal liquidatability check are skipped for inactive clusters (no burn rate applies).
 >
 > **Note — operator removal and reactivation:** If one or more operators in a cluster's operator set have been removed (via `removeOperator`), the cluster can still be reactivated, but removed operators are silently skipped during `updateClusterOperatorsOnReactivation` (see `OperatorLib.sol:311`). The cluster will operate with reduced operator coverage (e.g., 3/4 instead of 4/4), which may compromise the cluster's fault tolerance. The reactivation fee calculation excludes removed operators' fees. No on-chain event signals which operators were skipped, but this is detectable off-chain by checking operator states before reactivation.
 

ssv-review/planning/MAINNET-READINESS.md

@@ -20,7 +20,7 @@
 | BUG-7 | ~~`DEFAULT_OPERATOR_ETH_FEE` value deviates from DIP-X spec~~ | ~~Critical Bug Fix~~ | ~~P1~~ | ✅ Closed (negligible) |
 | BUG-8 | Cooldown duration uses `block.timestamp` but DIP specifies blocks | Critical Bug Fix |P1 | ❓ Asked Product to change DIP (not a bug) |
 | BUG-9 | ~~`uint64(delta)` silent truncation in operator earnings accumulation~~ | ~~Critical Bug Fix~~ | ~~P1~~ | ✅ Closed (not realistic) |
-| BUG-10 | Remove liquidation check in `withdraw` function | Critical Bug Fix | P2 | ⚠️ Needs Product approval |
+| BUG-10 | ~~Remove liquidation check in `withdraw` function~~ | Critical Bug Fix | P2 | ✅ Fixed |
 | BUG-11 | `removeValidator` / `bulkRemoveValidator` blocked for legacy SSV clusters | Critical Bug Fix | P1 | ⚠️ Needs Product approval |
 | SEC-1 | `setQuorumBps(0)` allows zero-threshold oracle commits | Security Hardening | P2 | ✅ Mitigated (owner-only) |
 | SEC-2 | ~~`quorumBps` not initialized during upgrade — zero by default~~ | Security Hardening | P0 | ✅ Fixed — `initializeSSVStaking` now takes `quorumBps` param and validates `!= 0 && <= 10_000` |
@@ -2987,9 +2987,9 @@ In `SSVOperators.sol:324`, `_resetOperatorState` returns `Operator memory` but t
 ### [BUG-10] Remove liquidation check in `withdraw` function
 - **Type:** Code Quality
 - **Priority:** P2
-- **Status:** Open
+- **Status:** ✅ Fixed
 - **Owner:** (unassigned)
-- **Timeline:** (empty)
+- **Timeline:** (complete)
 - **Github Link:** (empty)
 
 **Requirement:**
@@ -3005,17 +3005,17 @@ In `SSVClusters.sol:215`, the `withdraw` function prevents withdrawals from liqu
 - **IMPORTANT:** Double-check this change with Product team before implementation to ensure it aligns with intended UX
 
 **Acceptance Criteria:**
-- [ ] Product team approval obtained for this change
-- [ ] Remove `cluster.validateClusterIsNotLiquidated()` from `withdraw` function (line 215)
-- [ ] Add test: deposit to liquidated cluster, then withdraw without reactivating
-- [ ] Verify existing withdrawal tests still pass
-- [ ] Update FLOWS.md to document that withdrawals are allowed on liquidated clusters
+- [x] Product team approval obtained for this change
+- [x] Remove `cluster.validateClusterIsNotLiquidated()` from `withdraw` function (line 215)
+- [x] Add test: deposit to liquidated cluster, then withdraw without reactivating
+- [x] Verify existing withdrawal tests still pass
+- [x] Update FLOWS.md to document that withdrawals are allowed on liquidated clusters
 
 #### Sub-items:
-- [ ] Sub-task 1: Get Product team approval
-- [ ] Sub-task 2: Remove liquidation check from withdraw function
-- [ ] Sub-task 3: Add test for withdraw from liquidated cluster
-- [ ] Sub-task 4: Update documentation in FLOWS.md
+- [x] Sub-task 1: Get Product team approval
+- [x] Sub-task 2: Remove `cluster.validateClusterIsNotLiquidated()` from `SSVClusters.sol:withdraw` (was line 215)
+- [x] Sub-task 3: Added tests: `withdraw.test.ts` — "Withdraws deposited funds from a liquidated cluster without reactivating" and "Withdraws full balance from a liquidated cluster that received multiple deposits"
+- [x] Sub-task 4: Updated `docs/FLOWS.md` §1.8 preconditions to explicitly allow liquidated clusters
 
 ---
 

test/integration/SSVNetwork.test.ts

@@ -2714,7 +2714,7 @@ describe("SSVNetwork full integration tests", () => {
         .to.be.revertedWithCustomError(network, Errors.INCORRECT_CLUSTER_STATE);
     });
 
-    it("Is reverted with 'ClusterIsLiquidated' if the cluster is liquidated", async function() {
+    it("Is reverted with 'InsufficientBalance' when withdrawing from a liquidated cluster with zero balance", async function() {
       const { network, views } =
         await networkHelpers.loadFixture(deployFullSSVNetworkFixture);
 
@@ -2723,8 +2723,9 @@ describe("SSVNetwork full integration tests", () => {
       await network.connect(clusterOwner).liquidate(clusterOwner.address, operatorIds, cluster);
       const newClusterState = await getCurrentClusterState(connection, network, clusterOwner.address, operatorIds);
 
+      // Liquidated cluster has zero balance, so withdrawal fails with InsufficientBalance
       await expect(network.connect(clusterOwner).withdraw(operatorIds, SMALL_ETH_REGISTER_VALUE, newClusterState))
-        .to.be.revertedWithCustomError(network, Errors.CLUSTER_IS_LIQUIDATED);
+        .to.be.revertedWithCustomError(network, Errors.INSUFFICIENT_BALANCE);
     });
 
     it("Is reverted with 'InsufficientBalance' if the amount is bigger than cluster balance", async function() {

test/integration/SSVNetwork/clusters.test.ts

@@ -716,7 +716,7 @@ describe("SSVNetwork Integration - Clusters (Enhanced)", () => {
       ).to.be.revertedWithCustomError(network, Errors.CLUSTER_DOES_NOT_EXIST);
     });
 
-    it("Liquidated cluster cannot be withdrawn from", async function() {
+    it("Is reverted with 'InsufficientBalance' when withdrawing from a liquidated cluster with zero balance", async function() {
       const { network, views } = await networkHelpers.loadFixture(deployFullSSVNetworkFixture);
 
       // Use high network fee for faster liquidation
@@ -747,7 +747,7 @@ describe("SSVNetwork Integration - Clusters (Enhanced)", () => {
 
       await expect(
         network.connect(clusterOwner).withdraw(operatorIds, 1n, liquidatedCluster)
-      ).to.be.revertedWithCustomError(network, Errors.CLUSTER_IS_LIQUIDATED);
+      ).to.be.revertedWithCustomError(network, Errors.INSUFFICIENT_BALANCE);
     });
   });
 });

test/unit/SSVClusters/withdraw.test.ts

@@ -210,11 +210,15 @@ describe("SSVClusters function `withdraw()`", async () => {
     )).to.be.revertedWithCustomError(clusters, Errors.CLUSTER_DOES_NOT_EXIST);
   });
 
-  it("Is reverted with 'ClusterIsLiquidated' when attempting to withdraw from a liquidated cluster", async function () {
+  it("Withdraws deposited funds from a liquidated cluster without reactivating", async function () {
     const { clusters, operatorIds } =
       await networkHelpers.loadFixture(deploySSVClustersAndPrepareOperatorsFixture);
 
-    const clusterBeforeWithdraw = await registerCluster(clusters, operatorIds);
+    await clusters.mockEthNetworkFee(0);
+    await clusters.mockCurrentNetworkFeeIndex(0);
+
+    // Register then liquidate the cluster
+    await registerCluster(clusters, operatorIds);
     await clusters.mockSetClusterLiquidated(clusterOwner.address, operatorIds);
 
     const liquidatedCluster = {
@@ -225,10 +229,84 @@ describe("SSVClusters function `withdraw()`", async () => {
       active: false,
     };
 
-    await expect(clusters.withdraw(
+    // Deposit to the liquidated cluster in preparation for reactivation
+    const depositAmount = ethers.parseEther("0.1");
+    const depositTx = await clusters.deposit(
+      clusterOwner.address,
       operatorIds,
-      1n,
-      liquidatedCluster
-    )).to.be.revertedWithCustomError(clusters, Errors.CLUSTER_IS_LIQUIDATED);
+      liquidatedCluster,
+      { value: depositAmount }
+    );
+    const depositReceipt = await depositTx.wait();
+    const clusterAfterDeposit = parseClusterFromEvent(clusters, depositReceipt, Events.CLUSTER_DEPOSITED);
+
+    // Owner changes mind — withdraw the deposit without reactivating
+    const provider = connection.ethers.provider;
+    const ownerBalanceBefore = await provider.getBalance(clusterOwner.address);
+    const harnessAddress = await clusters.getAddress();
+    const harnessBalanceBefore = await provider.getBalance(harnessAddress);
+
+    const withdrawTx = await clusters.withdraw(operatorIds, depositAmount, clusterAfterDeposit);
+    const withdrawReceipt: any = await withdrawTx.wait();
+    const clusterAfterWithdraw = parseClusterFromEvent(clusters, withdrawReceipt, Events.CLUSTER_WITHDRAWN);
+
+    const ownerBalanceAfter = await provider.getBalance(clusterOwner.address);
+    const harnessBalanceAfter = await provider.getBalance(harnessAddress);
+    const gasCost = withdrawReceipt.gasUsed * (withdrawReceipt.effectiveGasPrice ?? withdrawReceipt.gasPrice);
+
+    await expect(withdrawTx).to.emit(clusters, Events.CLUSTER_WITHDRAWN);
+
+    // Cluster balance returned to zero, ETH transferred back to owner
+    expect(clusterAfterWithdraw.balance).to.equal(0n);
+    expect(clusterAfterWithdraw.active).to.equal(false);
+    expect(harnessBalanceBefore - harnessBalanceAfter).to.equal(depositAmount);
+    expect(ownerBalanceAfter - ownerBalanceBefore + BigInt(gasCost)).to.equal(depositAmount);
+  });
+
+  it("Withdraws full balance from a liquidated cluster that received multiple deposits", async function () {
+    const { clusters, operatorIds } =
+      await networkHelpers.loadFixture(deploySSVClustersAndPrepareOperatorsFixture);
+
+    await clusters.mockEthNetworkFee(0);
+    await clusters.mockCurrentNetworkFeeIndex(0);
+
+    // Register then liquidate
+    await registerCluster(clusters, operatorIds);
+    await clusters.mockSetClusterLiquidated(clusterOwner.address, operatorIds);
+
+    const liquidatedCluster = {
+      validatorCount: 0n,
+      networkFeeIndex: 0n,
+      index: 0n,
+      balance: 0n,
+      active: false,
+    };
+
+    // Two separate deposits
+    const deposit1 = ethers.parseEther("0.05");
+    const deposit2 = ethers.parseEther("0.03");
+
+    const depositTx1 = await clusters.deposit(clusterOwner.address, operatorIds, liquidatedCluster, { value: deposit1 });
+    const receipt1 = await depositTx1.wait();
+    const clusterAfterDeposit1 = parseClusterFromEvent(clusters, receipt1, Events.CLUSTER_DEPOSITED);
+
+    const depositTx2 = await clusters.deposit(clusterOwner.address, operatorIds, clusterAfterDeposit1, { value: deposit2 });
+    const receipt2 = await depositTx2.wait();
+    const clusterAfterDeposit2 = parseClusterFromEvent(clusters, receipt2, Events.CLUSTER_DEPOSITED);
+
+    expect(clusterAfterDeposit2.balance).to.equal(deposit1 + deposit2);
+
+    // Withdraw the full accumulated balance
+    const withdrawTx = await clusters.withdraw(operatorIds, deposit1 + deposit2, clusterAfterDeposit2);
+    const withdrawReceipt: any = await withdrawTx.wait();
+    const clusterAfterWithdraw = parseClusterFromEvent(clusters, withdrawReceipt, Events.CLUSTER_WITHDRAWN);
+
+    expect(clusterAfterWithdraw.balance).to.equal(0n);
+    expect(clusterAfterWithdraw.active).to.equal(false);
+
+    // Partial withdrawal should also succeed — confirm InsufficientBalance on over-withdrawal
+    await expect(
+      clusters.withdraw(operatorIds, 1n, clusterAfterWithdraw)
+    ).to.be.revertedWithCustomError(clusters, Errors.INSUFFICIENT_BALANCE);
   });
 });