Skip to content
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.

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

feat: slashing release #679

Open
wants to merge 29 commits into
base: custom-errors
Choose a base branch
from
Open

feat: slashing release #679

wants to merge 29 commits into from

Conversation

8sunyuan
Copy link
Collaborator

@8sunyuan 8sunyuan commented Aug 12, 2024

Updated 11/08/24

Contract Descriptions

DONT REVIEW THE REWARDS COORDINATOR

AVSDirectory

  • Currently on mainnet, all operator<>AVS registration takes place on the AVSDirectory contract which AVS contracts call into along with an operator signature to confirm operator registration in the EigenLayer protocol. This registration/deregistration flow will change in the Slashing release with the introduction of operatorSets. More on this below...
  • The AVSDirectory contract will be unchanged and allow for backwards compatibility. It will be deprecated (all data indexing ignored) at some future date when AVSs have migrated their systems over to the newly defined contract, the AllocationManager.

New Core contract: AllocationManager! (basically the Slasher contract)

  • Operator<>AVS registration/deregistration should be migrated here for existing AVSs if they wish to utilize Slashing functionality within EigenLayer. In addition to a different address entrypoint(previously AVSDirectory), the flow has actually changed to an operator registering themselves with the AllocationManager, and the AllocationManager will then call the corresponding callback setup by the AVS themselves. This is contrary to what exists today on the AVSDirectory which expects to be called by an AVS with an operator signature providing "registration consent".
  • AVSs now have the ability to define subsets of operators called operatorSets. This allows for more granularity around defining slashing and rewards conditions for their different operators and their tasks. An operator can be exclusively in one operatorSet or many all at once. It is up to the AVS to determine their own registration/deregistration conditions and the Strategies that are in each of their operatorSets.
  • The AllocationManager is the Slasher contract, pending decision on rename or just changing back to Slasher.
  • This implements "unique security", stake is not reused across AVSs/operatorSets and operators allocate slashable proportions to each operatorSet. Imagine this as a pie chart where each slice gets given to a different operatorSet.
  • modifyAllocations can be called by an operator to configure for a given operatorSet, the slashable proportions for each Strategies in the operatorSet. Ex. If an AVS operatorSet has stETH and rETH Strategies in their operatorSet, as an operator I can allocate 50% of my delegated stETH shares and 25% of my delegated rETH shares to be slashable by this operatorSet. An allocation can be thought of as the slashable proportion defined over the tuple (operator, operatorSet, Strategy).
  • slashOperator, called by an AVS, slashes an operator for the specific operatorSet they are in as well as the wadsToSlash(percentage slashed). For all Strategies that are in the operatorSet, the operator will be slashed a percentage according to how much allocations they have for each of those strategies. This function slashes from current magnitude as well as queued deallocations. Whatever magnitude is slashed is also decremented from the maxMagnitude from the (operator, Strategy) tuple.
  • Deallocations are slashable while pending. Pending allocations on the other hand are not slashable (referring to the increased amount) because allocations are increases that come existing proportional amounts that are not slashable. This is kept track of by the encumberedMagnitude value, increasing on any allocations, which can go up to a maximum of your maxMagnitude(by default maxMagnitude is 1e18, but decreases every time slashed).
  • Deallocation delays are fixed across all operators, providing a minimum security guarantee for AVSs about how much stake they have securing their systems.
  • Allocation delays are configurable on a per operator basis. Updating the allocationDelay has a delay for it to take effect itself. Open PR for this is here.

StrategyManager/EigenPodManager/DelegationManager - Changes to Deposits/Withdrawals

Shares are changed in a lot of ways with the introduction of Slashing with a lot more complex accounting, most of which can be found in SlashingLib.sol and the deposit/withdrawal flows. Below is an explanation on the types of 'shares' in the system.

  1. depositShares: These are the shares representing the amount of Strategy shares a staker has added to the system, either through deposits in the StrategyManager or positive shares increases in the EigenPodManager. Note that these can be compared 1:1 with the shares of the underlying Strategies. If you are delegated and deposit or you delegate with existing shares, then your operator shares will increase by the exact amount of these depositShares.
    The amount you can withdraw is dependent however on withdrawableShares, see more below...
    Location: StrategyManager - stakerDepositShares mapping
    EigenPodManager - podOwnerDepositShares mapping

  2. operatorShares: This is the delegated stake an operator has from all their delegated stakers; this can also be termed 'delegated' shares. An operator's operatorShares is also the summation of all their delegated stakers withdrawable shares. This is because operatorShares increase on each delegated staker deposit and decreases from withdrawals and slashed shares. The amount decreased from withdrawals are also scaled down depending on if any slashing has applied to the staker and their depositShares.
    Location: DelegationManager - operatorShares mapping

  3. withdrawableShares: This is the amount of withdrawable shares a staker can queue withdraw. Now if a staker is not delegated, then the following is true withdrawableShares == depositShares because they cannot be slashed. However if they are delegated to an operator and their operator got slashed while they had depositShares delegated to them, then their withdrawableShares are less than their depositShares. This value is not in storage but read by taking the staker's depositShares and scaling it down depending on if any slashing has affected their stake.
    Location: DelegationManager - getWithdrawableShares() function
    3a. scaledShares: You can see this in struct Withdrawal when queuing a withdrawal. We calculate withdrawableShares by some clever scaling factors and accounting but one thing we want to ensure is that withdrawals are still slashable while in the queue. This is done by dividing withdrawableShares at queue time by the delegated operator's maxMagnitude, and upon completion multiplying it by the maxMagnitude at the earliest withdrawal completion time. This will account for the proportional amount of shares slashed and decrement from withdrawn shares accordingly.
    Since we read the maxMagnitude while queuing the withdrawal, this design is more for convenience and optimizing reads.
    Note: If the strategy is the beaconChainStrategy this is also accounted for in a similar way.
    Location: IDelegationManager - struct Withdrawal

Additional Notes:

  • DM: EIGEN strategy delay is currently set to 7 days so all the strategies currently have the same delay. We remove strategy specific delays entirely here but all withdrawals will be configured to be the same as the deallocation delay.
  • DM: We've abstracted the EPM and SM behind a unified IShareManager interface
  • Accounting NOTEs:
    • depositScalingFactor is the k value used in the accounting docs
    • stakerDepositShares is s
    • operatorShares is op
    • maxMagnitude is m read from the AllocationManager
  • Using OZ CheckpointsUpgradeable library in the AllocationManager to keep track of historically timestamped magnitude values. This is required for staker withdrawal completion where the timestamped maxMagnitude value at completion may not be the current so we have a lookup for that. We renamed the Checkpoints library to Snapshots to avoid confusion with EigenPod checkpoints.
  • 0.8.27 custom errors with requires (saves a lot of bytecode size)
  • thirdPartyTransfersForbidden is removed entirely. This mapping will be deprecated and never read from again.
  • We will likely deploy with optimizer-runs = 0-10 to save on contract size
  • DONT LOOK AT SCRIPTS OR TESTS YET

TODOs:

  • burning of slashed shares
    • StrategyManager shares: Draft in review
    • EigenPodManager shares: requires scoping
  • BytecodeSize: Likely will compile contracts using --via-ir saving on bytecode size
  • EigenPodManager: Pectra related changes to EigenPod proofs as well as EIP7002 and burning of shares. Scope for first version of Slashing TBD

Deposits/Withdraws of Shares [WIP]

  • Confirm no overflow scenarios as part of scaling factor calculations in deposits/withdraws. Note that permissionless staking limits the number of shares in Strategies
  • Events

Allocator Configuration [WIP]

  • Figure out a tolerable rounding scheme for slashing magnitude allocations
  • Events

This comment was marked as spam.

@Layr-Labs Layr-Labs deleted a comment from github-actions bot Aug 19, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Aug 19, 2024
src/contracts/core/AVSDirectory.sol Fixed Show fixed Hide fixed
src/contracts/core/AVSDirectory.sol Fixed Show fixed Hide fixed
src/contracts/core/AVSDirectory.sol Fixed Show fixed Hide fixed
Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Review on alloc/dealloc

src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
@MinisculeTarantula
Copy link
Collaborator

require allocationDelay being set when calling modifyAllocations. Based on current implementation of a default allocation delay, if a shorter delay is set, it is possible to create a pending magnitudeUpdate that has a timestamp thats earlier than a already existing pending update. We need the checkpoints history to be asc sorted and figuring out some insertion method instead of pushing the history seems like too much overhead complexity.

Would it be bad / hard / undesirable to enforce this ordering on the checkpoints history level?

I would suggest that if an attempt is made to push an entry with a timestamp that is earlier than the timestamp of the previous entry, either (a) it's simply not allowed or (b) the new entry has its timestamp modified to match (or be 1 higher? not sure if the ascending ordering you have is strict or non-strict).

Perhaps some option (c) could work where you keep the original timestamp but overwrite the intentions of the other entry? IDK if that would be incompatible with other aspects of the storage model though.

I was initially thinking option (b) was the most logical but modifying entries before pushing them can be messy, especially if the entry or its contents is used elsewhere (e.g. if the timestamp is emitted in events, its hard to make sure the event emits the correct timestamp when you have conditional logic for modifying it).

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Minor slashing comments

src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
@ypatil12 ypatil12 mentioned this pull request Aug 29, 2024
@ypatil12 ypatil12 changed the base branch from feat/operator-set-release to dev August 29, 2024 20:24
* feat: add getMinimumSlashableStake

* fix: only account for deallocation when calculating minimum slashable
* fix: correctly update dsf when increasing delegation

* fix: fix bug where we modify an array while iterating over it

* chore: address review nits

* refactor: minor refactors for codesize

* refactor(nit): function ordering

* fix: only check input lengths when receiving tokens

* refactor: remove callstack indirection when completing a withdrawal

* chore: update comment
/// In this window, deallocations still remain slashable by the operatorSet they were allocated to.
uint32 public immutable DEALLOCATION_DELAY;

/// @notice Delay before alloaction delay modifications take effect.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: Delay before alloaction delay modifications take effect. => Delay before allocation modifications take effect.

also there's some 'edge case' behavior here, right? I recall this not being enforced on deallocations if the Operator isn't slashable by the AVS? can we be any more specific in this definition to reflect this?

* test(wip): todos

* chore: remove lcov

* test(wip): remaining alm todos

* test: final todos

* test(wip): todos

* chore: remove lcov
Comment on lines +103 to +109
emit AllocationUpdated(
params.operator,
operatorSet,
strategy,
_addInt128(allocation.currentMagnitude, allocation.pendingDiff),
allocation.effectBlock
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found this confusing because _updateAllocationInfo also emits the same type of event, but I see that this is specifically for the now-modified pending deallocation.
I think there may be semantic inconsistency with using this event in this way in addition to the other usage -- you may want to consider a separate event for this case, or at least clarify that events of both semantics will be processed smoothly?

Comment on lines +88 to +89
uint64 slashedMagnitude = uint64(uint256(allocation.currentMagnitude).mulWadRoundUp(params.wadToSlash));
uint256 sharesWadSlashed = uint256(slashedMagnitude).divWad(info.maxMagnitude);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should sharesWadSlashed also be rounded up? or is this not rounded up to ensure that delegated shares always >= sum of staker shares ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sharesWadSlashed is not as important in terms of rounding since it's just passed into the event. The DM.burnOperatorShares call has been changed back to use prevMaxMagnitude and newMaxMagnitude. That being said, yes we do round down the amount of shares to decrement from operatorShares to ensure that property you outlined. Here's the excerpt in the SlashingLib

function calcSlashedAmount(
        uint256 operatorShares,
        uint256 prevMaxMagnitude,
        uint256 newMaxMagnitude
    ) internal pure returns (uint256) {
        // round up mulDiv so we don't overslash
        return operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up);
    }
    

Copy link
Contributor

Choose a reason for hiding this comment

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

For the mocks and harnesses, can we cover all the contracts here for both

I think the RewardsCoordinator is missing a mock and we're missing a bunch of harnesses. This will help with some of the parameter changes downstream

Allocation memory allocation = allocations[operator][operatorSetKey][strategy];

// If we've reached a pending deallocation that isn't completable yet,
// we can stop. Any subsequent modificaitons will also be uncompletable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: modificaitons => modifications

Comment on lines +31 to +33
/// @notice Delay before deallocations are clearable and can be added back into freeMagnitude
/// In this window, deallocations still remain slashable by the operatorSet they were allocated to.
uint32 public immutable DEALLOCATION_DELAY;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably worth noting that DEALLOCATION_DELAY and ALLOCATION_CONFIGURATION_DELAY are in terms of blocks, specifically

// Given the max magnitudes of the operator the staker is now delegated to, calculate the current
// slashing factors to apply to each withdrawal if it is received as shares.
address newOperator = delegatedTo[withdrawal.staker];
uint256[] memory newSlashingFactors = _getSlashingFactors(withdrawal.staker, newOperator, withdrawal.strategies);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bit of an optimization nit, but I think this can be moved inside the if...else block where receiveAsToken = false

Comment on lines +520 to +521
delete queuedWithdrawals[withdrawalRoot];
delete pendingWithdrawals[withdrawalRoot];
Copy link
Collaborator

Choose a reason for hiding this comment

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

possible bug: moving this down in the function breaks the CEI pattern, maybe opening the path to reentrancy via a malicious token. Possible attack would look like (1) initiate withdrawal containing multiple tokens (2) once callstack makes it to malicious token, initiate the same withdrawal a second time via reentrant call (3) get 2x tokens

I highly recommend moving these deletions as far up in this function as possible, particularly before any calls external to the protocol may occur.

It's super possible there's no actual threat here, but this reordering should be purely safer either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have nonReentrant on all completeQueuedWithdrawal interfaces so this shouldn't be an issue but I do agree on following CEI more closely.

8sunyuan and others added 7 commits November 19, 2024 21:14
* refactor: pull beacon chain slashing out of slashing lib

* feat: initial draft for sync slashing

* fix: existing tests

* fix: cumulative shares and fmt

* fix: missing operator key in mapping

* refactor: cumulative scaled shares

* chore: cleanup

* chore: storage report

* fix: rename and beacon strategy

* fix: rebase

* fix: rounding

* test: happy path test cases for burn erc20s

* fix: address comments

* test: timing regression test and comments

* fix: slashable shares in queue amount

* refactor: burn refactor (#897)

* refactor: remove unused return values from _insert
* also removes safe cast
* refactor: pull unrelated operations out and condense library method usage

* test: additional unit test with multiple withdrawals

---------

Co-authored-by: wadealexc <[email protected]>
Co-authored-by: Alex <[email protected]>
* feat: initial draft for sync slashing

* refactor: cumulative scaled shares

* feat: initial draft for sync slashing

* fix: cumulative shares and fmt

* chore: cleanup src/test

* fix: delegation tests

* test: rebased and refactored tests

fix: rebase tests

test: delegation unit refactoring

fix: rounding tests

fix: continue fixing delegation tests

* test: include fuzz underflow tests

* fix: tests and rebase

* chore: comment nit

* fix: failing ci
* fix: remove env required

* fix: use envOr

* fix: remove env from CI for being required
* feat: local deploy

* fix: transfer ownership

* fix: comment
* feat: add `AVS` user

* test(wip): slashing integration

* test(wip): slashing integration

* test(wip): slashing integration

* test(wip): slashing integration

* fix: make tracing useful

* test(wip): slashing integration

* fix: toStringWad

* fix: eigenpods

* test(wip): slashing integration

* refactor: revert change

* test(review): changes

* fix: compile

* test(review): changes

* refactor: improve logging

* refactor: review changes

* fix: roll in `modifyAllocations`

* fix: roll in `modifyAllocations`

* refactor: review changes

* refactor: add back pause constants

---------

Co-authored-by: Yash Patil <[email protected]>
* refactor: eigenpod and beacon chain slashing
* checkpoints are not deleted on completion, saving gas when creating a new checkpoint

* refactor: pull bcsf out of delegationManager

* chore: formatting

* refactor: rename withdrawableRestakedExecutionLayerGwei
* maintains old interface, only state variable is renamed
* this is to reduce line length when using this variable

* refactor: remove branching and slashing math from eigenpod
* fix: clean up balance update conditions and ensure shares==0 is handled

* refactor: remove input validation and explicitly document assumptions

* fix: tests and roundup (#901)

* chore: address feedback

* chore: address feedback again

* chore: cleanup EPM withdrawSharesAsTokens

---------

Co-authored-by: Michael Sun <[email protected]>
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
@Layr-Labs Layr-Labs deleted a comment from github-actions bot Nov 22, 2024
Copy link

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                |98.0%    246| 100%    39|    -      0
core/DelegationManager.sol                |94.6%    224|92.7%    41|    -      0
core/RewardsCoordinator.sol               |94.3%    123|87.1%    31|    -      0
core/StrategyManager.sol                  |98.5%     68|91.3%    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%    11|    -      0
libraries/Snapshots.sol                   |93.1%     29|88.9%     9|    -      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%    127|96.2%    26|    -      0
pods/EigenPodManager.sol                  | 100%     81|94.1%    17|    -      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:|83.4%   1359|86.2%   320|    -      0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants