Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions contracts/interfaces/ISSVNetworkCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
2 changes: 2 additions & 0 deletions contracts/modules/SSVOperators.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions contracts/test/harness/SSVOperatorsHarness.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
38 changes: 19 additions & 19 deletions ssv-review/planning/MAINNET-READINESS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
Expand Down Expand Up @@ -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:**
Expand All @@ -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

---

Expand Down
1 change: 1 addition & 0 deletions test/common/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
48 changes: 48 additions & 0 deletions test/integration/SSVNetwork.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } =
Expand Down
10 changes: 5 additions & 5 deletions test/integration/SSVNetwork/legacy-ssv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
36 changes: 35 additions & 1 deletion test/unit/SSVOperators/withdrawOperatorEarningsSSV.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Expand Down Expand Up @@ -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
);
});
});
Loading