Closed
Conversation
* feat: add share helpers * fix: add deposit scaling factor * fix: rebase
* fix: slashable window boundaries * test: regression for alm * test: update withdrawal delay not passed reversion * test: burning indices * refactor: switch conditionals * fix: added unit tests * test: assert slashable shares in queue * fix: typos --------- Co-authored-by: Yash Patil <ypatil12@gmail.com>
refactor small cleanup chore: `forge fmt` fix: `getQueuedWithdrawals` + test fix: add constructor back test: `totalQueued` > `withdrawal.strategies.length` test(wip): `completeQueuedWithdrawals` currently failing fix: effectBlock test(wip): @8sunyuan patch fix: one flaky test fix: second flaky test
* feat: initial deploy * feat: slashing patch
* test(wip): todos * fix: dealloc issue * fix: remaining * fix: forktest upgrade issue * test: add `check_Withdrawal_AsShares_State_AfterSlash` * refactor: cleanup * fix: ci * refactor: review changes
* docs: add slashing docs * chore: bindings * docs: fixed commenting and updated queue withdrawal docs * docs: minor cleanup --------- Co-authored-by: Nadir Akhtar <nadirakhtar123@gmail.com>
* fix: correct expected share calc * chore: bindings * fix: rounding on failing unit test
* chore: clean comments and naming in dm * refactor: simplify undelegate method * feat: removed 0 address check because 0 stakers cant be delegated * feat: condensed non-staker caller logic * refactor: remove unnecessary check * feat: use checks-effects-interactions when completing withdrawals * feat: remove implicit public method for queuedWithdrawals and impl dedicated getter * feat: deprecate withdrawer field * chore: make bindings and clean compile errors * refactor: redelegate reuses delegateTo and undelegate * fix: broken integration test * docs: update to reflect deprecated field * feat: add getter for stakers withdrawal roots
* fix: initialization params * fix: roll blocks usage
* fix: integration test initialization params (#978) * fix: initialization params * fix: roll blocks usage * fix: `SignatureUtils` construction --------- Co-authored-by: Yash Patil <40046473+ypatil12@users.noreply.github.com> Co-authored-by: davidironblocks <david@ironblocks.com>
* fix: readd manual checks * chore: forge fmt
* feat: add step 1 * feat: step 1 & 2 complete; pending step 3 sanity * test: add `_validateProxyDomainSeparators` * feat: add rc validation --------- Co-authored-by: clandestine.eth <96172957+0xClandestine@users.noreply.github.com>
* fix: update alloc delay bound * test: remove unnecessary roll
* docs: shares accounting * docs: fix gh markdown view * docs: try fix gh again * docs: cleanup * docs: edit share accounting * docs: wrap up share accounting doc * docs: edit edge cases --------- Co-authored-by: wadealexc <pragma-services@proton.me>
* refactor: burning * chore: fmt * chore: update storage report * chore: update readme * refactor: add burnableShares for epm storage * chore: update storage report
* docs: finish delegation manager docs * docs: update docs readme * docs: permission controller * fix: small typos * docs: address feedback * docs: nit --------- Co-authored-by: Michael Sun <michaelsun97@gmail.com>
* docs: update StrategyManager docs with slashing delta * docs: remove references to thirdPartyTransfersForbidden * docs: update strategy docs to latest * also various edits to docs and natspec * chore: fmt and make bindings --------- Co-authored-by: wadealexc <pragma-services@proton.me>
…1149) **Motivation:** Improve slashing invariants, fix existing DM/ALM invariants, and refactor user generation logic **Modifications:** * Improve `IntegrationDeployer._randUser` generation logic so that additional helpers can be implemented to generate users with specific assets * Update some withdrawal invariants to distinguish between deposit and withdrawable shares. More work to be done here. * Implement several new tests in `SlashingWithdrawals` * Removes the vscode settings file since it was autoformatting tests. Spoke with Rajath about this - i'm fine to revisit this after slashing, but the current impact of this file is that it will create massive diffs across the most active part of the codebase while we're trying to get critical last-mile testing finished. We'll readd this after slashing :+1: **Followup**: * Additional cleanup for queue/complete withdrawal invariants and calling conventions * Additional tests needed in `SlashingWithdrawals` * Additional staker-level invariants needed when slashing an operator
**Motivation:** Implement new slashing invariants and add existing ones to checks. **Modifications:** - Explicit calculation of expected withdrawable shares and dsf after delegation and on completion of full withdrawal - Add "unchanged dsf" assertion - Add increased/decreased minSlashableStake assertions - Overload minSlashableStake assertions to handle address input - Add assertions to slashing flow state checks - Fix `_choose` helper function **Result:** More complete coverage for slashing invariants --------- Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local>
**Motivation:** The changes are focused on adding test coverage for pre-slashing withdrawal scenarios and how the slashing upgrade affects them. **Modifications:** 1. Added new test cases for withdrawal completion: - [x] `testFuzz_upgrade_delegate_queuePartial_completeAsShares` - [x] `testFuzz_upgrade_delegate_queuePartial_completeAsTokens` - [x] `testFuzz_delegate_deposit_queue_completeBeforeUpgrade_asShares` - [x] `testFuzz_delegate_deposit_queue_completeBeforeUpgrade_asTokens` - [x] `testFuzz_delegate_deposit_queue_completeBeforeUpgrade_partial_asShares` - [x] `testFuzz_delegate_deposit_queue_completeBeforeUpgrade_partial_asTokens` **Result:** Move coverage.
**Motivation:** The upstream repository has commented out path filtering in the foundry.yml workflow. To avoid PRs from getting blocked, we're making the same change in our branch. **Modifications:** * Commented out the `paths` section in the foundry.yml workflow. **Result:** PRs targeting `test/slashing-integration-testing` won't get blocked.
**Motivation:** We need to test full slashes for EigenPods. **Modifications:** Added the `SlashType` enum which slashes either the normal amount (10 gwei), half the validators balance, or fully slashes the validator. This needs to be passed in on every call to `slashValidators` Tests: - [x] test_fullSlash_Delegate - [x] test_fullSlash_Revert_Redeposit **Result:** Expressive BC slashing. Note, delegation does not revert when the slashing factor of a staker is zero.
**Motivation:** Add additional upgrade tests for beacon chain **Modifications:** *Describe the modifications you've done.* - [x] testFuzz_deposit_upgrade_slash_completeCheckpoint - [x] testFuzz_deposit_fullSlash_upgrade_delegate - [x] testFuzz_deposit_fullSlash_upgrade_deposit_delegate - [x] testFuzz_slash_migrate Move negative share tests into `EigenPod.t.sol`. Also consolidated migration file. **Result:** BC upgrade tests complete :)
**Motivation:** These tests cover scenarios that specifically validate correct behavior on the cusp of a delay completing. **Modifications:** This PR adds test that perform a slash on the following critical timing scenarios: * Before/after a partial/total withdrawal is no longer slashable * Before/after an operator fully deallocates * Before/after an operator completes deregistration from an operator set * Before/after an operator completes an allocation **Result:** These timing tests ensure that the correct behavior happens on either side of critical delays --------- Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local>
**Motivation:** On undelegation, deposit shares should be zero, there is no need to check the diff. **Modifications:** Added `assert_RemovedAll_Staker_DepositShares` and updated `assert_Snap_RemovedAll_Staker_WithdrawableShares` to be non-snap. **Result:** Stricter undelegate/redelegate invariants
**Motivation:** Tests that handle slashes by both the BC and AVS onto Native ETH. **Modifications:** Added assertions for BC/AVS slashings, no full slashed included - [x] testFuzz_avsSlash_bcSlash_checkpoint - [x] testFuzz_bcSlash_checkpoint_avsSlash - [x] testFuzz_avsSlash_verifyValidator_bcSlash_checkpoint - [x] testFuzz_avsSlash_bcSlash_verifyValidator_checkpoint - [x] testFuzz_avsSlash_bcSlash_checkpoint_verifyValidator - [x] testFuzz_avsSlash_bcSlash_balanceIncrease_checkpoint - [x] testFuzz_avsSlash_bcSlash_checkpoint_balanceIncrease **Result:** Test coverage for BC/AVS edge cases. Need to properly bound a lot of the AVS/BC slashing cases, but this is a start --------- Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local> Co-authored-by: Nadir Akhtar <nadir-akhtar@users.noreply.github.com>
**Motivation:** Slashable shares in queue aren't currently checked by any invariants. **Modifications:** - added assertion that slashable shares in queue decrease on slash if a withdrawal is queued - added assertion that slashable shares in queue increase on queued withdrawal - changed IntegrationDeployer to initialize starting block as nonzero to address edge cases **Result:** Invariant coverage for slashable shares in queue state --------- Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local>
**Motivation:**
We want to increase coverage for integration state validation.
**Modifications:**
- Added new test cases:
-
`testFuzz_deposit_delegate_allocate_partialSlash_redeposit_queue_complete`
- `testFuzz_deposit_delegate_undelegate_partialSlash_complete`
- `testFuzz_deposit_delegate_deallocate_partialSlash_queue_complete`
- `testFuzz_deposit_delegate_deregister_partialSlash_queue_complete`
-
`testFuzz_delegate_zeroShares_partialSlash_deposit_undelegate_complete`
- `testFuzz_deposit_delegate_allocate_partialSlash_deallocate`
**Result:**
Move coverage.
---------
Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local>
**Motivation:** The staker strategy list should properly account for strategies when deposited or queued completely for withdrawal. **Modifications:** Added the following checks: - assert_StrategyNotInStakerStrategyList - assert_StrategyInStakerStrategyList - assert_StakerStrategyListEmpty The first two are used on deposits or withdrawals. The last is used on undelegations. **Result:** More comprehensive invariant checks
**Motivation:** We want to check the DSF on - Delegation (increase when non-WAD maxMag, else stay same) - Deposits (increase on non-WAD slashing factor, else stay same) - Completing withdrawals as shares (same as deposit) **Modifications:** Added `assert_Snap_DSF_State_Deposit`, `assert_Snap_DSF_State_Delegation`, `assert_Snap_DSF_State_CompleteAsShares` **Result:** Stricter DSF invariants
**Motivation:** Need tests to ensure proper handling of edge cases when stakers interact with operators who have been fully slashed. **Modifications:** - Added new integration test file `SlashedOperator.t.sol` with three key test cases: - [x] `testFuzz_register_allocate_fullSlash_deposit_delegate`: Tests that delegating to a fully slashed operator fails after deposits - [x] `testFuzz_register_allocate_fullSlash_delegate_deposit`: Tests the interaction when delegation to a slashed operator occurs before deposits - [x] `testFuzz_register_allocate_fullSlash_delegate_redelegate_deposit`: Tests the redelegation flow when a staker moves from a slashed operator to a new operator **Result:** After these changes, the protocol has better test coverage for slashed operator scenarios.
**Motivation:** There exists an edge case where: 1. Alice queues a withdrawal for all her shares 2. Alice's operator is slashed 3. Alice checkpoints (no rewards or slashes). This actually increases her DSF, even though there wasn't a balance increase on the beacon chain 4. Alice completes withdrawal as tokens 5. Alice redeposits After step 5, Alice's withdrawable shares are overcounted due to increasing the DSF. Note that in step 4 if Alice completes her withdrawal as shares she can still inflate her withdrawable shares. **Modifications:** The fix is to prevent any state updates if `addedShares` is 0. In addition, we also update `recordBeaconChainETHBalanceUpdate` to only call the DM if there is a nonzero `balanceDeltaWei`, which is symmetric to the behavior in the `StrategyManager`. Regression tests: - [x] testFuzz_noCall_zeroBalanceUpdate - [x] testFuzz_deposit_delegate_allocate_slash_checkpointZeroBalance - [x] testFuzz_deposit_delegate_allocate_slashAndQueue_checkpoint_redeposit - [x] testFuzz_deposit_delegate_allocate_slashAndQueue_completeAsTokens_redeposit - [x] testFuzz_deposit_delegate_allocate_slashAndQueue_checkPoint_completeAsShares **Result:** Inflating withdrawable shares edge case fixed.
**Motivation:** Additional tests on beacon chain slashing **Modifications:** Full slash tests: - [x] testFuzz_proveValidator_checkpoint_queue_completeAsTokens - [x] test_fullSlash_Revert_Redeposit - [x] testFuzz_fullSlash_registerStakerAsOperator_Revert_Redeposit - [x] testFuzz_fullSlash_registerStakerAsOperator_delegate_undelegate_completeAsShares Partial Slash Tests: - [x] testFuzz_redeposit_queue_completeAsTokens - [x] testFuzz_redeposit_queue_completeAsShares Additionally: - Updated `_getExpectedTokenBalances` to factor in gwei rounding down for EigenPods - Updated `queuedWithdrawal` check when delegated or undelegated - Got rid of unnecessary checkpointing in `SlashedEigenPod.t.sol` **Result:** All single beacon chain slashing state validation complete.
**Motivation:** Final upgrade tests, focusing on completing withdrawals after an operator has been slashed. **Modifications:** 2 tests are failing. Dependent on #1176 - [x] testFuzz_delegate_deposit_queue_upgrade_slashFully_completeAsTokens - [x] testFuzz_delegate_deposit_queue_upgrade_slashFully_revertCompleteAsShares - [x] testFuzz_delegate_deposit_queue_upgrade_slash_completeAsTokens - [x] testFuzz_delegate_deposit_queue_upgrade_slash_completeAsShares **Result:** Upgrade tests complete
**Motivation:** Adding integration testing coverage **Modifications:** Added: - `testFuzz_fullSlash_undelegate_redeposit_complete` - `testFuzz_fullSlash_redelegate_redeposit_complete` - `testFuzz_delegateSlashedStaker_slashedOperator` Amended `Integration_Deposit_Delegate_Allocate_Slash_Queue_Redeposit` and `SlashingWithdrawals` with BC ETH. Fixed DSF assertion **Result:** Better coverage of scenarios involving BC ETH --------- Co-authored-by: Michael <michael@Michaels-MacBook-Pro.local>
**Motivation:** Address TODOs on `Slahsing_EigenPod_AVS.t.sol` **Modifications:** Uncomment out checks **Result:** Comprehensive test file
**Motivation:** We want to validate key invariant in our system for any share changes. **Modifications:** Adds `assert_DepositShares_GTE_WithdrawableShares` **Result:** More comprehensive invariants
**Motivation:** State Validation tests around slashing on beacon chain, relegating, getting slashed by an operator. **Modifications:** Adds the following tests, some are failing due to the DSF inflation bug. - [x] testFuzz_verifyAdditionalValidator_delegateSlashedStaker - [x] testFuzz_slashOnBC_delegate_verifyValidator_undelegate_completeAsTokens - [x] testFuzz_slashOnBC_delegate_verifyValidator_undelegate_completeAsShares - [x] testFuzz_slashOnBC_delegate_verifyValidator_redelegate_completeAsTokens - [x] testFuzz_slashOnBC_delegate_verifyValidator_redelegate_completeAsShares - [x] testFuzz_delegate_redelegate_slashAVS_slashBC_verifyValidator_completeAsShares - [x] testFuzz_delegate_redelegate_slashAVS_slashBC_verifyValidator_completeAsTokens - [x] testFuzz_delegate_redelegate_slashAVS_slashBC_completeAsShares_verifyValidator - [x] testFuzz_delegate_redelegate_slashAVS_slashBC_completeAsTokens_verifyValidator Updates withdrawal as shares assertion to handle dsf rounding. **Result:** Beacon Chain State Validation
<!--
🚨 ATTENTION! 🚨
This PR template is REQUIRED. PRs not following this format will be
closed without review.
Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
- Provide clear and specific details in each section
-->
**Motivation:**
Testing coverage for rounding edge cases.
**Modifications:**
Added test file in `Register_Allocate_Slash_VerifyWC_.t.sol`
The _init setup creates an Operator registered to an OperatorSet who
gets slashed down to 1 maxMagnitude.
- Removed duplicate function `assert_Snap_StakeBecomeUnslashable`
- Added setter for BCSF on EPMWrapper
- Fixed `assert_Snap_DSF_State_WithdrawalAsShares()` for when fully
slashed
- Fixed `assert_Snap_DSF_State_Delegation()` to use staker's operator
magnitudes instead of staker's magnitude itself
- Additional checks helpers
- Added helper `getSlashingFactor` for User.t.sol
**Result:**
Integration Tests with 1 operator MaxMagnitude
`test_slashBC_startCompleteCP_queue_complete`
`test_slash_undelegate_redeposit`
`test_slash_undelegate_completeAsTokens_verifyWC`
`test_slash_undelegate_completeAsShares_startCompleteCP`
`test_queueAllShares_completeAsTokens`
Integration Tests with 1 BCSF
`test_verifyWC_startCompleteCP`
`test_slashFullyBC_deposit`
`test_slashAVS_delegate_startCompleteCP`
Integration Tests with High DSF and repeat deposits
`test_multiple_deposits`
<!--
🚨 ATTENTION! 🚨
This PR template is REQUIRED. PRs not following this format will be
closed without review.
Requirements:
- PR title must follow commit conventions:
https://www.conventionalcommits.org/en/v1.0.0/
- Label your PR with the correct type (e.g., 🐛 Bug, ✨ Enhancement, 🧪
Test, etc.)
- Provide clear and specific details in each section
-->
**Motivation:**
`EigenPodManager.recordBeaconChainETHBalanceUpdate` currently has a bug
where shares increases of 0 addedShares will forward a call to the
DelegationManager and updates the DSF. This can be manipulated to
increase your DSF and inflate a staker's inflatable shares. The test in
this PR demonstrates a possible exploit scenario.
**Modifications:**
Added test file `Integration_CompleteCP_WithNoAddedShares` and
integration test `test_completeCP_withNoAddedShares`
Setup is as follows:
1. Setup a Staker EigenPod where they are initially slashed and have a
non-WAD BCSF
2. Queue withdraw all deposit shares setting it to 0
3. Start/Complete checkpoints with 0 addedShares
- This triggers the following code block in `SlashingLib.sol` when
updating the DSF and increases the staker's DSF
```
if (prevDepositShares == 0) {
// If this is the staker's first deposit or they are delegating to an operator,
// the slashing factor is inverted and applied to the existing DSF. This has the
// effect of "forgiving" prior slashing for any subsequent deposits.
dsf._scalingFactor = dsf.scalingFactor().divWad(slashingFactor);
return;
}
```
4. Deposit again (in this case complete withdrawals as shares from step
3)
5. Delegate to an Operator
Staker ends up having inflated withdrawable shares and as a result is
able to delegate way more share than they should be able to for an
operator.
**Result:**
After this issue is fixed, this integration test should pass
**Motivation:** Remaining dual slashing cases, focused on fully slashing on AVS and BC **Modifications:** Adds the following tests: - [x] testFuzz_fullDualSlash_undelegate_verifyValidator_checkpoint_exitEverything - [x] testFuzz_fullDualSlash_redeposit_revertCheckpoint - [x] testFuzz_fullDualSlash_checkpoint Also, consolidates the `checkpoint` checks to be less repetitive **Result:** Completed AVS/BC Slash Tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Slashing Integration Testing for Competition Audit
Modifications:
General State Validation
Upgrade Tests
Dual Slash Tests
Rounding Tests
EigenPod Tests
Invariants
depositShares>=withdrawableSharesinvariant #1201Result:
Comprehensive Test Coverage