security: anyone can drain a GI reward pool through test-data disputes (unauthenticated resolveTestDataDispute, zero default bond)

[umeradl]

Summary

Found while reviewing PR No. 204 (verification comment, surfaced item A). This is older than PR No. 204 and out of its scope. PR No. 204 records it as a caveat in DINTaskAuditor.md §13 No. 1, but nothing tracks a fix. Line numbers are for develop @ e373c8d.

Any address can take 25% of a GI's reward pool per batch through a test-data dispute. It needs no DIN, no stake, and no role in the batch. The attack can be repeated after every reassignment, and each round also blocks the batch.

Problem

foundry/src/DINTaskAuditor.sol:

• openTestDataDispute(gi, batchId) (L1461) has no caller check. It only needs a stored commitment, no active dispute, and no pending reassignment. It pulls disputeBondAmount, which is 0 at deploy (L283).
• resolveTestDataDispute(gi, batchId, K, plaintextHash) (L1495) is also callable by anyone. Its NatSpec says so: "Anyone may call this — the disputer is the beneficiary if upheld". It recomputes keccak256(gi, batchId, keccak256(K), plaintextHash) and compares it with the owner's commitment:
  • match (L1510) → dispute false, bond forfeited;
  • mismatch → dispute upheld: bond returned, disputePenaltyBps (2500 = 25%, L285) of giRewardPool[gi] burned or forwarded via _burnAndForward (L1527), and the batch set to pendingReassignment.

A mismatch can't tell "the owner committed bad test data" apart from "the caller passed a made-up K". K and plaintextHash are both caller-supplied and neither is checked independently, so the caller controls which branch runs.

Failure scenario

Proven with a throwaway forge test that extends EncryptedTestDataTest. It was run and deleted, not committed.

1. The model owner assigns batch 0's test data with a valid commitment (assignAuditTestDataset).
2. outsider has no DIN, isn't a batch auditor, and the bond is the default 0. They call openTestDataDispute(1, 0), then resolveTestDataDispute(1, 0, junkK, 0x0).
3. The dispute is upheld: giRewardPool(1) drops 25%, and batch 0 is pendingReassignment until the owner calls reassignAuditTestDataset.
4. The outsider repeats after each reassignment.

After 3 rounds the pool is at 0.75³ ≈ 42% of its starting value (asserted below 50%), and token.balanceOf(outsider) == 0 throughout. The existing suite already has this shape as intended behavior: test_resolveDispute_upheld_returnsBondAndPenalises has the disputer resolve their own dispute with wrongK and win.

Fix

This needs a small design decision first. Options:

1. Only the owner's reveal can resolve. Make resolveTestDataDispute owner-only: the owner reveals the true K and plaintext hash, and a match clears the dispute. If the owner doesn't reveal within disputeWindowBlocks, closeExpiredDispute upholds the dispute (bond back, penalty, reassignment) instead of forfeiting the bond. This reverses today's default: silence counts against the party who holds the data.
2. Restrict who can open. Only an auditor of that batch (isBatchAuditor) can open a dispute, and disputeBondAmount gets a non-zero default so each attempt costs something.
3. Separate the key check from the content check. Commit keccak256(K) on its own, so an on-chain check can prove a revealed K is the real key. A content mismatch (a bad plaintext) still can't be proven on-chain without the data, so it needs either option 1's "owner must reveal" rule or off-chain adjudication (see No. 38 on dispute resolution).

Option 1 + option 2 closes the free-griefing path without new cryptography. Tests to add or change:

• a non-owner resolve reverts, or can't reach the "upheld" branch;
• an owner reveal with the true K clears the dispute;
• an owner who stays silent past the window loses (upheld);
• a non-batch-auditor open reverts;
• update test_resolveDispute_upheld_*, which currently encode the attack.

Size note: this contract has headroom, unlike DINTaskCoordinator (No. 201 Part A).

Related

No. 38 (dispute resolution, P3-4.3), No. 115 (encrypted test-data key assignment), No. 192 (auditor commit hash), PR No. 204.

[umeradl]

Done by PR No. 215, merged into develop as ac6f8f9, with the review fix in 740a613 (outcome comment, green develop push run). Closing by hand, because a merge into develop doesn't trigger closing keywords (main is the default branch).

Checked on origin/develop @ 740a613, against options 1 + 2 as agreed in PR No. 209 Decision 3 (task_021026_19 Part E):

• Option 1, only the owner's reveal resolves: resolveTestDataDispute is onlyOwner (foundry/src/DINTaskAuditor.sol:1524). A matching reveal clears the dispute and forfeits the bond; a non-matching one upholds it (:1546). closeExpiredDispute upholds an unanswered dispute through the shared _upholdTestDataDispute (:1557-1563, :1571): bond back, disputePenaltyBps penalty, batch pendingReassignment.
• Option 2, restrict who can open: openTestDataDispute requires isBatchAuditor[gi][batchId][msg.sender] (:1485, TA_NotAssignedAuditor). disputeBondAmount defaults to 100 * 1e18 (:288) and stays settable through setDisputeBondAmount.
• Review fix (740a613): a dispute upheld after settleRewards used to take its penalty from tokens claimants were already owed. Now openTestDataDispute reverts with TA_RewardsAlreadySettled once the GI is settled (:1486), and _upholdTestDataDispute takes no pool penalty if the GI settled while the dispute was open (:1581).
• Tests (foundry/test/EncryptedTestData.t.sol): every test the issue asks for:
  • test_resolveDispute_nonOwner_reverts
  • test_resolveDispute_falseDispute_forfeitsBond: an owner reveal with the true K clears the dispute.
  • test_closeExpiredDispute_ownerSilent_upholds
  • test_openDispute_nonBatchAuditor_reverts
  • test_defaultDisputeBond_isNonZero
  • The rewritten test_resolveDispute_upheld_* now uphold through the owner's own non-matching reveal.
  • test_issue205_junkKeyDrain_isClosed: the failure scenario at the old 0 bond. The outsider can't open at all, so the repeat-after-reassignment loop can't start; a batch auditor who opens can't resolve; the pool is untouched.
  • Plus test_openDispute_afterSettle_reverts and test_upheldAfterSettle_skipsPenalty from 740a613.
  • In the PR No. 215 review and the merge, reverting any single fix fails its test.
• Verification: forge test 491 passed, 0 failed. pytest -m "not integration" 395 passed. The DINTaskAuditor size gate is green at 22,718 B (1,858 B margin). No dincli command calls these functions, and the bundled ABI is regenerated.
• Docs: DINTaskAuditor.md §13 No. 1 is marked fixed and states the remaining trust assumption. §3, §4, §10 and the change log are updated, and DINShared.md covers both errors.

Out of scope: option 3 (a separate keccak256(K) commitment) and the owner-judges-own-data trust assumption are tracked in issue No. 181 and issue No. 38, both open.