OZ C-01 - Postponed Pending Deposit Breaks verifyDeposit and verifyBalances

[naddison36]

Changes

• verifyDeposit:
  • checks that the validator of the first pending deposit is not exiting. It does this using a second proof to the validator's withdrawable epoch. This proof can be after the first pending deposit proof so deposits to new validators have had time to be registered on the beacon chain.
  • checks if the strategy's validator is exiting. If it is, it records the withdrawable epoch in the deposit data.
• verifyBalances:
  • Verifies validators before deposits so exited validators can have their state set to EXITED
  • If a deposit is to an exiting validator, it checks the withdrawable epoch has not been reached
  • will revert if there is a deposit to an exiting validator that is on or after the withdrawable epoch and the validator has not yet withdrawn

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

Deploy checklist

Two reviewers complete the following checklist:

- [ ] All deployed contracts are listed in the deploy PR's description
- [ ] Deployed contract's verified code (and all dependencies) match the code in master
- [ ] Contract constructors have correct arguments
- [ ] The transactions that interacted with the newly deployed contract match the deploy script.
- [ ] Governance proposal matches the deploy script
- [ ] Smoke tests pass after fork test execution of the governance proposal

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against bcf207f

[sparrowDom]

if we verify Index here do we have a need for a separate verifyValidator function?

[sparrowDom]

This function could promote a validator from STAKED to VERIFIED and add an entry to verifiedValidators

[naddison36]

There's a few options:

1. Consolidate verifyValidator into verifyDeposit
2. Add the validatorIndex to the mapping of the validator pub key hash
3. Iterate over the verifiedDeposits to get the validator index in verifiedValidators

[naddison36]

I've gone with option 2 which means I don't need to pass in the validator index or pubKeyProof in strategyValidatorData
See commit 93b398b

[sparrowDom]

This is probably the commit the comment was referring to: 7ee0a4d

[sparrowDom]

Left a few comments. Great job on these changes aside from 2 issues I think they are solid

[sparrowDom]

nit comment addition:
... validator is fully exited. The ETH deposited to an exiting validator remains in the beaconState.pendingDeposits as part of the beacon chain's deposits_to_postpone array inside which the deposit remains as long as the validator has not fully exited from the beacon chain. Once the validator has fully exited the postponed deposit is credited back to the strategy contract.

[sparrowDom]

🔴 in StakeEth the depositData.withdrawableEpoch is set to FAR_FUTURE_EPOCH. This means if verify deposit has not been called yet on a withdrawable validator the verificationEpoch < depositData.withdrawableEpoch check will always be true and make this whole require pass.

I think this require should look like this:

require(
  firstPendingDeposit.slot < depositData.slot ||
      (
         verificationEpoch < depositData.withdrawableEpoch &&
         depositData.withdrawableEpoch != FAR_FUTURE_EPOCH
       ) ||
      validatorState[depositData.pubKeyHash] ==
          VALIDATOR_STATE.EXITED,
      "Deposit likely processed"
  );

[naddison36]

Good spot. Yes, the logic was wrong. It was meant to be

// Check the stored deposit is still waiting to be processed on the beacon chain.
// That is, the first pending deposit slot is before the slot of the staking strategy's deposit.
// If the deposit has been processed, it will need to be verified with `verifyDeposit`.
// OR the deposit is to an exiting validator so check it is still not withdrawable.
// If the validator is not withdrawable, then the deposit can not have been processed yet.
// If the validator is now withdrawable, then the deposit may have been processed. The strategy
// now has to wait until the validator's balance is verified to be zero.
require(
    firstPendingDeposit.slot < depositData.slot ||
        (validatorState[depositData.pubKeyHash] ==
            VALIDATOR_STATE.EXITED &&
            verificationEpoch < depositData.withdrawableEpoch),
    "Deposit likely processed"
);

Fixed in 93b398b

[sparrowDom]

Nice that is cleaner and simpler than my suggestion

[sparrowDom]

Looking at this 2 possible implementations of require won't the latter block the verifyBalances until the validator that has been deposited to and is exiting has fully exited? While the former would allow for passing of verifyBalances even if the validator is exiting and the snapshot has been done before its withdrawable epoch?

[naddison36]

You are correct. I've fixed with commit 78e29f1

[codecov]

Codecov Report

❌ Patch coverage is 86.17021% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.61%. Comparing base (fd21ed6) to head (bcf207f).
⚠️ Report is 1 commits behind head on nicka/pectra.

Files with missing lines	Patch %	Lines
...gies/NativeStaking/CompoundingValidatorManager.sol	81.42%	13 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           nicka/pectra    #2622      +/-   ##
================================================
+ Coverage         40.54%   40.61%   +0.06%     
================================================
  Files               121      121              
  Lines              5628     5666      +38     
  Branches           1494     1502       +8     
================================================
+ Hits               2282     2301      +19     
- Misses             3344     3363      +19     
  Partials              2        2              

☔ 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.