fix(contracts): Disallow ejectors from cancelling other ejectors' ejections#2424
fix(contracts): Disallow ejectors from cancelling other ejectors' ejections#2424ethenotethan wants to merge 1 commit intomasterfrom
Conversation
ethenotethan
requested review from
0xClandestine,
Lulu-Zhou-EigenLabs,
cody-littley and
Copilot
There was a problem hiding this comment.
Pull request overview
This PR fixes a security vulnerability in the EigenDA ejection system where any ejector could cancel ejections initiated by other ejectors, creating an unwanted trust assumption. The fix adds a validation check to ensure only the ejector who initiated an ejection can cancel it.
Key Changes:
- Added
requirestatement validating thatmsg.sendermatches the ejector who initiated the ejection - Added comprehensive unit test to verify the new security constraint
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| contracts/src/periphery/ejection/EigenDAEjectionManager.sol | Added ejector ownership validation in cancelEjectionByEjector to prevent cross-ejector cancellations |
| contracts/test/unit/EigenDAEjectionManager.t.sol | Added test case verifying that different ejectors cannot cancel each other's ejections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function testCancelEjectionByEjectorRevertsWhenCalledByDifferentEjector() public { | ||
| // 0) create a second ejector and grant access for ejection role | ||
| address ejector2 = makeAddr("ejector2"); | ||
| accessControl.grantRole(AccessControlConstants.EJECTOR_ROLE, ejector); | ||
| accessControl.grantRole(AccessControlConstants.EJECTOR_ROLE, ejector2); | ||
| accessControl.grantRole(AccessControlConstants.OWNER_ROLE, ejector); | ||
|
|
||
| // 1) first ejector starts an ejection | ||
| vm.startPrank(ejector); | ||
| ejectionManager.setCooldown(0); | ||
| ejectionManager.setDelay(0); | ||
| ejectionManager.startEjection(ejectee, "0x"); | ||
| vm.stopPrank(); | ||
|
|
||
| // 2) verify the ejection was created with the first ejector | ||
| assertEq(ejectionManager.getEjector(ejectee), ejector); | ||
|
|
||
| // 3) attempting to cancel the ejection from a different ejector should revert | ||
| vm.startPrank(ejector2); | ||
| vm.expectRevert("only ejector that issued ejection can cancel"); | ||
| ejectionManager.cancelEjectionByEjector(ejectee); | ||
| vm.stopPrank(); | ||
| } |
There was a problem hiding this comment.
Consider adding a test case that verifies the original ejector can still successfully cancel their own ejection after the fix. This would provide more comprehensive coverage and confirm the fix doesn't break the intended behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2424 +/- ##
==========================================
- Coverage 39.15% 39.12% -0.03%
==========================================
Files 550 550
Lines 50734 50734
==========================================
- Hits 19864 19852 -12
- Misses 28376 28385 +9
- Partials 2494 2497 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bug identified by llm analysis - see thread here
Why are these changes needed?
This change ensures that only the ejector that initiated an ejection can cancel it. - otherwise there is a trust assumption on the ejector. In the current implementation an ejector can cancel ejections made by other ejectors.
Changes
Checks