test: checkpoint with no added shares#1191
Merged
8sunyuan merged 3 commits intotest/slashing-integration-testingfrom Mar 4, 2025
Merged
test: checkpoint with no added shares#11918sunyuan merged 3 commits intotest/slashing-integration-testingfrom
8sunyuan merged 3 commits intotest/slashing-integration-testingfrom
Conversation
0xClandestine
added
🗡️ Slashing Release
Contributor
|
Can we merge this into the fix PR? |
Contributor
Author
8sunyuan
force-pushed
the
test/checkpoint-noaddedshares
branch
from
8d72593 to
e9e2d71
Compare
ypatil12
reviewed
Mar 4, 2025
src/test/integration/tests/eigenpod/CompleteCP_WithNoAddedShares.t.sol
Outdated
Show resolved
Hide resolved
8sunyuan
force-pushed
the
test/checkpoint-noaddedshares
branch
from
fe396dd to
254a8f1
Compare
ypatil12
approved these changes
Mar 4, 2025
Contributor
ypatil12
left a comment
ypatil12
left a comment
There was a problem hiding this comment.
LGTM, one clarifying comment
| (uint256[] memory withdrawableSharesAfter, uint256[] memory depositSharesAfter) = delegationManager.getWithdrawableShares(address(staker), strategies); | ||
| assertEq(depositSharesAfter[0], initDelegatableShares[0], "Deposit shares should reset to reflect slash(es)"); | ||
| assertApproxEqAbs(withdrawableSharesAfter[0], depositSharesAfter[0], 100, "Withdrawable shares should equal deposit shares after withdrawal"); | ||
| assertApproxEqAbs(withdrawableSharesAfter[0], depositSharesAfter[0], 1000, "Withdrawable shares should equal deposit shares after withdrawal"); |
Contributor
There was a problem hiding this comment.
Assume this check is sanity? Since we do assert_Snap_Added_Staker_WithdrawableShares in the complete as shares check.
Contributor
Author
There was a problem hiding this comment.
There were some instances where it was failing with a real delta of 103. Perhaps because my added contract is inheriting the other tests
ypatil12
pushed a commit
that referenced
this pull request
Mar 5, 2025
<!--
🚨 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
This was referenced Mar 5, 2025
ypatil12
pushed a commit
that referenced
this pull request
Mar 5, 2025
<!--
🚨 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
ypatil12
added a commit
that referenced
this pull request
Mar 5, 2025
**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
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
3 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:
EigenPodManager.recordBeaconChainETHBalanceUpdatecurrently 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_WithNoAddedSharesand integration testtest_completeCP_withNoAddedSharesSetup is as follows:
SlashingLib.solwhen updating the DSF and increases the staker's DSFStaker 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