fix(task-auditor): close the test-data dispute reward-pool drain (#205)

[umermjd11]

Summary

PR 4 of task_021026_19, Part E. It implements #205 options 1 + 2 as agreed in PR #209 Decision 3. Closes #205.

The bug: any address could take 25% of a GI's reward pool per batch, with no DIN and no role, and repeat it after every reassignment:

• openTestDataDispute had no caller check.
• disputeBondAmount defaulted to 0.
• resolveTestDataDispute was callable by anyone, so a junk K always mismatched the commitment and upheld the dispute.

Changes (DINTaskAuditor.sol)

	Before	After
openTestDataDispute	Anyone	Only an auditor of that batch (isBatchAuditor). Anyone else gets TA_NotAssignedAuditor, and an unknown batch gets TA_BatchDoesNotExist
disputeBondAmount default	0	100 DIN (100 * 1e18), matching DINTaskCoordinator.disputeBond. Still settable through setDisputeBondAmount; revisit with #155's values
resolveTestDataDispute	Anyone	onlyOwner. A reveal that matches the commitment clears the dispute and forfeits the bond. A non-matching owner reveal upholds it
closeExpiredDispute (owner silent past the window)	Bond forfeited	Upheld: bond returned, disputePenaltyBps penalty, batch pendingReassignment. Silence counts against the party holding the data

• The upheld logic is shared by both paths through _upholdTestDataDispute.
• Event change: DisputeExpired(gi, batchId) drops its bondForfeited field, because an expiry no longer forfeits the bond. TestDataDisputeUpheld follows it. That is the only ABI change, and dincli/abis/DINTaskAuditor.json is regenerated. Neither the subgraph (PR feat(indexer): implement DIN Protocol subgraph — platform + task-level contracts #29) nor dincli consumes this event or calls these functions.
• Left out: option 3, a separate keccak256(K) commitment, is left to P3 dispute: decentralize adjudication — owner-adjudicator trust assumption must be removed before mainnet #181/P3 slashing: conditions taxonomy, penalty tiers, slashed-stake destination, dispute resolution (P3-4.1/4.2/4.3) #38. The NatSpec states the remaining trust assumption: the owner reveals evidence about their own data, and a bad plaintext behind a correct K can't be proven on-chain.

Tests

EncryptedTestData.t.sol (18 tests):

• The tests that encoded the attack, test_resolveDispute_upheld_*, now uphold through the owner's own non-matching reveal. The disputer is now batch 0's auditor.
• New:
  • test_resolveDispute_nonOwner_reverts: OwnableUnauthorizedAccount, and the dispute stays open.
  • test_openDispute_nonBatchAuditor_reverts
  • test_defaultDisputeBond_isNonZero: 100 * 1e18.
  • test_closeExpiredDispute_ownerSilent_upholds: permissionless; the bond is returned, the pool penalised and the batch flagged; checks the DisputeExpired event.
  • test_closeExpiredDispute_beforeWindowEnds_reverts
  • test_issue205_junkKeyDrain_isClosed: the security: anyone can drain a GI reward pool through test-data disputes (unauthenticated resolveTestDataDispute, zero default bond) #205 scenario at the old zero bond. The outsider can't open, a batch auditor who opens can't resolve, and the pool and batch are untouched.

TreasuryForwarding.t.sol:

• The dispute helper opens from a batch auditor.
• The expiry test now asserts the upheld split: penalty 50% burned / 50% to treasury, and the bond refunded.

Size

DINTaskAuditor runtime goes from 22,589 B to 22,649 B (+60 B), a 1,927 B margin. The size gate passes, with the 2,048 B warning.

Verification

• forge clean && forge build && contract_size_gate.py && forge test: 489 passed, 0 failed (39 suites).
• pytest -m "not integration": 313 passed. The torch-dependent files and the 2 test_integration_marker checks can't run in my local venv, the same as on develop.
• forge lint src/DINTaskAuditor.sol: the same single pre-existing warning as on develop.
• check_doc_links.py Documentation: all links resolve.
• Docs:
  • DINTaskAuditor.md: §3 default bond, §4 access table, §10 dispute table, §13 No. 1 (fixed), and the change log.
  • DINShared.md: the TA_NotAssignedAuditor row.
• git merge-tree against fix(cli): keep auditor commit salts across retries; name aggregate-t2 work after the T2 batch (#202) #214's head is clean.

🤖 Generated with Claude Code

[umeradl]

Reviewed against develop in an isolated worktree at PR head 469b707 (1 commit, merge-base 7785acf). The branch applies cleanly on top of current develop tip 4ec42d4: git merge-tree --write-tree exits 0 locally, and GitHub reports mergeable: MERGEABLE, mergeStateStatus: CLEAN. The only commit develop gained since the merge-base touches .claude/skills only. I built on the real via_ir profile, reverted each fix on its own, and wrote one extra scenario test (below) that the PR doesn't cover.

Claimed: forge build + size gate + forge test → 489 passed, 0 failed (39 suites); DINTaskAuditor runtime 22,589 B → 22,649 B, a 1,927 B margin, gate passes with the 2,048 B warning.

Verified — exact match: forge build (via_ir = true, after npm ci) succeeded. forge test → Ran 39 test suites … 489 tests passed, 0 failed, 0 skipped. .github/scripts/contract_size_gate.py → DINTaskAuditor 22649 B, margin 1927, warn, exit 0. DINTaskCoordinator is unchanged at 23,497 B, margin 1,079.

Claimed: the tests encode the fix, i.e. they would catch a regression.

Verified — break-then-fix: I reverted each fix in DINTaskAuditor.sol on its own, rebuilt with via_ir, and ran EncryptedTestData.t.sol:

Reverted fix	Result
onlyOwner on resolveTestDataDispute	2 failed (test_resolveDispute_nonOwner_reverts, test_issue205_junkKeyDrain_isClosed)
isBatchAuditor check in openTestDataDispute	2 failed (test_openDispute_nonBatchAuditor_reverts, test_issue205_junkKeyDrain_isClosed)
closeExpiredDispute back to forfeiting the bond (old behaviour)	1 failed (test_closeExpiredDispute_ownerSilent_upholds: bond not returned)
disputeBondAmount default back to 0	1 failed (test_defaultDisputeBond_isNonZero)

After restoring: 18/18 pass.

Claimed: DisputeExpired dropping bondForfeited is the only ABI change; dincli/abis/DINTaskAuditor.json is regenerated; nothing consumes the event or these functions.

Verified: the bundled dincli/abis/DINTaskAuditor.json is identical (as a sorted entry set) to foundry/out/DINTaskAuditor.sol/DINTaskAuditor.json from my build. A repo-wide grep across *.py/*.ts/*.graphql/*.yaml (excluding build output and hardhat/) finds no consumer of DisputeExpired, openTestDataDispute, resolveTestDataDispute or closeExpiredDispute.

Claimed: pytest -m "not integration": 313 passed (venv without torch); forge lint shows only the pre-existing warning; doc links resolve.

Verified: 389 passed in a venv with torch (the gap is the torch-dependent files, as the PR says). .github/scripts/forge_lint_gate.py → 0 diagnostics at error/warning level. check_doc_links.py Documentation: all links resolve. git diff --check 7785acf 469b707 is clean. GitHub CI on the PR head: Solidity / Python / Docs / CI OK all pass.

Finding — an upheld dispute after settleRewards leaves the GI's claims underfunded

openTestDataDispute has no GI-phase or settlement check. _upholdTestDataDispute takes disputePenaltyBps of giRewardPool[gi]. However, settleRewards has already snapshotted that pool into giRewardSnapshot[gi] (clientPool/auditorPool/aggregatorPool), and claims pay from the snapshot, not from giRewardPool. So once a GI is settled, the penalty burns/forwards tokens that are already owed to claimants. Those tokens come out of the contract's commingled balance, so either the last claimants of that GI can't withdraw, or they are paid from another GI's deposits.

I wrote a scratch test on top of EncryptedTestData.t.sol's harness (not committed). It deposits 1,000 DIN for GI 1, has the coordinator call settleRewards(1, 0), then has batch 0's auditor open a dispute at the default 100 DIN bond. The owner stays silent and anyone calls closeExpiredDispute:

owed to claimants (snapshot):   950.95 DIN
contract balance after settle:  950.95 DIN
giRewardPool(1) after:          750.75 DIN
contract balance after upheld:  700.70 DIN   -> 250.25 DIN short of what the snapshot owes

This predates the PR, because the old mismatch branch did the same thing, and before this PR anyone could reach it for free. The PR narrows who can trigger it (a batch auditor, with a bond, plus owner silence or a non-matching owner reveal). But the new rule that silence upholds the dispute makes it reachable without any owner action. The dispute can be opened against any past GI, so an owner who stops watching an old GI loses by default.

Possible fixes, smallest first:

• in openTestDataDispute, revert once giRewardSnapshot[gi].settled; and
• in _upholdTestDataDispute, skip the pool penalty (or take it from the snapshot pools instead) if the GI settled while the dispute was open. A guard on open alone doesn't cover a dispute opened before settlement that is upheld after it.

Either way it is a few bytes inside the 1,927 B margin. Umer's call whether this belongs in this PR or in a follow-up issue.

Not independently re-verified: the trust assumption the PR states. The owner judges disputes about their own data, and a bad plaintext behind a correct K can't be proven on-chain. It's a design limit tracked in issue No. 181, not something to run.

---

Every checkable claim in the PR held up, and each part of the issue No. 205 fix is pinned by a test that fails when it is reverted. The original drain (any address, zero bond, junk K) is closed. The one finding above is pre-existing behaviour that this PR's silence rule makes reachable without any owner action. I'd fix it before merge if the margin allows (it does), or otherwise open a follow-up issue.

[umeradl]

Files changed (6) — as of 469b707 (PR head)

Diffed against merge-base 7785acf (develop). develop has moved 1 commit since (4ec42d4, .claude/skills only), which doesn't overlap with this PR's files. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: CLEAN. A local git merge-tree --write-tree dry run is clean (exit 0). PR No. 214 also touches Documentation/technical/contracts/DINTaskAuditor.md, but in §13 No. 9 only, while this PR edits §3/§4/§10/§13 No. 1/change log. The hunks are disjoint, as the PR's own merge-tree check against No. 214 shows.

foundry/src/DINTaskAuditor.sol

Field	Value
Change	Modified
Lines	+54/-30
Diff (what exactly is in this PR)	openTestDataDispute: new TA_BatchDoesNotExist + isBatchAuditor checks (TA_NotAssignedAuditor). disputeBondAmount defaults to 100 * 1e18. resolveTestDataDispute becomes onlyOwner. closeExpiredDispute now emits DisputeExpired(gi, batchId) (no bondForfeited) and upholds the dispute through the new internal _upholdTestDataDispute (bond back, disputePenaltyBps of giRewardPool[gi] burned/forwarded, pendingReassignment), which the mismatch branch of resolveTestDataDispute shares. NatSpec states the remaining owner-judges-own-data assumption.
Functionality — how & why	How: only an assigned auditor can stake a bond to challenge the batch's test data. Only the data holder can answer, by revealing K. A match forfeits the bond; a mismatch or silence past the window upholds. Why: issue No. 205: any address could open with a 0 bond and "resolve" with a junk K. The mismatch always upheld, so a free, repeatable 25% pool burn per batch. Each of the four changes is pinned by a test (see the verification comment's break-then-fix). Size 22,649 B (+60), margin 1,927 B, gate green (warn band).
Diff vs current develop HEAD	None — untouched by develop since merge-base
Recommended merge proposal	Merge, with one fix (pre-existing behaviour, now reachable through owner silence): an upheld dispute after settleRewards penalises giRewardPool[gi], which claims no longer read. That leaves the GI's snapshot underfunded (scratch test: 950.95 DIN owed, 700.70 DIN held). Suggested: in openTestDataDispute, revert when giRewardSnapshot[gi].settled, and in _upholdTestDataDispute, skip the penalty (or take it from the snapshot pools) if the GI settled while the dispute was open.
Actual merge proposal	Merged as-is in ac6f8f9, then fixed in 740a613 (the settlement fix, done in this merge). openTestDataDispute reverts with the new TA_RewardsAlreadySettled (in DINShared.sol) once giRewardSnapshot[gi].settled. _upholdTestDataDispute takes no pool penalty if the GI settled while the dispute was open; the bond is still returned and the batch still flagged. Size: 22,718 B runtime (+69 B), 1,858 B margin, warning band. Break-then-fix: reverting either guard fails its new test. Evidence on 740a613: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 491 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed.
Pending proposal	Post-settlement dispute penalty: fix in this PR or open a follow-up issue (Umer's call).
Local merge conflict	No
GitHub merge conflict	No

foundry/test/EncryptedTestData.t.sol

Field	Value
Change	Modified
Lines	+95/-16
Diff (what exactly is in this PR)	The disputer is now batch 0's auditor (disputer = auditors[0] in _assignBatch0). The upheld_* tests uphold through the owner's own non-matching reveal. New: test_resolveDispute_nonOwner_reverts, test_openDispute_nonBatchAuditor_reverts, test_defaultDisputeBond_isNonZero, test_closeExpiredDispute_ownerSilent_upholds, test_closeExpiredDispute_beforeWindowEnds_reverts, and test_issue205_junkKeyDrain_isClosed (the issue's scenario at a 0 bond).
Functionality — how & why	How: these drive a real platform + task pair to AuditorsBatchesCreated, assign batch 0 with a known commitment, then exercise each dispute path. Why: the old upheld tests encoded the attack (a non-owner junk reveal winning). The new set fails when any of the four fixes is reverted. 18/18 pass.
Diff vs current develop HEAD	None — untouched by develop since merge-base
Recommended merge proposal	Merge as-is. If the settlement fix goes in, add a test for it here (open/uphold after settleRewards).
Actual merge proposal	Merged as-is in ac6f8f9. 740a613 adds test_openDispute_afterSettle_reverts and test_upheldAfterSettle_skipsPenalty (bond refunded, pool untouched, contract balance covers the snapshot pools, batch flagged), and updates the header coverage notes (expiry now upholds; settlement). 20/20 in the suite. Evidence on 740a613: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 491 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed.
Pending proposal	Settlement-path test, if the source fix lands.
Local merge conflict	No
GitHub merge conflict	No

foundry/test/TreasuryForwarding.t.sol

Field	Value
Change	Modified
Lines	+12/-9
Diff (what exactly is in this PR)	The dispute helper opens from a batch auditor. The expiry test asserts the upheld split instead: the penalty goes 50% burned / 50% to the treasury, and the bond is refunded.
Functionality — how & why	How: same helpers, new expectations. Why: an expiry no longer forfeits the bond, so the old "bond forfeited 50/50" assertion would encode removed behaviour. The full suite passes (489/489).
Diff vs current develop HEAD	None — untouched by develop since merge-base
Recommended merge proposal	Merge as-is.
Actual merge proposal	Merged as-is in ac6f8f9, byte-identical to PR head. Evidence on 740a613: forge clean && forge build (via_ir) clean; size gate exit 0; forge test 491 passed / 0 failed / 0 skipped; pytest -m "not integration" 395 passed / 0 failed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

dincli/abis/DINTaskAuditor.json

Field	Value
Change	Modified
Lines	+0/-6
Diff (what exactly is in this PR)	Removes the bondForfeited input from the DisputeExpired event entry.
Functionality — how & why	How: regenerated ABI. It is identical (as a sorted entry set) to foundry/out/DINTaskAuditor.sol/DINTaskAuditor.json from my via_ir build. Why: keeps the bundled ABI in step with the contract. No dincli or other repo consumer reads this event.
Diff vs current develop HEAD	None — untouched by develop since merge-base
Recommended merge proposal	Merge as-is (regenerate again if the settlement fix changes the ABI; a guard alone won't).
Actual merge proposal	Merged as-is in ac6f8f9, then regenerated in 740a613 with dump-abi --official from the merged build. The only diff is the new TA_RewardsAlreadySettled error entry.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

Documentation/technical/contracts/DINTaskAuditor.md, Documentation/technical/contracts/DINShared.md

Field	Value
Change	Modified (2 files)
Lines	+8/-6, +1/-1
Diff (what exactly is in this PR)	DINTaskAuditor.md: §3 default bond 100 DIN; §4 access table (new "Auditor of the disputed batch" row; openTestDataDispute/resolveTestDataDispute removed from "Any address"); §10 dispute table rewritten for owner resolve + silence upholds; §13 No. 1 marked Fixed, with the trust assumption and issue No. 181; change-log line. DINShared.md: TA_NotAssignedAuditor row now covers openTestDataDispute.
Functionality — how & why	How: documentation only. Why: it keeps the contract docs describing develop. Link check: all resolve. No other doc under Documentation/ describes the old anyone-resolves or bond-forfeited-on-expiry behaviour (grep).
Diff vs current develop HEAD	None. PR No. 214 edits DINTaskAuditor.md §13 No. 9 only, which is disjoint from these hunks.
Recommended merge proposal	Merge as-is. If the settlement fix lands, add a line to §10/§13 noting that disputes close at settlement.
Actual merge proposal	Merged as-is in ac6f8f9; DINTaskAuditor.md auto-merged with PR No. 214's §13 No. 9 hunk. 740a613: §10 table (settled-GI guard, no penalty after settlement), a stale §10 line that still said anyone can open and win a dispute is replaced, a change-log line, and a TA_RewardsAlreadySettled row in DINShared.md. check_doc_links.py Documentation: 258 links, all resolve.
Pending proposal	Doc line for the settlement fix, if it lands.
Local merge conflict	No
GitHub merge conflict	No

Verification

Full detail is in the verification comment above. In short: forge build (via_ir) passes. forge test gives 489/489 in 39 suites. The size gate is green (DINTaskAuditor 22,649 B, margin 1,927 B, warn band). Break-then-fix: 4 of 4 reverted fixes are caught. The bundled ABI matches the build output. pytest -m "not integration" gives 389 passed. The lint gate is clean, doc links resolve, git diff --check is clean, and GitHub CI is green. One finding: an upheld dispute after settleRewards underfunds the GI's claims (pre-existing, but now reachable through owner silence).

Local vs. GitHub agree: both report a clean merge; no file overlap with the 1 commit develop gained since the merge-base.

[umeradl]

Actual outcome — PR No. 215 merged + settlement fix applied (pushed)

This supersedes the pre-merge review comment above with what actually happened when landing this PR on develop.

Two commits, both on origin/develop:

1. ac6f8f9 — real merge of this PR (merge commit, not squash) at its head 469b707, authored as umermjd11, on top of develop e491f0b (PR No. 214). No conflicts, as predicted: DINTaskAuditor.md auto-merged with PR No. 214's disjoint §13 No. 9 hunk. GitHub agrees: PR shows MERGED.
2. 740a613 — follow-up commit fixing the review finding (an upheld dispute after settleRewards underfunds the GI's claims), done in this merge instead of a follow-up issue.

Files unchanged from the PR (1 of 6)

• foundry/test/TreasuryForwarding.t.sol is byte-identical to 469b707.

Files changed by 740a613 (5 of 6, plus 1 file outside the PR)

File	What changed vs. this PR's merged version
foundry/src/DINTaskAuditor.sol	openTestDataDispute reverts with TA_RewardsAlreadySettled once giRewardSnapshot[gi].settled. _upholdTestDataDispute sets the penalty to 0 if the GI settled while the dispute was open. The bond is still returned, the batch still flagged pendingReassignment, and TestDataDisputeUpheld still emitted (with ownerPenalty 0). NatSpec updated.
foundry/src/DINShared.sol (outside the PR)	New error TA_RewardsAlreadySettled().
foundry/test/EncryptedTestData.t.sol	New test_openDispute_afterSettle_reverts. New test_upheldAfterSettle_skipsPenalty: open, settle, owner silent, expire. It asserts the bond is refunded, giRewardPool is untouched, the contract balance covers the snapshot's client + auditor + aggregator pools, and the batch is flagged. Header coverage notes updated.
dincli/abis/DINTaskAuditor.json	Regenerated with dump-abi --official; the only diff is the new error entry.
Documentation/technical/contracts/DINTaskAuditor.md	§10 table: the settled-GI guard on open, and no penalty after settlement. The §10 line "anyone can open one … any caller can trigger the upheld branch with a wrong K" was stale after this PR; it's replaced with the settlement rule. Change-log line added.
Documentation/technical/contracts/DINShared.md	TA_RewardsAlreadySettled row.

Why the change

Not develop drift. It's the review finding: claims pay from giRewardSnapshot[gi], so a giRewardPool[gi] penalty after settleRewards took tokens claimants were already owed (scratch test: 950.95 DIN owed, 700.70 DIN held). The bug predates this PR, but the PR's silence-upholds rule made it reachable without any owner action. The open guard alone doesn't cover a dispute opened before settlement and upheld after it, hence the second check in _upholdTestDataDispute.

Verification

On the pushed tree, 740a613 (PR No. 214 + this PR + the fix):

• forge clean && forge build (via_ir) is clean.
• contract_size_gate.py exits 0. DINTaskAuditor is 22,718 B (+69 B over the PR, 1,858 B margin) and DINTaskCoordinator is unchanged at 23,497 B (1,079 B); both are in the warning band.
• forge test: 491 passed, 0 failed (the PR's 489 + 2 new).
• Break-then-fix: reverting the open guard fails test_openDispute_afterSettle_reverts; reverting the penalty skip fails test_upheldAfterSettle_skipsPenalty.
• pytest -m "not integration": 395 passed, 0 failed.
• forge_lint_gate.py: 0 diagnostics at error/warning level. Doc links in Documentation (258) all resolve. git diff --check is clean.
• The local CI mirror on 740a613 passed: docs, pytest, via_ir build, forge test and hardhat.
• GitHub push run on develop: green (Docs, Python, Solidity incl. the contract size gate, CI OK).

Local vs. GitHub agree: clean merge with no conflicts, as both predicted. Issue No. 205 is fixed by this PR plus 740a613.