Skip to content

fix: DSF increase on checkpointed nonzero shares #1176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Mar 3, 2025
Merged

fix: DSF increase on checkpointed nonzero shares #1176

merged 9 commits into from
Mar 3, 2025

Conversation

ypatil12
Copy link
Collaborator

@ypatil12 ypatil12 commented Feb 26, 2025 •
edited
Loading

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:

  • testFuzz_noCall_zeroBalanceUpdate
  • testFuzz_deposit_delegate_allocate_slash_checkpointZeroBalance
  • testFuzz_deposit_delegate_allocate_slashAndQueue_checkpoint_redeposit
  • testFuzz_deposit_delegate_allocate_slashAndQueue_completeAsTokens_redeposit
  • testFuzz_deposit_delegate_allocate_slashAndQueue_checkPoint_completeAsShares

Result:

Inflating withdrawable shares edge case fixed.

@ypatil12 ypatil12 force-pushed the yash/zeroShares-dsf-update-fix branch from c83dbf3 to dca5e5a Compare February 26, 2025 20:18
@ypatil12
Copy link
Collaborator Author

ypatil12 commented Feb 26, 2025 •
edited
Loading

Regression test catching edge case:

Screenshot 2025-02-26 at 5 57 41 PM

@github-actions github-actions bot added 🐞 Bug Something isn't working. 📖 Documentation Improvements or additions to documentation. 🧪 Test Test-related changes (unit, integration, etc.). 🔧 Fix A bug fix or minor correction. labels Feb 26, 2025
@ypatil12 ypatil12 force-pushed the yash/zeroShares-dsf-update-fix branch from 789c02d to 2097f54 Compare February 26, 2025 23:24
@ypatil12 ypatil12 added the 🗡️ Slashing Release Changes for the slashing release. label Feb 26, 2025
@github-actions github-actions bot removed the 🗡️ Slashing Release Changes for the slashing release. label Feb 26, 2025
@ypatil12 ypatil12 force-pushed the yash/zeroShares-dsf-update-fix branch from 4d08720 to 9209df6 Compare February 26, 2025 23:33
8sunyuan
8sunyuan previously approved these changes Feb 27, 2025
View reviewed changes
eigenmikem
eigenmikem previously approved these changes Feb 27, 2025
View reviewed changes
src/test/integration/IntegrationBase.t.sol
@@ -2709,6 +2735,10 @@ abstract contract IntegrationBase is IntegrationDeployer, TypeImporter {
return curShares;
}

function _getExpectedWithdrawableSharesAfterCompletion(User staker, uint scaledShares, uint depositScalingFactor, uint slashingFactor) internal view returns (uint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, just want to note that I have separate code dealing with this scenario in our testing branch so we'll have to remove one or the other down the line

@0xClandestine 0xClandestine mentioned this pull request Feb 27, 2025
Merged
0xClandestine added a commit that referenced this pull request Feb 27, 2025
@0xClandestine
chore: improve labeler config (#1185)
62ae1cb 
**Motivation:**

Enhance the Labeler CI automation to improve accuracy and efficiency.

**Modifications:**

- Prevent both Bug and Fix labels from being added simultaneously. See
#1176

**Result:**

Labeler CI automation should be more streamlined, and less error prone.
0xClandestine
0xClandestine requested changes Feb 27, 2025
View reviewed changes
@0xClandestine 0xClandestine force-pushed the yash/zeroShares-dsf-update-fix branch from 9209df6 to 5af968a Compare February 27, 2025 16:05
@github-actions github-actions bot added ✨ Enhancement New feature or request. and removed 🐞 Bug Something isn't working. labels Feb 27, 2025
@ypatil12 ypatil12 mentioned this pull request Feb 27, 2025
Merged
4 tasks
@0xClandestine 0xClandestine self-requested a review February 27, 2025 16:29
0xClandestine
0xClandestine previously approved these changes Feb 27, 2025
View reviewed changes
This was referenced Feb 27, 2025
Merged
Merged
@ypatil12 ypatil12 dismissed stale reviews from 0xClandestine, 8sunyuan, and eigenmikem via f173fed March 3, 2025 01:41
wadealexc
wadealexc previously approved these changes Mar 3, 2025
View reviewed changes
@ypatil12 ypatil12 force-pushed the yash/zeroShares-dsf-update-fix branch from f173fed to aff1ce1 Compare March 3, 2025 16:45
nadir-akhtar
nadir-akhtar previously approved these changes Mar 3, 2025
View reviewed changes
8sunyuan
8sunyuan previously approved these changes Mar 3, 2025
View reviewed changes
@ypatil12 ypatil12 dismissed stale reviews from 8sunyuan, nadir-akhtar, and wadealexc via 0e60c46 March 3, 2025 18:45
@ypatil12 ypatil12 merged commit 13c5acf into dev Mar 3, 2025
12 checks passed
@ypatil12 ypatil12 deleted the yash/zeroShares-dsf-update-fix branch March 3, 2025 19:47
ypatil12 added a commit that referenced this pull request Mar 3, 2025
@ypatil12
fix: DSF increase on checkpointed nonzero shares (#1176)
9cd1d2d 
**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.
ypatil12 added a commit that referenced this pull request Mar 4, 2025
@ypatil12
test: remaining upgrade tests (#1187)
b3978ee 
**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
ypatil12 added a commit that referenced this pull request Mar 5, 2025
@ypatil12
fix: DSF increase on checkpointed nonzero shares (#1176)
67c4722 
**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.
ypatil12 added a commit that referenced this pull request Mar 5, 2025
@ypatil12
test: remaining upgrade tests (#1187)
5e0ec6e 
**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
This was referenced Mar 5, 2025
Closed
Merged
ypatil12 added a commit that referenced this pull request Mar 5, 2025
@ypatil12
fix: DSF increase on checkpointed nonzero shares (#1176)
2c28727 
**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.
ypatil12 added a commit that referenced this pull request Mar 5, 2025
@ypatil12
test: remaining upgrade tests (#1187)
87eba8d 
**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
ypatil12 added a commit that referenced this pull request Mar 5, 2025
@ypatil12
test: slashing integration testing (#1211)
2185542 
**Motivation:**

Slashing Integration Testing for Competition Audit

**Modifications:**

***General State Validation***
- #1204
- #1198
- #1169
- #1158

***Upgrade Tests***
- #1187
- #1171
- #1143

***Dual Slash Tests***
- #1195
- #1153

***Rounding Tests***
- #1178

***EigenPod Tests***
- #1191
- #1188
- #1203
- #1194
- #1163

***Invariants***
- #1201
- #1176
- #1192
- #1197
- #1175
- #1189
- #1150
- #1149

**Result:**

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

@eigenmikem eigenmikem eigenmikem approved these changes

@nadir-akhtar nadir-akhtar nadir-akhtar left review comments

@8sunyuan 8sunyuan 8sunyuan left review comments

@0xClandestine 0xClandestine 0xClandestine left review comments

+1 more reviewer

@wadealexc wadealexc wadealexc left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
📖 Documentation Improvements or additions to documentation. ✨ Enhancement New feature or request. 🔧 Fix A bug fix or minor correction. 🧪 Test Test-related changes (unit, integration, etc.).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ypatil12 @nadir-akhtar @wadealexc @8sunyuan @0xClandestine @eigenmikem