feat: slashing audit fixes (#1046)

docs/core/AllocationManager.md

@@ -22,7 +22,9 @@ Libraries and Mixins:
 
 ## Overview
 
-The `AllocationManager` manages registration and deregistration of operators to operator sets, handles allocation and slashing of operators' slashable stake, and is the entry point an AVS uses to slash an operator. The `AllocationManager's` responsibilities are broken down into the following concepts:
+The `AllocationManager` manages AVS metadata registration, registration and deregistration of operators to operator sets, handles allocation and slashing of operators' slashable stake, and is the entry point an AVS uses to slash an operator. The `AllocationManager's` responsibilities are broken down into the following concepts:
+
+* [AVS Metadata](#avs-metadata)
 * [Operator Sets](#operator-sets)
 * [Allocations and Slashing](#allocations-and-slashing)
 * [Config](#config)
@@ -38,6 +40,104 @@ The `AllocationManager` manages registration and deregistration of operators to
 
 ---
 
+## AVS Metadata
+
+AVSs must register their metadata to declare themselves who they are as the first step, before they can create operator sets or register operators into operator sets. `AllocationManager` keeps track of AVSs that have registered metadata.
+
+**Methods:**
+* [`updateAVSMetadataURI`](#updateavsmetadatauri)
+
+
+#### `updateAVSMetadataURI`
+
+```solidity
+/**
+ *  @notice Called by an AVS to emit an `AVSMetadataURIUpdated` event indicating the information has updated.
+ *
+ *  @param metadataURI The URI for metadata associated with an AVS.
+ *
+ *  @dev Note that the `metadataURI` is *never stored* and is only emitted in the `AVSMetadataURIUpdated` event.
+ */
+function updateAVSMetadataURI(
+    address avs, 
+    string calldata metadataURI
+) 
+    external
+    checkCanCall(avs)
+```
+
+_Note: this method can be called directly by an AVS, or by a caller authorized by the AVS. See [`PermissionController.md`](../permissions/PermissionController.md) for details._
+
+Below is the format AVSs should use when updating their metadata URI initially. This is not validated onchain.
+
+```json
+{
+    "name": "AVS",
+    "website": "https.avs.xyz/",
+    "description": "Some description about",
+    "logo": "http://github.com/logo.png",
+    "twitter": "https://twitter.com/avs",
+}
+```
+
+
+Later on, once AVSs have created operator sets, content in their metadata URI can be updated subsequently.
+
+```json
+{
+    "name": "AVS",
+    "website": "https.avs.xyz/",
+    "description": "Some description about",
+    "logo": "http://github.com/logo.png",
+    "twitter": "https://twitter.com/avs",
+    "operatorSets": [
+        {
+            "name": "ETH Set",
+            "id": "1", // Note: we use this param to match the opSet id in the Allocation Manager
+            "description": "The ETH operatorSet for AVS",
+            "software": [
+                {
+                    "name": "NetworkMonitor",
+                    "description": "",
+                    "url": "https://link-to-binary-or-github.com"
+                },
+                {
+                    "name": "ValidatorClient",
+                    "description": "",
+                    "url": "https://link-to-binary-or-github.com"
+                }
+            ],
+            "slashingConditions": ["Condition A", "Condition B"]
+        },
+        {
+            "name": "EIGEN Set",
+            "id": "2", // Note: we use this param to match the opSet id in the Allocation Manager
+            "description": "The EIGEN operatorSet for AVS",
+            "software": [
+                {
+                    "name": "NetworkMonitor",
+                    "description": "",
+                    "url": "https://link-to-binary-or-github.com"
+                },
+                {
+                    "name": "ValidatorClient",
+                    "description": "",
+                    "url": "https://link-to-binary-or-github.com"
+                }
+            ],
+            "slashingConditions": ["Condition A", "Condition B"]
+        }
+    ]
+}
+```
+
+*Effects*:
+* Emits an `AVSMetadataURIUpdated` event for use in offchain services
+
+*Requirements*:
+* Caller MUST be authorized, either as the AVS itself or an admin/appointee (see [`PermissionController.md`](../permissions/PermissionController.md))
+
+
 ## Operator Sets
 
 Operator sets, as described in [ELIP-002](https://github.com/eigenfoundation/ELIPs/blob/main/ELIPs/ELIP-002.md#operator-sets), are useful for AVSs to configure operator groupings which can be assigned different tasks, rewarded based on their strategy allocations, and slashed according to different rules. Operator sets are defined in [`libraries/OperatorSetLib.sol`](../../src/contracts/libraries/OperatorSetLib.sol):
@@ -145,6 +245,7 @@ Optionally, the `avs` can provide a list of `strategies`, specifying which strat
 
 *Requirements*:
 * Caller MUST be authorized, either as the AVS itself or an admin/appointee (see [`PermissionController.md`](../permissions/PermissionController.md))
+* AVS MUST have registered metadata
 * For each `CreateSetParams` element:
     * Each `params.operatorSetId` MUST NOT already exist in `_operatorSets[avs]`
 
@@ -585,7 +686,26 @@ struct SlashingParams {
 }
 
 /**
- * @notice Called by an AVS to slash an operator in a given operator set
+ * @notice Called by an AVS to slash an operator in a given operator set. The operator must be registered
+ * and have slashable stake allocated to the operator set.
+ *
+ * @param avs The AVS address initiating the slash.
+ * @param params The slashing parameters, containing:
+ *  - operator: The operator to slash.
+ *  - operatorSetId: The ID of the operator set the operator is being slashed from.
+ *  - strategies: Array of strategies to slash allocations from (must be in ascending order).
+ *  - wadsToSlash: Array of proportions to slash from each strategy (must be between 0 and 1e18).
+ *  - description: Description of why the operator was slashed.
+ *
+ * @dev For each strategy:
+ *      1. Reduces the operator's current allocation magnitude by wadToSlash proportion.
+ *      2. Reduces the strategy's max and encumbered magnitudes proportionally.
+ *      3. If there is a pending deallocation, reduces it proportionally.
+ *      4. Updates the operator's shares in the DelegationManager.
+ *
+ * @dev Small slashing amounts may not result in actual token burns due to
+ *      rounding, which will result in small amounts of tokens locked in the contract
+ *      rather than fully burning through the burn mechanism.
  */
 function slashOperator(
     address avs,
@@ -646,7 +766,6 @@ Once slashing is processed for a strategy, [slashed stake is burned via the `Del
 **Methods:**
 * [`setAllocationDelay`](#setallocationdelay)
 * [`setAVSRegistrar`](#setavsregistrar)
-* [`updateAVSMetadataURI`](#updateavsmetadatauri)
 
 #### `setAllocationDelay`
 
@@ -737,79 +856,3 @@ Note that when an operator registers, registration will FAIL if the call to `IAV
 
 *Requirements*:
 * Caller MUST be authorized, either as the AVS itself or an admin/appointee (see [`PermissionController.md`](../permissions/PermissionController.md))
-
-#### `updateAVSMetadataURI`
-
-```solidity
-/**
- *  @notice Called by an AVS to emit an `AVSMetadataURIUpdated` event indicating the information has updated.
- *
- *  @param metadataURI The URI for metadata associated with an AVS.
- *
- *  @dev Note that the `metadataURI` is *never stored* and is only emitted in the `AVSMetadataURIUpdated` event.
- */
-function updateAVSMetadataURI(
-    address avs, 
-    string calldata metadataURI
-) 
-    external
-    checkCanCall(avs)
-```
-
-_Note: this method can be called directly by an AVS, or by a caller authorized by the AVS. See [`PermissionController.md`](../permissions/PermissionController.md) for details._
-
-Below is the format AVSs should use when updating their metadata URI. This is not validated onchain.
-
-```json
-{
-    "name": "AVS",
-    "website": "https.avs.xyz/",
-    "description": "Some description about",
-    "logo": "http://github.com/logo.png",
-    "twitter": "https://twitter.com/avs",
-    "operatorSets": [
-        {
-            "name": "ETH Set",
-            "id": "1", // Note: we use this param to match the opSet id in the Allocation Manager
-            "description": "The ETH operatorSet for AVS",
-            "software": [
-                {
-                    "name": "NetworkMonitor",
-                    "description": "",
-                    "url": "https://link-to-binary-or-github.com"
-                },
-                {
-                    "name": "ValidatorClient",
-                    "description": "",
-                    "url": "https://link-to-binary-or-github.com"
-                }
-            ],
-            "slashingConditions": ["Condition A", "Condition B"]
-        },
-        {
-            "name": "EIGEN Set",
-            "id": "2", // Note: we use this param to match the opSet id in the Allocation Manager
-            "description": "The EIGEN operatorSet for AVS",
-            "software": [
-                {
-                    "name": "NetworkMonitor",
-                    "description": "",
-                    "url": "https://link-to-binary-or-github.com"
-                },
-                {
-                    "name": "ValidatorClient",
-                    "description": "",
-                    "url": "https://link-to-binary-or-github.com"
-                }
-            ],
-            "slashingConditions": ["Condition A", "Condition B"]
-        }
-    ]
-}
-```
-
-*Effects*:
-* Emits an `AVSMetadataURIUpdated` event for use in offchain services
-
-*Requirements*:
-* Caller MUST be authorized, either as the AVS itself or an admin/appointee (see [`PermissionController.md`](../permissions/PermissionController.md))

docs/core/EigenPodManager.md

@@ -223,7 +223,6 @@ Note that the amount of deposit shares removed while in the withdrawal queue may
 function addShares(
     address staker,
     IStrategy strategy,
-    IERC20,
     uint256 shares
 )
     external

docs/core/StrategyManager.md

@@ -190,13 +190,11 @@ Note that the amount of deposit shares removed while in the withdrawal queue may
 ```solidity
 /// @notice Used by the DelegationManager to award a Staker some shares that have passed through the withdrawal queue
 /// @dev strategy must be beaconChainETH when talking to the EigenPodManager
-/// @dev token is not validated; it is only emitted as an event
 /// @return existingDepositShares the shares the staker had before any were added
 /// @return addedShares the new shares added to the staker's balance
 function addShares(
     address staker,
     IStrategy strategy,
-    IERC20 token,
     uint256 shares
 )
     external
@@ -252,7 +250,7 @@ This method directs the `strategy` to convert the input deposit shares to tokens
 
 ## Burning Slashed Shares
 
-The following methods handle burning of slashed shares:
+Slashed shares are marked as burnable, and anyone can call `burnShares` to transfer them to the default burn address. Burnable shares are stored in `burnableShares`, an [EnumerableMap](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.0/contracts/utils/structs/EnumerableMap.sol) with strategy contract addresses as keys and associated view functions. The following methods handle burning of slashed shares:
 * [`StrategyManager.increaseBurnableShares`](#increaseburnableshares)
 * [`StrategyManager.burnShares`](#burnshares)
 
@@ -296,11 +294,12 @@ function burnShares(
     IStrategy strategy
 )
     external
-    onlyDelegationManager
 ```
 
 Anyone can call this method to burn slashed shares previously added by the `DelegationManager` via `increaseBurnableShares`. This method resets the strategy's burnable shares to 0, and directs the corresponding `strategy` to convert the shares to tokens and transfer them to `DEFAULT_BURN_ADDRESS`, rendering them unrecoverable.
 
+The `strategy` is not called if the strategy had no burnable shares.
+
 *Effects*:
 * Resets the strategy's burnable shares to 0
 * Calls `withdraw` on the `strategy`, withdrawing shares and sending a corresponding amount of tokens to the `DEFAULT_BURN_ADDRESS`

docs/core/accounting/SharesAccounting.md

@@ -162,6 +162,67 @@ See implementation in:
 * [`StrategyManager.depositIntoStrategy`](../../../src/contracts/core/StrategyManager.sol)
 * [`EigenPodManager.recordBeaconChainETHBalanceUpdate`](../../../src/contracts/pods/EigenPodManager.sol)
 
+
+---
+
+### Delegation
+
+Suppose we have an undelegated staker who decides to delegate to an operator.
+We have the following properties that should be preserved.
+
+#### Operator Level
+
+Operator shares should be increased by the amount of delegatable shares the staker has, this is synonymous to their withdrawable shares $a_n$. Therefore,
+
+$$
+op_{n+1} = op_{n} + a_n
+$$
+
+$$
+= op_{n} + s_n k_n l_n m_n
+$$
+
+
+#### Staker Level
+
+withdrawable shares should remain unchanged
+
+$$
+a_{n+1} = a_n
+$$
+
+deposit shares should remain unchanged
+
+$$
+s_{n+1} = s_n
+$$
+
+beaconChainSlashingFactor and maxMagnitude should also remain unchanged. In this case, since the staker is not delegated, then their maxMagnitude should by default be equal to 1.
+
+$$
+l_{n+1} = l_n
+$$
+
+Now the question is what is the new depositScalingFactor equal to?
+
+$$
+a_{n+1} = a_n
+$$
+
+$$
+=> s_{n+1} k_{n+1} l_{n+1} m_{n+1} = s_n k_n l_n m_n
+$$
+
+$$
+=> s_{n} k_{n+1} l_{n} m_{n+1} = s_n k_n l_n m_n
+$$
+
+$$
+=> k_{n+1} = \frac {k_n m_n} { m_{n+1} }
+$$
+
+Notice how the staker variables that update $k_{n+1}$ and $m_{n+1}$ do not affect previously queued withdrawals and shares received upon withdrawal completion. This is because the maxMagnitude that is looked up is dependent on the operator at the time of the queued withdrawal and the $k_n$ is effectively stored in the scaled shares field.
+
 ---
 
 ### Slashing
@@ -297,6 +358,8 @@ $$
 
 Note that when a withdrawal is queued, a `Withdrawal` struct is created with _scaled shares_ defined as $q_t = x_t k_t$ where $t$ is the time of the queuing. The reason we define and store scaled shares like this will be clearer in [Complete Withdrawal](#complete-withdrawal) below.
 
+Additionally, we reset the depositScalingFactor when a user queues a withdrawal for all their shares, either through un/redelegation or directly. This is because the DSF at the time of withdrawal is stored in the scaled shares, and any "new" deposits or delegations by the staker should be considered as new. Note that withdrawal completion is treated as a kind of deposit when done as shares, which again will be clearer below.
+
 See implementation in:
 * `DelegationManager.queueWithdrawals`
 * `SlashingLib.scaleForQueueWithdrawal`

docs/core/accounting/SharesAccountingEdgeCases.md

@@ -145,6 +145,28 @@ Since the operatorShares are simply incrementing by the exact depositShares, the
 Granted the initial deposit amount was `4.418e28` which is magnitudes larger than the discrepancy here but this its important to note the side effects of the redesigned accounting model.
 Instead of purely incremented/decremented amounts, we have introduced magnitudes and scaling factor variables which now result in small amounts of rounding error from division in several places. We deem this rounding behavior to be tolerable given the costs associated for the number of transactions to emulate this and the proportional error is very small.
 
+### Slashing and Rounding Up Operator Shares and Rounding down on Staker Withdrawable Shares
+
+As can be observed in the `SlashingLib.sol` library, we round up on the operatorShares when slashing and round down on the staker's withdrawableShares. If we look at a core invariant of the shares accounting model, we ideally want to preserve the following:
+
+$$
+op_n = \sum_{i=1}^{k} a_{n,i}
+$$
+
+where $op_n$ is the operatorShares at time $n$ and $a_{n,i}$ is the staker's withdrawableShares at time $n$ for the $i^{th}$ staker.
+
+However due to rounding limitations, there will be some error introduced in calculating the amount of operator shares to slash above and also in calculating the staker's withdrawableShares. To prevent a situation where all stakers were to attempt to withdraw and the operatorShares underflows, we round up on the operatorShares when slashing and round down on the staker's withdrawableShares.
+
+So in practice, the above invariant becomes.
+
+$$
+op_n \geq \sum_{i=1}^{k} a_{n,i}
+$$
+
+Upwards rounding on calculating the amount of operatorShares to give to an operator after slashing is intentionally performed in `SlashingLib.calcSlashedAmount`.
+For calculating a staker's withdrawableShares, there are many different factors to consider such as calculating their depositScalingFactor, their slashingFactor, and calculating the amount of withdrawable shares altogether with their depositShares. These variables are all by default rounded down in calculation and is expected behavior for stakers.
+
+
 ## Upper bound on Residual Operator Shares
 
 Related to the above rounding error on deposits, we want to calculate what is the worst case rounding error for a staker depositing shares into EigenLayer.