Skip to content

fix(subgraph,contracts): enforce write ownership via sender-recording DataEdge - #676

Open
tmigone wants to merge 1 commit into
docs/key-derivation-and-stale-commentsfrom
audit/high-fact-write-authorization
Open

tmigone wants to merge 1 commit into
docs/key-derivation-and-stale-commentsfrom
audit/high-fact-write-authorization

Conversation

@tmigone

@tmigone tmigone commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

What

Anyone can currently overwrite, tombstone or reassign any user's memories: take a fact id from the public subgraph, call the DataEdge contract directly with that id and someone else's address, and the subgraph accepts it — the DataEdge contract emits only the payload and not the caller, so ownership is whatever the payload claims.

This PR adds EventfulDataEdgeV2, which emits msg.sender, and a subgraph that takes each fact's owner from the sender and rejects writes to facts owned by someone else. A second legacy data source keeps existing facts readable and unchanged; clients need no change.

Warning

Not deployed. The V2 contract addresses, start blocks, and the legacy endBlock are placeholders; the subgraph protects nothing until the contract is deployed and the staging exploit-replay E2E passes.

Verification

The vulnerability was reproduced on staging (Gnosis chain 100), DataEdge 0xE7a4D2677B686e13775Ba9092631089e35F0BB91, fact id audit-11-bb3eadb2:

  • victim (0x24039B90C7Dacda99e8935C7C423112dCedDe7C3) wrote the fact — tx 0xeac1688cb12475b7b876cb3108a71d008fa8d93c7a15538fd0ad13da8304da55
  • attacker (0xE8020eb0fa58828C35d33242FBf1E1AC8c36dC8E, unrelated wallet) tombstoned it, isActive → false — tx 0x3a8f551f27e38ddfa67255b2a233d62bb325744402f93471664a7ecf59519c46
  • same attacker reassigned its owner to itself — tx 0x73ee61d9fc2da7bdab1fe980a62cb44e0a035393f6cfdcd9018c76d0669be2a2

Other checks:

  • contracts: 41 forge tests pass, including 5 news (sender == caller, distinct senders for the samepayload, permissionless append, receive() emits nothing).
  • subgraph: graph codegen + graph build pass for both the mainnet and staging manifests (two data sources into one entity
    store).
  • Pending (deploy-gated): write→index→reanning the exploit above against V2 to confirm thecross-owner write is rejected. Code-complete but not validated end-to-end until this runs.

Checklist

  • No recovery phrase, API key, or other secret appears anywhere in this diff or its logs.
  • Docs touched by this change are updated (guides / feature-comparison / CLAUDE.md matrix where relevant). (N/A — no user-facing surface changes; the DataEdge contract family in CLAUDE.md should be updated when V2 is actually deployed.)
  • For behavior changes: E2E validated against staging (never production). (Pending the deploy)

… DataEdge

Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
@tmigone
tmigone added this pull request to stack #675 October 15, 2026 19:03
@p-diogo

p-diogo commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks @tmigone, confirmed on our side, and a really clear write-up. Here's the plan to get this merged and shipped, so it lives in one place. This stays your PR. I'll review and handle the deploy steps that need infra access.

Production impact: we checked every write ever made to the production DataEdge (1,321 logs, the contract's full history). Each one came through the EntryPoint from the account named as the owner, with no cross-owner writes. Production is also still pre-launch, so everything on-chain there is our own test data.

That simplifies this a lot. We don't need to carry existing facts over, so V2 can be a clean break: a new subgraph that indexes only V2 from its deploy block, with no legacy data source and no endBlock freeze.

Before merge (you)

  • Rebase on main (conflicts with chore: point repo references at the graphprotocol org #677).
  • Drop the legacy data source and handleLog. The manifests carry only V2.
  • Id squatting. V2 is first-writer-wins on a client-chosen id, and the id is visible in the mempool before the write lands. Someone can front-run a create with the same id from their own address. The owner's write is then rejected, so the fact never indexes and the client can't tell. Keying the entity by (sender, id) removes cross-owner collisions altogether, which leaves nothing to authorize. One catch: clients pass fact.id back as the payload id when they tombstone, so the mapping would also need to accept an id that's already prefixed with the sender's own address. Open to other approaches.
  • Matchstick tests covering:
    • owner = sender
    • a write from another account can't create, update, tombstone or reassign a fact
    • same-owner update and tombstone work
    • batched (executeBatch) writes index under the Smart Account
  • Deploy script: fail on an unrecognised target instead of falling back to the staging salt.
  • Regenerate .gas-snapshot (it still carries 23 SessionKeyModuleTest entries that chore: remove deprecated contracts and dead code #673 removes), and make the expectEmit assertions check the sender topic.
  • Manifests: real staging values after deploy step 1, and prod values in the prod deploy step, not as placeholders on main.

Deploy sequence (me, after merge)

  1. Deploy V2 and the new subgraph to staging.
  2. Re-run your staging repro against V2 as the E2E. Cross-owner writes should be rejected, and normal client writes (single and batched) should index and recall.
  3. Point the staging relay at V2 and run the batch E2E. Chain-aware clients pick up the new address from /v1/billing/status, so no release is needed.
  4. Production: deploy V2, publish the subgraph to the network, and repoint the relay once the Gateway serves it at head. Existing production facts drop out of the product at this point, intentionally.
  5. Update the clients' hardcoded fallback address in their next releases. Until then, two kinds of writes land on the old contract and won't show up: writes from a client that can't reach billing, and all ZeroClaw writes, since it always uses the constant.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants