-
Notifications
You must be signed in to change notification settings - Fork 465
fix: address slashing commitments informationals from Certora audit #1689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: address slashing commitments informationals from Certora audit #1689
Conversation
1681f10 to
5dbff68
Compare
| /// @notice Delay before allocation delay modifications take effect. | ||
| uint32 public immutable ALLOCATION_CONFIGURATION_DELAY; | ||
|
|
||
| /// @notice Delay before slasher changes take effect. | ||
| /// @dev Currently set to the same value as ALLOCATION_CONFIGURATION_DELAY. | ||
| uint32 public immutable SLASHER_CONFIGURATION_DELAY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note sure I agree with their solution, strange to have to two constants with the same value but different names, will lead to confusion down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly just due to separation of concerns - it's not clear that the ALLOCATION_CONFIGURATION_DELAY var should also be used for the slasher delay
0xClandestine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment, lgtm otherwise
- Fix I-01: Re-proposing same pending slasher is now a no-op (delay countdown not restarted) - Fix I-02: Add NatSpec documentation for getSlasher/getPendingSlasher return values on non-existent operator sets - Fix I-03: Add separate SLASHER_CONFIGURATION_DELAY constant (currently same as ALLOCATION_CONFIGURATION_DELAY) - Fix I-05: Add gas warning documentation for migrateSlashers regarding O(appointees) cost 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
5dbff68 to
d44717b
Compare
9959779
into
feat/slashing-improvements-audit-fixes
…1689) **Motivation:** Address informational findings from the Certora audit of EigenLayer Slashing UX Improvements (PR-1645/PR-544). These are quality-of-life and documentation improvements that enhance code clarity and prevent potential edge case issues. **Modifications:** 1. **Fix I-01: Re-proposing same pending slasher is now a no-op** - Added check in `_updateSlasher()` to skip processing if the proposed slasher is already pending and hasn't taken effect - Prevents accidentally restarting the delay countdown when re-proposing the same slasher 2. **Fix I-02: Add NatSpec documentation for getSlasher/getPendingSlasher** - Updated interface and implementation NatSpec to document that these functions return `address(0)`/`0` for non-existent operator sets - Helps callers understand expected behavior without validation 3. **Fix I-03: Add separate SLASHER_CONFIGURATION_DELAY constant** - Added new `SLASHER_CONFIGURATION_DELAY` immutable in `AllocationManagerStorage` - Currently set to same value as `ALLOCATION_CONFIGURATION_DELAY` but can be changed independently in future upgrades - Updated `_updateSlasher()` to use the new constant - Added interface getter 4. **Fix I-05: Add gas warning documentation for migrateSlashers** - Added NatSpec warning about O(appointees) gas cost per operator set - `PermissionController.getAppointees()` enumerates full appointee set which can be expensive - Documented that large appointee sets may cause block gas limit issues **Result:** - `updateSlasher` is idempotent when re-proposing the same slasher - Clearer documentation on view function return values - Slasher delay can be configured independently in future upgrades - Users are warned about potential gas issues with `migrateSlashers` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
**Motivation:** Address findings from the Certora audit of EigenLayer Slashing UX Improvements. This branch consolidates fixes for low-severity findings (L-01) and informational findings (I-01 through I-05) across AllocationManager and ProtocolRegistry contracts. **Modifications:** ### AllocationManager Fixes **L-01: State inconsistency fixes** - Update `slasher` field immediately when `instantEffectBlock=true` to prevent stale storage (#1687) - Update `delay` and `isSet` fields immediately for newly registered operators to ensure storage consistency (#1688) **I-01: Re-proposing same pending slasher is now a no-op** (#1689) - Added check in `_updateSlasher()` to skip processing if the proposed slasher is already pending - Prevents accidentally restarting the delay countdown **I-02: Add NatSpec documentation for getSlasher/getPendingSlasher** (#1689) - Document that these functions return `address(0)`/`0` for non-existent operator sets **I-03: Add separate SLASHER_CONFIGURATION_DELAY constant** (#1689) - New immutable allows independent configuration of slasher delay in future upgrades - Currently set to same value as `ALLOCATION_CONFIGURATION_DELAY` **I-05: Add gas warning documentation for migrateSlashers** (#1689) - Document O(appointees) gas cost per operator set - Warn about potential block gas limit issues with large appointee sets ### ProtocolRegistry Fixes **I-01: ship() lacks validation** (#1690) - Added array length validation for addresses, configs, and names - Added zero address validation with new `ArrayLengthMismatch()` and `InputAddressZero()` errors **I-02: Orphaned configs on name overwrite** (#1690) - Delete old address's `DeploymentConfig` when re-shipping a name with a new address - Added `DeploymentConfigDeleted(address)` event **I-03: configure() for unshipped addresses** (#1690) - Updated `configure` to require a name parameter - Added validation that address must be a shipped deployment **I-04: Fix misleading NatSpec** (#1690) - Clarified `ship()` behavior when re-shipping names - Updated `configure()` NatSpec for address requirements **I-05: Document pauseAll blocking** (#1690) - Added warning that `pauseAll()` reverts if ANY pausable deployment fails ### Other Changes - v1.9.0 upgrade script fixes for testnet deploy (#1677) - Added Claude skills for development workflow (#1686) **Result:** - Audit fixes complete
Motivation:
Address informational findings from the Certora audit of EigenLayer Slashing UX Improvements (PR-1645/PR-544). These are quality-of-life and documentation improvements that enhance code clarity and prevent potential edge case issues.
Modifications:
Fix I-01: Re-proposing same pending slasher is now a no-op
_updateSlasher()to skip processing if the proposed slasher is already pending and hasn't taken effectFix I-02: Add NatSpec documentation for getSlasher/getPendingSlasher
address(0)/0for non-existent operator setsFix I-03: Add separate SLASHER_CONFIGURATION_DELAY constant
SLASHER_CONFIGURATION_DELAYimmutable inAllocationManagerStorageALLOCATION_CONFIGURATION_DELAYbut can be changed independently in future upgrades_updateSlasher()to use the new constantFix I-05: Add gas warning documentation for migrateSlashers
PermissionController.getAppointees()enumerates full appointee set which can be expensiveResult:
updateSlasheris idempotent when re-proposing the same slashermigrateSlashers🤖 Generated with Claude Code