fix(access-control): replace ownPublicKey() auth with witness-derived keypair pattern#1
Conversation
…-derived keypair `ownPublicKey()` returns a value the prover claims to know, not a proof of key ownership. Storing it on the ledger and asserting `ownPublicKey() == storedAdmin` lets any attacker read the stored value off-chain, supply it as their own `ownPublicKey()`, and produce a valid proof. The access control is non-existent. This rewrites the Access Control page around the witness-derived keypair pattern: the ledger stores `H(domain || secret)`; the caller must provide a witness `sk` such that `H(domain || sk)` matches. Hash preimage resistance makes the on-chain value useless to an attacker. The page now also includes an explicit anti-pattern section, a "when to use ownPublicKey()" section for identifier-as-target uses, a role-based variant, and a pointer to signature and commitment/nullifier patterns. All Compact blocks compile against language version 0.22.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR comprehensively rewrites the access control documentation page to teach witness-derived keypair authorization patterns. It replaces unsafe ChangesAccess Control Authorization Patterns
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Summary
Replace the
ownPublicKey()-as-authorization pattern on the Access Control page with the witness-derived keypair pattern, and document the original pattern explicitly as broken.Why this matters
ownPublicKey()returns a value the prover claims to know — it is not a proof of key ownership. The pattern previously taught on this page:…is bypassable by anyone. An attacker reads
adminfrom the public ledger, passes it back as their ownownPublicKey()claim, and produces a mathematically valid proof. The proof correctly demonstrates "the prover knows a value equal toadmin" — which is exactly what every chain observer also knows.Solidity gets caller authentication implicitly from
msg.sender+ EVM signature verification. Compact has no direct analogue; you have to construct the authorization check yourself.What this PR does
<Warning>banner at the top of the page calling out the vulnerability.H(domain || secret); the witness supplies the secret; the assertion verifies the hash matches. The on-chain value is preimage-resistant, so reading the ledger gives an attacker nothing replayable. The domain string scopes the key to the contract.ownPublicKey()is fine as a target identifier (mint(ownPublicKey(), tokenId),recipient = disclose(ownPublicKey())); it is only dangerous when used inassert(ownPublicKey() == ...).---le: "Access Control"block.Other instances of the same anti-pattern (NOT fixed here)
Per task scope, these are flagged but left for follow-up:
ownPublicKey()asfromin_transfer(from, to, amount). The page's "Implicit authorization" claim (lines 91–97) is incorrect — an attacker can spooffromto transfer from any account._approve(owner = ownPublicKey(), spender, amount)lets anyone approve spends from any owner._spendAllowance(from, spender = ownPublicKey(), amount)— the spender check is the same anti-pattern.ownPublicKey()foraccount— attackers can burn anyone's tokens.assert(from == caller || isApprovedForAll(from, caller))pattern withcaller = ownPublicKey()— trivially bypassable.All of these share the same root cause: treating
ownPublicKey()asmsg.sender. They should be migrated to the witness-derived keypair pattern (or, for transfer/approve flows, to a balance-bound version of it). Tracking separately so this PR stays scoped to the page security review is most likely to land on first.Test plan
compactcode blocks in the new file compile cleanly under language version 0.22 (compactc 0.30.0 --skip-zk).<Warning>,<Accordion>,<Card>,<CardGroup>already used elsewhere on the site.---le:block removed).Summary by CodeRabbit