fix(task-auditor): bind the auditor and slot into the audit commit hash (#192)

[umermjd11]

Summary

PR 2 of task_021026_19, Part C. It implements amendments 1–6 of the #192 approval review. Closes #192.

The attack: revealAuditScore checked keccak256(abi.encodePacked(score, vote, salt)), which carries no address, GI, batch or model. An assigned auditor could copy a peer's commit hash, which is public, wait for the peer's reveal, and replay it. That earned a quorum vote and reward weight with no evaluation.

The fix: the hash is now keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)), matching the aggregation side (#156 M-1). Function signatures don't change, and the bundled dincli/abis/DINTaskAuditor.json is unchanged: I regenerated it and the diff was empty.

Changes by amendment

1. Contract (DINTaskAuditor.sol): the new formula in revealAuditScore. The NatSpec that describes the formula is updated at the state comment, at the commitAuditScore @dev, and at its @param.
2. dincli: a new _audit_commit_hash(score, vote, salt, sender, gi, batch_id, model_index) in dincli/cli/auditor.py. It uses eth_abi.encode + Web3.keccak, modelled on _agg_commit_hash, and evaluate --submit now calls it. reveal_lms sends the stored values and is unchanged.
3. Per-auditor hashing in tests:
   • The formula lives in one shared helper, foundry/test/utils/AuditCommitHash.sol, as auditCommitHash(...).
   • All 25 commitAuditScore call sites in 11 files now hash per pranked auditor. Before, most built one hash and reused it across the auditor loop.
   • The files are AggregatorCommitReveal, AuditorCommitReveal, DisputeResolution, GasSimulation, LifecycleEvents, PR146SlashingRegression, RewardEngine, ScoringValidation, SecurityFindings, StakingEnforcement and TreasuryForwarding.
4. Regression tests in AuditorCommitReveal.t.sol:
   • test_copyAttack_replayingPeerCommitAndReveal_reverts: B commits A's hash and reveals A's (score, vote, salt). It reverts with TA_RevealHashMismatch, and B gets no vote.
   • test_commitHashBoundToSlot_otherModelBatchOrGI_reverts: a hash built for another model, batch or GI doesn't reveal.
   • test_honestPath_boundHash_allRevealAndClose: every auditor reveals its own hash, and evaluation closes with the expected score.
   • pytest: tests/test_auditor_commit_hash.py pins two golden vectors computed independently with cast abi-encode and cast keccak, and checks that the hash varies with the sender and with the slot.
5. Docs:
   • DINTaskAuditor.md: §7, §13 No. 2 marked fixed, and a change-log entry.
   • DINShared.md: the TA_RevealHashMismatch row and the commit-reveal section.
   • The security review's M-1 note.
6. Size: DINTaskAuditor runtime goes from 22,567 B to 22,589 B (+22 B), a 1,987 B margin. That clears Contract size headroom for all contracts (DINTaskCoordinator −9 B over EIP-170 with PR No. 197) + slash reason for committed-but-unrevealed across commit-reveal flows #201's 1,024 B budget but sits in the 2,048 B warning band.

hardhat/contracts/DINTaskAuditor.sol (the stale mirror) is left alone, as the approval review allows.

Relation to #211

• This PR branches from develop, independent of fix(task-coordinator): back under EIP-170 with a CI size gate (#201 Part A) + onlyCurrentGI on registerDINaggregator (#206) #211, and git merge-tree against fix(task-coordinator): back under EIP-170 with a CI size gate (#201 Part A) + onlyCurrentGI on registerDINaggregator (#206) #211's head is clean.
• DINTaskCoordinator here is still develop's 24,585 B; fix(task-coordinator): back under EIP-170 with a CI size gate (#201 Part A) + onlyCurrentGI on registerDINaggregator (#206) #211 fixes that. This PR doesn't touch the coordinator.
• The size gate lands with fix(task-coordinator): back under EIP-170 with a CI size gate (#201 Part A) + onlyCurrentGI on registerDINaggregator (#206) #211. Once both are in, the gate will report DINTaskAuditor at 1,987 B.

Migration note

Any commit made under the old formula can't be revealed after this lands. That isn't a live concern, because DINTaskAuditor from develop isn't deployed on any persistent network.

Verification

• forge clean && forge build && forge test: 479 passed, 0 failed (39 suites).
• pytest -m "not integration": 313 passed, including the 4 new tests. The torch-dependent files and the 2 test_integration_marker checks that collect them can't run in my local venv (no torch), the same as on develop. CI installs the full deps.
• forge lint src/DINTaskAuditor.sol: 1 warning, the same pre-existing divide-before-multiply as on develop.
• check_doc_links.py Documentation: all links resolve.

🤖 Generated with Claude Code

[umeradl]

Reviewed against develop in an isolated worktree. The branch is cut from the current tip, 8c0af09, so the merge-base is the tip itself and develop hasn't moved since. It has 1 commit (210e359) and no conflicts: git merge-tree --write-tree exits 0, and GitHub reports MERGEABLE / CLEAN. Everything below ran on the real via_ir = true profile (forge 1.7.1).

Claimed: revealAuditScore now checks keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)), matching the aggregation side, with no signature or ABI change.

Verified:

• revealAuditScore's signature is unchanged (uint256 gi, uint batchId, uint modelIndex, uint256 score, bool vote, bytes32 salt), and commitAuditScore's too.
• The only code change is the expectedHash line plus three NatSpec/state comments.
• The PR changes no file under dincli/abis/.
• A repo-wide grep for the old formula (encodePacked(score…, solidity_keccak(["uint256","bool",…) outside the stale hardhat mirror finds only historical task/design docs and prose comments. No live code or Documentation/ page still describes the old hash.

Claimed: the dincli hash _audit_commit_hash matches the contract, pinned by two golden vectors computed independently with cast.

Verified, exact, plus break-then-fix:

• I recomputed both vectors with cast keccak $(cast abi-encode "f(uint256,bool,bytes32,address,uint256,uint256,uint256)" …). They give 0xa2683a58…c8c97c and 0x82fd3121…a5a127, identical to the test's constants. Vector 1 under the old packed formula gives 0xa3b33607…bac2fc, so the vectors do tell the formulas apart.
• Two mutations of _audit_commit_hash, each run against the test file and then reverted:
  • swapping gi/batch_id in the encode list fails 2 of 4 tests (both golden vectors);
  • replacing abi_encode + keccak with Web3.solidity_keccak (packed) also fails 2 of 4.
• eth-abi>=4.0.0 is already declared in pyproject.toml, and aggregator.py already imports it the same way.
• The evaluate --submit call site passes account.address, curr_GI, batch_id, model_index. Those are the same values it passes to commitAuditScore on the next line, so the hash and the commit slot can't drift apart.

Claimed: all 25 commitAuditScore call sites in 11 test files now hash per pranked auditor.

Verified:

• grep -rn "commitAuditScore(" foundry/test finds 29 call lines in 11 files: the 25 rewritten sites plus 4 in the new regression tests (1 copy-attack, 3 slot-mismatch).
• I read every one. Each of the 25 passes the same auditor variable to auditCommitHash that it vm.pranks, and the same (gi, batchId, modelIndex) it commits to. None reuses one hash across an auditor loop any more.

Claimed: the new regression tests catch the copy attack and cross-slot reuse.

Verified, break-then-fix: In both revealAuditScore and the shared auditCommitHash test helper, I replaced msg.sender/auditor with address(0) and modelIndex with uint256(0). This keeps the honest fixtures valid and removes the sender and model bindings. After a via_ir rebuild, AuditorCommitRevealTest gives 13 passed, 2 failed. The failures are exactly test_copyAttack_replayingPeerCommitAndReveal_reverts and test_commitHashBoundToSlot_otherModelBatchOrGI_reverts, both with "next call did not revert as expected": the copier's replay succeeds, and so does the wrong-model reveal. test_honestPath_boundHash_allRevealAndClose still passes. With the source restored, all pass in the full 479-test run.

Claimed: DINTaskAuditor runtime 22,567 → 22,589 B (+22 B, 1,987 B margin).

Verified, exact: I ran PR No. 211's contract_size_gate.py against this branch's via_ir build. It reports DINTaskAuditor runtime 22589 B leaves only 1987 B with initcode 23,178 B. It also reports DINTaskCoordinator at 24,585 B (−9). That is develop's own overrun, which this PR doesn't touch and PR No. 211 fixes.

Claimed: 479 forge tests pass, and pytest passes (313 without torch).

Verified: forge test: 479 passed, 0 failed, 39 suites. pytest -m "not integration" in a venv with torch: 389 passed, 133 deselected, 0 failed. That's develop's 385 plus the 4 new tests, and includes the torch-dependent files. CI's Python, Solidity and Docs jobs are green.

Beyond the PR — combined with PR No. 211: both PRs touch AggregatorCommitReveal.t.sol and StakingEnforcement.t.sol. Git merges them without conflict, but a clean textual merge doesn't prove the two sets of test edits work together. So in a throwaway worktree I merged PR No. 211's head into this branch (--no-commit, nothing pushed) and ran the full via_ir build, the size gate and forge test: the build succeeds. The size gate exits 0 with both task contracts in the warning band (DINTaskCoordinator 23,497 B / 1,079 B margin, DINTaskAuditor 22,589 B / 1,987 B margin). forge test gives 484 passed, 0 failed, 39 suites, i.e. develop's 476 plus 5 from PR No. 211 plus 3 from this PR. Either merge order is safe.

Not independently re-verified: forge lint src/DINTaskAuditor.sol (1 pre-existing warning) and the local check_doc_links.py run. CI's Docs job, which runs the link check, is green.

Minor, optional: DINTaskAuditor.md writes issue #192 in §7 and §13 No. 2, but issue No. 192 in the Change Log entry. The rest of that file uses the "No." form (No. 201, No. 205).

---

Every checkable claim held up. The contract fix is minimal and mirrors the coordinator's sender-bound hash. The dincli hash matches the contract byte-for-byte against cast. The regression tests fail when either binding is removed, and the PR combines cleanly with PR No. 211. Looks good to merge as-is.

[umeradl]

Files changed (18) — as of 210e359 (PR head)

Diffed against merge-base 8c0af09 (develop). That is the current develop tip, so develop hasn't moved since the branch was cut and none of the 18 files overlap with new develop work. GitHub agrees: mergeable: MERGEABLE, mergeStateStatus: CLEAN. A local git merge-tree --write-tree origin/develop pr-212-review dry run confirms it: exit 0, clean. The PR also merges cleanly with PR No. 211's head, both in git merge-tree and in a full combined build/test (see the verification comment).

foundry/src/DINTaskAuditor.sol

Field	Value
Change	Modified
Lines	+14/-5
Diff (what exactly is in this PR)	revealAuditScore's expectedHash changes from keccak256(abi.encodePacked(score, vote, salt)) to keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)). The formula is updated in the state comment, in the commitAuditScore @dev, and in its @param.
Functionality — how & why	How: at reveal, the contract rebuilds the hash from the revealed (score, vote, salt) plus the caller's own address and the slot it is revealing for. It then compares that against auditScoreCommits[gi][batchId][msg.sender][modelIndex]. A hash copied from a peer was built with the peer's address, so it can't match under the copier's msg.sender. A hash built for another (gi, batchId, modelIndex) can't match either. Why: the old hash carried no identity. An assigned auditor could commit a peer's public hash, wait for the peer's reveal, and replay it, earning a quorum vote and auditorGIWeight reward weight without evaluating anything (issue No. 192, the auditor-side residual of the M-1 finding).
Diff vs current develop HEAD	None: untouched since the merge-base.
Recommended merge proposal	Merge as-is. +22 B runtime, giving 22,589 B and a 1,987 B margin (gate warning band, above the 1,024 B fail line). Break-then-fix: dropping the msg.sender + modelIndex binding (contract and helper) fails exactly the copy-attack and slot tests; the honest-path test still passes.
Actual merge proposal	Applied as-is, byte-identical to PR head. Size gate: 22,589 B runtime (1,987 B margin, warning band). dump-abi --official from the merged build leaves dincli/abis/DINTaskAuditor.json unchanged. Evidence on the merged tree (develop @ 420d48d + this PR): forge clean && forge build (via_ir) clean; size gate exit 0; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 389 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

dincli/cli/auditor.py, tests/test_auditor_commit_hash.py

Field	Value
Change	Modified / New
Lines	+15/-2, +51/-0
Diff (what exactly is in this PR)	Adds _audit_commit_hash(score, vote, salt, sender, gi, batch_id, model_index), which uses eth_abi.encode + Web3.keccak, and uses it in evaluate --submit in place of Web3.solidity_keccak. The new test file pins two cast-computed golden vectors and checks that the hash varies with the sender and with each slot field.
Functionality — how & why	How: evaluate --submit hashes with the same account.address, curr_GI, batch_id, model_index it then passes to commitAuditScore. It still caches (score, vote, salt) locally. reveal_lms sends those cached values unchanged and lets the contract recompute the hash. Why: with the contract change alone, every dincli reveal would revert TA_RevealHashMismatch, and the auditor would be S1-slashed for a missed vote.
Diff vs current develop HEAD	None
Recommended merge proposal	Merge as-is. Golden vectors re-derived with cast, identical. Swapping gi/batch_id, or switching to packed encoding, fails both golden-vector tests. Full pytest with torch: 389 passed.
Actual merge proposal	Both applied as-is (test file copied), byte-identical to PR head. Evidence on the merged tree (develop @ 420d48d + this PR): forge clean && forge build (via_ir) clean; size gate exit 0; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 389 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

foundry/test/utils/AuditCommitHash.sol, foundry/test/AuditorCommitReveal.t.sol

Field	Value
Change	New / Modified
Lines	+19/-0, +85/-4
Diff (what exactly is in this PR)	A new free function auditCommitHash(...), the one shared copy of the formula for tests. AuditorCommitReveal moves its helper and one inline hash onto it and adds three tests: • test_copyAttack_replayingPeerCommitAndReveal_reverts • test_commitHashBoundToSlot_otherModelBatchOrGI_reverts • test_honestPath_boundHash_allRevealAndClose
Functionality — how & why	How: • Copy attack: B commits A's stored hash and reveals A's exact values. TA_RevealHashMismatch, and B gets no hasAuditedLM. • Slot test: three auditors commit on model 0 hashes built for another model, batch or GI, and every reveal reverts. • Honest path: all commit and reveal their own hashes, and the median closes at 70. Why: these pin the No. 192 fix and the slot binding, and show the honest flow still finalizes.
Diff vs current develop HEAD	None
Recommended merge proposal	Merge as-is. Break-then-fix: dropping the msg.sender + modelIndex binding (contract and helper) fails exactly the copy-attack and slot tests; the honest-path test still passes.
Actual merge proposal	Both applied as-is (helper copied), byte-identical to PR head; the three new tests pass in the full run. Evidence on the merged tree (develop @ 420d48d + this PR): forge clean && forge build (via_ir) clean; size gate exit 0; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 389 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

foundry/test/{AggregatorCommitReveal,DisputeResolution,GasSimulation,LifecycleEvents,PR146SlashingRegression,RewardEngine,ScoringValidation,SecurityFindings,StakingEnforcement,TreasuryForwarding}.t.sol

Field	Value
Change	Modified (10 files)
Lines	+2/-2, +2/-4, +8/-14, +3/-4, +2/-2, +3/-8, +2/-4, +7/-12, +2/-2, +2/-2
Diff (what exactly is in this PR)	Imports auditCommitHash, and every commitAuditScore fixture call now hashes with the pranked auditor and its own (gi, batchId, modelIndex). Most of these used to build one hash before the auditor loop and reuse it.
Functionality — how & why	How: fixture-only; no assertions change. Why: a shared hash no longer reveals for anyone but the auditor it was built for, so every fixture that drives a GI through evaluation would otherwise revert at reveal.
Diff vs current develop HEAD	None. AggregatorCommitReveal.t.sol and StakingEnforcement.t.sol are also touched by PR No. 211, in different hunks. The combined merge is clean and its full suite is green.
Recommended merge proposal	Merge as-is. All 25 rewritten sites checked by hand: each hashes the pranked auditor and the committed slot. 479/479 on this branch.
Actual merge proposal	All 10 applied cleanly. 8 are byte-identical to PR head. AggregatorCommitReveal.t.sol and StakingEnforcement.t.sol also carry PR No. 211's hunks, which landed first; both match git's own 3-way merge (git merge-tree --write-tree) byte-for-byte. Evidence on the merged tree (develop @ 420d48d + this PR): forge clean && forge build (via_ir) clean; size gate exit 0; forge test 484 passed / 0 failed / 0 skipped; pytest -m "not integration" 389 passed / 0 failed. Uncommitted in the develop cwd, not committed or pushed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

Documentation/technical/contracts/DINTaskAuditor.md, Documentation/technical/contracts/DINShared.md, Documentation/technical/audits/foundry-src-security-review.md

Field	Value
Change	Modified (all three)
Lines	+3/-2, +2/-2, +1/-1
Diff (what exactly is in this PR)	• DINTaskAuditor.md: the §7 formula, §13 No. 2 marked fixed, and a Change Log entry. • DINShared.md: the TA_RevealHashMismatch row and the commit-reveal section. • Security review: the M-1 note now records the auditor side as fixed for No. 192.
Functionality — how & why	How / why: keeps the code-reader docs in line with develop. The repo-wide grep finds no Documentation/ page still giving the old formula.
Diff vs current develop HEAD	None
Recommended merge proposal	Merge as-is. Docs CI job green. Optional nit: DINTaskAuditor.md §7 and §13 No. 2 say issue #192, while its Change Log and the rest of the file use issue No. N.
Actual merge proposal	All three applied. DINShared.md and the security review are byte-identical to PR head. DINTaskAuditor.md also takes the optional review nit: (issue #192) becomes (issue No. 192) in §7 and §13 No. 2, matching its Change Log and other issue refs. Only those two lines differ from PR head. check_doc_links.py on Documentation (258) and Developer (242): all resolve. Uncommitted in the develop cwd, not committed or pushed.
Pending proposal	None
Local merge conflict	No
GitHub merge conflict	No

Verification

All on the real via_ir profile:

• forge test: 479 passed / 0 failed (39 suites).
• pytest -m "not integration" with torch: 389 passed.
• DINTaskAuditor: 22,589 B (1,987 margin).
• Golden vectors re-derived with cast.
• Break-then-fix on both the Python hash and the contract bindings.
• Combined with PR No. 211: 484 passed / 0 failed, size gate green (two warnings).

Full detail is in the verification comment above.

Local vs. GitHub agree: both report a clean merge with no conflicts.

[umeradl]

Actual outcome — PR No. 212 merged + review nit 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. 9fb6507 — real merge of this PR (merge commit, not squash) at its head 210e359, authored as umermjd11. GitHub agrees: PR shows MERGED.
   • It landed on top of PR No. 211 (84d0eef + 420d48d), which was pushed first.
   • Git auto-merged the two overlapping test files, AggregatorCommitReveal.t.sol and StakingEnforcement.t.sol, with no conflicts, as predicted.
2. 7785acf — follow-up commit applying the optional review nit.

Files unchanged from the PR (17 of 18)

• 15 files are byte-identical to 210e359:
  • foundry/src/DINTaskAuditor.sol, dincli/cli/auditor.py, tests/test_auditor_commit_hash.py
  • foundry/test/utils/AuditCommitHash.sol, foundry/test/AuditorCommitReveal.t.sol
  • the 8 suites DisputeResolution, GasSimulation, LifecycleEvents, PR146SlashingRegression, RewardEngine, ScoringValidation, SecurityFindings and TreasuryForwarding (.t.sol)
  • Documentation/technical/contracts/DINShared.md and Documentation/technical/audits/foundry-src-security-review.md
• foundry/test/AggregatorCommitReveal.t.sol and foundry/test/StakingEnforcement.t.sol hold this PR's hunks unchanged, plus PR No. 211's. Both match git merge-tree --write-tree byte-for-byte.

Files changed by 7785acf (1 of 18)

File	What changed vs. this PR's merged version
Documentation/technical/contracts/DINTaskAuditor.md	§7 and §13 No. 2 now say issue No. 192 instead of issue #192, matching the file's Change Log entry and its other issue references. Only those two lines changed.

Why the change

It's the optional wording nit from the review, not develop drift. Nothing in this PR needed a functional fix.

Verification

On the pushed tree, 7785acf (PR No. 211 + this PR):

• forge clean && forge build (via_ir) is clean.
• contract_size_gate.py exits 0. DINTaskAuditor is 22,589 B (1,987 B margin) and DINTaskCoordinator 23,497 B (1,079 B), both in the warning band.
• forge test: 484 passed, 0 failed.
• pytest -m "not integration" with an empty HOME: 389 passed, 0 failed.
• dump-abi --official leaves both task ABIs unchanged.
• Doc links in Documentation (258) and Developer (242) all resolve.
• The local CI mirror on 7785acf passed: docs, pytest, via_ir build, forge test and hardhat.
• GitHub push run on develop: green, including the contract size gate step.

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