[DO NOT MERGE] Slashing Release Branch Internal Security Review #331
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* feat: ejector * improvments - trailing window - try/catch ejection * percentage * fix * nits * feat: non signing metric helpers - adds operatorId on reg and dereg events - adds function to OperatorStateRetriever to get bitmaps for a set of operators at a timestamp * fix: conflict * refactor: ejection * fix: stake recording * test: unit * style: natspec * fix: totalEjected > totalEjectable nit: else
* Use uint32 for number of ejected operators * fix test
* feat: service manager payments * test: unit tests * feat: refactor serviceManager interfaces * chore: requested changes
* feat: batch operator id conversions * docs: natspec
* fix: vm assume too many rejections * fix: typo
* feat: rereg delay * test: fix tests * chore: natspec * test: unit * fix: grief * refactor: ejection manager * fix: units * fix: name nit * docs: ejector dev tag
* feat: add ecdsa service manager * feat: split up signature checking from stake regsitry * feat(wip): add external calls to stake registry from signature checker * test: setup signature checker unit tests * feat: add ui related function implementations * chore: remove separating signature checker for now * chore: remove unused test file * docs: add natspec and inherit doc for clarity * docs: add more natspec documentation * refactor: update logic to internal functions that are internal and virtual * docs: add more documentation to the internal functions * chore: update external functions to also be overrideable
#251) * fix: init function for staleStakesForbidden in BLSSignatureChecker * chore: formatting use forge fmt * chore: remove initialiable
* chore: update license adds an Additional Use Grant and updates the change dates of the repository * chore: formatting. add line breaks for consistency
* feat: add operator key rotation * test: update existing tests to account for signing key * fix: frontrunning with different signing key * test: verify the checkpoint logic for the signing keys * feat: improve event for signing key update * chore: clean up test function names and order functions * fix: storage layout gap * feat: prevent signing at current block * test: add two more test cases for RBN * fix: typo in function signature * chore: remove unnecessary test contract * fix: typo for invalid quorum * fix: invalidQuorum -> validQuorum for NotOwner test
* feat: payment initiator integration - creating ServiceManagerBaseStorage - adding paymentInitiator storage var to ServiceManagerBase - adding ability for owner to change paymentInitiator - formatting based on forge fmt for contracts modified * fix: typos * fix: remediations - removing erroneous gap from `ServiceManagerBase` - typo 50 to 49 - making inheritance backward compatible
…269) * fix: deprecated struct field for earning receiver * perf: move modifier logic to internal function
* fix: deprecated struct field for earning receiver * perf: move modifier logic to internal function
…#266) * fix: deprecated struct field for earning receiver * refactor: require statements to internal functions * test: update revert reason strings
updated inheritance to allow the EigenDAServiceManager to maintain its current storage layout
* feat: reward initiator for ECDSAServiceManagerBase * fix: update gap * feat: changes to onlyRewardsInitiator modifier * fix: error code
* chore: checkout migration branch * feat: implement migration function * chore: update to release branch * feat: operator set creation for each quorum number * feat: migration with merge sorted array of operators and their quorums * feat: operator set migration working * chore: revert change from testing * chore: revert change from testing * chore: remove extra logging and commented out asserts * chore: remove unused file * fix: remove console logs * refactor: to view functions * chore: nit and remove unneeded function * fix: remove duplication of looping for all the operators * chore: remove comment * feat: allow migrating in two transactions * feat: finalization of migration * chore: use string errors and fix migration issues * chore: rename * feat: use library for merge sort * test: fuzz view function
* chore: checkout migration branch * feat: implement migration function * chore: update to release branch * feat: operator set creation for each quorum number * feat: migration with merge sorted array of operators and their quorums * feat: operator set migration working * chore: revert change from testing * chore: revert change from testing * chore: remove extra logging and commented out asserts * chore: remove unused file * fix: remove console logs * feat: create quorum post operator set migration * test(wip): create quorum test adds new operator set * test: migration create quorum * refactor: to view functions * chore: nit and remove unneeded function * fix: remove duplication of looping for all the operators * chore: remove comment * feat: allow migrating in two transactions * feat: finalization of migration * chore: use string errors and fix migration issues * chore: rename * feat: use library for merge sort * test: fuzz view function * fix: updates from merge * chore: use interface
* build: update core submodule to new release * fix: flakey tests from pepe upgrade
* chore: update to latest eigenlayer-contracts feat/operator-set-release * chore: add method to mock
Co-authored-by: steven <[email protected]>
* feat: update stakes handle direct deregistration on AVSDirectory * test: update stake for quorum if operator directly unregistered from the AVSDirectory * chore: simplify setup * chore: simplify setup * chore: make service manager immutable on stakeRegistry
* feat: register and deregister to operator sets * fix: bytecode wrangling * chore: shorter errors to reduce bytecode * test: register after migration * chore: remove stale todos * chore: add back commented line * chore: remain consistent with m2 events * fix: side effect of merge from _deregister function * fix: bytecode massaging * chore: rename for clarity * chore: remove comments and whitespace * docs: add natspec to the library
* feat: upgrade and test pre prod upgrade and migration * fix: add param introduced in merge
* chore: bump to slashing branch * chore: bump compiler version * fix: dep interface changes * fix: compiler errors from interface changes and type changes * fix: compiler errors * chore: bump dependencies * chore: bump core dependency and resolve issues * chore: bump core dependency and fix compiler errors * feat: integrate AllocationManager * feat: add a slashing permission to the service manager * chore: remove unneeded casting * feat: implement a slasher permission and forward call to AllocationManager * feat: add simiple slasher starting point * chore: bump slashing magnitudes * chore: bump core slashing-magnitudes branch
* chore: bump to slashing branch * chore: bump compiler version * fix: dep interface changes * fix: compiler errors from interface changes and type changes * fix: compiler errors * chore: bump dependencies * chore: bump core dependency and resolve issues * chore: bump core dependency and fix compiler errors * feat: integrate AllocationManager * feat: add a slashing permission to the service manager * chore: remove unneeded casting * feat: implement a slasher permission and forward call to AllocationManager * feat: add simiple slasher starting point * feat: slashers * chore: change around slashed event * fix: call dm * feat: add proposal mechanism for updating slasher * fix: set to completed instead of delete * chore: use struct instead of params directly * chore: clean up params more * chore: simplify and organize files * chore: cleanup logic and couple event with internal func * fix: pass correct params * chore: organize and add interface * chore: nits * chore: cleanup more nits * fix: storage gap * chore: nits refactor * chore: go back to fulfill being onlySlasher * test: fixes from core updates * fix: use delegated stake per operator set instead of per AVS * fix: update to 14 days * feat: configurable lookahead and stake type
* feat: remove both option * chore: remove unused test util contracts * chore: remove diff
#317) * feat: remove both option * feat: total delegated stake and total slashable stake per quorum config * test: resolve some breaking changes to tests * chore: move stake type to file level definition * chore: refactor loop * test: add unit test for slashble stake quorum init * test: assert on state and event * test: delegated stake quorum and assertions
* fix: revert making library function vis external * fix: signature checker internal
* feat: remove both option * feat: total delegated stake and total slashable stake per quorum config * test: resolve some breaking changes to tests * chore: bump core dependency * chore: bump dependency * chore: bump to latest slashing mags * fix: creation of registry coordinator * test: wip * feat: integrate registrar interfaces * test: add function to delegation mock to set operator status * test: additional test case * chore: bumping core dep and adding UAM (#325) * test: use permission controller mock * chore: label fuzz tests * test: various fixes from config changes * chore: remove comment * test: fix permission controlled functions * test: fix config issue in integration tests * test: fix avs directory initialize * feat: wip prevent m2 registration flows after migration
* chore: add note * fix: remove handling of forceDeregistration * fix: fix total delegated stake usage * fix: integration tests * test: fix remaining integration tests * test: add back log check * test: add additional tests for transition to operator sets * test: add more test cases * feat: record m2 quorums on migration * chore: add note about churn support * fix: prevent operator set registration changes for m2 quorums * feat: require strings * chore: add dev note and add require string
nadir-akhtar
commented
Dec 12, 2024
| import {IStakeRegistry} from "./interfaces/IStakeRegistry.sol"; | ||
| import {IIndexRegistry} from "./interfaces/IIndexRegistry.sol"; | ||
|
|
||
| abstract contract AVSRegistrar is IAVSRegistrar { |
There was a problem hiding this comment.
Curious why we instantiate this abstract contract even though it adds no new functions and implements none either
| @@ -183,22 +193,30 @@ contract BLSSignatureChecker is IBLSSignatureChecker { | |||
| */ | |||
| { | |||
| bool _staleStakesForbidden = staleStakesForbidden; | |||
| uint256 withdrawalDelayBlocks = _staleStakesForbidden ? delegation.minWithdrawalDelayBlocks() : 0; | |||
| /// TODO: FIX | |||
There was a problem hiding this comment.
Pretty big TODO catching my eye -- can we resolve or remove?
There was a problem hiding this comment.
Addressed here a1295fa
This does seem like a breaking change for core to make though since the selector for the minWithdrawal delay changed. Any AVS that uses staleStakesForBidden will have their checkSignatures function bricked when the core protocol performs it's upgrade
Comment on lines
+338
to
+340
| function _setStaleStakesForbidden(bool value) internal { | ||
| staleStakesForbidden = value; | ||
| emit StaleStakesForbiddenUpdate(value); |
There was a problem hiding this comment.
Any interest in requiring that it not be the previous value?
| */ | ||
| function amountEjectableForQuorum(uint8 _quorumNumber) public view returns (uint256) { | ||
| uint256 cutoffTime = block.timestamp - quorumEjectionParams[_quorumNumber].rateLimitWindow; | ||
| uint256 totalEjectable = uint256(quorumEjectionParams[_quorumNumber].ejectableStakePercent) * uint256(stakeRegistry.getCurrentTotalStake(_quorumNumber)) / uint256(BIPS_DENOMINATOR); |
There was a problem hiding this comment.
Wondering if we can store everything as uint256 considering we cast it here, and I don't believe we use it elsewhere
There was a problem hiding this comment.
I believe the issue here was from truncation that occurred without the casting to a larger data type
Comment on lines
+330
to
+332
| /// TODO: Register with Churn doesn't seem to be used in practice. I would advocate for not even handling the | ||
| /// the case and just killing off the function. This would free up code size as well | ||
| /// TODO: alternatively, Correctly handle decoding the registration with churn and the normal registration flow parameters |
There was a problem hiding this comment.
Another TODO worth addressing or removing
Comment on lines
+124
to
+125
| /// TODO: | ||
| // _avsDirectory.createOperatorSets(operatorSetIds); |
There was a problem hiding this comment.
Another open TODO
Seems there's a couple other functions in this file that have yet to be completed as well
Comment on lines
+464
to
+477
| function _checkRewardsInitiator() internal view { | ||
| require( | ||
| msg.sender == rewardsInitiator, | ||
| "ServiceManagerBase.onlyRewardsInitiator: caller is not the rewards initiator" | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| function _checkSlasher() internal view { | ||
| require( | ||
| msg.sender == slasher, | ||
| "ServiceManagerBase.onlySlasher: caller is not the slasher" | ||
| ); | ||
| } |
There was a problem hiding this comment.
General remark for this file, would be valuable to organize file as:
- External / public functions
- Internal functions
- External / public view functions
Believe this captures our function ordering practice
Comment on lines
+158
to
+159
| // TODO: logic for determining if it's an operator set quorum number or not | ||
| // avsDirectory.isOperatorSetAVS(address(serviceManager)); |
Comment on lines
+244
to
+260
| /** | ||
| * @notice Sets the stake type for the registry for a specific quorum | ||
| * @param quorumNumber The quorum number to set the stake type for | ||
| * @param _stakeType The type of stake to track (TOTAL_DELEGATED, TOTAL_SLASHABLE, or BOTH) | ||
| */ | ||
| function setStakeType(uint8 quorumNumber, StakeType _stakeType) external onlyCoordinatorOwner { | ||
| _setStakeType(quorumNumber, _stakeType); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Sets the look ahead time for checking operator shares for a specific quorum | ||
| * @param quorumNumber The quorum number to set the look ahead period for | ||
| * @param _lookAheadPeriod The number of days to look ahead when checking shares | ||
| */ | ||
| function setSlashableStakeLookahead(uint8 quorumNumber, uint32 _lookAheadPeriod) external onlyCoordinatorOwner { | ||
| _setLookAheadPeriod(quorumNumber, _lookAheadPeriod); | ||
| } |
There was a problem hiding this comment.
You could consider requiring _checkQuorumExists() for these. It would prevent setting details for non-existent quorums, but also not a major issue if it happens.
|
|
||
| /// @title QuorumBitmapHistoryLib | ||
| /// @notice This library operates on the _operatorBitmapHistory in the RegistryCoordinator | ||
| library QuorumBitmapHistoryLib { |
There was a problem hiding this comment.
Seems that this library is entirely untested -- strongly recommend adding substantial coverage given its criticality
There was a problem hiding this comment.
Yeah can add separate tests for this. This was pulled out of the functions in registry coordinator without modification so it's implicitly tested e2e there
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creating a draft PR for internal security review and creating comments