feat: burn erc20s on slashing #881

8sunyuan · 2024-11-08T16:26:18Z

Synchronous Burning

One solution for burning slashed shares is to simply take the existing logic, take the sharesToDecrement in calcSlashedAmount and just burn that amount. But for all shares that were withdrawn and are still slashable for MIN_WITHDRAWAL_DELAY_BLOCKS, they would have to be slashed and burned when those withdrawals are completed.

Instead, completed withdrawals don't burn and we calculate all the shares to burn upon slashing. We burn synchronously all those slashabe shares in the queue by reading all the cumulative withdrawn shares from the time of slashing and MIN_WITHDRAWAL_DELAY_BLOCKS into the past. Here is where checkpointed values come into play again.

Changes:

Snapshots.sol: Added back OZ CheckpointsUpgradeable library for blocknumbers to the Snapshots library. The reasoning is because we want to keep track of cumulative queue withdrawn shares from an operator for a specific strategy and not using a default WAD value like we use for magnitudes.
DelegationManager.sol: renamed function decreaseAndBurnOperatorShares which will now read all the slashable shares queue withdrawn from an operator for a specific strategy, calculate amount of shares to burn, and call StrategyManager.burnShares.
StrategyManager.sol: burnShares function is exact same as the withdraw function but with the destination address being the DEFAULT_BURN_ADDRESS (address 0).
SlashingLib.sol: Changed calcSlashedAmount back to taking in as input prevMaxMagnitude and newMaxMagnitude because the way that burned shares are calculated with cumulative scaled shares.

Note: The call trace begins from ALM.slashOperator -> DM.decreaseAndBurnOperatorShares -> SM.burnShares -> Strategy.withdraw -> underlyingToken.transfer Each of these calls are permissioned by the core contracts in the protocol with the exception of slashOperator which is only callable by the AVS. We may want to additionally consider for a reentrancy guard on slashOperator as a result.

TODO:

Added unit and integration tests

src/contracts/core/DelegationManagerStorage.sol

gpsanant · 2024-11-08T17:55:43Z

src/contracts/core/StrategyManagerStorage.sol

@@ -26,6 +26,9 @@ abstract contract StrategyManagerStorage is IStrategyManager {
    // index for flag that pauses deposits when set
    uint8 internal constant PAUSED_DEPOSITS = 0;

+    /// @notice default address for burning slashed shares and transferring underlying tokens
+    address public constant DEFAULT_BURN_ADDRESS = address(0);


can we do some analysis on if this is ok for LSTs?

It's for sure not, placeholder value for now. Personally a fan of 0xdeadbeef

91a806b
0xDeaDBeef

We should at least do 0x000000000000000000000000000000000000dEaD or address(0) since other protocols use that. Working with Matt Nelson to get feedback

Looks like we cannot do address(0) given that stETH's contract

0x E 1 6 E 4 (eigen)

gpsanant · 2024-11-08T18:04:23Z

src/contracts/libraries/Snapshots.sol

+     * withdrawal snapshot is probably "recent", defined as being among the last sqrt(N) withdrawal snapshots where N is the number of
+     * withdrawal snapshots.
+     */
+    function getAtProbablyRecentBlock(


just do upperLookup imo. is this any different?

It's the same logic but named more clearly for blocknumber usage. I can change this back though to be more consistent with DefaultWadHistory.

changing the name makes it clearer to me imo

gpsanant · 2024-11-08T18:08:03Z

src/contracts/core/DelegationManager.sol

@@ -685,6 +699,26 @@ contract DelegationManager is
        _beaconChainSlashingFactor[staker] = bsf;
    }

+    /// @dev Calculate amount of withdrawable shares from an operator for a strategy that is still in the queue
+    /// and therefore slashable.
+    function _getSlashableSharesInQueue(address operator, IStrategy strategy) internal view returns (uint256) {


can we expose a public view func for this?

gpsanant · 2024-11-08T18:11:39Z

src/contracts/core/DelegationManager.sol

+        IStrategy strategy,
+        uint256 queueWithdrawnShares
+    ) internal {
+        uint256 currCumulativeWithdrawalShares = uint256(_cumulativeWithdrawalsHistory[operator][strategy].latest());


this PR looks nice!

my main feedback is that we should store cumulative scaled shares queued for withdrawal, not shares.

Upon burning we should multiply the scaled shares in the withdrawal queue by the slashed magnitude. This would make it so that view functions for introspection into the withdrawal queue automatically update to reflect correct values after slashing

138eef7
Added, had to change calcSlashedAmount back to old interface but it is more consistent with the way scaledShares are also calculated.

github-actions · 2024-11-11T20:42:38Z

Reading tracefile ./lcov.info.pruned
                                        |Lines       |Functions  |Branches    
Filename                                  |Rate     Num|Rate    Num|Rate     Num
================================================================================
[src/contracts/]
core/AVSDirectory.sol                     | 100%     16| 100%     7|    -      0
core/AllocationManager.sol                |90.0%    229|89.2%    37|    -      0
core/DelegationManager.sol                |91.4%    222|87.8%    41|    -      0
core/RewardsCoordinator.sol               |94.3%    123|87.1%    31|    -      0
core/StrategyManager.sol                  |88.1%     67|82.6%    23|    -      0
libraries/BeaconChainProofs.sol           | 100%     22| 100%    11|    -      0
libraries/BytesLib.sol                    | 0.0%    156| 0.0%    14|    -      0
libraries/Endian.sol                      | 100%      2| 100%     1|    -      0
libraries/Merkle.sol                      | 100%     38| 100%     5|    -      0
libraries/OperatorSetLib.sol              | 100%      2| 100%     2|    -      0
libraries/SlashingLib.sol                 | 100%     19| 100%    10|    -      0
libraries/Snapshots.sol                   |76.3%     59|87.5%     8|    -      0
mixins/SignatureUtils.sol                 | 100%      7| 100%     5|    -      0
permissions/Pausable.sol                  |94.7%     19|90.0%    10|    -      0
permissions/PauserRegistry.sol            | 100%     12| 100%     6|    -      0
pods/EigenPod.sol                         | 100%    131|96.2%    26|    -      0
pods/EigenPodManager.sol                  | 100%     68|93.3%    15|    -      0
strategies/EigenStrategy.sol              | 0.0%     10| 0.0%     4|    -      0
strategies/StrategyBase.sol               |90.9%     44|78.9%    19|    -      0
strategies/StrategyBaseTVLLimits.sol      | 100%     12| 100%     5|    -      0
strategies/StrategyFactory.sol            | 100%     32| 100%     8|    -      0
token/BackingEigen.sol                    |83.3%     30|69.2%    13|    -      0
token/Eigen.sol                           |45.0%     40|61.5%    13|    -      0
================================================================================
                                  Total:|80.2%   1360|83.4%   314|    -      0

github-actions · 2024-11-11T21:12:33Z

Reading tracefile ./lcov.info.pruned
                                        |Lines       |Functions  |Branches    
Filename                                  |Rate     Num|Rate    Num|Rate     Num
================================================================================
[src/contracts/]
core/AVSDirectory.sol                     | 100%     16| 100%     7|    -      0
core/AllocationManager.sol                |90.0%    229|89.2%    37|    -      0
core/DelegationManager.sol                |91.4%    222|87.8%    41|    -      0
core/RewardsCoordinator.sol               |94.3%    123|87.1%    31|    -      0
core/StrategyManager.sol                  |88.1%     67|82.6%    23|    -      0
libraries/BeaconChainProofs.sol           | 100%     22| 100%    11|    -      0
libraries/BytesLib.sol                    | 0.0%    156| 0.0%    14|    -      0
libraries/Endian.sol                      | 100%      2| 100%     1|    -      0
libraries/Merkle.sol                      | 100%     38| 100%     5|    -      0
libraries/OperatorSetLib.sol              | 100%      2| 100%     2|    -      0
libraries/SlashingLib.sol                 | 100%     19| 100%    10|    -      0
libraries/Snapshots.sol                   |76.3%     59|87.5%     8|    -      0
mixins/SignatureUtils.sol                 | 100%      7| 100%     5|    -      0
permissions/Pausable.sol                  |94.7%     19|90.0%    10|    -      0
permissions/PauserRegistry.sol            | 100%     12| 100%     6|    -      0
pods/EigenPod.sol                         | 100%    131|96.2%    26|    -      0
pods/EigenPodManager.sol                  | 100%     68|93.3%    15|    -      0
strategies/EigenStrategy.sol              | 0.0%     10| 0.0%     4|    -      0
strategies/StrategyBase.sol               |90.9%     44|78.9%    19|    -      0
strategies/StrategyBaseTVLLimits.sol      | 100%     12| 100%     5|    -      0
strategies/StrategyFactory.sol            | 100%     32| 100%     8|    -      0
token/BackingEigen.sol                    |83.3%     30|69.2%    13|    -      0
token/Eigen.sol                           |45.0%     40|61.5%    13|    -      0
================================================================================
                                  Total:|80.2%   1360|83.4%   314|    -      0

gpsanant · 2024-11-13T18:43:47Z

src/contracts/core/DelegationManager.sol

+                // Staker was delegated and remains slashable during the withdrawal delay period
+                // Cumulative withdrawn scaled shares are updated for the strategy, this is for accounting
+                // purposes for burning shares if slashed
+                _updateCumulativeScaledShares(operator, strategies[i], scaledShares[i]);


we don't want to do this for beaconchain ETH, because this value is sorta useless since it doesn't take slashing factors into account

Good catch, yes this only for SM Strategies

github-actions · 2024-11-16T20:00:35Z

Reading tracefile ./lcov.info.pruned
                                        |Lines       |Functions  |Branches    
Filename                                  |Rate     Num|Rate    Num|Rate     Num
================================================================================
[src/contracts/]
core/AVSDirectory.sol                     | 100%     16| 100%     7|    -      0
core/AllocationManager.sol                |90.0%    229|89.2%    37|    -      0
core/DelegationManager.sol                |91.5%    223|87.8%    41|    -      0
core/RewardsCoordinator.sol               |94.3%    123|87.1%    31|    -      0
core/StrategyManager.sol                  |88.1%     67|82.6%    23|    -      0
libraries/BeaconChainProofs.sol           | 100%     22| 100%    11|    -      0
libraries/BytesLib.sol                    | 0.0%    156| 0.0%    14|    -      0
libraries/Endian.sol                      | 100%      2| 100%     1|    -      0
libraries/Merkle.sol                      | 100%     38| 100%     5|    -      0
libraries/OperatorSetLib.sol              | 100%      2| 100%     2|    -      0
libraries/SlashingLib.sol                 | 100%     19| 100%    10|    -      0
libraries/Snapshots.sol                   |76.3%     59|87.5%     8|    -      0
mixins/SignatureUtils.sol                 | 100%      7| 100%     5|    -      0
permissions/Pausable.sol                  |94.7%     19|90.0%    10|    -      0
permissions/PauserRegistry.sol            | 100%     12| 100%     6|    -      0
pods/EigenPod.sol                         | 100%    131|96.2%    26|    -      0
pods/EigenPodManager.sol                  | 100%     68|93.3%    15|    -      0
strategies/EigenStrategy.sol              | 0.0%     10| 0.0%     4|    -      0
strategies/StrategyBase.sol               |90.9%     44|78.9%    19|    -      0
strategies/StrategyBaseTVLLimits.sol      | 100%     12| 100%     5|    -      0
strategies/StrategyFactory.sol            | 100%     32| 100%     8|    -      0
token/BackingEigen.sol                    |83.3%     30|69.2%    13|    -      0
token/Eigen.sol                           |45.0%     40|61.5%    13|    -      0
================================================================================
                                  Total:|80.2%   1361|83.4%   314|    -      0

wadealexc and others added 4 commits November 6, 2024 18:24

refactor: pull beacon chain slashing out of slashing lib

9005652

feat: initial draft for sync slashing

855381b

fix: existing tests

4f86176

fix: cumulative shares and fmt

63c9ff1

ypatil12 reviewed Nov 8, 2024

View reviewed changes

src/contracts/core/DelegationManagerStorage.sol Outdated Show resolved Hide resolved

fix: missing operator key in mapping

aea6ab4

gpsanant reviewed Nov 8, 2024

View reviewed changes

8sunyuan added the slashing label Nov 8, 2024

8sunyuan added 3 commits November 11, 2024 14:29

refactor: cumulative scaled shares

138eef7

chore: cleanup

91a806b

chore: storage report

ffcbab3

gpsanant reviewed Nov 13, 2024

View reviewed changes

wadealexc force-pushed the alex/dm-refactor branch from 080a510 to 859359e Compare November 14, 2024 15:58

Base automatically changed from alex/dm-refactor to slashing-magnitudes November 14, 2024 16:03

fix: rename and beacon strategy

b0ed123

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: burn erc20s on slashing #881

feat: burn erc20s on slashing #881

8sunyuan commented Nov 8, 2024 •

edited

Loading

gpsanant Nov 8, 2024

8sunyuan Nov 8, 2024

8sunyuan Nov 11, 2024

ypatil12 Nov 14, 2024 •

edited

Loading

ypatil12 Nov 14, 2024

0xClandestine Nov 14, 2024

gpsanant Nov 8, 2024

8sunyuan Nov 8, 2024

gpsanant Nov 13, 2024

gpsanant Nov 8, 2024

8sunyuan Nov 11, 2024

gpsanant Nov 8, 2024

8sunyuan Nov 11, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

gpsanant Nov 13, 2024

8sunyuan Nov 15, 2024

github-actions bot commented Nov 16, 2024

feat: burn erc20s on slashing #881

Are you sure you want to change the base?

feat: burn erc20s on slashing #881

Conversation

8sunyuan commented Nov 8, 2024 • edited Loading

Synchronous Burning

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypatil12 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 16, 2024

8sunyuan commented Nov 8, 2024 •

edited

Loading

ypatil12 Nov 14, 2024 •

edited

Loading