task_240926_18.md

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

ID: task_240926_18 Assigned to: Similoluwa Abidoye (@Abidoyesimze) Created: 2026-09-24 Status: Open (assigned) Dates: Sep 24 – Oct 1, 2026 (7 days). Part A can start immediately. Parts B and C are gated on task_240926_16 PRs merging (see §Sequencing), so if those slip, say so early in the tracking discussion. Repo: https://github.com/InfiniteZeroFoundation/DevNet Base branch: develop — spec written against commit 3e52351 (2026-09-24); re-pull before starting regardless of when you begin. Roadmap ref: P3-6 (security hardening before audit); findings H-2, M-1, L-2, L-3, L-6 of foundry-src-security-review.md (your own review, task_060726_4) Tracking:

• Part A: #159, #160, #161 (all close, pending review). Scope agreed in the review comments: #159, #160, #161.
• Parts B + C: #156 (closes once both land). Scope and sequencing agreed in the #156 review comment, which supersedes the issue's keccak256(cid, salt) hash and its auditor-side claim.
• Discussion #165 (progress updates, questions, PR links)
• Related, owned by Robbert, not in scope: task_240926_16 (dispute seed #151, treasury forwarding #152, lifecycle events #153), task_240926_17 (adversarial suite #154, tokenomics params #155)

---

Status check at 3e52351

None of the four issues is implemented on develop:

Issue	Finding	State at 3e52351
#159	L-2	DinCoordinator.depositAndMint (:86-94) still has no mintAmount == 0 check
#160	L-3	DINModelRegistry.requestModelRegistration (:161-200) still only checks msg.value < requiredFee and records feePaid: msg.value
#161	L-6	Both task-contract constructors (DINTaskCoordinator.sol:223, DINTaskAuditor.sol:400) still take dependency addresses unchecked. Both now also take a modelId_ argument, which the issue text predates.
#156	H-2/M-1, aggregation side	_shuffleAddressArray still seeds from blockhash(block.number - 1) (DINTaskCoordinator.sol:553), _shuffleUintArray from block.timestamp + msg.sender (:565). submitT1Aggregation/submitT2Aggregation still take a plaintext CID (:694, :790).

Two corrections to #156 as written:

• "Already fixed on the auditor side by PR #63" is only half true. PR #63 added commit-reveal for auditor scores (M-1, auditor side). The auditor batch shuffle (DINTaskAuditor._shuffleAddressArray/_shuffleUintArray, :743/:779, used by createAuditorsBatches) is still blockhash/timestamp-seeded. H-2 is open on both sides. Part B covers both.
• The issue only names the address shuffle. _shuffleUintArray (the model-index shuffle) decides which approved models land in which T1 batch. task_240926_17's threat model relies on it for the "owner routes a poisoned model into a colluding T1 batch" row, so it's in scope too.

---

Sequencing with task_240926_16 / task_240926_17

Robbert's two tasks run Sep 24 – Oct 1 and touch the same files. Each part below is ordered to avoid conflicts:

Part	Touches	Conflicts with	Rule
A (#159/#160/#161)	DinCoordinator.depositAndMint, DINModelRegistry.requestModelRegistration, both task-contract constructors + setDINTaskAuditorContract	Nothing in task_16/17 touches these functions. task_17 Part 3a calls updateDinPerEth from the deploy script only.	Start immediately.
B (#156 H-2)	_shuffleAddressArray/_shuffleUintArray in both task contracts, autoCreateTier1AndTier2, createAuditorsBatches	task_16 Part A1 changes _shuffleAddressArray to take an explicit bytes32 seed, and explicitly leaves the autoCreateTier1AndTier2 seed to #156.	Start after task_16 PR 1 (Part A) merges. Build on its helper and don't fork a second shuffle.
C (#156 M-1)	submitT1Aggregation/submitT2Aggregation/finalize*, the S2 slash loop, GIstates enum, dincli aggregator commands	task_16 Part C adds _setGIstate and emits T1AggregationSubmitted/T2AggregationSubmitted in the submit functions. Its subgraph half (C5) regenerates the uint8 → GIstates mapping. task_17 Part 3b edits the S2 slash loop (:888).	Start after task_16 PR 3 (Part C) merges. Rebase on task_17 PR 3 if it has merged.

task_17 Part 2 will contain test_knownGap_ tests for the batch-routing / shuffle-steering rows. When Part B (or C) closes one of those gaps, flip that test to test_defended_ in your PR, as task_17's Notes require.

---

Part A — Low findings L-2, L-3, L-6 (#159, #160, #161) — PR 1

Three small, independent fixes. One PR, one commit per issue.

A1. #159 — depositAndMint zero-mint (L-2)

• In DinCoordinator.depositAndMint, after computing mintAmount, add if (mintAmount == 0) revert ZeroMintAmount(); with a new custom error.
• DinCoordinator is behind a transparent proxy. A new error adds no storage, but run UpgradeValidation.t.sol anyway.
• Test in DinCoordinator.t.sol: set a non-round dinPerEth via updateDinPerEth, send a msg.value that rounds to 0, and assert the revert and that no ETH is retained.

A2. #160 — requestModelRegistration overpayment (L-3)

• Shape: refund the excess. This is the issue's own preference, and it tolerates the stale-fee race. If you prefer exact-match, raise it on #160 first.
• Record feePaid: requiredFee, not msg.value.
• Refund msg.value - requiredFee to msg.sender after the modelRequests.push and the event (checks-effects-interactions), and revert on a failed send with a custom error.
• Reentrancy: DINModelRegistry has no guard today. Add ReentrancyGuardTransient, as DinCoordinator already does (DinCoordinator.sol:7). It uses transient storage, so the proxy storage layout is unchanged. Confirm that against storage_layout.md and UpgradeValidation.t.sol.
• dincli/cli/task.py:263 already sends exactly required_fee, so no dincli change is needed. Confirm that.
• Tests in DINModelRegistry.t.sol:
  • an overpayment is refunded and feePaid == requiredFee
  • an exact payment is unchanged
  • a refund to a reverting receiver reverts the whole call
• Out of scope, just note it in the PR: rejectModel (:250) doesn't refund feePaid at all. Whether a rejected request should get its fee back is a policy question, not an L-3 fix.

A3. #161 — zero-address dependencies (L-6)

• DINTaskCoordinator constructor: revert TC_InvalidAddress if dinvalidatorStakeContract_address == address(0).
• DINTaskAuditor constructor: revert TA_InvalidAddress if either address argument is zero.
• Also DINTaskCoordinator.setDINTaskAuditorContract (:234). It's one-shot and advances GIstate right away, so passing address(0) there bricks the contract the same way. Same check, same error.
• Both errors already exist. Don't add new ones.
• Tests: one revert test per argument, plus the setter. Put them wherever the existing constructor/setup tests live, or in SecurityFindings.t.sol next to your other PoCs, and say which in the PR.

Deliverables (Part A)

• depositAndMint reverts on a zero mintAmount; test at a non-round dinPerEth
• requestModelRegistration refunds the excess, records feePaid = requiredFee, and is nonReentrant; tests for overpay/exact/reverting-receiver; storage layout confirmed unchanged
• Zero-address checks on both constructors and setDINTaskAuditorContract; revert tests for each
• forge build && forge test green, full suite, including UpgradeValidation.t.sol
• Bundled ABIs in dincli/abis/ refreshed if they changed (new errors change the ABI)
• Audit doc: mark L-2/L-3/L-6 as fixed, with a link to the PR

Part B — Ungrindable batch-assignment seed (#156 H-2, both tiers) — PR 2

Starts after task_16 PR 1 (Part A) merges.

Design: reuse task_16 Part A's future-block seed

task_16 Part A fixes the dispute-redo shuffle (BL-11) with a future-block seed fixed in advance plus a permissionless lock. Part B uses the same pattern for the two regular batch-creation calls. We end up with one randomness pattern across the protocol, and one sequencer-trust caveat.

• Anchor at the preceding state transition, which the owner can't aim because the seed block is still in the future:
  • T1/T2: closeLMsubmissionsEvaluation (DINTaskCoordinator.sol:472) sets aggSeedBlock[GI] = block.number + seedDelay
  • auditor batches: closeLMsubmissions (:411, the transition to LMSclosed) sets auditSeedBlock[GI] the same way
• A permissionless lock…Seed(GI) requires block.number > seedBlock. It stores keccak256(abi.encodePacked(blockhash(seedBlock), GI, <domain tag>)), and if more than 256 blocks have passed (blockhash == 0) it re-anchors. Mirror task_16 A3 exactly, and never store or use a zero-derived seed.
• autoCreateTier1AndTier2 and createAuditorsBatches revert if the seed isn't locked. Otherwise they don't change.
• Both shuffles take the locked seed, domain-separated so the address and index shuffles don't correlate, e.g. keccak256(seed, "AGG") / keccak256(seed, "IDX"). Drop block.timestamp and msg.sender from _shuffleUintArray entirely.
• Delay parameter: reuse task_16's disputeSeedDelay if it fits, otherwise add one with the same setter + non-zero-guard pattern. Say which in the PR.
• DINTaskAuditor's shuffles: give them the same seed-taking signature as task_16 A1's coordinator helper. Don't add a third shuffle variant.
• NatSpec: state the residual trust assumption (the OP Stack sequencer produces blockhash), exactly as task_16 A4 does. VRF stays the mainnet follow-up.

dincli (Part B)

• The model owner now needs one extra call per batch-creation step (lock the seed), and must wait seedDelay blocks first.
• Add it to dincli/cli/modelownerd/aggregation.py (before autoCreateTier1AndTier2, :36) and auditor_batches.py. Poll until block.number > seedBlock, then lock, then create. If the seed is already locked (someone else locked it), skip the lock step.
• Update Documentation/public/workflows/model-workflow.md for the extra step.
• Tests in tests/ using the existing monkeypatch + CliRunner pattern.

Deliverables (Part B)

• Seed anchored at the preceding transition; permissionless lock with the >256-block re-anchor; zero seed never used
• autoCreateTier1AndTier2 and createAuditorsBatches require a locked seed; all four shuffles (address + index, both contracts) use it with domain separation; no blockhash(block.number - 1), block.timestamp, or msg.sender entropy left in any shuffle
• Tests:
  • timing independence: the same GI batched at two different blocks (vm.snapshot/vm.revertTo + vm.roll) yields identical T1 batches and identical auditor batches
  • gating, re-anchor, lock guards, permissionless lock (as in task_16 A's test list)
• Any task_17 test_knownGap_ for shuffle steering / batch routing flipped to test_defended_
• dincli lock-then-create flow + tests; model-workflow.md updated
• forge build --sizes before/after in the PR. DINTaskAuditor is 22,382 B at 3e52351, ~2.2 KB under EIP-170, and task_16 B/C will add to it. If either contract crosses 24,576 B, stop and raise it in the tracking discussion. Don't split contracts on your own initiative.
• Bundled ABIs refreshed; audit doc H-2 marked fixed (both sides); storage_layout.md updated if it records the changed state

Part C — Commit-reveal on T1/T2 submissions (#156 M-1, aggregation side) — PR 3

Starts after task_16 PR 3 (Part C) merges.

Design

Mirror PR #63's auditor commit-reveal, with one hardening:

• Bind the committer into the hash: commitHash = keccak256(abi.encode(cid, salt, msg.sender, GI, tier, batchId)), not keccak256(cid, salt) as #156 proposes.
  • Without msg.sender, a lazy aggregator copies an honest peer's commit hash, waits for that peer's reveal, and reveals the same (cid, salt). That is exactly the free-riding M-1 is meant to stop.
  • PR #63's auditor hash (keccak256(abi.encodePacked(score, vote, salt)), DINTaskAuditor.sol:1127) has this same weakness. Don't fix it here. Open a new issue for it (auditor-side M-1 residual) and link it in the PR.
• Reveal-window states. T1AggregationStarted becomes the commit window, then a new T1AggregationRevealStarted state, then finalizeT1Aggregation. Same for T2 with a new T2AggregationRevealStarted. The owner opens each reveal window, the same way the auditor-side reveal is opened.
  • Enum placement: insert in lifecycle position, not append. This is the 2026-08-27 precedent recorded in DINShared.sol:25-34.
  • This shifts every later ordinal, so you must:
    • update dincli/cli/utils.py's states/stateDescription mirrors
    • update DINShared.md §2.1
    • post the new ordinal table on PR #29 so the subgraph mapping (task_16 C5) gets regenerated. Robbert owns the subgraph; don't push to his branch.
  • Route every new transition through task_16's _setGIstate.
• Votes count only on reveal. t1Votes/t2Votes and t1SubmissionCID/t2SubmissionCID are written in reveal, not commit. finalize* counts only revealed submissions. The T1AggregationSubmitted/T2AggregationSubmitted events (task_16 C1) move to the reveal function, with signatures unchanged. Add T1AggregationCommitted/T2AggregationCommitted events only if the subgraph needs them; ask on #29.
• Commit but no reveal = missed submission for S2. Check that the S2 slash loop and the dispute/recomputation paths (:912, :947, :1275) read revealed state only, and that reward-weight accounting in finalize* (task_100926_11's aggregatorWeight) is unchanged for revealed aggregators.
• Don't change dispute semantics. Disputes open against the finalized CID exactly as today.

dincli (Part C)

This is the same shape of work as PR #63's auditor side (auditor.py, lms_evaluation.py, utils.py):

• dincli/cli/aggregator.py: split the T1 (:342) and T2 (:494) submit paths into commit + reveal. Persist the salt locally the same way the auditor commit-reveal does, and never regenerate it on retry.
• dincli/cli/modelownerd/aggregation.py: commands to open the T1/T2 reveal windows.
• Tests following PR #63's pattern.

Deliverables (Part C)

• commitT1Aggregation/revealT1Aggregation and T2 equivalents; hash binds msg.sender, GI, tier, batchId; new reveal states inserted in lifecycle position
• Votes, CIDs, and task_16's submission events are written on reveal only; unrevealed = missed for S2; rewards/disputes unaffected
• Tests (a new AggregatorCommitReveal.t.sol, mirroring AuditorCommitReveal.t.sol):
  • copy attack defeated: an aggregator replaying a peer's commit hash can't reveal it
  • a wrong salt/CID reveal reverts; reveal outside the window reverts; double commit/reveal reverts
  • a commit without reveal is S2-slashed
  • full GI walk through the new states
• dincli/cli/utils.py state mirrors, DINShared.md §2.1, and the ordinal table posted on PR #29
• dincli commit/reveal for aggregators + model-owner reveal-window commands, with tests; model-workflow.md updated
• New issue opened for the auditor-side commit-hash binding gap and linked
• forge build --sizes before/after in the PR (same EIP-170 rule as Part B; DINTaskCoordinator is 20,159 B at 3e52351)
• forge build && forge test + pytest green; ABIs refreshed; audit doc M-1 (aggregation side) marked fixed
• Update #156's issue text (or comment) to reflect the three-PR split, the sender-bound hash, and the auditor-shuffle correction above

Submission

Three PRs from separate branches into develop, each rebased on develop after the previous one (and the gating task_16 PR) merges:

• PR 1 (Part A): references #159, #160, #161 (closes). Say whether A3's tests went into SecurityFindings.t.sol and confirm A2's storage layout is unchanged.
• PR 2 (Part B): references #156 (H-2 half). Include the delay-parameter decision, --sizes numbers, and any task_17 tests flipped.
• PR 3 (Part C): references #156 (closes). Include the new ordinal table, --sizes numbers, and the new auditor-side issue link.

Post each PR link and progress updates in Discussion #165.

Notes

• Flag every DINTaskCoordinator.sol/DINTaskAuditor.sol/DINShared.sol change for Umer's review, same as prior tasks in this area.
• Parts B and C will rebase onto Robbert's in-flight work. If a task_16 PR stalls, don't build Part B/C against its branch. Say so in the tracking discussion, and pick up Part A follow-ups or the auditor-side issue write-up in the meantime.
• If you'd rather use VRF or multi-party commit-reveal than the future-block seed for Part B, raise it on #156 before building it. The point of Part B is one consistent pattern with task_16 Part A.
• C-1 (registration gas-DoS) and M-2 (disableModel kill-switch) remain out of scope, as #156 already states.