task_240926_18 — Security-Review Follow-Through: L-2/L-3/L-6 (#159, #160, #161) + Aggregation-Side H-2/M-1 (#156) (Similoluwa)

[umeradl]

cc @abrahamnash — this one's scoped and ready for your approval / HR-side pre-processing before I formally assign it to @Abidoyesimze.

Full spec: Developer/tasks/task_240926_18.md

The task file has the full specification: exact file/line references, the sequencing against Robbert's in-flight work, and the deliverables checklists. This post is the summary and the decisions that need flagging.

Timeline

7 days: Sep 24 – Oct 1, 2026. Part A can start immediately. Parts B and C are gated on Robbert's task_240926_16 Part A and Part C PRs merging (same Sep 24 – Oct 1 window), so if those slip, say so here early rather than building against his branches.

Summary

Similoluwa opened #156, #159, #160 and #161 as follow-throughs on his own July security review (foundry-src-security-review.md, task_060726_4). None of the four is implemented on develop at 3e52351. Each issue now has a review comment with the agreed scope. The task is three PRs into develop:

• Part A: low findings, #159 / #160 / #161. Can start immediately.
  • L-2: depositAndMint reverts on a zero mintAmount (review).
  • L-3: requestModelRegistration refunds overpayment, records feePaid = requiredFee, and gains a transient reentrancy guard (review).
  • L-6: zero-address checks on both task-contract constructors, plus setDINTaskAuditorContract, which the issue missed and which bricks the contract on a zero address (review).
• Part B: #156 H-2, ungrindable batch-assignment seed, both tiers.
  • Reuses task_240926_16 Part A's future-block seed + permissionless lock for autoCreateTier1AndTier2 and createAuditorsBatches.
  • Covers all four shuffles (address + model-index, both task contracts). The issue's "already fixed on the auditor side by PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63" only applies to score commit-reveal; the auditor batch shuffle is still grindable.
• Part C: #156 M-1, commit-reveal on T1/T2 aggregator submissions.
  • Mirrors PR feat(auditing): commit-then-reveal scoring, encrypted test-data keys, resampling policy #63's auditor side, but the commit hash binds msg.sender, GI, tier and batchId. The issue's keccak256(cid, salt) lets an aggregator copy a peer's commit hash and reveal the same values.
  • New reveal states inserted at their place in the lifecycle; dincli aggregator commit/reveal + model-owner reveal-window commands (review).

Sequencing with Robbert's work

The four issues were checked against task_240926_16 and task_240926_17 (both Sep 24 – Oct 1). Nothing was cancelled. Where they overlap, this task waits:

Part	Overlap	Resolution
A	None (task_17 only calls updateDinPerEth from the deploy script)	Start now
B	task_16 Part A changes _shuffleAddressArray to take a seed and leaves this call site to #156	Start after task_16's Part A PR merges; build on its helper
C	task_16 Part C adds _setGIstate + submission events in the same functions; its subgraph half regenerates the GIstates mapping. task_17 Part 3b edits the S2 slash loop.	Start after task_16's Part C PR merges; rebase on task_17 Part 3

task_17 Part 2's test_knownGap_ tests for shuffle steering get flipped to test_defended_ in whichever PR here closes the gap.

Decisions to flag, not guess

1. fix: requestModelRegistration doesn't refund overpayment above the required fee (L-3) #160 shape: refund the excess, not exact-match. Raise it on fix: requestModelRegistration doesn't refund overpayment above the required fee (L-3) #160 first if you disagree.
2. Part B delay parameter: reuse task_16's disputeSeedDelay or add a separate one with the same setter pattern. Say which in the PR.
3. Part B design: if VRF or multi-party commit-reveal looks better than the future-block seed once you're in the code, raise it on security: aggregator batch shuffle uses grindable blockhash + no commit-reveal on T1/T2 submissions (H-2/M-1 aggregation-side) #156 before building it. The aim is one randomness pattern across the protocol.
4. Part C, new enum states: these shift every later GIstates ordinal. Update dincli/cli/utils.py's 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 for Robbert's subgraph. Don't push to his branch.
5. Contract size: DINTaskAuditor is 22,382 B at 3e52351 (~2.2 KB under EIP-170) and task_16 adds to it. Put forge build --sizes before/after in Parts B and C. If either contract crosses 24,576 B, stop and raise it here rather than splitting the contract.
6. Out of scope, to open as issues: the auditor-side commit hash has the same copying weakness (Part C opens an issue for it). rejectModel never refunds feePaid, which is a policy question noted in Part A.

Reference material

• Developer/tasks/task_240926_18.md: full spec
• #156, #159, #160, #161: tracking issues, each with a review comment holding the agreed scope
• Documentation/technical/audits/foundry-src-security-review.md: H-2, M-1, L-2, L-3, L-6
• Discussion #163 / #164: Robbert's task_16 / task_17 tracking

Once assigned: post progress updates, questions, and PR links here, same convention as prior task-tracking discussions. If anything in the spec doesn't feel right, is ambiguous, or needs clarification, comment here and cc @umeradl.

[abrahamnash]

Approved, @umeradl - thank you.

[Abidoyesimze]

Part A done — PR #176. Closes #159, #160, #161, one commit per issue:

• fix: depositAndMint can retain ETH with a zero mint on rounding (L-2) #159 (L-2): depositAndMint reverts on a zero-rounded mint.
• fix: requestModelRegistration doesn't refund overpayment above the required fee (L-3) #160 (L-3): requestModelRegistration refunds overpayment, feePaid records requiredFee; added ReentrancyGuardTransient to DINModelRegistry for the refund's external call (confirmed transient-storage-backed, storage layout unaffected). rejectModel's separate non-refund-on-rejection left explicitly out of scope, per the task.
• fix: DINTaskCoordinator/DINTaskAuditor constructors accept zero-address dependencies unchecked (L-6) #161 (L-6): zero-address checks on both task-contract constructors, plus setDINTaskAuditorContract (a bricking risk the original issue didn't name).

forge test: 375 passed / 0 failed / 0 skipped, full suite including UpgradeValidation.t.sol.

One thing worth a separate issue, not fixed in this PR: regenerating the bundled ABIs from a fresh forge build (rather than the surgical single-error append I actually did) showed they're significantly stale beyond just these three fixes — dincli/abis/DINTaskCoordinator.json was missing 58 entries that already exist on develop (functions/events/errors from the dispute-resolution and per-model-stake work, at minimum). I appended only the 4 new error entries this PR introduces to keep the diff focused; the underlying staleness is real and separate. Happy to open that issue if useful.

Starting on Parts B/C is still blocked on task_240926_16 PRs #171 (Part A) and #173 (Part C) merging — both still open as of now. Will pick up Part A follow-ups or the auditor-side commit-hash-binding issue (Part C's deliverable list) in the meantime if that gap widens.

[umeradl]

Heads-up: dincli dindao is now dincli dinrep — PR No. 175 landed on develop (merge 5864f7d, follow-up 8f9a90b). The DIN-Representative command group is renamed to match the role: dincli/cli/dindao.py → dincli/cli/dinrep.py, Documentation/public/roles/dindao.md → dinrep.md. Breaking, no alias — dincli dindao … now returns "No such command". Subcommands are unchanged (dincli dinrep deploy …, dincli dinrep add-slasher …, dincli dinrep registry …).

For this task: nothing to change — this task file and its branches don't reference the old name. Use dincli dinrep from now on.

[umeradl]

Status check (2026-09-28): not closing yet. Part A is done; Parts B and C haven't started.

Checked against origin/develop at 7b66392:

Part	Status	Evidence
A: L-2 / L-3 / L-6 (No. 159, No. 160, No. 161)	✅ Landed	PR No. 176 merged 2026-09-27 (merge 6154f80, review follow-ups 9bc63c5). DinCoordinator.depositAndMint reverts with ZeroMintAmount. DINModelRegistry is ReentrancyGuardTransient, and both requestModelRegistration and requestManifestUpdate refund the excess. Zero-address guards are in both task-contract constructors and in setDINTaskAuditorContract. The audit doc marks L-2/L-3/L-6 as fixed. All three issues are closed.
B: H-2 ungrindable batch seed (No. 156)	⏳ Not started	No PR yet. DINTaskCoordinator.sol:541 still calls _shuffleAddressArray(valPool, blockhash(block.number - 1)), and there's no aggSeedBlock/auditSeedBlock or lock function on develop.
C: M-1 aggregator commit-reveal (No. 156)	⏳ Not started	No PR yet. There's no commitT1Aggregation/revealT1Aggregation or T1AggregationRevealStarted state on develop.

The blockers are cleared: task_240926_16's Part A (PR No. 171, merged 2026-09-26) and Part C (PR No. 173, merged 2026-09-27) are both on develop, so @Abidoyesimze, Parts B and C can start now, building on task_16's seed helper and _setGIstate. The window runs to Oct 1; if that's tight, say so here.

This stays open until the Part B and Part C PRs merge and No. 156 closes.

[Abidoyesimze]

Yes I'm on it, will raise a PR soon.
Thanks.

[Abidoyesimze]

Part B done — PR #191 open. Fixes the batch-assignment half of #156's H-2: createAuditorsBatches/autoCreateTier1AndTier2 shuffled auditor/aggregator batches with blockhash/block.timestamp/msg.sender, all known before the model owner submits, so they could grind for a favorable block. Replaced with the same future-block-seed + permissionless-lock pattern PR #171 shipped for the dispute-outcome seed — lockAuditSeed/lockAggSeed, anchored at closeLMsubmissions/closeLMsubmissionsEvaluation, reusing disputeSeedDelay rather than a new parameter.

PoC (test_createAuditorsBatches_timingIndependent / test_autoCreateTier1AndTier2_timingIndependent) snapshots state and asserts the batch assignment is identical regardless of which block the call lands in — that's the actual property H-2 is about, not just "uses a different entropy source."

dincli's auditor-batches create / aggregation create-t1nt2-batches now wait for and lock the seed automatically before creating batches (short pause, permissionless, no new commands to learn).

Full suite: 417/417 foundry tests pass, dincli pytest unaffected (180 passed, same 16 pre-existing environment-only failures as a clean develop checkout). Contract size headroom: DINTaskCoordinator +1,298 B (1,936 B margin left), DINTaskAuditor +123 B (2,047 B margin left) — both well under the 24,576 B limit.

Part C (#156's M-1 half — commit-reveal on T1/T2 aggregation submissions) is next.

[Abidoyesimze]

Part C done — PR #197 open, closes #156. Fixes the aggregation half of #156's M-1: submitT1Aggregation/submitT2Aggregation accepted a plaintext CID in one transaction, letting a lazy aggregator read every prior submission and copy it instead of doing the aggregation work. Replaced with commitT1Aggregation/revealT1Aggregation and the T2 equivalents, mirroring PR #63's auditor-side shape.

One deliberate hardening past #156's own proposal: the commit hash binds msg.sender (keccak256(abi.encode(cid, salt, msg.sender, GI, tierKind, batchId)), not keccak256(cid, salt)). Without that, an aggregator could copy a peer's commit hash and reveal the peer's (cid, salt) under their own name once the peer reveals — same free-riding, one step removed. PoC in AggregatorCommitReveal.t.sol (test_copyAttack_replayingPeerCommitHash_cannotReveal) confirms the replay is rejected. PR #63's auditor-side hash has this identical gap and is intentionally left unfixed here — opened #192 to track it separately.

GIstates gains T1AggregationRevealStarted/T2AggregationRevealStarted (inserted in lifecycle position); full ordinal table posted on PR #29 for the subgraph mapping.

Full suite: 416/416 foundry tests pass (12 new in AggregatorCommitReveal.t.sol). Contract size: DINTaskCoordinator +1,900 B, 1,334 B margin left under the 24,576 B EIP-170 limit; DINTaskAuditor untouched.

That's all three PRs for task_240926_18 now open: #191 (Part B, H-2), #197 (Part C, M-1), plus the already-merged #176/#186 (Part A). #156 closes once #197 lands.

[umeradl]

Status check (2026-09-30): all three parts have landed on develop. This task is ready to close.

Checked against origin/develop at 6ccc28c. CI on the last code commit (00c66e0) is green.

Part	Status	Evidence
A: L-2 / L-3 / L-6 (No. 159, No. 160, No. 161)	✅ Landed	PR No. 176, merged 2026-09-27 (6154f80, follow-ups 9bc63c5). All three issues closed. Verified in the Sep 28 check.
B: H-2 ungrindable batch seed (No. 156)	✅ Landed	PR No. 191's branch was merged directly at 6fb1f64 (525f256), with review follow-ups No. 6 and No. 7 plus the validator-side lock in d95abdf. The PR shows as closed rather than merged because a later push raced the merge; the code is on develop.
C: M-1 aggregator commit-reveal (No. 156)	✅ Landed	PR No. 197, merged 2026-09-30 (d68f73a, follow-ups e373c8d). No. 156 closed.

Part B deliverables, checked on develop

Deliverable	Evidence
Seed anchored at the preceding transition; permissionless lock; re-anchor after 256 blocks	closeLMsubmissions sets auditSeedBlock, closeLMsubmissionsEvaluation sets aggSeedBlock (both block.number + disputeSeedDelay). lockAggSeed/lockAuditSeed in DINTaskCoordinator.sol, with AggSeedReanchored/AuditSeedReanchored.
Batch creation requires a locked seed; four shuffles domain-separated; no block entropy left	TC_AggSeedNotLocked / TC_AuditSeedNotLocked / TA_AuditSeedNotLocked. Tags AGG_ADDR, AGG_IDX, AUD_ADDR, AUD_IDX. All four _shuffle* helpers are pure and take an explicit seed. No blockhash(block.number - 1) remains in foundry/src.
Tests	SecurityFindings.t.sol: timing independence for both batch types, gating, lock guards, permissionless lock, and re-anchor (14 tests).
dincli lock-then-create + tests; workflow doc	modelownerd/aggregation.py and auditor_batches.py wait and lock before creating. dincli aggregator lock-seed and dincli auditor lock-seed added. tests/test_batch_seed_lock.py (10 tests). model-workflow.md updated.
Delay parameter	Reuses disputeSeedDelay.
Audit doc H-2 marked fixed; residuals	Marked fixed with residuals in foundry-src-security-review.md. Residuals tracked as BL-26.

Part C deliverables, checked on develop

Deliverable	Evidence
Commit/reveal functions; sender-bound hash; reveal states in lifecycle position	commitT1Aggregation/revealT1Aggregation and the T2 pair. Hash is keccak256(abi.encode(cid, salt, msg.sender, GI, tier, batchId)). T1AggregationRevealStarted = 18, T2AggregationRevealStarted = 21 in DINShared.sol.
Tests	AggregatorCommitReveal.t.sol (21 tests): copy attack on both tiers, wrong salt/CID, reveal outside the window, double commit/reveal, commit-without-reveal S2-slashed, full GI walk.
State mirrors, DINShared.md §2.1, ordinal table on PR No. 29	dincli/cli/utils.py and DINShared.md updated. Table posted on PR No. 29.
dincli commit/reveal + reveal-window commands, with tests	aggregator reveal-t1 / reveal-t2, model-owner start-reveal for T1 and T2. Salt persisted locally and not regenerated on retry (e373c8d). tests/test_aggregator_commit_hash.py, tests/test_aggregator_commit_retry.py.
New issue for the auditor-side hash-binding gap	No. 192, open.
Audit doc M-1 (aggregation side) marked fixed; No. 156 updated	Marked fixed in the audit doc. No. 156 carries the three-PR split, sender-bound hash, and auditor-shuffle correction in its comments.

Two deviations from the task file, both deliberate

• EIP-170. The task said to stop if either contract crossed 24,576 B. DINTaskCoordinator is 24,585 B with Part C, 9 B over. This was raised in the PR No. 197 re-review and accepted at merge; the size budget and a CI gate are tracked in No. 201 Part A, which blocks any deploy from develop.
• test_knownGap_ flips. Nothing to flip: task_240926_17's P3Adversarial.t.sol doesn't exist yet, so those rows will be written as test_defended_ from the start. The threat-model doc rows were updated in d95abdf.

storage_layout.md needed no change; it covers the proxied platform contracts only.

Follow-ups that outlive this task: No. 192 (auditor-side hash binding), No. 201 (contract size, slash reason for committed-but-unrevealed), BL-26 (seed-lock residuals).

Thanks @Abidoyesimze.

[umeradl]

@abrahamnash this one is ready for post processing.

[abrahamnash]

Thank you, @umeradl