From 42688cdbf572435b5bee8660e38fba5d6f76c42a Mon Sep 17 00:00:00 2001 From: wadealexc Date: Tue, 19 Nov 2024 20:55:56 +0000 Subject: [PATCH 1/2] refactor: remove unused return values from _insert * also removes safe cast --- src/contracts/libraries/Snapshots.sol | 78 ++++++++++++--------------- 1 file changed, 33 insertions(+), 45 deletions(-) diff --git a/src/contracts/libraries/Snapshots.sol b/src/contracts/libraries/Snapshots.sol index 199502bba..839aec0d4 100644 --- a/src/contracts/libraries/Snapshots.sol +++ b/src/contracts/libraries/Snapshots.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.0; import "@openzeppelin-upgrades/contracts/utils/math/MathUpgradeable.sol"; -import "@openzeppelin-upgrades/contracts/utils/math/SafeCastUpgradeable.sol"; import "./SlashingLib.sol"; @@ -24,8 +23,6 @@ import "./SlashingLib.sol"; * _Available since v4.5._ */ library Snapshots { - using SafeCastUpgradeable for uint256; - struct DefaultWadHistory { Snapshot[] _snapshots; } @@ -43,22 +40,17 @@ library Snapshots { /** * @dev Pushes a (`key`, `value`) pair into a DefaultWadHistory so that it is stored as the snapshot. - * - * Returns previous value and new value. */ - function push(DefaultWadHistory storage self, uint32 key, uint64 value) internal returns (uint64, uint64) { - (uint224 prevValue, uint224 currValue) = _insert(self._snapshots, key, value); - return (uint64(prevValue), uint64(currValue)); + function push(DefaultWadHistory storage self, uint32 key, uint64 value) internal { + _insert(self._snapshots, key, value); } /** * @dev Pushes a (`key`, `value`) pair into a DefaultZeroHistory so that it is stored as the snapshot. - * Peforms a safecast to uint224 for the value. - * - * Returns previous value and new value. + * `value` is cast to uint224. Responsibility for the safety of this operation falls outside of this library. */ - function push(DefaultZeroHistory storage self, uint32 key, uint256 value) internal returns (uint256, uint256) { - return _insert(self._snapshots, key, value.toUint224()); + function push(DefaultZeroHistory storage self, uint32 key, uint256 value) internal { + _insert(self._snapshots, key, uint224(value)); } /** @@ -77,19 +69,6 @@ library Snapshots { return _upperLookup(self._snapshots, key, 0); } - /** - * @dev Returns the value in the last (most recent) snapshot with key lower or equal than the search key, or `defaultValue` if there is none. - */ - function _upperLookup( - Snapshot[] storage snapshots, - uint32 key, - uint224 defaultValue - ) private view returns (uint224) { - uint256 len = snapshots.length; - uint256 pos = _upperBinaryLookup(snapshots, key, 0, len); - return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value; - } - /** * @dev Returns the value in the most recent snapshot, or WAD if there are no snapshots. */ @@ -108,14 +87,6 @@ library Snapshots { return uint256(_latest(self._snapshots, 0)); } - /** - * @dev Returns the value in the most recent snapshot, or `defaultValue` if there are no snapshots. - */ - function _latest(Snapshot[] storage snapshots, uint224 defaultValue) private view returns (uint224) { - uint256 pos = snapshots.length; - return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value; - } - /** * @dev Returns the number of snapshots. */ @@ -138,27 +109,44 @@ library Snapshots { * @dev Pushes a (`key`, `value`) pair into an ordered list of snapshots, either by inserting a new snapshot, * or by updating the last one. */ - function _insert(Snapshot[] storage self, uint32 key, uint224 value) private returns (uint224, uint224) { + function _insert(Snapshot[] storage self, uint32 key, uint224 value) private { uint256 pos = self.length; if (pos > 0) { - // Copying to memory is important here. + // Validate that inserted keys are always >= the previous key Snapshot memory last = _unsafeAccess(self, pos - 1); - - // Snapshot keys must be non-decreasing. require(last._key <= key, InvalidSnapshotOrdering()); - // Update or push new snapshot + // Update existing snapshot if `key` matches if (last._key == key) { _unsafeAccess(self, pos - 1)._value = value; - } else { - self.push(Snapshot({_key: key, _value: value})); + return; } - return (last._value, value); - } else { - self.push(Snapshot({_key: key, _value: value})); - return (0, value); } + + // `key` was not in the list; push as a new entry + self.push(Snapshot({_key: key, _value: value})); + } + + /** + * @dev Returns the value in the last (most recent) snapshot with key lower or equal than the search key, or `defaultValue` if there is none. + */ + function _upperLookup( + Snapshot[] storage snapshots, + uint32 key, + uint224 defaultValue + ) private view returns (uint224) { + uint256 len = snapshots.length; + uint256 pos = _upperBinaryLookup(snapshots, key, 0, len); + return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value; + } + + /** + * @dev Returns the value in the most recent snapshot, or `defaultValue` if there are no snapshots. + */ + function _latest(Snapshot[] storage snapshots, uint224 defaultValue) private view returns (uint224) { + uint256 pos = snapshots.length; + return pos == 0 ? defaultValue : _unsafeAccess(snapshots, pos - 1)._value; } /** From 3f13f072ae06b0a39d6c7bc66870b2ea42bb954d Mon Sep 17 00:00:00 2001 From: wadealexc Date: Tue, 19 Nov 2024 21:53:34 +0000 Subject: [PATCH 2/2] refactor: pull unrelated operations out and condense library method usage --- .../complete_withdrawal_from_strategy.s.sol | 2 +- src/contracts/core/DelegationManager.sol | 37 +++++++++++++------ src/contracts/libraries/SlashingLib.sol | 29 ++++++--------- src/test/mocks/DelegationManagerMock.sol | 4 +- src/test/unit/DelegationUnit.t.sol | 2 +- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/script/tasks/complete_withdrawal_from_strategy.s.sol b/script/tasks/complete_withdrawal_from_strategy.s.sol index c1f15a59b..281b233f7 100644 --- a/script/tasks/complete_withdrawal_from_strategy.s.sol +++ b/script/tasks/complete_withdrawal_from_strategy.s.sol @@ -67,7 +67,7 @@ contract CompleteWithdrawFromStrategy is Script, Test { // Get scaled shares for the given amount uint256[] memory scaledShares = new uint256[](1); - scaledShares[0] = SlashingLib.scaleSharesForQueuedWithdrawal({ + scaledShares[0] = SlashingLib.scaleForQueueWithdrawal({ sharesToWithdraw: sharesToWithdraw, slashingFactor: slashingFactor }); diff --git a/src/contracts/core/DelegationManager.sol b/src/contracts/core/DelegationManager.sol index 7e4f32f14..a02961179 100644 --- a/src/contracts/core/DelegationManager.sol +++ b/src/contracts/core/DelegationManager.sol @@ -349,13 +349,19 @@ contract DelegationManager is require(newMaxMagnitude < prevMaxMagnitude, MaxMagnitudeCantIncrease()); /// forgefmt: disable-next-item - (uint256 sharesToDecrement, uint256 sharesToBurn) = SlashingLib.calcSlashedAmount({ + uint256 sharesToDecrement = SlashingLib.calcSlashedAmount({ operatorShares: operatorShares[operator][strategy], - slashableSharesInQueue: _getSlashedSharesInQueue(operator, strategy, prevMaxMagnitude, newMaxMagnitude), prevMaxMagnitude: prevMaxMagnitude, newMaxMagnitude: newMaxMagnitude }); + // While `sharesToDecrement` describes the amount we should directly remove from the operator's delegated + // shares, `sharesToBurn` also includes any shares that have been queued for withdrawal and are still + // slashable given the withdrawal delay. + uint256 sharesToBurn = + sharesToDecrement + _getSlashedSharesInQueue(operator, strategy, prevMaxMagnitude, newMaxMagnitude); + + // Remove shares from operator _decreaseDelegation({ operator: operator, staker: address(0), // we treat this as a decrease for the zero address staker @@ -493,7 +499,7 @@ contract DelegationManager is IShareManager shareManager = _getShareManager(withdrawal.strategies[i]); // Calculate how much slashing to apply, as well as shares to withdraw - uint256 sharesToWithdraw = SlashingLib.scaleSharesForCompleteWithdrawal({ + uint256 sharesToWithdraw = SlashingLib.scaleForCompleteWithdrawal({ scaledShares: withdrawal.scaledShares[i], slashingFactor: prevSlashingFactors[i] }); @@ -641,7 +647,7 @@ contract DelegationManager is sharesToWithdraw[i] = dsf.calcWithdrawable(depositSharesToWithdraw[i], slashingFactors[i]); // Apply slashing. If the staker or operator has been fully slashed, this will return 0 - scaledShares[i] = SlashingLib.scaleSharesForQueuedWithdrawal({ + scaledShares[i] = SlashingLib.scaleForQueueWithdrawal({ sharesToWithdraw: sharesToWithdraw[i], slashingFactor: slashingFactors[i] }); @@ -763,16 +769,23 @@ contract DelegationManager is uint64 prevMaxMagnitude, uint64 newMaxMagnitude ) internal view returns (uint256) { - uint256 currCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest(); - // Note: this will simply return 0 if no history exists, same for latest() as well - uint256 pastCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({ + // Fetch the cumulative scaled shares sitting in the withdrawal queue both now and before + // the withdrawal delay. + uint256 curCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].latest(); + uint256 prevCumulativeScaledShares = _cumulativeScaledSharesHistory[operator][strategy].upperLookup({ key: uint32(block.number) - MIN_WITHDRAWAL_DELAY_BLOCKS }); - uint256 slashableShareInQueue = SlashingLib.scaleSharesForBurning( - pastCumulativeScaledShares, currCumulativeScaledShares, prevMaxMagnitude, newMaxMagnitude - ); - return slashableShareInQueue; + // The difference between these values represents the number of scaled shares that entered the + // withdrawal queue less than `MIN_WITHDRAWAL_DELAY_BLOCKS` ago. These shares are still slashable, + // so we use them to calculate the number of slashable shares in the withdrawal queue. + uint256 slashableScaledShares = curCumulativeScaledShares - prevCumulativeScaledShares; + + return SlashingLib.scaleForBurning({ + scaledShares: slashableScaledShares, + prevMaxMagnitude: prevMaxMagnitude, + newMaxMagnitude: newMaxMagnitude + }); } /// @dev Add to the cumulative withdrawn scaled shares from an operator for a given strategy @@ -952,7 +965,7 @@ contract DelegationManager is uint256[] memory slashingFactors = _getSlashingFactors(staker, operator, withdrawals[i].strategies); for (uint256 j; j < withdrawals[i].strategies.length; ++j) { - shares[i][j] = SlashingLib.scaleSharesForCompleteWithdrawal({ + shares[i][j] = SlashingLib.scaleForCompleteWithdrawal({ scaledShares: withdrawals[i].scaledShares[j], slashingFactor: slashingFactors[i] }); diff --git a/src/contracts/libraries/SlashingLib.sol b/src/contracts/libraries/SlashingLib.sol index 9afd2de93..dfe715f7a 100644 --- a/src/contracts/libraries/SlashingLib.sol +++ b/src/contracts/libraries/SlashingLib.sol @@ -62,7 +62,7 @@ library SlashingLib { return dsf._scalingFactor == 0 ? WAD : dsf._scalingFactor; } - function scaleSharesForQueuedWithdrawal( + function scaleForQueueWithdrawal( uint256 sharesToWithdraw, uint256 slashingFactor ) internal pure returns (uint256) { @@ -73,25 +73,22 @@ library SlashingLib { return sharesToWithdraw.divWad(slashingFactor); } - function scaleSharesForCompleteWithdrawal( - uint256 scaledShares, - uint256 slashingFactor - ) internal pure returns (uint256) { + function scaleForCompleteWithdrawal(uint256 scaledShares, uint256 slashingFactor) internal pure returns (uint256) { return scaledShares.mulWad(slashingFactor); } /** - * @notice Scales the share difference between two cumulative scaled shares. This is used - * to read the total slashable/burnable shares that are queued for withdrawal from an operator. - * This is because shares are still slashable for the duration they are sitting in the withdrawal queue. + * @notice Scales shares according to the difference in an operator's magnitude before and + * after being slashed. This is used to calculate the number of slashable shares in the + * withdrawal queue. + * NOTE: max magnitude is guaranteed to only ever decrease. */ - function scaleSharesForBurning( - uint256 pastCumulativeScaledShares, - uint256 currCumulativeScaledShares, + function scaleForBurning( + uint256 scaledShares, uint64 prevMaxMagnitude, uint64 newMaxMagnitude ) internal pure returns (uint256) { - return (currCumulativeScaledShares - pastCumulativeScaledShares).mulWad(prevMaxMagnitude - newMaxMagnitude); + return scaledShares.mulWad(prevMaxMagnitude - newMaxMagnitude); } function update( @@ -157,12 +154,10 @@ library SlashingLib { function calcSlashedAmount( uint256 operatorShares, - uint256 slashableSharesInQueue, uint256 prevMaxMagnitude, uint256 newMaxMagnitude - ) internal pure returns (uint256 sharesToDecrement, uint256 sharesToBurn) { - // round up mulDiv so we don't round up sharesToDecrement and overslash - sharesToDecrement = operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up); - sharesToBurn = sharesToDecrement + slashableSharesInQueue; + ) internal pure returns (uint256) { + // round up mulDiv so we don't overslash + return operatorShares - operatorShares.mulDiv(newMaxMagnitude, prevMaxMagnitude, Math.Rounding.Up); } } diff --git a/src/test/mocks/DelegationManagerMock.sol b/src/test/mocks/DelegationManagerMock.sol index 53f6d1611..771c4e502 100644 --- a/src/test/mocks/DelegationManagerMock.sol +++ b/src/test/mocks/DelegationManagerMock.sol @@ -28,13 +28,11 @@ contract DelegationManagerMock is Test { function burnOperatorShares( address operator, IStrategy strategy, - uint256 slashableSharesInQueue, uint64 prevMaxMagnitude, uint64 newMaxMagnitude ) external { - (uint256 amountSlashed, /*uint256 amountBurned*/) = SlashingLib.calcSlashedAmount({ + uint256 amountSlashed = SlashingLib.calcSlashedAmount({ operatorShares: operatorShares[operator][strategy], - slashableSharesInQueue: slashableSharesInQueue, prevMaxMagnitude: prevMaxMagnitude, newMaxMagnitude: newMaxMagnitude }); diff --git a/src/test/unit/DelegationUnit.t.sol b/src/test/unit/DelegationUnit.t.sol index 5c1832350..6051d1d23 100644 --- a/src/test/unit/DelegationUnit.t.sol +++ b/src/test/unit/DelegationUnit.t.sol @@ -381,7 +381,7 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag DepositScalingFactor memory _dsf = DepositScalingFactor(delegationManager.depositScalingFactor(staker, strategy)); uint256 sharesToWithdraw = _dsf.calcWithdrawable(depositSharesToWithdraw, slashingFactor); - uint256 scaledShares = SlashingLib.scaleSharesForQueuedWithdrawal({ + uint256 scaledShares = SlashingLib.scaleForQueueWithdrawal({ sharesToWithdraw: sharesToWithdraw, slashingFactor: slashingFactor });