From d06412289ac7316e265eac3958fb63f689dd1eca Mon Sep 17 00:00:00 2001 From: yuriissv Date: Thu, 12 Mar 2026 14:39:41 +0100 Subject: [PATCH 1/2] feat: restrict eth operators from calling ssv withdraws --- contracts/interfaces/ISSVNetworkCore.sol | 5 ++ contracts/modules/SSVOperators.sol | 2 + .../test/harness/SSVOperatorsHarness.sol | 3 ++ test/common/errors.ts | 1 + test/integration/SSVNetwork.test.ts | 48 +++++++++++++++++++ .../integration/SSVNetwork/legacy-ssv.test.ts | 10 ++-- .../withdrawOperatorEarningsSSV.test.ts | 36 +++++++++++++- 7 files changed, 99 insertions(+), 6 deletions(-) diff --git a/contracts/interfaces/ISSVNetworkCore.sol b/contracts/interfaces/ISSVNetworkCore.sol index 5322ce524..4aaefb001 100644 --- a/contracts/interfaces/ISSVNetworkCore.sol +++ b/contracts/interfaces/ISSVNetworkCore.sol @@ -249,6 +249,11 @@ interface ISSVNetworkCore { */ error IncorrectOperatorVersion(uint8 operatorVersion); // 0xf222e863 + /** + * @dev Thrown when an operator with no SSV activity attempts to withdraw SSV earnings + */ + error NoSSVEarnings(); // 0x08d08b0b + /** * @dev Thrown when cluster version is incorrect */ diff --git a/contracts/modules/SSVOperators.sol b/contracts/modules/SSVOperators.sol index 8bc9fe50f..b88cfd46f 100644 --- a/contracts/modules/SSVOperators.sol +++ b/contracts/modules/SSVOperators.sol @@ -303,6 +303,8 @@ contract SSVOperators is ISSVOperators, SSVReentrancyGuard { _transferOperatorBalanceUnsafe(operatorId, PackedETHLib.unpack(shrunkWithdrawn)); } else if (version == VERSION_SSV) { + if (operator.snapshot.block == 0) revert NoSSVEarnings(); + PackedSSV shrunkWithdrawn; PackedSSV shrunkAmount = PackedSSVLib.pack(amount); OperatorLib.updateSnapshotStSSV(operator); diff --git a/contracts/test/harness/SSVOperatorsHarness.sol b/contracts/test/harness/SSVOperatorsHarness.sol index 68f1881b8..a7a435d59 100644 --- a/contracts/test/harness/SSVOperatorsHarness.sol +++ b/contracts/test/harness/SSVOperatorsHarness.sol @@ -64,6 +64,9 @@ contract SSVOperatorsHarness is SSVOperators { StorageData storage s = SSVStorage.load(); s.operators[operatorId].ethSnapshot.balance = PackedETH.wrap(ethSnapshotBalance); s.operators[operatorId].snapshot.balance = PackedSSV.wrap(ssvSnapshotBalance); + if (ssvSnapshotBalance > 0 && s.operators[operatorId].snapshot.block == 0) { + s.operators[operatorId].snapshot.block = uint32(block.number); + } } function mockSetToken(address token) external { diff --git a/test/common/errors.ts b/test/common/errors.ts index 09c03750a..36745c268 100644 --- a/test/common/errors.ts +++ b/test/common/errors.ts @@ -66,4 +66,5 @@ export const Errors = { ORACLE_HAS_ZERO_WEIGHT: "OracleHasZeroWeight", MAX_VALUE_EXCEEDED: "MaxValueExceeded", MAX_PRECISION_EXCEEDED: "MaxPrecisionExceeded", + NO_SSV_EARNINGS: "NoSSVEarnings", } as const; diff --git a/test/integration/SSVNetwork.test.ts b/test/integration/SSVNetwork.test.ts index 059b8d062..74e3d0d62 100644 --- a/test/integration/SSVNetwork.test.ts +++ b/test/integration/SSVNetwork.test.ts @@ -1271,6 +1271,54 @@ describe("SSVNetwork full integration tests", () => { }); }); + describe("Function 'withdrawOperatorEarningsSSV()' — ETH-only operator guard", async function() { + it("Is reverted with 'NoSSVEarnings' when ETH-only operator calls withdrawOperatorEarningsSSV", async function() { + const { network } = + await networkHelpers.loadFixture(deployFullSSVNetworkFixture); + const operatorIds = await registerOperators(network, operatorOwner, 4); + + await expect(network.withdrawOperatorEarningsSSV(operatorIds[0], 0n)) + .to.be.revertedWithCustomError(network, Errors.NO_SSV_EARNINGS); + }); + + it("Is reverted with 'NoSSVEarnings' when ETH-only operator calls withdrawAllOperatorEarningsSSV", async function() { + const { network } = + await networkHelpers.loadFixture(deployFullSSVNetworkFixture); + const operatorIds = await registerOperators(network, operatorOwner, 4); + + await expect(network.withdrawAllOperatorEarningsSSV(operatorIds[0])) + .to.be.revertedWithCustomError(network, Errors.NO_SSV_EARNINGS); + }); + + it("ETH-only operator can still withdraw ETH earnings normally", async function() { + const { network, views } = + await networkHelpers.loadFixture(deployFullSSVNetworkFixture); + + const operatorIds = await registerOperators(network, operatorOwner, 4); + await whitelistAddresses(network, operatorOwner, operatorIds, [clusterOwner.address]); + + await network.connect(clusterOwner).registerValidator( + makePublicKey(1), + operatorIds, + DEFAULT_SHARES, + EMPTY_CLUSTER, + { value: DEFAULT_ETH_REGISTER_VALUE } + ); + + await connection.networkHelpers.mine(100n); + + const earnings: bigint = await views.getOperatorEarnings(operatorIds[0]); + expect(earnings).to.be.greaterThan(0n); + + await expect(network.withdrawOperatorEarnings(operatorIds[0], earnings)) + .to.emit(network, Events.OPERATOR_WITHDRAWN) + .withArgs(operatorOwner.address, operatorIds[0], earnings); + + await expect(network.withdrawOperatorEarningsSSV(operatorIds[0], 0n)) + .to.be.revertedWithCustomError(network, Errors.NO_SSV_EARNINGS); + }); + }); + describe("Function 'setFeeRecipientAddress()'", async function(){ it("Emits the correct event with the correct input data", async function(){ const { network } = diff --git a/test/integration/SSVNetwork/legacy-ssv.test.ts b/test/integration/SSVNetwork/legacy-ssv.test.ts index d5625804b..e4753ddf8 100644 --- a/test/integration/SSVNetwork/legacy-ssv.test.ts +++ b/test/integration/SSVNetwork/legacy-ssv.test.ts @@ -289,21 +289,21 @@ describe("SSVNetwork Integration - Legacy SSV Accounting", () => { await connection.networkHelpers.mine(100); - // SSV earnings should be 0, use precision-safe amount (10_000_000n is the shrink factor) + // SSV earnings should be 0 — operator has no SSV activity (snapshot.block == 0) const precisionSafeAmount = 10_000_000n; await expect(network.connect(operatorOwner).withdrawOperatorEarningsSSV(operatorIds[0], precisionSafeAmount)) - .to.be.revertedWithCustomError(network, Errors.INSUFFICIENT_BALANCE); + .to.be.revertedWithCustomError(network, Errors.NO_SSV_EARNINGS); }); - it("withdrawAllOperatorEarningsSSV reverts with InsufficientBalance when no SSV earnings", async function () { + it("withdrawAllOperatorEarningsSSV reverts with NoSSVEarnings when no SSV earnings", async function () { const { network } = await networkHelpers.loadFixture(deployFullSSVNetworkFixture); const operatorIds = await registerOperators(network, operatorOwner, 4); - // No cluster registered, so no earnings + // No cluster registered, so no earnings — operator has no SSV activity await expect(network.connect(operatorOwner).withdrawAllOperatorEarningsSSV(operatorIds[0])) - .to.be.revertedWithCustomError(network, Errors.INSUFFICIENT_BALANCE); + .to.be.revertedWithCustomError(network, Errors.NO_SSV_EARNINGS); }); it("withdrawOperatorEarningsSSV requires operator ownership", async function () { diff --git a/test/unit/SSVOperators/withdrawOperatorEarningsSSV.test.ts b/test/unit/SSVOperators/withdrawOperatorEarningsSSV.test.ts index 0be33b34e..432594b37 100644 --- a/test/unit/SSVOperators/withdrawOperatorEarningsSSV.test.ts +++ b/test/unit/SSVOperators/withdrawOperatorEarningsSSV.test.ts @@ -106,8 +106,9 @@ describe("SSVOperators SSV earnings withdrawals", async () => { operators.registerOperator(makeOperatorKey(1), Number(MINIMAL_OPERATOR_ETH_FEE), false), [GasGroup.REGISTER_OPERATOR] ); + await seedOperatorWithSSVBalance(operators, 1, 1n); - await expect(operators.withdrawOperatorEarningsSSV(1, DEDUCTED_DIGITS)).to.be.revertedWithCustomError( + await expect(operators.withdrawOperatorEarningsSSV(1, 2n * DEDUCTED_DIGITS)).to.be.revertedWithCustomError( operators, Errors.INSUFFICIENT_BALANCE ); @@ -141,4 +142,37 @@ describe("SSVOperators SSV earnings withdrawals", async () => { Errors.CALLER_NOT_OWNER ); }); + + it("Is reverted with 'NoSSVEarnings' when ETH-only operator calls withdrawOperatorEarningsSSV", async function () { + const { operators } = await networkHelpers.loadFixture(deployOperatorsFixture); + + await trackGas( + operators.registerOperator(makeOperatorKey(1), Number(MINIMAL_OPERATOR_ETH_FEE), false), + [GasGroup.REGISTER_OPERATOR] + ); + + await expect(operators.withdrawOperatorEarningsSSV(1, 0n)).to.be.revertedWithCustomError( + operators, + Errors.NO_SSV_EARNINGS + ); + + await expect(operators.withdrawOperatorEarningsSSV(1, DEDUCTED_DIGITS)).to.be.revertedWithCustomError( + operators, + Errors.NO_SSV_EARNINGS + ); + }); + + it("Is reverted with 'NoSSVEarnings' when ETH-only operator calls withdrawAllOperatorEarningsSSV", async function () { + const { operators } = await networkHelpers.loadFixture(deployOperatorsFixture); + + await trackGas( + operators.registerOperator(makeOperatorKey(1), Number(MINIMAL_OPERATOR_ETH_FEE), false), + [GasGroup.REGISTER_OPERATOR] + ); + + await expect(operators.withdrawAllOperatorEarningsSSV(1)).to.be.revertedWithCustomError( + operators, + Errors.NO_SSV_EARNINGS + ); + }); }); From 028ee563c25966aa379b1cf405530eb682771034 Mon Sep 17 00:00:00 2001 From: yuriissv Date: Thu, 12 Mar 2026 14:59:27 +0100 Subject: [PATCH 2/2] feat: update readiness doc --- ssv-review/planning/MAINNET-READINESS.md | 38 ++++++++++++------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/ssv-review/planning/MAINNET-READINESS.md b/ssv-review/planning/MAINNET-READINESS.md index aa5b13b30..23b87b62d 100644 --- a/ssv-review/planning/MAINNET-READINESS.md +++ b/ssv-review/planning/MAINNET-READINESS.md @@ -43,7 +43,7 @@ | SEC-16 | ~~Missing zero-value/zero-address guards on deposit and withdraw~~ | Security Hardening | P2 | ✅ Closed | | SEC-16b | ~~Dust ETH stranded in `accrued` after full cSSV transfer + claim~~ | Security Hardening | P1 | ✅ Fixed | | SEC-17 | DAO governance functions lack input guardrails (min/max/non-zero) | Security Hardening | P1 | M | -| SEC-18 | ETH-only operators can call `withdrawOperatorEarningsSSV` (no-op but wastes gas) | Security Hardening | P3 | S | +| SEC-18 | ~~ETH-only operators can call `withdrawOperatorEarningsSSV` (no-op but wastes gas)~~ | Security Hardening | P3 | ✅ Fixed | | SEC-19 | ~~`minBlocksBetweenUpdates` never initialized — EB update rate limit silently disabled~~ | Security Hardening | P1 | ✅ Fixed | | TEST-1 | ~~Validator register/remove with non-zero operator fees~~ | Unit Test Completeness | P0 | ✅ Closed (Addressed in PR #443) | | TEST-2 | ~~EB-weighted operator earnings accumulation~~ | Unit Test Completeness | P0 | ✅ Closed (Addressed in PR #444) | @@ -1098,12 +1098,12 @@ All other setters accept any value, including 0 and extreme values that could br --- -### [SEC-18] ETH-only operators can call `withdrawOperatorEarningsSSV` (no-op but wastes gas) +### [SEC-18] ~~ETH-only operators can call `withdrawOperatorEarningsSSV` (no-op but wastes gas)~~ - **Type:** Security Hardening - **Priority:** P3 -- **Status:** Open -- **Owner:** (unassigned) -- **Timeline:** (empty) +- **Status:** ✅ Fixed +- **Owner:** (resolved) +- **Timeline:** (complete) - **Github Link:** (empty) **Requirement:** @@ -1112,23 +1112,23 @@ Add an early-exit guard in `withdrawOperatorEarningsSSV` (or its underlying help **Context:** Operators registered after the v2.0.0 migration may be ETH-only (`snapshot.block == 0`, `ethSnapshot.block != 0`). New validator registrations for these operators use the ETH payment path exclusively, so they can never accumulate SSV earnings. Despite this, nothing prevents their owner from calling `withdrawOperatorEarningsSSV`. The call will succeed (the SSV balance is 0, so no tokens move), but the user pays gas for a no-op. Echidna invariants already confirm that the accounting system cannot credit SSV earnings to ETH-only operators, so there is no risk of fund loss — this is purely a UX/gas waste issue. -**Acceptance Criteria:** -- [ ] `withdrawOperatorEarningsSSV` reverts with a descriptive error (e.g., `NoSSVEarnings()`) when the operator has `snapshot.block == 0` (ETH-only) -- [ ] ETH-capable operators (both `snapshot.block != 0` and `ethSnapshot.block != 0`) are unaffected -- [ ] Confirm via Echidna that SSV balance of ETH-only operators cannot be artificially inflated +**Resolution:** +Added `NoSSVEarnings` custom error (selector `0x08d08b0b`) in `ISSVNetworkCore.sol`. In `SSVOperators.sol:_withdrawOperatorEarnings`, at the start of the `VERSION_SSV` branch, added: `if (operator.snapshot.block == 0) revert NoSSVEarnings();`. This checks whether the operator ever had SSV activity — `snapshot.block` is only set to non-zero by `updateSnapshotStSSV`, which only runs during SSV cluster operations. ETH-only operators never trigger this path, so `snapshot.block` remains 0. The guard fires before the snapshot update SSTORE, saving gas. Also updated `mockSetOperatorBalances` in the test harness to set `snapshot.block` when seeding non-zero SSV balance, so harness state is realistic. -**Agent Instructions:** -1. Read `contracts/modules/SSVOperators.sol`, focus on `withdrawOperatorEarningsSSV` and its internal helper. -2. After the `checkOwner` call, add: `if (operator.snapshot.block == 0) revert NoSSVEarnings();` -3. Define `NoSSVEarnings` error in `contracts/interfaces/ISSVNetworkCore.sol` if not already present. -4. Add a unit test: register an ETH-only operator → call `withdrawOperatorEarningsSSV` → expect revert with `NoSSVEarnings`. -5. Run `npm run test:unit`. +**Acceptance Criteria:** +- [x] `withdrawOperatorEarningsSSV` reverts with `NoSSVEarnings()` when the operator has `snapshot.block == 0` (ETH-only) +- [x] ETH-capable operators (both `snapshot.block != 0` and `ethSnapshot.block != 0`) are unaffected +- [x] Confirm via Echidna that SSV balance of ETH-only operators cannot be artificially inflated +- [x] Unit tests: ETH-only operator calling `withdrawOperatorEarningsSSV` and `withdrawAllOperatorEarningsSSV` both revert with `NoSSVEarnings` +- [x] Integration tests: ETH-only operator SSV withdrawal reverts, ETH withdrawal still works +- [x] All 1188 tests pass #### Sub-items: -- [ ] Sub-task 1: Add ETH-only operator guard to `withdrawOperatorEarningsSSV` -- [ ] Sub-task 2: Define `NoSSVEarnings` custom error -- [ ] Sub-task 3: Add unit test for ETH-only operator calling SSV withdrawal -- [ ] Sub-task 4: Run full test suite +- [x] Sub-task 1: Add ETH-only operator guard to `withdrawOperatorEarningsSSV` +- [x] Sub-task 2: Define `NoSSVEarnings` custom error in `ISSVNetworkCore.sol` +- [x] Sub-task 3: Add unit test for ETH-only operator calling SSV withdrawal +- [x] Sub-task 4: Add integration test for ETH-only operator SSV withdrawal guard +- [x] Sub-task 5: Run full test suite ---