From 361f2ddb9cb14e60d9729915246513dbc1d8b130 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Wed, 25 Feb 2026 00:50:44 +0100 Subject: [PATCH] fix: remove withdraw liquidation check --- contracts/modules/SSVClusters.sol | 1 - docs/FLOWS.md | 3 +- ssv-review/planning/MAINNET-READINESS.md | 24 +++--- test/integration/SSVNetwork.test.ts | 5 +- test/integration/SSVNetwork/clusters.test.ts | 4 +- test/unit/SSVClusters/withdraw.test.ts | 90 ++++++++++++++++++-- 6 files changed, 103 insertions(+), 24 deletions(-) diff --git a/contracts/modules/SSVClusters.sol b/contracts/modules/SSVClusters.sol index 92e5deb3..d75f85b5 100644 --- a/contracts/modules/SSVClusters.sol +++ b/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(); diff --git a/docs/FLOWS.md b/docs/FLOWS.md index 1b6566d9..09069cba 100644 --- a/docs/FLOWS.md +++ b/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. diff --git a/ssv-review/planning/MAINNET-READINESS.md b/ssv-review/planning/MAINNET-READINESS.md index 6725a97a..7e2d6d8d 100644 --- a/ssv-review/planning/MAINNET-READINESS.md +++ b/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 --- diff --git a/test/integration/SSVNetwork.test.ts b/test/integration/SSVNetwork.test.ts index a31ac7bd..52800f13 100644 --- a/test/integration/SSVNetwork.test.ts +++ b/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() { diff --git a/test/integration/SSVNetwork/clusters.test.ts b/test/integration/SSVNetwork/clusters.test.ts index 40dc0f09..1ee43911 100644 --- a/test/integration/SSVNetwork/clusters.test.ts +++ b/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); }); }); }); diff --git a/test/unit/SSVClusters/withdraw.test.ts b/test/unit/SSVClusters/withdraw.test.ts index e7760bb8..ddbcad6e 100644 --- a/test/unit/SSVClusters/withdraw.test.ts +++ b/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); }); });