Add PrivacyHook#15
Conversation
|
Hey dropping some questions!
Critical question: If the key holder service goes offline, can the deliverable still be decrypted? If not, this becomes a single point of failure for job completion.
Request: Document the "no-opinion" mode explicitly. If the dependency isn't truly optional, we need to discuss interface extraction.
Then PrivacyHook uses any IPrivacyVerifier implementation — ThoughtProof, Lit Protocol, decentralized KMS, etc. |
|
Hey Miratisu, thanks for the detailed questions. Really appreciate the thoroughness. 1. Key Management & Service Availability No centralized service or middleman. Participants hold their own secp256k1 keys (same ones used to There's no key management service. Client holds their key, evaluator holds theirs. Nothing in between, 2. ThoughtProof Dependency I think there might be a mix-up here. This implementation doesn't have a ThoughtProof dependency. The
3. Forward Secrecy Every submission uses a fresh ephemeral key pair. The provider generates it, wraps the AES key for This means even if an attacker compromises the provider's infrastructure after the fact, they can't For recipient key rotation, it's straightforward. A participant starts using a new key for new jobs. 4. Gas Economics Ciphertext never touches the chain. The on-chain footprint is just the CID hash (32B), wrapped keys 5. Interface Neutrality Agreed on keeping things vendor-agnostic. On-chain encrypt/decrypt isn't practical gas-wise, which is Happy to adjust anything based on your feedback. |
|
I think this is a good privacy-pattern hook and it is close. It feels like a real example hook rather than a bundled system. Before merge, could you please rebase on the current shared plumbing fixes, add also if it has been auditted and keep the PR description and docs template-like rather than product-style. Once that cleanup is in, I think this is one of the cleaner examples to merge. |
acf3fd6 to
30a94b0
Compare
@miratisu_ps Thanks for the continued review and guidance on this PR. As an observer following the ERC-8183 standardization effort, I have a couple of questions:
Appreciate all the work going into making this a clean, composable standard. Looking forward to seeing how privacy extensions evolve alongside the core job primitive. |
|
Thanks @psmiratisu, appreciate the detailed review. Addressed all three points:
Let me know if there's anything else you'd like changed. |
|
Hi @PRXVT,
|
|
Thanks @ariessa. Both points addressed. Rebase MultiHookRouter support
|
ariessa
left a comment
There was a problem hiding this comment.
The PR is more polished now. Please address all requested changes.
The blockers are all in the trust model rather than the code mechanics. As written, a provider can quietly weaken the privacy guarantees the hook advertises: race the client to lock the config, pick a verifier the hook does not actually vet, and exploit the empty requiredSelectors to brick jobs registered through MultiHookRouter. Together these turn "privacy-enforcing hook" into "envelope shape checker that the provider controls."
I've left inline comments with suggestion blocks where the fix is mechanical, and a description for the verifier whitelist (it needs Ownable + an admin setter, so it doesn't fit a one-line suggestion).
|
Thanks @ariessa. All addressed. The three things from your summary:
Smaller stuff:
Constructor takes Let me know if you want anything else tweaked. |
ariessa
left a comment
There was a problem hiding this comment.
One last thing before this hook can be merged.
https://github.com/erc-8183/hook-contracts/pull/15/changes#r3193694787
|
Thanks @ariessa. Removed the _postComplete and _postReject overrides so envelopeCommitments and Also rebased on current main and removed the zero-address check in the constructor since BaseERC8183Hook covers it. |
|
Hi @PRXVT, These four items block merge. Approve once all four land. A blocker here is one of: the hook does not behave as its own docs say (1), it silently narrows a public core flow without telling the caller (2), a typed field disagrees with the documented contract (3), or the code will not compile against current main and violates the hook style rules (4). Items in the non-blocking section at the bottom are tradeoffs you have already opted into or hardening that does not change shipped behavior. Record them, but they do not gate this PR.
Non-blocking. None of these gate merge. Each one is either a tradeoff the design already takes on or a hardening step that does not change shipped behavior. Address in this PR if convenient, otherwise file follow-ups.
|
|
Thanks @ariessa, took another pass and got all four blockers plus the non-blockers in. Blockers:
Non-blockers:
|
ariessa
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Merging this PR as it passes all requirements 🎉
… error rename
- WalletStateHook.sol: convert to named imports (`{BaseERC8183Hook}`, `{IERC8183HookMetadata}`) per PR erc-8183#45 hook PR rules.
- WalletStateHook.t.sol: update test_Constructor_RevertsOnZeroCore to expect `BaseERC8183Hook.InvalidERC8183Contract` (per upstream BaseERC8183Hook zero-core revert added in `0179241`).
Rebase resolves the README hook-table conflict from PR erc-8183#15 (PrivacyHook) by keeping both rows. forge test: 12/12 passing.
Profile C hook. Providers submit deliverables as encrypted envelopes instead of plaintext. The hook validates
envelope structure on-chain and optionally runs a ZK proof over the encrypted contents.
Mechanism
Off-chain, the provider AES-256-GCM encrypts the deliverable and uploads the ciphertext to IPFS. Only the CID hits the
chain.
The AES key is ECDH-wrapped per recipient (client, evaluator, and so on) using secp256k1, so participants use the same
keys they already sign transactions with. Ephemeral keys are fresh per submission and discarded after.
setBudget.optParamscarries the per-job privacy config:(zkVerifier, minWrappedKeys), or(zkVerifier, minWrappedKeys, numPublicInputs)when the circuit needs more than 2 public inputs. Config is immutable once set.submit.optParamscarries(cid, wrappedKeys, zkProof). The hook checks that the CID matches the submit'sdeliverable hash, validates wrapped-key count and v1 format (94 bytes), and calls
IZKVerifier.verifyif one wasconfigured. Setting
zkVerifier = address(0)at config skips ZK and enforces envelope shape only.Key properties
Audit status
Self-reviewed for hook auth, envelope validation, commitment handling, and reentrancy via a
malicious verifier. No findings. Unaudited by a third party.
Answers to earlier review questions
Participants hold their own secp256k1 keys, no registry or third-party service. Ephemeral keys are per-submission and
zeroized after use, so past deliverables stay safe even if a long-term key is later compromised. ZK verification is
fully optional (
address(0)disables it).