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.
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
autoCreateTier1AndTier2assigns aggregators to T1/T2 batches via_shuffleAddressArray:blockhash(block.number - 1)is public and known beforeautoCreateTier1AndTier2is 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 inautoCreateTier1AndTier2has 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/submitT2Aggregationaccept a plaintext_aggregationCIDin 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:blockhash/timestamp-based_shuffleAddressArraywith commit-reveal (e.g. the model owner or a designated party commits to a seed one block beforeautoCreateTier1AndTier2runs, 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.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 wayLMSevaluationRevealStartedgates auditor reveals.finalizeT1Aggregation/finalizeT2Aggregationshould only count revealed submissions towardt1Votes/t2Votes, same ashasAuditedLMonly 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
_assignFreshSubgroupshuffle — 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.disableModelkill-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.