feat: burn erc20s on slashing #881
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
can we do some analysis on if this is ok for LSTs?
There was a problem hiding this comment.
It's for sure not, placeholder value for now. Personally a fan of 0xdeadbeef
There was a problem hiding this comment.
91a806b
0xDeaDBeef
There was a problem hiding this comment.
We should at least do 0x000000000000000000000000000000000000dEaD or address(0) since other protocols use that. Working with Matt Nelson to get feedback
There was a problem hiding this comment.
Looks like we cannot do address(0) given that stETH's contract uses this
There was a problem hiding this comment.
0x E 1 6 E 4 (eigen)
| * 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( |
There was a problem hiding this comment.
just do upperLookup imo. is this any different?
There was a problem hiding this comment.
It's the same logic but named more clearly for blocknumber usage. I can change this back though to be more consistent with DefaultWadHistory.
There was a problem hiding this comment.
changing the name makes it clearer to me imo
| @@ -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) { | |||
There was a problem hiding this comment.
can we expose a public view func for this?
| IStrategy strategy, | ||
| uint256 queueWithdrawnShares | ||
| ) internal { | ||
| uint256 currCumulativeWithdrawalShares = uint256(_cumulativeWithdrawalsHistory[operator][strategy].latest()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
138eef7
Added, had to change calcSlashedAmount back to old interface but it is more consistent with the way scaledShares are also calculated.
|
1 similar comment
|
wadealexc
force-pushed
the
alex/dm-refactor
branch
from
080a510
to
859359e
Compare
|
8sunyuan
force-pushed
the
burn-erc20s
branch
from
b0ed123
to
b3a7da6
Compare
|
|
||
| /// @dev Update the cumulative withdrawn scaled shares from an operator for a given strategy | ||
| function _updateCumulativeScaledShares(address operator, IStrategy strategy, uint256 scaledShares) internal { | ||
| if (strategy != beaconChainETHStrategy) { |
There was a problem hiding this comment.
Why would beacon chain stuff also not be added?
Do we add it to this array eventually when we have burning for native ETH?
There was a problem hiding this comment.
At minimum, in the burnOperatorShares method we wouldn't be able to use these cumulative scaled shares to efficiently burn beaconChain shares in a callback like the one we make to the StrategyManager.
However, the scaled shares does take into account slashing factors and therefore the bcsf. We could potentially store this value as well?
There was a problem hiding this comment.
IMO let's make this call in a separate PR once we have a better idea of the direction we want to take for beacon chain eth
|
* refactor: remove unused return values from _insert * also removes safe cast * refactor: pull unrelated operations out and condense library method usage
Synchronous Burning
One solution for burning slashed shares is to simply take the existing logic, take the sharesToDecrement in
calcSlashedAmountand just burn that amount. But for all shares that were withdrawn and are still slashable forMIN_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_BLOCKSinto 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 functiondecreaseAndBurnOperatorShareswhich will now read all the slashable shares queue withdrawn from an operator for a specific strategy, calculate amount of shares to burn, and callStrategyManager.burnShares.StrategyManager.sol: burnShares function is exact same as the withdraw function but with the destination address being theDEFAULT_BURN_ADDRESS(address 0).SlashingLib.sol: ChangedcalcSlashedAmountback 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.transferEach 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 onslashOperatoras a result.TODO: