feat: remove enableE3Program onlyOwner modifier#1421
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ctrlc03
previously approved these changes
Mar 13, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/enclave-contracts/contracts/Enclave.sol (1)
636-643: Harden permissionless registration with basic address validation.Line 636 currently allows enabling zero-address/EOA entries, which can pollute the program registry with unusable targets. Add minimal guards before mutating
e3Programs.Suggested patch
function enableE3Program(IE3Program e3Program) public { + require( + address(e3Program) != address(0) && + address(e3Program).code.length > 0, + "Invalid E3 program" + ); require( !e3Programs[e3Program], ModuleAlreadyEnabled(address(e3Program)) ); e3Programs[e3Program] = true; emit E3ProgramEnabled(e3Program); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 636 - 643, The enableE3Program function currently allows zero-address or EOA entries into the e3Programs mapping; add a guard before mutating e3Programs to require address(e3Program) != address(0) and that the target is a contract (use Address.isContract(address(e3Program)) or equivalent code size check) to reject EOAs and the zero address; keep the existing ModuleAlreadyEnabled check and then set e3Programs[e3Program] = true and emit E3ProgramEnabled(e3Program).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/evm-helpers/src/contracts.rs`:
- Line 128: The ABI declaration for enableE3Program incorrectly includes a
return value; remove the "returns (bool success)" clause from the function
signature in the contracts ABI so it matches the Solidity implementation and
interface (function enableE3Program(address e3Program) public;), which prevents
ABI decode mismatches when calling enableE3Program from bindings; update the
function signature in the same file (contracts.rs) where enableE3Program is
declared and ensure any generated bindings are re-derived if applicable.
---
Nitpick comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 636-643: The enableE3Program function currently allows
zero-address or EOA entries into the e3Programs mapping; add a guard before
mutating e3Programs to require address(e3Program) != address(0) and that the
target is a contract (use Address.isContract(address(e3Program)) or equivalent
code size check) to reject EOAs and the zero address; keep the existing
ModuleAlreadyEnabled check and then set e3Programs[e3Program] = true and emit
E3ProgramEnabled(e3Program).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2d32632-3dba-4c3b-8736-48d10cb99f0b
📒 Files selected for processing (9)
crates/evm-helpers/src/contracts.rspackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/test/Enclave.spec.ts
💤 Files with no reviewable changes (1)
- packages/enclave-contracts/test/Enclave.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/Enclave.sol (1)
634-641:⚠️ Potential issue | 🟠 MajorPublic enable currently nullifies owner disable controls.
With Line 634 permissionless and
disableE3Programstill owner-only, any account can immediately re-enable a program after owner disable. That removes practical quarantine capability for compromised programs.Admin-veto pattern that preserves permissionless registration
+ mapping(IE3Program e3Program => bool blocked) public e3ProgramBlocked; + error ModuleBlocked(address module); + function enableE3Program(IE3Program e3Program) public { + require(!e3ProgramBlocked[e3Program], ModuleBlocked(address(e3Program))); require( !e3Programs[e3Program], ModuleAlreadyEnabled(address(e3Program)) ); e3Programs[e3Program] = true; emit E3ProgramEnabled(e3Program); } function disableE3Program(IE3Program e3Program) public onlyOwner { require(e3Programs[e3Program], ModuleNotEnabled(address(e3Program))); delete e3Programs[e3Program]; + e3ProgramBlocked[e3Program] = true; emit E3ProgramDisabled(e3Program); } + + function unblockE3Program(IE3Program e3Program) external onlyOwner { + e3ProgramBlocked[e3Program] = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 634 - 641, The enableE3Program function is public which allows any account to re-enable an E3 program even after the owner has disabled it; add an owner-veto check so permissionless registration remains allowed for new programs but re-enabling a program that was disabled/quarantined requires owner authorization. Concretely, introduce or reuse a quarantine/disabled marker (e.g., a mapping like e3ProgramQuarantined or consult disableE3Program state) and update enableE3Program to permit setting e3Programs[e3Program]=true unconditionally only if the program has never been disabled, otherwise require onlyOwner before flipping it back on; keep the E3ProgramEnabled event and ensure disableE3Program still sets the quarantine marker so the owner can veto re-enables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 332-343: The code persists E3 lifecycle/payment state
(e3Payments[e3Id], _e3FeeTokens[e3Id], _e3Stages[e3Id], _e3Requesters[e3Id],
_e3Deadlines[e3Id]) before making external calls feeToken.safeTransferFrom and
e3Program.validate, creating a reentrancy risk; reorder to follow
Checks-Effects-Interactions: perform all external interactions (call
feeToken.safeTransferFrom and e3Program.validate) first, handle their
return/require results, then write the persistent state (assign e3Payments,
_e3FeeTokens, _e3Stages, _e3Requesters, _e3Deadlines.computeDeadline) only after
those calls succeed, or alternatively add a reentrancy guard around the function
if state must be set earlier; reference the variables/functions above when
making the changes.
---
Outside diff comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 634-641: The enableE3Program function is public which allows any
account to re-enable an E3 program even after the owner has disabled it; add an
owner-veto check so permissionless registration remains allowed for new programs
but re-enabling a program that was disabled/quarantined requires owner
authorization. Concretely, introduce or reuse a quarantine/disabled marker
(e.g., a mapping like e3ProgramQuarantined or consult disableE3Program state)
and update enableE3Program to permit setting e3Programs[e3Program]=true
unconditionally only if the program has never been disabled, otherwise require
onlyOwner before flipping it back on; keep the E3ProgramEnabled event and ensure
disableE3Program still sets the quarantine marker so the owner can veto
re-enables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ee1e23c-0ab1-4eab-bfeb-ea88de6e95ef
📒 Files selected for processing (1)
packages/enclave-contracts/contracts/Enclave.sol
ctrlc03
approved these changes
Mar 13, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR makes enableE3Program permissionless so anyone can register an E3 program. We want to let people use E3s without needing the protocol owner to whitelist their program first. The encryption scheme registry (decryptionVerifiers) remains owner-gated, which is the real security boundary, a permissionlessly registered E3 program still can't introduce new encryption schemes or verifiers.
There are two places where we call into a (now-untrusted) E3 program:
1.
request()->e3Program.validate()If
validate()tries to re-enter:request(): Gets a newe3Id, has to pay a new fee. Just creates an independent E3, no harm done.publishCiphertextOutput/publishPlaintextOutput: Stage isRequested, both require later stages (KeyPublished/CiphertextReady), so it'll reverts.markE3Failed: CallsgetCommitteeDeadline(e3Id)on the registry, but the committee hasn't been requested yet (requestCommitteeruns aftervalidate), so the registry reverts withCommitteeNotRequested.processE3Failure: Stage isRequested, notFailed-> reverts.The fee transfer before
validate()is important, it means even with an ERC-777-style callback token, you can't create E3s without paying.2.
publishCiphertextOutput()->e3Program.verify()The
ciphertextOutputhash is written to storage before callingverify(). Ifverify()re-enterspublishCiphertextOutputfor the same E3, theciphertextOutput == bytes32(0)check sees the already-written value and reverts withCiphertextOutputAlreadyPublished. Stage hasn't advanced yet so no other function accepts this E3 either.Risk
Anyone can spam
enableE3Programto pollute the mapping (costs only gas). A malicious program could grief ciphernodes by creating junk E3s, but the attacker still pay E3 fee per request and ciphernodes get paid if the E3 completes. Owner can alwaysdisableE3Programto revoke bad actors.Summary by CodeRabbit