[DO NOT MERGE] Slashing Release Branch Internal Security Review #331

nadir-akhtar · 2024-12-10T23:46:10Z

Creating a draft PR for internal security review and creating comments

* 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

…y registered (#256)

* 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

* 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

No high severity findings. Some open TODOs noted and other lower-severity findings called out here. Various changes in #332 address other findings (primarily informational).

As specified in the description of #332, this review serves as a sanity check before an external audit.

src/AVSRegistrar.sol

nadir-akhtar · 2024-12-11T18:53:14Z

src/BLSSignatureChecker.sol

        {
            bool _staleStakesForbidden = staleStakesForbidden;
-            uint256 withdrawalDelayBlocks = _staleStakesForbidden ? delegation.minWithdrawalDelayBlocks() : 0;
+            /// TODO: FIX


Pretty big TODO catching my eye -- can we resolve or remove?

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

nadir-akhtar · 2024-12-11T18:55:12Z

src/BLSSignatureChecker.sol

+    function _setStaleStakesForbidden(bool value) internal {
+        staleStakesForbidden = value;
+        emit StaleStakesForbiddenUpdate(value);


Any interest in requiring that it not be the previous value?

sounds reasonable

src/EjectionManager.sol

nadir-akhtar · 2024-12-11T21:50:13Z

src/RegistryCoordinator.sol

+        /// 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


Another TODO worth addressing or removing

Removed the TODO for now

nadir-akhtar · 2024-12-11T22:01:42Z

src/ServiceManagerBase.sol

+        /// TODO:
+        // _avsDirectory.createOperatorSets(operatorSetIds);


Another open TODO

Seems there's a couple other functions in this file that have yet to be completed as well

Addressing atm

Addressed here #334

nadir-akhtar · 2024-12-11T22:08:42Z

src/ServiceManagerBase.sol

+    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"
+        );
+    }


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

nadir-akhtar · 2024-12-11T22:10:56Z

src/StakeRegistry.sol

+        // TODO: logic for determining if it's an operator set quorum number or not
+        // avsDirectory.isOperatorSetAVS(address(serviceManager));


Another TODO

removed adb8ca4

nadir-akhtar · 2024-12-12T17:42:08Z

src/StakeRegistry.sol

+    /**
+     * @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);
+    }


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.

Added here 7b865e6

nadir-akhtar · 2024-12-12T19:01:32Z

src/libraries/QuorumBitmapHistoryLib.sol

+
+/// @title QuorumBitmapHistoryLib
+/// @notice This library operates on the _operatorBitmapHistory in the RegistryCoordinator
+library QuorumBitmapHistoryLib {


Seems that this library is entirely untested -- strongly recommend adding substantial coverage given its criticality

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

* docs: match file and library name, and add docstrings * docs: update error strings to match function * style: forge fmt + moving variables around + fixing error strings * fix: enforce max quorum count for EjectionManager * style: more error string fixes + formatting * fix: correctly set StakeRegistryStorage gap * style: more error string corrections + typos

* fix: withdrawal delay check * fix: add operator set strategies (#334) * fix: remove registerOperatorToOperatorSet interface function * fix: update interface functions with alm interface * fix: operator set strategies in ALM * chore: remove todo * fix: add strategies by stake registry * fix: params for deregister * chore: remove old migration functions * chore: check quorum exists before setting params * fix: deregister flow for operator set quorums * test: fix test for DM withdrawal delay blocks * chore: update all pragmas to 0.8.27 (#336) * feat: custom require errors for registry coordinator (#337) * feat: custom require errors for registry coordinator * ci: update the ci to use the correct compiler settings

0x0aa0 and others added 30 commits April 11, 2024 17:32

chore: bump forge-std and remove ds-test dependency (#227)

f224cf9

feat: allowlist mapping (#226)

6454c05

feat: return ops ejected (#228)

0328639

Use uint32 for number of ejected operators (#232)

59ebd32

* Use uint32 for number of ejected operators * fix test

feat: service manager payments (#242)

e18c19e

* feat: service manager payments * test: unit tests * feat: refactor serviceManager interfaces * chore: requested changes

fix: spacing (#244)

2b65511

chore: rename audit report for clarity and consistency with core (#245)

deebd4b

feat: batch operator id conversions (#248)

1feb6ae

* feat: batch operator id conversions * docs: natspec

fix: vm assume too many rejections (#247)

1703d25

* fix: vm assume too many rejections * fix: typo

feat: reregistration delay (#246)

74c86d2

* feat: rereg delay * test: fix tests * chore: natspec * test: unit * fix: grief * refactor: ejection manager * fix: units * fix: name nit * docs: ejector dev tag

fix: add init function for staleStakesForbidden in BLSSignatureChecker (

8ed82a4

#251) * fix: init function for staleStakesForbidden in BLSSignatureChecker * chore: formatting use forge fmt * chore: remove initialiable

feat: update core contracts submodule (#254)

4ad5c85

fix: multiple rangepayments with same token (#257)

c5b419a

test: fix flaky fuzz tests hitting the default operator who is alread…

a23de11

…y registered (#256)

chore: update license (#259)

0351419

* chore: update license adds an Additional Use Grant and updates the change dates of the repository * chore: formatting. add line breaks for consistency

fix: storage gap (#261)

1142437

chore: refactor with RewardsCoordinator (#262)

51e36f1

chore: rewards release submodule (#264)

c75eb8e

fix: deprecated struct field for earning receiver (#265)

b45dced

perf: refactor to modifers to use internal functions (#272)

da5cf42

perf: refactor index registry to use internal functions for modifier (#…

9f92f72

…269) * fix: deprecated struct field for earning receiver * perf: move modifier logic to internal function

perf: refactor apk modifiers to use internal functions (#268)

b1f706b

* fix: deprecated struct field for earning receiver * perf: move modifier logic to internal function

perf: refactor modifiers to use internal functions for stake registry (…

89cbd3c

…#266) * fix: deprecated struct field for earning receiver * refactor: require statements to internal functions * test: update revert reason strings

fix: storage layout (#275)

dc5385e

updated inheritance to allow the EigenDAServiceManager to maintain its current storage layout

feat: reward initiator for ECDSAServiceManagerBase (#274)

b816138

* feat: reward initiator for ECDSAServiceManagerBase * fix: update gap * feat: changes to onlyRewardsInitiator modifier * fix: error code

fix: comment typos in IServiceManagerUI (#278)

d643f3b

stevennevins and others added 21 commits August 15, 2024 09:03

docs: add natspec

788cd09

feat: add checks operator was registered for quorums when migrating

d64f4af

fix: resolve code size issue in RegistryCoordinator

a92d51a

build: update core submodule to new release (#298)

74438b7

* build: update core submodule to new release * fix: flakey tests from pepe upgrade

chore: update to latest core (#299)

32148de

* chore: update to latest eigenlayer-contracts feat/operator-set-release * chore: add method to mock

add overrides (#302)

cb2b334

Co-authored-by: steven <[email protected]>

merge dev

3e4cb2b

feat(op sets): upgrade and migrate script (#303)

e0a79f1

* feat: upgrade and test pre prod upgrade and migration * fix: add param introduced in merge

chore: remove unused test util contracts (#319)

145bdaf

* feat: remove both option * chore: remove unused test util contracts * chore: remove diff

feat: remove both option

9a975df

fix: storage gap remove one slot (#320)

fa04f06

fix: use libraries with only internal vis (#324)

158a890

* fix: revert making library function vis external * fix: signature checker internal

chore: bump dependency for slashing mags updates (#329)

6fd466e

Merge branch 'mainnet' into feat/slashing-release-branch

f9ce2fc

nadir-akhtar commented Dec 12, 2024

View reviewed changes

nadir-akhtar and others added 7 commits December 13, 2024 09:45

fix: revert cases

cb298ea

chore: bump core dependency to pull in latest updates

7307b08

chore: bump core dependency

1cc4983

chore: bump forge-std

44e1a40

docs: update readme with note on slashing (#341)

865b490

nadir-akhtar closed this Jan 6, 2025

		/// TODO:
		// _avsDirectory.createOperatorSets(operatorSetIds);

		// TODO: logic for determining if it's an operator set quorum number or not
		// avsDirectory.isOperatorSetAVS(address(serviceManager));

[DO NOT MERGE] Slashing Release Branch Internal Security Review #331

[DO NOT MERGE] Slashing Release Branch Internal Security Review #331

Uh oh!

Conversation

nadir-akhtar commented Dec 10, 2024

Uh oh!

nadir-akhtar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants