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

Documentation/technical/audits/foundry-src-security-review.md

@@ -151,7 +151,7 @@ The caller of these functions (the model owner, since both are gated `onlyOwner`
 
 ### M-1. No commit-reveal on aggregation/scoring submissions — "copy the leader" free-riding
 
-**Fixed — auditor scoring: PR #63 (task_210726_6 §2a, predates this report's follow-up numbering). Aggregation-side (`submitT1Aggregation`/`submitT2Aggregation`, described below): [PR #197](https://github.com/InfiniteZeroFoundation/DevNet/pull/197) (issue #156, task_240926_18 Part C).** `commitT1Aggregation`/`revealT1Aggregation` and `commitT2Aggregation`/`revealT2Aggregation` replace the single-shot submit functions; only revealed CIDs count toward finalization, closing the "read every prior submission, then copy the leader" path this finding describes. The commit hash binds `msg.sender` (`keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId))`), deliberately hardening past this finding's own `keccak256(cid, salt)` recommendation — without the sender binding, a lazy aggregator could copy a peer's *commit hash* itself and reveal the peer's `(cid, salt)` under their own name once the peer reveals, reproducing the same free-riding this fix is meant to close. PR #63's auditor-side commit hash (`keccak256(abi.encodePacked(score, vote, salt))`) has this same unbound-sender weakness and was **not** fixed as part of this PR — see the new issue opened for it, linked from the PR.
+**Fixed — auditor scoring: PR #63 (task_210726_6 §2a, predates this report's follow-up numbering). Aggregation-side (`submitT1Aggregation`/`submitT2Aggregation`, described below): [PR #197](https://github.com/InfiniteZeroFoundation/DevNet/pull/197) (issue #156, task_240926_18 Part C).** `commitT1Aggregation`/`revealT1Aggregation` and `commitT2Aggregation`/`revealT2Aggregation` replace the single-shot submit functions; only revealed CIDs count toward finalization, closing the "read every prior submission, then copy the leader" path this finding describes. The commit hash binds `msg.sender` (`keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId))`), deliberately hardening past this finding's own `keccak256(cid, salt)` recommendation — without the sender binding, a lazy aggregator could copy a peer's *commit hash* itself and reveal the peer's `(cid, salt)` under their own name once the peer reveals, reproducing the same free-riding this fix is meant to close. PR #63's auditor-side commit hash (`keccak256(abi.encodePacked(score, vote, salt))`) had this same unbound-sender weakness and was not fixed in PR #197. It was fixed separately for issue #192: the auditor hash is now `keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex))`.
 
 **Contracts / functions:** `DINTaskCoordinator.submitT1Aggregation()` / `submitT2Aggregation()` (L520-543, L600-622); `DINTaskAuditor.setAuditScorenEligibility()` (L515-544).
 

Documentation/technical/contracts/DINShared.md

@@ -213,7 +213,7 @@ Used by: `DINTaskCoordinator`
 | `TA_EmptyCommitHash` | `commitHash` argument is `bytes32(0)` |
 | `TA_RevealPhaseNotOpen` | `revealAuditScore` called while `GIstate != LMSevaluationRevealStarted` |
 | `TA_NoCommitFound` | No prior `commitAuditScore` recorded for this auditor/model — reveal without a commit |
-| `TA_RevealHashMismatch` | `keccak256(abi.encodePacked(score, vote, salt))` does not match the stored commit hash |
+| `TA_RevealHashMismatch` | `keccak256(abi.encode(score, vote, salt, auditor, gi, batchId, modelIndex))`, with `auditor` = `msg.sender`, does not match the stored commit hash |
 | `TC_RevealCannotBeStarted` | `startLMsubmissionsEvaluationReveal` called while `GIstate != LMSevaluationStarted` |
 | `TA_EncryptedKeyCountMismatch` | `assignAuditTestDataset`'s `encryptedKeys` array length does not match the batch's auditor count |
 
@@ -398,4 +398,4 @@ The `TA_` and `TC_` prefixes make it immediately clear in stack traces and event
 
 ### Commit-Then-Reveal Auditor Scoring
 
-`LMSevaluationStarted` and `LMSevaluationRevealStarted` split what was previously a single evaluation phase into two: auditors first commit `keccak256(score, vote, salt)` (hiding their vote from other auditors until everyone has committed), then, once the model owner closes the commit window via `DINTaskCoordinator.startLMsubmissionsEvaluationReveal`, reveal the underlying `(score, vote, salt)` for it to be counted. An auditor who commits but never reveals is simply excluded from quorum/median counting, and remains slashable via the existing "missed vote" check in `slashAuditors` — no separate non-reveal handling needed.
+`LMSevaluationStarted` and `LMSevaluationRevealStarted` split what was previously a single evaluation phase into two: auditors first commit `keccak256(abi.encode(score, vote, salt, auditor, gi, batchId, modelIndex))` (hiding their vote from other auditors until everyone has committed), then, once the model owner closes the commit window via `DINTaskCoordinator.startLMsubmissionsEvaluationReveal`, reveal the underlying `(score, vote, salt)` for it to be counted. An auditor who commits but never reveals is simply excluded from quorum/median counting, and remains slashable via the existing "missed vote" check in `slashAuditors` — no separate non-reveal handling needed.

Documentation/technical/contracts/DINTaskAuditor.md

@@ -120,7 +120,7 @@ The reward split and the S1 fraction are explicitly provisional (MECHANISM_DESIG
 
 ## 7. Commit-then-Reveal Scoring
 
-1. **Commit** (`LMSevaluationStarted`): `commitAuditScore(gi, batchId, modelIndex, commitHash)` with `commitHash = keccak256(abi.encodePacked(score, vote, salt))`. The caller must be an assigned, active auditor; one commit each; a zero hash is rejected.
+1. **Commit** (`LMSevaluationStarted`): `commitAuditScore(gi, batchId, modelIndex, commitHash)` with `commitHash = keccak256(abi.encode(score, vote, salt, auditor, gi, batchId, modelIndex))`, where `auditor` is the committing address. Binding the auditor and the slot means a peer can't copy the hash and replay the reveal, and one commit can't be reused for another model, batch or GI (issue #192). The caller must be an assigned, active auditor; one commit each; a zero hash is rejected.
 2. **Reveal** (`LMSevaluationRevealStarted`): `revealAuditScore(gi, batchId, modelIndex, score, vote, salt)`. Checks the auditor is active, `score ≤ 100`, a commit exists, no prior reveal, and the hash matches. Records the score and vote, sets `hasAuditedLM`, increments `auditorGIWeight` / `giTotalAuditWeight` (the auditor reward basis), and tries to finalize eligibility.
 3. **Eligibility** (`_tryFinalizeEligibility`): once revealed votes ≥ `minEligibilityQuorum`, `eligible = (yesVotes ≥ minEligibilityQuorum)`. With the defaults that means 2 "yes" votes out of 3.
 4. **`finalizeEvaluation(gi)`** (from the coordinator's `closeLMsubmissionsEvaluation`, still in the reveal state): for each batch model with ≥ `minScoreQuorum` revealed scores, `finalMedianScore = median`, `evaluated = true`, `approved = eligible && median ≥ passScore`. The first time a model is approved, its median is added to `giTotalApprovedScore` (the client reward basis). Emits `AuditorScoreDeviation` for every revealing auditor (S3 shadow data). Returns `true` if at least one model reached quorum.
@@ -196,7 +196,7 @@ Registration & data: `DINAuditorRegistered`, `LocalModelSubmitted`, `AuditorsBat
 Read alongside the [foundry/src security review](../audits/foundry-src-security-review.md).
 
 - **No. 1 — Test-data disputes can be won by the challenger alone.** `resolveTestDataDispute` is callable by anyone, and any commitment *mismatch* upholds the dispute. A challenger can call it with an arbitrary `K` and win: bond back, the model owner's GI pool cut by `disputePenaltyBps`, and the batch blocked until reassignment. Only the model owner revealing the real `K` should be able to reach the "match" branch, and a mismatch from a non-owner caller should not count as evidence. `disputeBondAmount` defaults to 0, so this costs the challenger nothing and can be repeated after every reassignment. Tracked in issue No. 205.
-- **No. 2 — Commit hashes are not bound to the auditor.** `keccak256(score, vote, salt)` carries no address, GI, batch or model. An auditor in the same batch can copy another's commit hash, wait for their reveal, and replay it. Tracked in issue No. 192. (The aggregation commits on the coordinator do bind `msg.sender`.)
+- **No. 2 — Fixed: commit hashes are now bound to the auditor.** The old hash, `keccak256(abi.encodePacked(score, vote, salt))`, carried no address, GI, batch or model. An assigned auditor could copy a peer's commit hash, wait for the peer's reveal, and replay it for a free vote. The hash now binds `msg.sender`, `gi`, `batchId` and `modelIndex` (§7), as the aggregation commits on the coordinator do (issue #192).
 - **No. 3 — Committed-but-unrevealed is slashed as a liveness miss.** An auditor who commits and then withholds the reveal pays the S1 fraction (`AUD_NO_VOTE`, 30% of `minStake` by default). That is less than the full-`minStake` S3 slash a revealed outlier would pay once `s3SlashingEnabled` is on, so an auditor who sees they will be in the minority can choose not to reveal. Whether this case gets its own reason code and fraction is open in issue No. 201 (Part B).
 - **No. 4 — Unclaimable remainders.** If no model is approved (`giTotalApprovedScore == 0`), or nobody reveals, or no aggregator weight exists, that role's pool share stays in the contract with no reclaim path.
 - **No. 5 — `setTestDataAssignedFlag` gates nothing:** scoring can open before any test data is assigned.
@@ -218,3 +218,4 @@ Read alongside the [foundry/src security review](../audits/foundry-src-security-
 - Registration caps and floors, the active-registration counter, and the `modelId` constructor argument.
 - Treasury shares and forfeitures forwarded to the platform treasury (`slashTreasury()`), replacing the per-contract treasury address (issue No. 152).
 - `createAuditorsBatches` takes the coordinator's locked audit seed (issue No. 156 H-2, PR No. 191).
+- The audit commit hash binds the auditor and the slot: `keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex))` replaces `keccak256(abi.encodePacked(score, vote, salt))` (issue No. 192). Function signatures and the ABI are unchanged; in-flight commits made under the old formula can't be revealed after the switch.

dincli/cli/auditor.py

@@ -7,6 +7,7 @@
 import typer
 from rich.table import Table
 from web3 import Web3
+from eth_abi import encode as abi_encode
 from nacl.public import Box, PrivateKey, PublicKey
 import nacl.encoding
 from cryptography.hazmat.primitives.ciphers.aead import AESGCM
@@ -69,6 +70,18 @@ def _load_commit(model_base_dir: Path, gi: int, batch_id: int, model_index: int)
     data["salt"] = bytes.fromhex(data["salt"])
     return data
 
+
+def _audit_commit_hash(score: int, vote: bool, salt: bytes, sender: str, gi: int, batch_id: int, model_index: int) -> bytes:
+    """keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)) --
+    must match DINTaskAuditor.revealAuditScore exactly, including plain
+    (non-packed) ABI encoding. Binding the auditor and the (gi, batch, model)
+    slot stops a peer replaying this auditor's commit and reveal (issue #192)."""
+    encoded = abi_encode(
+        ["uint256", "bool", "bytes32", "address", "uint256", "uint256", "uint256"],
+        [score, vote, salt, Web3.to_checksum_address(sender), gi, batch_id, model_index],
+    )
+    return Web3.keccak(encoded)
+
 app = typer.Typer(help="Commands for Auditors in DIN.")
 
 dintoken_app = typer.Typer(help="Commands for DIN Token in DIN.")
@@ -531,8 +544,8 @@ def evaluate_lms(
                 score_int = int(score)
                 vote_bool = bool(eligible)
                 salt = secrets.token_bytes(32)
-                commit_hash = Web3.solidity_keccak(
-                    ["uint256", "bool", "bytes32"], [score_int, vote_bool, salt]
+                commit_hash = _audit_commit_hash(
+                    score_int, vote_bool, salt, account.address, curr_GI, batch_id, model_index
                 )
 
                 try:

foundry/src/DINTaskAuditor.sol

@@ -241,7 +241,10 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient {
     mapping(uint256 => mapping(uint => mapping(address => mapping(uint => bool)))) // GI // batchId // auditor // modelIndex // has voted
         public hasAuditedLM;
 
-    // Commit-then-reveal (task_210726_6 §2a). commitHash = keccak256(abi.encodePacked(score, vote, salt)).
+    // Commit-then-reveal (task_210726_6 §2a). commitHash =
+    // keccak256(abi.encode(score, vote, salt, auditor, gi, batchId, modelIndex)):
+    // binding the auditor and (gi, batchId, modelIndex) stops a peer copying
+    // another auditor's commit hash and reveal (issue #192).
     // hasCommittedLM is distinct from hasAuditedLM: hasAuditedLM is set only
     // on a successful reveal and remains the single source of truth for
     // quorum/median counting, exactly as before -- an auditor who commits
@@ -1077,8 +1080,12 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient {
     /// @notice Phase 1 of commit-then-reveal auditor scoring: lock in a
     ///         hidden (score, vote) pair.
     /// @dev Caller must be the assigned auditor for this batch and model
-    ///      index. `commitHash` must equal `keccak256(abi.encodePacked(score,
-    ///      vote, salt))` for the values the auditor intends to reveal later
+    ///      index. `commitHash` must equal `keccak256(abi.encode(score, vote,
+    ///      salt, msg.sender, gi, batchId, modelIndex))` for the values the
+    ///      auditor intends to reveal later. Binding the auditor's own address
+    ///      and the (gi, batchId, modelIndex) slot means a peer can't copy this
+    ///      hash and later replay this auditor's reveal (issue #192), and an
+    ///      auditor can't reuse one commit for another model, batch or GI
     ///      -- the contract cannot and does not validate this at commit time
     ///      (that's the point; nothing about score/vote is visible yet).
     ///      Open only while GIstate == LMSevaluationStarted (the commit
@@ -1088,7 +1095,7 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient {
     /// @param gi Current GI index.
     /// @param batchId Batch index containing this model.
     /// @param modelIndex Index into lmSubmissions[gi] for the model being scored.
-    /// @param commitHash keccak256(abi.encodePacked(score, vote, salt)).
+    /// @param commitHash keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)).
     function commitAuditScore(
         uint256 gi,
         uint batchId,
@@ -1157,7 +1164,9 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient {
         if (hasAuditedLM[gi][batchId][msg.sender][modelIndex])
             revert TA_AlreadyVoted();
 
-        bytes32 expectedHash = keccak256(abi.encodePacked(score, vote, salt));
+        bytes32 expectedHash = keccak256(
+            abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex)
+        );
         if (
             expectedHash !=
             auditScoreCommits[gi][batchId][msg.sender][modelIndex]

foundry/test/AggregatorCommitReveal.t.sol

@@ -37,6 +37,7 @@ import {
     TC_T2RevealHashMismatch,
     TC_T2RevealPhaseNotOpen
 } from "../src/DINShared.sol";
+import {auditCommitHash} from "./utils/AuditCommitHash.sol";
 
 contract AggregatorCommitRevealTest is Test {
     DinToken tokenImpl;
@@ -219,11 +220,10 @@ contract AggregatorCommitRevealTest is Test {
         vm.stopPrank();
 
         (, address[] memory batchAuditors, uint[] memory modelIdxs, ) = ta.getAuditorsBatch(1, 0);
-        bytes32 scoreCommit = keccak256(abi.encodePacked(uint256(100), true, TEST_SALT));
         for (uint i = 0; i < batchAuditors.length; i++) {
             for (uint m = 0; m < modelIdxs.length; m++) {
                 vm.prank(batchAuditors[i]);
-                ta.commitAuditScore(1, 0, modelIdxs[m], scoreCommit);
+                ta.commitAuditScore(1, 0, modelIdxs[m], auditCommitHash(uint256(100), true, TEST_SALT, batchAuditors[i], 1, 0, modelIdxs[m]));
             }
         }