fix!: remediate DKG recovery and E3 program registration - #1767
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe change adds durable terminal failure handling for threshold keyshares and requires one deployed E3 program during Interfold initialization. Later E3 program registration is owner-only. Deployment validation, restart redrive, tests, and documentation reflect these contracts. ChangesDurable keyshare failure recovery
E3 program bootstrap and governance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant KeyshareRepository
participant EventBus
participant ThresholdKeyshare
Collector->>KeyshareRepository: persist KeyshareState::Failed
KeyshareRepository->>EventBus: publish E3Failed
Collector->>ThresholdKeyshare: stop actor
ThresholdKeyshare->>KeyshareRepository: hydrate Failed state
ThresholdKeyshare->>EventBus: replay E3Failed after EffectsEnabled
sequenceDiagram
participant DeploymentConfig
participant Interfold
participant Safe
DeploymentConfig->>Interfold: initialize(initialE3Program)
Interfold->>Interfold: registerE3Program(initialE3Program)
Interfold->>Safe: transferOwnership()
Safe->>Interfold: registerE3Program(program)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
5 tasks
hmzakhalid
marked this pull request as ready for review
August 3, 2026 23:24
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/interfold-contracts/contracts/Interfold.sol (2)
633-639: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPrevent undeployed E3 addresses from entering the deployment flow.
The current flow can register an address with no runtime code and then report it as enabled. The supplied zero address can therefore pass registration while E3 requests cannot call
validate.
packages/interfold-contracts/contracts/Interfold.sol#L633-L639: reject zero and no-code addresses inregisterE3Program.packages/interfold-contracts/contracts/Interfold.sol#L209-L233: routeinitialE3Programthrough the same guard.packages/interfold-contracts/scripts/protocol/values.ts#L183-L186: reject zero during configuration validation.packages/interfold-contracts/scripts/protocol/validate.ts#L125-L130: check provider bytecode in addition to mapping membership.packages/interfold-contracts/deploy/protocol/example.protocol.config.json#L77-L77: replace zero with an actual deployed E3 program address.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/Interfold.sol` around lines 633 - 639, Prevent undeployed or zero E3 addresses from entering deployment flows: update Interfold.sol lines 633-639 in registerE3Program and lines 209-233 for initialE3Program to apply zero-address and runtime-code checks; update packages/interfold-contracts/scripts/protocol/values.ts lines 183-186 to reject zero configuration values; update packages/interfold-contracts/scripts/protocol/validate.ts lines 125-130 to verify provider bytecode alongside mapping membership; replace the zero E3 address in packages/interfold-contracts/deploy/protocol/example.protocol.config.json line 77 with an actual deployed E3 program address.
633-639: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRoute
enableE3through the Interfold owner.The task uses the first signer, but protocol deployment sets
config.safeas the Interfold owner. Submit the call through the Safe or use an owner signer. Skip the call whene3Programs[e3Address]is alreadytrue, because deployment registersconfig.e3Programs[0].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/Interfold.sol` around lines 633 - 639, The deployment flow calling registerE3Program must use the Interfold owner configured as config.safe, submitting through the Safe or an owner signer rather than assuming the first signer. Before invoking it, check e3Programs[e3Address] and skip the call when already enabled, preserving the existing registration behavior for unregistered programs.
🧹 Nitpick comments (1)
packages/interfold-contracts/test/fixtures/system.ts (1)
413-413: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise initialization with a deployed E3 program.
The fixture and the initialization test use placeholder addresses. They do not verify the deployed-program requirement and register invalid addresses alongside the real mock program.
packages/interfold-contracts/test/fixtures/system.ts#L413-L413: deploy the mock before Interfold initialization or require a real initial program.packages/interfold-contracts/test/Interfold.spec.ts#L71-L73: pass the deployed mock address instead ofAddressTwo.packages/interfold-contracts/test/Interfold.spec.ts#L92-L96: assert that the deployed mock address is registered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/fixtures/system.ts` at line 413, Update the system fixture’s initialE3Program setup to deploy the mock E3 program before Interfold initialization, or require a valid deployed program instead of defaulting to ADDRESS_ONE. In packages/interfold-contracts/test/Interfold.spec.ts lines 71-73, pass the deployed mock address rather than AddressTwo; at lines 92-96, assert that this deployed mock address is registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/keyshare/src/threshold_keyshare/state.rs`:
- Around line 146-147: Update the transition logic around the `new_state` match
in the keyshare state machine to reject any transition from an existing
`K::Failed` state unless the incoming failure is exactly equal and therefore
idempotent. Ensure the earlier same-variant handling cannot permit a different
failure stage or reason, and add a test covering a second `Failed` transition
with different payloads.
In `@packages/interfold-contracts/ignition/modules/interfold.ts`:
- Around line 15-18: Require a real, bytecode-backed E3 program across all
deployment entry points: in
packages/interfold-contracts/ignition/modules/interfold.ts:15-18, remove the 0x1
default or validate the configured initialE3Program before initialization; in
packages/interfold-contracts/scripts/deployAndSave/interfold.ts:34, make
initialE3Program required; and at
packages/interfold-contracts/scripts/deployAndSave/interfold.ts:51, remove the
ADDRESS_ONE fallback and reject the address when deployed code is absent.
---
Outside diff comments:
In `@packages/interfold-contracts/contracts/Interfold.sol`:
- Around line 633-639: Prevent undeployed or zero E3 addresses from entering
deployment flows: update Interfold.sol lines 633-639 in registerE3Program and
lines 209-233 for initialE3Program to apply zero-address and runtime-code
checks; update packages/interfold-contracts/scripts/protocol/values.ts lines
183-186 to reject zero configuration values; update
packages/interfold-contracts/scripts/protocol/validate.ts lines 125-130 to
verify provider bytecode alongside mapping membership; replace the zero E3
address in
packages/interfold-contracts/deploy/protocol/example.protocol.config.json line
77 with an actual deployed E3 program address.
- Around line 633-639: The deployment flow calling registerE3Program must use
the Interfold owner configured as config.safe, submitting through the Safe or an
owner signer rather than assuming the first signer. Before invoking it, check
e3Programs[e3Address] and skip the call when already enabled, preserving the
existing registration behavior for unregistered programs.
---
Nitpick comments:
In `@packages/interfold-contracts/test/fixtures/system.ts`:
- Line 413: Update the system fixture’s initialE3Program setup to deploy the
mock E3 program before Interfold initialization, or require a valid deployed
program instead of defaulting to ADDRESS_ONE. In
packages/interfold-contracts/test/Interfold.spec.ts lines 71-73, pass the
deployed mock address rather than AddressTwo; at lines 92-96, assert that this
deployed mock address is registered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b9a2c641-e885-42db-b59c-0af8fa1eddee
📒 Files selected for processing (31)
agent/CRATES_ARCHITECTURE.mdagent/INVARIANTS.mdagent/flow-trace/00_INDEX.mdagent/flow-trace/03_E3_REQUEST_AND_COMMITTEE.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcrates/keyshare/src/threshold_keyshare/effects/create_decryption_share.rscrates/keyshare/src/threshold_keyshare/effects/initialize_dkg.rscrates/keyshare/src/threshold_keyshare/handlers.rscrates/keyshare/src/threshold_keyshare/state.rscrates/keyshare/src/threshold_keyshare/state_tests.rscrates/keyshare/src/threshold_keyshare/tests.rspackages/interfold-contracts/README.mdpackages/interfold-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/interfold-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/interfold-contracts/artifacts/contracts/interfaces/IInterfold.sol/IInterfold.jsonpackages/interfold-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/interfold-contracts/artifacts/contracts/token/InterfoldTicketToken.sol/InterfoldTicketToken.jsonpackages/interfold-contracts/contracts/Interfold.solpackages/interfold-contracts/contracts/interfaces/IInterfold.solpackages/interfold-contracts/deploy/protocol/example.protocol.config.jsonpackages/interfold-contracts/ignition/modules/interfold.tspackages/interfold-contracts/scripts/deployAndSave/interfold.tspackages/interfold-contracts/scripts/deploymentRecords.tspackages/interfold-contracts/scripts/protocol/actions.tspackages/interfold-contracts/scripts/protocol/deployContracts.tspackages/interfold-contracts/scripts/protocol/tx/interfold.tspackages/interfold-contracts/scripts/protocol/types.tspackages/interfold-contracts/scripts/protocol/validate.tspackages/interfold-contracts/scripts/protocol/values.tspackages/interfold-contracts/test/Interfold.spec.tspackages/interfold-contracts/test/fixtures/system.ts
💤 Files with no reviewable changes (1)
- packages/interfold-contracts/scripts/protocol/tx/interfold.ts
ctrlc03
previously approved these changes
Aug 4, 2026
Persist terminal keyshare failures before timeout events. Bootstrap one production E3 program before Safe ownership, and restrict later registrations to the owner. Refs #1765
hmzakhalid
force-pushed
the
fix/audit-remediations-28516
branch
from
August 4, 2026 19:52
df1c419 to
cafc5f3
Compare
ctrlc03
approved these changes
Aug 4, 2026
ctrlc03
enabled auto-merge (squash)
August 4, 2026 20:11
hmzakhalid
added a commit
that referenced
this pull request
Aug 16, 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.
What
Closes #1765.
This change remediates three audit findings:
KeyshareState::Failedbefore they publishE3Failed.EffectsEnabledredrives the saved terminal payload after startup.registerE3Programnow requires the Interfold owner.Interfold.initializebefore ownership transfers to the Safe.Review follow-up also makes a terminal failure payload immutable, rejects zero and no-code E3 program addresses on-chain and in deployment validation, initializes test deployments with a deployed mock program, and routes the
interfold:enableE3task through an available owner signer. The checked-in example configuration remains an intentionally incomplete template. Configuration validation rejects its zero E3 program until an operator supplies a deployed address.The initializer ABI now includes
initialE3Program. The contract adds no storage. The final Interfold runtime is 24,281 bytes, which passes the project release budget and leaves 295 bytes below EIP-170.The tracked Hardhat artifacts were regenerated from the final Solidity sources because the initializer ABI and imported source metadata changed. They are compiler output and were not edited manually.
Checklist
cargo test -p e3-keyshare(34 passing);pnpm --filter @interfold/contracts test:interfold(58 passing);pnpm evm:test(568 passing, 6 pending).agent/INVARIANTS.md,agent/CRATES_ARCHITECTURE.md, and the E3 request and DKG flow traces.pnpm check:invariants,pnpm --filter @interfold/contracts validate:upgrade, andpnpm --filter @interfold/contracts size:checkpass.fix!:; merge this PR only with a breaking release.Additional gates:
pnpm lint,pnpm format:check,pnpm check:docs,pnpm check:committee,pnpm check:license,pnpm check:pnpm, TypeScript compilation, and upgrade-layout validation.Summary by CodeRabbit
New Features
Bug Fixes
Documentation