security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side)

Ref: Documentation/technical/audits/foundry-src-security-review.md H-2, M-1 · discussion #88 ("aggregation side" of H-2/M-1, still unaddressed as of the Aug 18 status comment) · related: BL-11 (dispute-redo instance of the same shuffle) · precedent: PR #63 (identical fix, auditor side)

Problem

Two related gaps in DINTaskCoordinator.sol's Tier-1/Tier-2 aggregator flow, both already fixed on the auditor side by PR #63 but never carried over to aggregators:

H-2 (aggregation side): grindable shuffle entropy

autoCreateTier1AndTier2 assigns aggregators to T1/T2 batches via _shuffleAddressArray:

function _shuffleAddressArray(address[] memory arr) internal view {
    if (arr.length < 2) return;
    for (uint i = arr.length - 1; i > 0; i--) {
        uint j = uint(
            keccak256(
                abi.encodePacked(blockhash(block.number - 1), i, arr.length)
            )
        ) % (i + 1);
        (arr[i], arr[j]) = (arr[j], arr[i]);
    }
}

blockhash(block.number - 1) is public and known before autoCreateTier1AndTier2 is even called. The caller (model owner, onlyOwner) can simulate the resulting batch assignment off-chain and choose when to submit the transaction — waiting for a block whose hash produces a favorable clustering of their own Sybil-controlled aggregators into the same batch, at zero cost beyond a normal L2 transaction retry.

This is the same root cause BL-11 tracks for the dispute-redo reassignment path (_assignFreshSubgroup) — but BL-11 is scoped narrowly to that one function. The original batch-assignment shuffle in autoCreateTier1AndTier2 has no tracking issue of its own and is untouched by BL-11's scope.

M-1 (aggregation side): no commit-reveal on T1/T2 submissions

submitT1Aggregation/submitT2Aggregation accept a plaintext _aggregationCID in a single transaction, with no commit phase. Any aggregator who is not first to submit in their batch can read every prior submission from public contract state (t1SubmissionCID/t2SubmissionCID) before deciding what to submit — a lazy or dishonest aggregator can copy an already-submitted CID instead of doing the actual aggregation work, guaranteeing they match consensus and avoid slashing while contributing nothing.

PR #63 fixed the identical problem for auditor scoring (commitAuditScore/revealAuditScore, keccak-committed (score, vote, salt), revealed in a second window). Aggregator submissions were explicitly left out of that PR's scope.

Fix

Mirror PR #63's shape, applied to DINTaskCoordinator:

Shuffle entropy: replace blockhash/timestamp-based _shuffleAddressArray with commit-reveal (e.g. the model owner or a designated party commits to a seed one block before autoCreateTier1AndTier2 runs, so the shuffle input can't be chosen post-hoc) or VRF-based randomness (Chainlink VRF or similar). At minimum, the same address that benefits from the shuffle outcome (the model owner) should not also control the exact block it executes in.
T1/T2 submission commit-reveal: add commitT1Aggregation(gi, batchId, commitHash) / revealT1Aggregation(gi, batchId, aggregationCID, salt) (and the T2 equivalents), commitHash = keccak256(abi.encodePacked(aggregationCID, salt)), gated by a reveal-window state transition the same way LMSevaluationRevealStarted gates auditor reveals. finalizeT1Aggregation/finalizeT2Aggregation should only count revealed submissions toward t1Votes/t2Votes, same as hasAuditedLM only counts post-reveal.

Scope note

These two are related (both are "can the aggregator side be gamed via public on-chain visibility before commitment") but are separate mechanisms — shuffle entropy controls who is assigned to a batch, commit-reveal controls what they submit once assigned. Can land as one PR (discussion #88 groups them as one follow-on) or split if that's cleaner for review.

Out of scope

BL-11's dispute-redo _assignFreshSubgroup shuffle — same fix shape, but tracked separately; worth resolving together per BL-11's own text ("should be resolved together with H-2") but not duplicating scope here.
C-1 (registration/finalize gas-DoS) and M-2 (disableModel kill-switch) — separate findings, both still gated on open design decisions per discussion Smart Contract Security Architecture: Three-Phase Plan (DevNet → Testnet → Mainnet) #88, not ready for implementation yet.

Review — go-ahead on the problem, but sequenced behind Robbert's task_16 and with a revised mechanism

Checked against develop @ 3e52351.

Diagnosis: confirmed, still unfixed. autoCreateTier1AndTier2 (DINTaskCoordinator.sol:488) still shuffles aggregators via _shuffleAddressArray seeded from blockhash(block.number - 1) (:553). submitT1Aggregation/submitT2Aggregation (:694, :790) still take a plaintext CID with no commit phase.

Corrections to the issue text

H-2 is not fixed on the auditor side. PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63 added commit-reveal for auditor scores (M-1). The auditor batch shuffle (DINTaskAuditor._shuffleAddressArray/_shuffleUintArray, :743/:779, used by createAuditorsBatches) still uses the same blockhash/timestamp entropy. So H-2 is open on both sides, and the fix should cover both.
The model-index shuffle is in scope too. _shuffleUintArray (:565, block.timestamp + msg.sender, and msg.sender is always the owner) decides which approved models land in which T1 batch. That's the "owner routes a poisoned model into a colluding T1 batch" path in task_240926_17's threat model, so fixing only the address shuffle leaves it open.
keccak256(cid, salt) doesn't stop copying. A lazy aggregator can copy a peer's commit hash, wait for the peer to reveal, then reveal the same (cid, salt). The hash has to bind the committer: keccak256(abi.encode(cid, salt, msg.sender, GI, tier, batchId)). PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor hash (keccak256(abi.encodePacked(score, vote, salt)), DINTaskAuditor.sol:1127) has the same weakness. Please open a separate issue for that rather than folding it in here.

Overlap with Robbert's work: yes, two direct overlaps. Resolution: wait, don't cancel

H-2 half vs task_240926_16 Part A (BL-11: Replace blockhash randomness in _assignFreshSubgroup with commit-reveal #151).
- Part A1 changes _shuffleAddressArray to take an explicit bytes32 seed and passes blockhash(block.number - 1) at the autoCreateTier1AndTier2 call site explicitly, leaving that seed for this issue to replace.
- Part A2–A3 introduces the future-block seed + permissionless lockDisputeSeed pattern.
- → Wait for task_16's Part A PR to merge, then build on its helper and use the same pattern. Don't fork a second shuffle, and don't start on a branch off Robbert's.
M-1 half vs task_16 Part C (Add missing task-level Solidity events to unblock 5 subgraph handlers #153).
- Part C adds _setGIstate for every state transition and emits T1AggregationSubmitted/T2AggregationSubmitted inside the exact submit functions this issue rewrites.
- Its subgraph half regenerates the uint8 → GIstates mapping, which new reveal states would shift.
- → Wait for task_16's Part C PR to merge. Then move those two events to the reveal function (signatures unchanged), and route new transitions through _setGIstate.
Minor: task_17 Part 3b (Decide and set testnet tokenomics parameters: mintCap, dinPerEth, and DinEmission values #155) edits the S2 slash loop (:888) that non-reveal handling touches. Rebase on it if it's merged by then. No design conflict.
task_17 Part 2 will have test_knownGap_ tests for shuffle steering / batch routing. Whichever PR here closes those gaps flips them to test_defended_.

Split into two PRs

PR A — H-2 (both tiers). Start after task_16 Part A merges.

Reuse task_16's future-block seed, not commit-reveal or VRF.
- Anchor seedBlock = block.number + delay at the preceding transition: closeLMsubmissionsEvaluation (:472) for T1/T2, and closeLMsubmissions (:411) for auditor batches.
- Add a permissionless lock with the same >256-block re-anchor.
- autoCreateTier1AndTier2 and createAuditorsBatches require a locked seed.
All four shuffles (address + index, both contracts) take the locked seed, domain-separated. No blockhash(block.number - 1), block.timestamp, or msg.sender entropy remains.
Same sequencer-trust caveat in NatSpec as task_16 A4. VRF stays the mainnet follow-up.
dincli: the model-owner flow becomes wait → lock → create (modelownerd/aggregation.py, auditor_batches.py).

PR B — M-1 (T1/T2 commit-reveal). Start after task_16 Part C merges.

commitT{1,2}Aggregation / revealT{1,2}Aggregation with the sender-bound hash above.
New T1AggregationRevealStarted/T2AggregationRevealStarted states, inserted at their place in the lifecycle (the 2026-08-27 precedent in DINShared.sol:25-34). Update dincli/cli/utils.py's state mirrors and DINShared.md §2.1, and post the new ordinal table on PR feat(indexer): implement DIN Protocol subgraph — platform + task-level contracts #29 so the subgraph mapping gets regenerated.
Votes and t1SubmissionCID/t2SubmissionCID are written on reveal only. Commit-without-reveal counts as a missed submission for S2. Disputes and reward-weight accounting read revealed state only, with behaviour otherwise unchanged.
dincli: split aggregator.py's T1/T2 submit into commit + reveal (persist the salt locally, as the auditor side does), plus model-owner commands to open the reveal windows.

Contract size — check before and after

At 3e52351, DINTaskCoordinator is 20,159 B and DINTaskAuditor is 22,382 B, only ~2.2 KB under EIP-170. task_16 Parts B/C will add to both. Put forge build --sizes before/after in each PR. If either contract crosses 24,576 B, stop and raise it here rather than splitting the contract.

Acceptance criteria I'd add

Timing independence: the same GI batched at two different blocks (vm.snapshot/vm.revertTo + vm.roll) yields identical T1 batches and identical auditor batches.
Copy attack defeated: an aggregator replaying a peer's commit hash can't reveal it.
A commit without a reveal is S2-slashed. A reveal outside the window or with a wrong salt reverts.
A full GI walk through the new states.

Go-ahead on the scope above, sequenced behind task_16 Parts A and C. If you'd rather use VRF or multi-party commit-reveal than the future-block seed for the shuffle, raise it here first. The aim is one randomness pattern across the protocol.

docs(tasks): add task_240926_18 — L-2/L-3/L-6 fixes + aggregation-sid…

feat(issue-151): replace grindable blockhash shuffle with future-bloc…

Progress update per the review comment's three-PR split (task_240926_18 Parts B/C):

Part B (H-2, both tiers — batch-assignment shuffle): PR #191, open. Replaces autoCreateTier1AndTier2/createAuditorsBatches's blockhash/timestamp/msg.sender shuffle entropy with the same future-block-seed + permissionless-lock pattern PR feat(issue-151): BL-11 future-block dispute seed for _assignFreshSubgroup #171 shipped for the dispute-outcome seed.
Part C (M-1, aggregation side — commit-reveal on T1/T2 submissions): PR incoming shortly, will link here once open.

Two corrections/clarifications now that Part C is implemented, since the issue text as originally written doesn't reflect either:

Commit hash is keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId)), not keccak256(cid, salt) as this issue's Fix section proposes. Without binding the committer's own address (and GI/tier/batchId) into the hash, a lazy aggregator could copy a peer's commit hash verbatim and reveal the peer's (cid, salt) under their own name once the peer reveals — reproducing the exact free-riding commit-reveal exists to prevent, just one step removed from M-1's original "copy the plaintext CID" description. Confirmed via a dedicated PoC (test_copyAttack_replayingPeerCommitHash_cannotReveal, foundry/test/AggregatorCommitReveal.t.sol) that this replay is rejected.
PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor-side commit-reveal (commitAuditScore/revealAuditScore) has this identical unbound-sender weakness (keccak256(abi.encodePacked(score, vote, salt)), no msg.sender bound in) and is not fixed as part of Part C — it's a separate, narrower-window gap (requires the copier to already be an assigned auditor for the same batch/model). Opened #192 to track it.

GIstates gains T1AggregationRevealStarted/T2AggregationRevealStarted (inserted in lifecycle position, same precedent as LMSevaluationRevealStarted) — posted the full updated ordinal table on PR #29 for the subgraph mapping.

docs(issue-154): adversarial threat model — scenario matrix, trust as…

feat(foundry): ungrindable future-block seed for batch-assignment shu…

feat(dincli): wait-and-lock batch-assignment seed before creating bat…

docs: note seed-lock wait in model-workflow, mark H-2 batch-seed fixed

merge: develop into feat/issue-156-part-c-commit-reveal

feat(foundry): commit-then-reveal for T1/T2 aggregation submissions

Closing — both findings are fixed and merged into develop.

H-2 (grindable batch-assignment shuffle): PR #191 — autoCreateTier1AndTier2/createAuditorsBatches now consume a future-block-anchored, permissionlessly-lockable seed (lockAggSeed/lockAuditSeed) instead of blockhash/block.timestamp/msg.sender. Merged (with a validator-side lock-seed CLI added on review). Residuals (decline-to-lock re-roll, post-lock pool reshaping via unstaking) tracked as BL-26 in Developer/BACK_LOG.md, same pattern as BL-11's identical dispute-seed residual.
M-1 (no commit-reveal on T1/T2 submissions): PR #197 — commitT1Aggregation/revealT1Aggregation and the T2 equivalents replace the single-shot submit functions, with the commit hash binding msg.sender (hardening past this issue's own keccak256(cid, salt) proposal — see the PR for why). Merged, plus a follow-up commit fixing a salt-overwrite-on-retry bug, doc updates, and 9 more T2 negative-path tests.

Two things came out of the review that are real but out of this issue's scope, tracked separately rather than reopening this:

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 — landing fix(foundry+dincli): commit-then-reveal for T1/T2 aggregation (issue #156 M-1, Part C) #197 pushed DINTaskCoordinator to 24,585 B, 9 bytes over the EIP-170 24,576 B limit — it can't currently be deployed. Also covers the committed-but-unrevealed slash-severity question (currently 30% liveness-miss, arguably should be 100%) for both commit-reveal flows.
dincli: auditor commit retry overwrites the committed salt (auditor-side twin of PR No. 197 No. 1) + aggregate-t2 uses a stale T1 batch id #202 — the auditor-side twin of fix(foundry+dincli): commit-then-reveal for T1/T2 aggregation (issue #156 M-1, Part C) #197's salt-overwrite bug, plus a stale-T1-batch-id issue in aggregate-t2.
Auditor commit-reveal (PR #63) doesn't bind msg.sender into the commit hash — same free-riding gap M-1 just closed on the aggregation side #192 — PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor-side commit-reveal has the same unbound-msg.sender hash weakness M-1's fix hardens against, opened separately during fix(foundry+dincli): commit-then-reveal for T1/T2 aggregation (issue #156 M-1, Part C) #197's implementation.

#201 in particular is worth flagging as a near-term blocker for anyone trying to deploy off current develop for the DINTaskCoordinator model.

fix(task-auditor): bind the auditor and slot into the audit commit ha…

Navigation Menu

Repository navigation

security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side) #156

Problem

H-2 (aggregation side): grindable shuffle entropy

M-1 (aggregation side): no commit-reveal on T1/T2 submissions

Fix

Scope note

Out of scope

Review — go-ahead on the problem, but sequenced behind Robbert's task_16 and with a revised mechanism

Corrections to the issue text

Overlap with Robbert's work: yes, two direct overlaps. Resolution: wait, don't cancel

Split into two PRs

Contract size — check before and after

Acceptance criteria I'd add

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side) #156

Description

Problem

H-2 (aggregation side): grindable shuffle entropy

M-1 (aggregation side): no commit-reveal on T1/T2 submissions

Fix

Scope note

Out of scope

Activity

umeradl commented on Sep 24, 2026

Review — go-ahead on the problem, but sequenced behind Robbert's task_16 and with a revised mechanism

Corrections to the issue text

Overlap with Robbert's work: yes, two direct overlaps. Resolution: wait, don't cancel

Split into two PRs

Contract size — check before and after

Acceptance criteria I'd add

Abidoyesimze commented on Sep 28, 2026

Abidoyesimze commented on Sep 30, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions