Skip to content

Vanilla validator compounding staking strategy#2905

Open
naddison36 wants to merge 19 commits into
nicka/initial-deposit-32from
nicka/vanilla-staking
Open

Vanilla validator compounding staking strategy#2905
naddison36 wants to merge 19 commits into
nicka/initial-deposit-32from
nicka/vanilla-staking

Conversation

@naddison36
Copy link
Copy Markdown
Collaborator

@naddison36 naddison36 commented May 28, 2026

Summary

This PR splits the compounding staking strategy into a deployable vanilla strategy and an SSV-specific extension, then wires the deployment flow for migrating away from the old compounding SSV strategy.

  • Refactors shared compounding validator logic into CompoundingStakingStrategy
  • Keeps SSV-specific behavior in CompoundingStakingSSVStrategy
  • Adds CompoundingStakingStrategyProxy for staking to vanilla compounding validators
  • Adds CompoundingStakingStrategyView
  • Updates ConsolidationController to target the new vanilla compounding strategy
  • Adds withdrawSsvClusterEth support to SSV strategies so empty SSV cluster ETH can be recovered
  • Allows resetFirstDeposit to be called by either Governor or Strategist

Details

CompoundingStakingStrategy now contains the shared compounding validator logic:

  • strategy initialization
  • WETH/ETH deposit and withdrawal behavior
  • validator staking, withdrawal, proof verification, balance verification, and accounting
  • pause/config/admin functions
  • unsupported pToken/reward hooks

CompoundingStakingSSVStrategy now only contains SSV-specific behavior:

  • registerSsvValidator
  • removeSsvValidator
  • withdrawSsvClusterEth
  • SSV validator events and errors
  • SSV first-deposit admission override requiring prior REGISTERED state

The vanilla strategy allows the first deposit to an unregistered validator:

  • NON_REGISTERED -> STAKED
  • follow-up deposits still require VERIFIED or ACTIVE
  • only one pending first deposit is allowed at a time

The initial validator deposit amount can be configured up to 2048 ETH, and first deposits are allowed up to the stored initialDepositAmountWei.

First Deposit Reset

resetFirstDeposit can now be called by either:

  • Governor
  • Vault Strategist

This lets the Strategist clear the first-deposit guard after the pending first deposit has been reviewed/handled, while still rejecting regular users.

SSV Cluster ETH Withdrawal

withdrawSsvClusterEth was added to both:

  • NativeStakingSSVStrategy
  • CompoundingStakingSSVStrategy

The function is governor-only and withdraws ETH from an empty SSV cluster. Any ETH held by the strategy is wrapped to WETH and transferred back to the OETH Vault.

Consolidation

ConsolidationController now targets the new CompoundingStakingStrategy and only supports nativeStakingStrategy2 as a source strategy.

It forwards old native staking operations through to nativeStakingStrategy2, including:

  • registerSsvValidators
  • stakeEth
  • doAccounting
  • exitSsvValidator
  • removeSsvValidator

It also forwards target strategy operations needed during consolidation:

  • snapBalances
  • verifyBalances
  • validatorWithdrawal
  • compounding stakeEth

Deploy Script

196_deploy_compounding_staking_strategy.js:

  • deploys new CompoundingStakingSSVStrategy implementation
  • deploys new NativeStakingSSVStrategy2 implementation
  • deploys CompoundingStakingStrategyProxy
  • deploys CompoundingStakingStrategy
  • initializes with a 2030 ETH initial validator deposit amount
  • reuses the existing deployed BeaconProofs
  • deploys CompoundingStakingStrategyView
  • deploys a new ConsolidationController

Governance proposal actions:

  • upgrade the old CompoundingStakingSSVStrategyProxy
  • withdraw ETH from the old compounding SSV cluster once empty
  • upgrade NativeStakingSSVStrategy2Proxy
  • approve the new compounding strategy on the OETH Vault
  • set the new compounding strategy as the OETH Vault default strategy
  • remove the old compounding SSV strategy from the OETH Vault
  • set the new compounding strategy registrator to the controller
  • set nativeStakingStrategy2 registrator to the controller

On fork tests, the deploy script skips the SSV cluster withdrawal and old strategy removal when the old compounding SSV cluster is not yet empty.

Testing

  • Added focused vanilla compounding staking unit tests
  • Updated SSV compounding unit tests for the split strategy behavior
  • Updated beacon proof fork test validator fixtures:
    • active validator: 1938267
    • exited validator: 1998612

Dependencies

This change is dependent on:

Verification

  • pnpm prettier:sol
  • pnpm prettier:js
  • pnpm hardhat compile
  • pnpm test test/strategies/compoundingStaking.js
  • pnpm test test/strategies/compoundingSSVStaking.js
  • Ran 197_deploy_compounding_staking_strategy.js against a mainnet fork; deployment and governance proposal execution completed successfully.

Code Change Checklist

To be completed before internal review begins:

  • The contract code is complete
  • Executable deployment file
  • Fork tests that test after the deployment file runs
  • Unit tests *if needed
  • The owner has done a full checklist review of the code + tests

Internal review:

  • Two approvals by internal reviewers

Refactored CompoundingStakingSSVStrategy to inherit CompoundingStakingStrategy.
Removed ConsolidationController.
Removed migrateClusterToETH.
@naddison36 naddison36 changed the title New CompoundingStakingStrategy without SSV functions. Vanilla compounding staking strategy and isolate SSV extension May 28, 2026
@naddison36 naddison36 changed the title Vanilla compounding staking strategy and isolate SSV extension Vanilla compounding staking strategy May 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 28, 2026

Codecov Report

❌ Patch coverage is 79.45205% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.95%. Comparing base (fa78fa9) to head (957631c).

Files with missing lines Patch % Lines
...rategies/NativeStaking/ConsolidationController.sol 10.00% 9 Missing ⚠️
...ategies/NativeStaking/NativeStakingSSVStrategy.sol 18.18% 9 Missing ⚠️
...egies/NativeStaking/CompoundingStakingStrategy.sol 92.47% 7 Missing ⚠️
...es/NativeStaking/CompoundingStakingSSVStrategy.sol 80.76% 5 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           nicka/initial-deposit-32    #2905      +/-   ##
============================================================
+ Coverage                     50.92%   50.95%   +0.03%     
============================================================
  Files                           110      110              
  Lines                          4882     4914      +32     
  Branches                       1356     1358       +2     
============================================================
+ Hits                           2486     2504      +18     
- Misses                         2392     2406      +14     
  Partials                          4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@naddison36 naddison36 changed the title Vanilla compounding staking strategy Vanilla validator compounding staking strategy May 29, 2026
Copy link
Copy Markdown
Member

@sparrowDom sparrowDom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is another approach to vanilla staking where the risk of not being able to fully verify the deposit on-chain is transferred from trusting the node operator's honesty to trusting our strategist's honesty.

currentState == ValidatorState.NON_REGISTERED ||
currentState == ValidatorState.VERIFIED ||
currentState == ValidatorState.ACTIVE,
"Not registered or verified"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we are mixing reverts with error messages with reverts throwing error objects ("CompoundingStakingSSVStrategy"). Are we hitting contract size limits and need to use error objects because of that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we are hitting size limits

{
ValidatorState currentState = validator[pubKeyHash].state;
require(
currentState == ValidatorState.NON_REGISTERED ||
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per this comment we could allow for currentState == ValidatorState.STAKED to also admit a stake. This way we can:

  • do an initial deposit of 1 ETH
  • verify on beaconChain that it has been deposited
  • reset the first deposit (ideally via a strategist action)
  • deposit remaining 2029 ETH

We can not get front-run this way by the node operator. Extra risk is carried by the strategist though. I guess we are to decide who we can trust more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with doing a 1 ETH deposit first is it currently takes 55 days for the deposit to be processed on the beacon chain. That's needed before the validator and deposit can be verified. It then takes another 55 days for the second 2029 ETH deposit to be processed. So that's 110 days if no yield.
While we have a large amount of ETH to deposit, we are taking on the risk that a large validator deposit could be front-run by the node operator.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the point I am trying to make is that we can do 1ETH & 2029ETH deposits both in 1 day. The verification that we aren't front ran can be done off-chain without waiting for the deposit to be processed by the beacon chain.

@sparrowDom
Copy link
Copy Markdown
Member

Claude's review

🔴 Blocker — 1. New implementation exceeds the EIP-170 size limit; migration 196 will fail to deploy on mainnet

CompoundingStakingSSVStrategy deployed (runtime) bytecode at HEAD = 25,077 bytes vs the 24,576-byte EIP-170 limit → over by 501 bytes, using
the repo's committed compiler settings (solc 0.8.28, optimizer.enabled = true, default runs, no overrides in hardhat.config.js). The
currently-deployed mainnet impl is 24,223 bytes.

Tests/compile pass locally only because allowUnlimitedContractSize: true is set on the hardhat network — but mainnet enforces EIP-170 at deploy
time
, so deployWithConfirmation("CompoundingStakingSSVStrategy", …) inside migration 196 would revert on mainnet.

Worth chasing: the committed config produces 25,077 bytes locally, yet the deployed mainnet artifact is only 24,223 bytes (854 smaller) — implying the
production pipeline historically used different/more-aggressive optimizer settings than what's committed, or the contract has grown since.

Action: Before queuing 196, compile under the exact production profile and confirm runtime bytecode < 24,576 (CONTRACT_SIZE=true pnpm hardhat compile). If over, raise optimizer.runs / add a per-contract override / viaIR, or move logic into a library.


🟠 High — 2. Off-chain stakeValidator skips BLS signature verification for first deposits below the cap

The contract treats any deposit to a REGISTERED validator as the validator-creating deposit (amount in [1 ETH, cap]). The beacon chain only checks
the BLS signature on that creating deposit; top-ups are not signature-checked. But the task gates verification on exact equality:

const amountWei = parseUnits(amount.toString(), 18);
const initialDepositAmountWei = await strategy.initialDepositAmountWei();
if (amountWei.eq(initialDepositAmountWei)) { /* verify real BLS sig */ }
else { /* substitute dummy 0x…0001 signature */ }

A first deposit of any amount below the cap (now a supported path, exercised by the new "Should stake less than the initial deposit amount" test)
takes the else branch and submits a dummy signature for a validator-creating deposit → the validator is never created and the ETH is stuck/lost.

Concretely reachable via the P2P/uuid flow: utils/p2pValidatorCompound.js hardcodes INITIAL_DEPOSIT_SIZE = "32000000000" (32 ETH), and the
--uuid path overwrites amount with it (tasks/validatorCompound.js:165). Migration 196 sets the cap to 32.25 ETH. 32 ≠ 32.25 → the task strips
the real P2P signature and submits a dummy one for the activating deposit.

Action: Detect the first/creating deposit by on-chain validator state (REGISTERED), not amount equality, and verify the real BLS signature whenever
state is REGISTERED regardless of amount. Align INITIAL_DEPOSIT_SIZE with the on-chain cap (or drive it from initialDepositAmountWei). Add an
integration test of the uuid path at the raised cap.


🟡 Medium — 3. Reusable upgradeCompoundingStakingSSVStrategy helper is a bare upgrade → initialDepositAmountWei == 0 DoS

The shared helper (deployActions.js:209–243, used by deploy/hoodi/003,006,007,009,010) does only upgradeTo(...) with no setInitialDepositAmount. On
any already-initialized proxy upgraded through it, the new slot defaults to 0; since first deposits require both >= 1 ether and <= initialDepositAmountWei, those constraints become mutually unsatisfiable → every new-validator first deposit reverts "Invalid first deposit amount"
until a governor sets the cap. Mainnet is safe (196 sets it atomically), but this is a footgun for future/testnet upgrades.

Action: Have the helper set a sane default (e.g. 1 ETH) if initialDepositAmountWei == 0 post-upgrade, or assert it's non-zero.

🟡 Medium — 4. Test coverage gaps

  • No stakeEth-level test of the production config. The renamed "…above the initial deposit amount" test never calls setInitialDepositAmount, so it
    only checks the cap against the default 1 ETH — same as the old == 1 ETH test. Add a test that raises the cap to 32.25 ETH and asserts a 32.25 ETH
    first deposit succeeds / >32.25 reverts.
  • No fork test for migration 196 (the PR's own "Fork tests that test after the deployment file runs" box is unchecked). Should assert
    initialDepositAmountWei() == 32.25 ETH post-proposal and that adjacent storage survives the gap shrink.
  • The off-chain sub-cap-signature path (Local Compound #2) has no test.

🟢 Low / Info / Nit

  • Stale storage-layout snapshot: storageLayout/mainnet/CompoundingStakingSSVStrategy.json still shows depositedWethAccountedFor → __gap[41] with no
    initialDepositAmountWei, and is not in the diff. Self-heals on mainnet deploy, but until then it can't catch a slot shift in review. Regenerate and
    commit.
  • PR description doesn't quantify the security trade-off: raising the cap moves worst-case per-incident front-run exposure 1 → 32.25 ETH (~32×). Worth
    one sentence + the mitigation (single-pending firstDeposit guard + resetFirstDeposit).
  • 32× per-incident exposure is an intentional, time-limited tradeoff; aggregate exposure stays bounded to one validator, front-run funds are correctly
    non-recoverable and accounted (verifyValidator:651–680). Ensure monitoring + resetFirstDeposit/pause runbooks are ready.
  • PlantUML diagram now says "1 ETH" — consistent with the default but doesn't depict the 32.25 ETH single-deposit window. Intentional per commit
    bb69b1e7a; consider a footnote.
  • ValidatorState.ACTIVE comment says "at least 32.25 ETH" but verifyBalances uses strict > (:1122) — should read "more than 32.25 ETH".
    Pre-existing; only the number changed here.

@naddison36
Copy link
Copy Markdown
Collaborator Author

Thanks for your review @sparrowDom

  1. I've got the contracts back under size. I've had to use more custom errors. These are not used by external users so will be ok.
  2. I've pushed a fix to the stakeValidator Hardhat task
  3. This upgrade helper is not used by the deploy script but I've updated it anyway.
  4. It's hard to fork test the consolidation to the new compounding staking contract when the contract or validators do not yet exist. We only have three more consolidations from the old NativeStakingStrategy. We could create unit tests with mocked beacon chain behaviour, but the beacon chain behaviour is key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants