audit(foundry): security review of foundry/src/ at d136ff3

[Abidoyesimze]

Summary

Security review of all 7 contracts in foundry/src/, pinned to commit d136ff3. Findings-only — no API/storage/behavior changes to any contract. Full report: Developer/audits/2026-07_foundry-src-security-review.md. PoC tests: foundry/test/SecurityFindings.t.sol (9 new tests, all passing; existing UpgradeValidation.t.sol suite untouched and still green).

Findings: 0 Critical · 4 High · 4 Medium · 8 Low/Informational

The four upgradeable platform contracts (DinToken, DinCoordinator, DinValidatorStake, DINModelRegistry) came back clean on every proxy-specific check: initializer protection is verified at runtime (not just present in source — see PoC), storage layout is confirmed correct via forge inspect storage-layout, and there's no in-scope upgrade-path access-control issue since Transparent Proxy keeps that authority entirely outside the implementations. The real risk is concentrated in the two non-upgradeable task contracts, DINTaskCoordinator and DINTaskAuditor, which run the actual federated-learning round.

The three most important findings:

H-1 (Sybil-cheap gas-DoS via unbounded registration) confirms the seeded lead: slashAuditors, slashAggregators, finalizeEvaluation, finalizeT1Aggregation, and finalizeT2Aggregation all iterate the entire GI's registrants in one unpaginated transaction, and none of them cap registration the way client submissions are capped at MAX_LM_SUBMISSIONS. Using the contract's own documented "spec" parameters (10 auditors/batch, 100 models/batch), a few hundred registered auditors is enough to blow past any realistic L2 block gas limit — and since MIN_STAKE is 10 DIN at ~0.00001 ETH under the default exchange rate, an attacker can register hundreds of Sybil identities for a few cents and deliberately brick any model's GI on demand, with no recovery path since the task contracts aren't upgradeable.

H-2 (zero-CID sentinel collision, PoC-confirmed) is a genuinely new find: finalizeT1Aggregation/finalizeT2Aggregation use bytes32(0) to mean "nobody submitted," but never reject _aggregationCID == 0 at submission time. Any single assigned aggregator who submits the zero CID — and is the only one of their batch to respond in time, a routine scenario, not an edge case — makes that ambiguity real: the finalize call reverts with TC_NoSubmissions on a batch that had a legitimate submission, and since finalization processes every batch in one transaction, one poisoned batch blocks the whole GI. Permanently, with no override. See test_finalizeT1Aggregation_zeroCID_bricksEntireGI.

H-3 (no quorum on T1/T2 finalization) rounds out the "GI integrity" cluster: unlike DINTaskAuditor, which explicitly enforces minEligibilityQuorum/minScoreQuorum, nothing stops finalizeT1Aggregation/finalizeT2Aggregation from accepting a single aggregator's submission as the batch's "consensus" result if the other two assigned aggregators simply haven't responded yet. That output flows into Tier-2 and ultimately the new global model with zero cross-validation, defeating the point of assigning multiple aggregators per batch.

H-4 confirms the third seeded lead (blockhash/timestamp shuffle is predictable and grindable by the very party — the model owner — who benefits from the outcome). M-1 through M-4 and the Low/Informational table round out the rest; see the report for full detail on each.

What I'd do differently with more time

• Get an exact gas number for H-1 at spec-scale parameters instead of the closed-form estimate — the setup to drive a GI through 8+ phases at ~500 registered participants is straightforward but slow to script.
• H-3 and M-1 (no commit-reveal) probably want to be fixed together — commit-reveal naturally gives you a place to also enforce N-of-M-revealed before finalize is callable.
• Didn't find a fuzzing target worth the setup cost in the exchange-rate/stake-accounting math (0.8's overflow checks close off the classic wraparound class); would still fuzz DinValidatorStake.slash()'s active/pending-withdrawal split given more time since it's the one place subtracting across two balances in the same function.

Test plan

• forge build — clean at pinned commit and after adding PoC tests
• forge test — 13/13 passing (4 pre-existing UpgradeValidation.t.sol + 9 new SecurityFindings.t.sol)
• Every finding's line-number citation spot-checked against source
• Team review of severity ratings, in particular whether H-1/H-2/H-3 should be Critical given the "no recovery path, non-upgradeable contracts" framing — I called them High and explained the reasoning in the report, but reasonable people could land on Critical

[Abidoyesimze]

@umeradl Kindly check this findings and let me know if anything is needed
Thank you

[umeradl]

Overall Review — task_060726_4, Part 1

Thanks @Abidoyesimze — reviewed in full, and this is a strong submission. I pulled the branch, spot-checked the findings against source at the pinned commit, and ran the PoC suite locally. The 9 new tests in SecurityFindings.t.sol all pass, and I independently confirmed H-2 at source level: submitT1Aggregation accepts an unvalidated _aggregationCID while finalizeT1Aggregation uses winningCID == bytes32(0) as its "no submissions" sentinel — the collision is real, and the PoC that drives a full GI through 8+ phase transitions to reach it is well constructed.

What stood out

• Boundary respected exactly. Findings-only, additive tests, no API/storage/behavior changes — as required while the platform contracts are mid-flight in PR Feature/platform upgradeable #13.
• All three seeded leads confirmed with reasoning, and you correctly generalized the unbounded-loop issue (H-1) to the DINTaskCoordinator aggregator path, which wasn't in the seed list.
• Original findings beyond the seeds — H-2 (zero-CID brick), H-3 (missing T1/T2 quorum), and M-2 (the disableModel kill-switch never reaching live task contracts) are all correct and genuinely useful. M-2 especially.
• Proxy checks done properly — initializer protection verified at runtime (direct-impl + double-init), storage layout via forge inspect, and a correct read on Transparent Proxy keeping upgrade authority out of the implementations.
• Candid "what I'd do differently" section, and you flagged the H-1/H-2/H-3 severity question rather than burying it. Appreciated.

Follow-ups before merge

1. Severity calibration on H-1 and H-2. Both produce a permanent, unrecoverable brick of a GI on non-upgradeable contracts, and H-1 is cheap to trigger deliberately. I'm inclined to elevate both to Critical — you raised this exact question yourself, and I think the "permanent brick + cheap trigger + no recovery path" framing lands on Critical. Let's align on that and update the report's severity table.
2. H-1 gas number. The closed-form estimate is convincing, but an actual forge gas measurement at spec-scale params (auditorsPerBatch=10, modelsPerBatch=100, ~500 registrants) would make it airtight. If the setup cost is too high this week, fine to leave as-is with the estimate clearly labeled — but flag it explicitly as an estimate in the report.
3. Minor: in my environment the 4 pre-existing UpgradeValidation.t.sol tests fail due to an FFI/npx path issue in the OZ upgrades CLI (not your code — the 9 new tests pass clean). Just confirming the 13/13 in your description was from your local run.

Context — this closes out Part 1 of the task

For the record, linking the related threads so the full picture is in one place:

• Part 2 (Discussion Filecoin storage support: which provider, given Lighthouse retrieval is payment-gated? #18): your Storacha retrieval verification and the pivot to Filecoin Onchain Cloud after Storacha's infra deprecation are captured in the discussion and in docs(guides): add Storacha integration guide (Web UI, CLI, UCAN auth) #26 / docs(guides): add Filecoin Onchain Cloud integration guide #28. Good adaptability when the recommended provider collapsed mid-task.
• docs(guides): add Storacha integration guide (Web UI, CLI, UCAN auth) #26 (Storacha guide) and docs(guides): add Filecoin Onchain Cloud integration guide #28 (Filecoin Onchain Cloud guide) — noted; docs(guides): add Filecoin Onchain Cloud integration guide #28's content has been relocated/merged into develop.

Net: strong pass on Part 1. Let's settle the two severity ratings and the gas-estimate labeling, and this is good to merge. Nice work.

[umeradl]

Merged manually to develop ✅

@Abidoyesimze — I've landed your Part 1 deliverable on develop, with you as the commit author. Before merging I confirmed the 7 contracts in foundry/src/ are byte-identical between your pinned commit (d136ff3) and current develop, so every finding and line reference applies as-is to the live contracts. Ran your PoC suite against develop too: 9/9 passing (forge test --match-contract SecurityFindingsTest).

Commit: 204268baf14335611ece9b44dbb2745321e876bb

File locations on develop:

• Report: Documentation/technical/audits/foundry-src-security-review.md (relocated from the PR's Developer/audits/ path — audits now live under Documentation/technical/)
• PoC tests: foundry/test/SecurityFindings.t.sol (unchanged location)

Great work on this — accurate, boundary-respecting, all three seeded leads confirmed, and genuinely new findings (H-2, H-3, M-2) on top.

---

Two open follow-ups before we close this out

1. Severity of H-1 and H-2 → I'd like to elevate both to Critical. Both produce a permanent, unrecoverable brick of a GI on non-upgradeable task contracts, and H-1 is cheap to trigger deliberately (~0.00001 ETH per Sybil identity). The "permanent brick + cheap trigger + no recovery path" combination lands on Critical for me — and you raised exactly this question in your PR description. Do you agree? If so, please update the severity table (and the summary counts) in the report; if you see a reason to hold them at High, make the case and let's settle it.

2. H-1 gas number. The closed-form estimate is convincing, but an actual forge gas measurement at spec-scale params (auditorsPerBatch=10, modelsPerBatch=100, ~500 registrants) would make it airtight. If the setup cost is too high this week, it's fine to leave the estimate in — just label it explicitly as an estimate in the report so no one mistakes it for a measured figure.

Since the work is already merged, both of these are follow-up edits directly to Documentation/technical/audits/foundry-src-security-review.md on a small PR into develop — no need to touch anything else. Let me know your thinking on #1 in particular.

[Abidoyesimze]

Thanks for the thorough review — replying to both follow-ups. Sent as a small PR into develop: #30.

1. Severity — agree, H-1 and H-2 → Critical. I'd land on the same call independently: both permanently brick a GI on non-upgradeable contracts with zero recovery path, and both are cheap (H-1) or free (H-2) for a single unprivileged participant to trigger. Renumbered to C-1/C-2, remaining High findings shifted to H-1/H-2, every cross-reference in the report updated. No finding content changed, only labels.

2. Gas number. Went with the real measurement rather than just labeling the estimate. One thing worth surfacing explicitly, since it shapes how the number had to be built: Params.auditorsPerBatch/modelsPerBatch is hardcoded to the demo defaults (3, 3) in the DINTaskAuditor constructor with no setter — there's no way to actually deploy a batch with literal spec-scale internal dimensions without modifying the contract, which is out of scope here. So it's two explicit steps in the report, not blurred together: a real measured slope (two registrant scales, 10 and 30 batches) giving a real per-(auditor,model)-pair cost of 4,471 gas, then a clearly-labeled structural extrapolation of that real number to spec-scale pair/batch counts. Lands at ≈223.5M gas (finalizeEvaluation) and ≈169.5M gas floor (slashAuditors — its early-break-on-first-miss behavior undermeasures the true worst case, noted in the report). Both 5.6×–7.5× over the ~30M block limit — corroborates the original estimate with real data rather than changing the conclusion.

3. FFI/npx note — reproduced independently in a second environment, same failure class, confirmed unrelated to this report's PoCs.

New PoC test (test_gas_finalizeEvaluation_and_slashAuditors_atScale) brings the suite to 10/10 passing.