From f5afbd30484838c4a47b7635c093068ca31aad94 Mon Sep 17 00:00:00 2001 From: Similoluwa Abidoye Date: Wed, 8 Jul 2026 11:09:18 +0100 Subject: [PATCH] audit(foundry): security review of foundry/src/ at d136ff3 Severity-tagged findings report (0 Critical / 4 High / 4 Medium / 8 Low-Informational) covering all 7 contracts, plus proxy-specific checks for the 4 upgradeable platform contracts (initializer protection verified at runtime, storage layout confirmed via forge inspect). Confirms all 3 seeded DINTaskAuditor leads as real. Findings-only: no contract in foundry/src/ was modified. PoC tests added in foundry/test/SecurityFindings.t.sol (9 new, 13/13 passing alongside the existing UpgradeValidation.t.sol suite). --- .../2026-07_foundry-src-security-review.md | 240 ++++++++++++++ foundry/test/SecurityFindings.t.sol | 307 ++++++++++++++++++ 2 files changed, 547 insertions(+) create mode 100644 Developer/audits/2026-07_foundry-src-security-review.md create mode 100644 foundry/test/SecurityFindings.t.sol diff --git a/Developer/audits/2026-07_foundry-src-security-review.md b/Developer/audits/2026-07_foundry-src-security-review.md new file mode 100644 index 0000000..09d9aeb --- /dev/null +++ b/Developer/audits/2026-07_foundry-src-security-review.md @@ -0,0 +1,240 @@ +# Foundry `src/` Security Review — July 2026 + +**Reviewer:** Similoluwa Abidoye (@Abidoyesimze) +**Scope:** All 7 contracts in `foundry/src/` +**Pinned commit:** `d136ff3ecef06670a3b785807669c40e027bcdf3` — reviewed as-is; `develop` has moved on since (see `Developer/tasks/task_060726_4.md` §7 for why the pin is deliberate). All line numbers below refer to this commit. +**Task:** `Developer/tasks/task_060726_4.md`, Part 1 +**Companion reading:** `Documentation/DIN-workflow.md`, `Documentation/Model-workflow.md`, `Documentation/technical/upgradable-contracts/` (as of `develop@805ce9d`) + +This is a findings-only report. No contract in `foundry/src/` was modified. PoC tests are additive, in `foundry/test/SecurityFindings.t.sol`, and run with: + +```bash +cd foundry && forge test --match-contract SecurityFindingsTest -vv +``` + +Baseline confirmed before review: `forge build` and `forge test` both pass clean at the pinned commit (4/4 existing tests in `UpgradeValidation.t.sol`). + +--- + +## Summary + +| Severity | Count | +|---|---| +| Critical | 0 | +| High | 4 | +| Medium | 4 | +| Low / Informational | 8 | + +The headline risk isn't in the four upgradeable platform contracts — their proxy conversion is careful and the initializer/storage-layout checks in this report all came back clean. It's in the two **non-upgradeable task contracts** (`DINTaskCoordinator`, `DINTaskAuditor`), which run the actual federated-learning round: they have several ways for a single low-stake participant to permanently brick a Global Iteration, and no on-chain recovery once that happens (redeploy is the only path, per the documented "task contracts are disposable" design — but that design assumes *planned* redeployment, not mid-round griefing). + +--- + +## High Severity + +### H-1. Sybil-cheap gas-DoS: unbounded auditor/aggregator registration feeds four O(n) or worse loops with no pagination + +**Contracts / functions:** +- `DINTaskAuditor.sol`: `_activeAuditorPool()` (L275-297), `slashAuditors()` (L620-659), `finalizeEvaluation()` (L552-612) +- `DINTaskCoordinator.sol`: `_activeAggregatorPool()` (L410-432), `slashAggregators()` (L678-755), `finalizeT1Aggregation()` (L549-582), `finalizeT2Aggregation()` (L627-660) + +**This is the seeded lead, confirmed real — not refuted.** + +`registerDINAuditor()` and `registerDINaggregator()` have **no registration cap**, unlike client submissions which are capped at `MAX_LM_SUBMISSIONS = 10000`. Every one of the functions above iterates over the full historical registrant list (`dinAuditors[_GI]` / `dinAggregators[_GI]`), and several nest that iteration inside a second loop over batches and a third over models-per-batch: + +```solidity +// DINTaskAuditor.slashAuditors — triple-nested, no chunking +for (uint b = 0; b < batchCount; b++) { // O(batches) + for (uint a = 0; a < batch.auditors.length; a++) { // O(auditorsPerBatch) + for (uint m = 0; m < batch.modelIndexes.length; m++) { // O(modelsPerBatch) + if (!hasAuditedLM[...]) { missedVote = true; break; } + } + if (missedVote) dinvalidatorStakeContract.slash(...); // external call + SSTORE + event + } +} +``` + +batches scales linearly with registered auditors (`batches ≈ registeredAuditors / auditorsPerBatch`), so total loop body executions are `O(registeredAuditors × modelsPerBatch)`, and every "missed vote" auditor costs one more external `slash()` call (~50k+ gas each with the SSTORE + event in `DinValidatorStake`). + +**Concrete cost math, using the contract's own documented parameters** (`Params` struct comments in `DINTaskAuditor.sol` L36-43 literally say "demo: 3… spec: 10" / "demo: 3… spec: 100"): + +- At spec scale (`auditorsPerBatch=10`, `modelsPerBatch=100`): **one single batch** already requires 1,000 `hasAuditedLM` reads before any slashing even starts. 50 batches (500 registered auditors — a modest number for a "decentralized" network) is 50,000 loop iterations plus up to 500 external `slash()` calls. That is well past any realistic L2 block gas limit (Optimism's is ~30M as of this review); the transaction reverts with out-of-gas every time it's retried, and there is **no smaller-batch entry point to work around it** — `slashAuditors`/`slashAggregators`/`finalizeT1Aggregation`/`finalizeT2Aggregation`/`finalizeEvaluation` all process the *entire* GI in one call. +- **And it's cheap to trigger deliberately.** `MIN_STAKE` is 10 DIN, bought at the default rate of 1,000,000 DIN/ETH (`DinCoordinator.initialize`, L58) — **0.00001 ETH per Sybil identity**. Registering 1,000 Sybil auditor addresses costs ~0.01 ETH plus L2 gas, and since staked DIN isn't consumed by registering (only by being slashed), the attacker can unstake and reuse it after the 7-day unbonding window. This is not a scaling accident that only bites at organic success — it's a cheap, repeatable attack a single actor can execute against any model's GI today. + +**Failure scenario:** An attacker registers a few hundred throwaway addresses as auditors for a target model's GI (trivial cost, no collusion needed with anyone else). Once the model owner calls `slashAuditors()` (or `finalizeEvaluation()`, or the coordinator's `slashAggregators()`/`finalizeT1Aggregation()`/`finalizeT2Aggregation()` with a matching flood of aggregator Sybils), the call runs out of gas every time. The GI is now stuck at that phase permanently — `DINTaskCoordinator`/`DINTaskAuditor` are not upgradeable, so there is no way to patch around it; the model owner's only recourse is to abandon the GI and redeploy new task contracts (losing all state and honest participants' in-flight work for that round). + +**Recommendation:** Cap registrant counts the same way `MAX_LM_SUBMISSIONS` caps client submissions, and/or add pagination to every finalize/slash function (e.g. `slashAuditors(uint _GI, uint startBatch, uint endBatch)`), tracking a `lastProcessedBatch` cursor so a GI can be advanced through many smaller transactions instead of one unbounded one. + +--- + +### H-2. Zero-CID sentinel collision permanently bricks `finalizeT1Aggregation` / `finalizeT2Aggregation` for the whole GI + +**Contract / functions:** `DINTaskCoordinator.sol`, `finalizeT1Aggregation()` (L549-582), `finalizeT2Aggregation()` (L627-660), and the corresponding `submitT1Aggregation()` / `submitT2Aggregation()` which accept an unvalidated `_aggregationCID`. + +**Confirmed exploitable — PoC: `test_finalizeT1Aggregation_zeroCID_bricksEntireGI` in `foundry/test/SecurityFindings.t.sol`.** + +Both finalize functions use `bytes32(0)` as a sentinel for "nobody submitted anything in this batch": + +```solidity +bytes32 winningCID = ""; +uint maxVotes = 0; +for (uint j = 0; j < b.aggregators.length; j++) { + ... + if (votes > maxVotes) { maxVotes = votes; winningCID = cid; } +} +if (winningCID == bytes32(0)) revert TC_NoSubmissions(); // <-- ambiguous +``` + +`submitT1Aggregation`/`submitT2Aggregation` never validate `_aggregationCID != bytes32(0)`. Any assigned aggregator can submit `bytes32(0)` as their own genuine vote. If that vote ends up as the batch's plurality (trivially true if they're the only one of the 3 assigned aggregators who submits before the window closes — a routine "not everyone responded" scenario, not an edge case), `winningCID` legitimately equals `bytes32(0)` and the code cannot distinguish "a real submission of the zero CID" from "no one submitted." The function reverts. + +Because `finalizeT1Aggregation`/`finalizeT2Aggregation` loop over **every batch in the GI in one transaction** and revert the whole call on the first bad batch, **one poisoned batch blocks finalization for every other (honest) batch too**. There is no admin override to skip a stuck batch, and the GI cannot progress past `T1AggregationStarted`/`T2AggregationStarted` — permanently, since these contracts aren't upgradeable. + +**Failure scenario (see PoC):** 3 aggregators are assigned to a Tier-1 batch. Two never submit (they don't have to be malicious — just slow, offline, or the model owner closes the window before they respond). The third submits `bytes32(0)`. It is the only submission, so it's unambiguously the plurality winner. `finalizeT1Aggregation` reverts with `TC_NoSubmissions` even though a legitimate submission exists, and stays reverting on every retry. + +**Recommendation:** Reject `_aggregationCID == bytes32(0)` at submission time in both `submitT1Aggregation` and `submitT2Aggregation` (fail fast, cheaply, on the individual submitter rather than the whole GI), and/or track submission presence with an explicit `bool hasSubmissions` per batch rather than inferring it from the winning CID's value. + +--- + +### H-3. No quorum enforced before Tier-1/Tier-2 aggregation is accepted as "final" — a single aggregator's output can become the consensus result + +**Contract / functions:** `DINTaskCoordinator.sol`, `finalizeT1Aggregation()` (L549-582), `finalizeT2Aggregation()` (L627-660). + +Compare this to `DINTaskAuditor`, which explicitly enforces `minEligibilityQuorum`/`minScoreQuorum` before treating a vote as final (`_tryFinalizeEligibility`, L487-489; `finalizeEvaluation`, L596). `finalizeT1Aggregation`/`finalizeT2Aggregation` have **no equivalent check**. The only condition for accepting a "winning" CID is `winningCID != bytes32(0)` — i.e., *someone* submitted *something*. If only 1 of the 3 assigned aggregators submits before the model owner closes the window (no malice required — the other two may simply not have finished their off-chain aggregation yet), that lone submission is accepted as the batch's final, majority-agreed result with zero cross-validation from any other party. + +**Failure scenario:** A malicious or buggy aggregator submits a garbage/incorrect CID and is the only one of their batch to submit in time. `finalizeT1Aggregation` accepts it as `finalCID` with no dissent recorded anywhere, and that CID flows into Tier-2 aggregation and ultimately becomes (part of) the new global model — silently, with no on-chain signal that only 1-of-3 parties actually agreed. The entire point of a multi-aggregator batch (fault tolerance / cross-validation) is defeated by the absence of a minimum-participation check. + +**Recommendation:** Require a minimum submission count (e.g. `submitted.length >= T1_AGGREGATORS_PER_BATCH / 2 + 1`, mirroring the auditor contract's quorum pattern) before `finalizeT1Aggregation`/`finalizeT2Aggregation` will accept a winning CID; revert (or mark the batch as "unresolved" for out-of-band handling) otherwise. + +--- + +### H-4. Predictable, grindable pseudo-randomness in auditor/aggregator batch shuffling enables collusion + +**Contracts / functions:** +- `DINTaskAuditor.sol`: `_shuffleAddressArray()` (L263-273), `_shuffleUintArray()` (L299-308), both used by `createAuditorsBatches()` +- `DINTaskCoordinator.sol`: `_shuffleAddressArray()` (L379-389), `_shuffleUintArray()` (L391-400), both used by `autoCreateTier1AndTier2()` + +**This is the seeded lead, confirmed real — not refuted.** + +Both contracts derive their Fisher-Yates shuffle entropy from `blockhash(block.number - 1)` (address shuffle) and `block.timestamp` (model-index shuffle, further combined with `msg.sender`, which is always the same fixed coordinator address per model — contributing no real entropy). Both inputs are public and known *before* the batch-creation transaction is even submitted, since the previous block is already final by the time anyone calls `createAuditorsBatches`/`autoCreateTier1AndTier2`. + +The caller of these functions (the model owner, since both are gated `onlyOwner`/routed through the owner-only coordinator call) can therefore **compute the resulting batch assignment offline before submitting**, and can choose *when* to submit (i.e., wait for a block whose hash produces a favorable shuffle) since retrying costs only gas on an L2. Combined with H-1's cheap Sybil registration: + +**Failure scenario:** A model owner registers a handful of Sybil-controlled auditor addresses alongside honest ones. Before calling `createAuditorsBatches`, they simulate the shuffle for the current `blockhash(block.number - 1)` locally; if the resulting batch doesn't cluster ≥2 of their Sybils into the same batch as their target model (default `auditorsPerBatch=3`, `minEligibilityQuorum=2` — only 2-of-3 needed to control a batch's eligibility outcome), they simply wait for the next block and recompute. Once a favorable block arrives, they submit. Their colluding auditors then rubber-stamp their own (possibly low-quality or malicious) submitted model as eligible, defeating the purpose of independent auditor review. The same grinding applies to `autoCreateTier1AndTier2`'s aggregator/model shuffle. + +**Recommendation:** Replace blockhash/timestamp entropy with a source the caller cannot pre-compute against before choosing whether to submit — e.g. commit-reveal (commit to a seed one block before creating batches, so the batch-creator can't selectively decide post-hoc) or a VRF (Chainlink VRF or similar). At minimum, do not let the same address that benefits from the shuffle outcome (the model owner) be the one who chooses the exact block it executes in. + +--- + +## Medium Severity + +### M-1. No commit-reveal on aggregation/scoring submissions — "copy the leader" free-riding + +**Contracts / functions:** `DINTaskCoordinator.submitT1Aggregation()` / `submitT2Aggregation()` (L520-543, L600-622); `DINTaskAuditor.setAuditScorenEligibility()` (L515-544). + +Votes/scores/CIDs are submitted in the clear and tallied by direct value match; there is no commit-then-reveal step. Any participant who is not the first to submit for a given batch/model can read every prior submission from public contract state before deciding what to submit themselves. A lazy or dishonest aggregator can copy another party's already-submitted CID instead of doing the aggregation work, guaranteeing they "match consensus" and avoid `AGG_T1_BAD_CONSENSUS`/`AGG_T2_BAD_CONSENSUS` slashing while contributing nothing. The same applies to an auditor submitting last on `setAuditScorenEligibility` — they can see the running vote tally (`_tryFinalizeEligibility` is invoked after every vote, so intermediate state is observable) and simply match the emerging majority. + +This doesn't cause direct fund loss, but it undermines the core assumption that agreement among independently-computed results is meaningful signal — with copying possible, "consensus" can be manufactured by a single honest party plus N idle followers. + +**Recommendation:** Commit-reveal: submitters post `keccak256(cid, salt)` during the submission window, then reveal `(cid, salt)` in a second window after submissions close. Standard mitigation for this exact class of on-chain "peek and copy" issue. + +--- + +### M-2. `DINModelRegistry.disableModel()` kill-switch doesn't reach the live task contracts + +**Contracts / functions:** `DINModelRegistry.sol`, `disableModel()`/`enableModel()` (L404-416); contrast with `DINTaskCoordinator.sol` and `DINTaskAuditor.sol`, neither of which references `DINModelRegistry` at all. + +`modelDisabled[modelId]` only gates `requestManifestUpdate` (via the `notDisabled` modifier) inside the registry itself. It has **zero effect** on the model's actual `DINTaskCoordinator`/`DINTaskAuditor` — those contracts have no dependency on, or awareness of, the registry. A model that DIN-Representative has disabled (e.g., because it was found to be malicious, or is slashing honest validators due to a bug) can keep running full GIs — registration, LMS, evaluation, aggregation, slashing — completely unaffected. + +**Failure scenario:** DIN-Representative discovers a model's task contracts have a bug that's wrongfully slashing honest auditors (or is otherwise harmful) and calls `disableModel()` expecting it to function as an emergency stop. It doesn't — the GI in progress continues, and honest validators keep getting exposed to (and slashed by) the disabled model until the model owner voluntarily stops calling functions on it (which they have no obligation to do, especially if they're the malicious party). + +**Recommendation:** If `disableModel` is meant to be an emergency stop (the task's own contract table calls it a "kill-switch"), either (a) have the task contracts check `DINModelRegistry.modelDisabled(modelId)` before allowing state-changing calls (requires wiring the model ID and registry address into the task contracts — a larger change, out of scope for this findings-only pass, but worth a design ticket), or (b) explicitly document that `disableModel` is registry-metadata-only and does not stop a live GI, so operators don't rely on it as a circuit breaker it isn't. + +--- + +### M-3. `DINTaskAuditor.slashAuditors()` has no internal GI-state gate + +**Contract / function:** `DINTaskAuditor.sol`, `slashAuditors()` (L620-659). + +Every other state-changing function in `DINTaskAuditor` independently re-checks `dintaskcoordinatorContract.GIstate()` before acting (`createAuditorsBatches` requires `LMSclosed`; `setTestDataAssignedFlag` requires `AuditorsBatchesCreated`; `finalizeEvaluation` requires `LMSevaluationStarted`). `slashAuditors()` breaks that pattern — it only checks `onlyTaskCoordinator` + `onlyCurrentGI`, with no GI-state precondition of its own. Correctness currently depends entirely on `DINTaskCoordinator.slashAuditors()` (L665-671) gating the call to `GIstate == T2AggregationDone` — i.e. a single point of trust with no defense-in-depth, unlike everywhere else in this pair of contracts. + +Today this isn't independently exploitable (the coordinator's gate holds), but it's the one function in the file that would let a future change to `DINTaskCoordinator` (or a bug in it) trigger slashing at the wrong phase with no second check catching it. + +**Recommendation:** Add an explicit `GIstates` check inside `DINTaskAuditor.slashAuditors()` itself, consistent with every other function in the contract. + +--- + +### M-4. `DinValidatorStake`'s `Jailed` status is dead code + +**Contract / function:** `DinValidatorStake.sol` — `ValidatorStatus.Jailed` (L46), `jailedUntil` field (L54), read at L269 and L324-325. + +No function anywhere in `DinValidatorStake.sol` (or any other contract in scope) ever writes a non-zero value to `jailedUntil`, or sets `status = ValidatorStatus.Jailed`, except the restore-path inside `unblacklistValidator()` — which itself only re-enters `Jailed` if `jailedUntil > block.timestamp`, a condition that can never be true since nothing ever sets `jailedUntil`. The entire jailing mechanism referenced in the enum and struct is unreachable. + +This isn't exploitable, but it either indicates a missing feature (a `jail()` function was planned/removed but the supporting state/logic was left behind) or dead state that should be removed. Worth a product/eng decision rather than silent removal, since `Documentation/DIN-workflow.md`/`Model-workflow.md` don't mention jailing as a current mechanism either. + +**Recommendation:** Confirm with the team whether jailing is planned; if not, remove `Jailed`/`jailedUntil` to avoid confusing future readers (flagging only — no change made here per the findings-only boundary). + +--- + +## Low / Informational + +| # | Finding | Location | +|---|---|---| +| L-1 | `stake()` calls `DIN_TOKEN.safeTransferFrom` (external call) before updating `activeStake` — violates checks-effects-interactions. Mitigated today by `nonReentrant` + `DIN_TOKEN` being a trusted, protocol-deployed contract, but worth fixing on principle. | `DinValidatorStake.sol` L113-126 | +| L-2 | `depositAndMint()` has no minimum-deposit / non-zero-mint check. If the owner ever sets `dinPerEth` to a value that isn't a clean multiple of `1e18`, a small enough `msg.value` can round `mintAmount` to 0 via integer division while the ETH is still retained by the contract. | `DinCoordinator.sol` L64-71 | +| L-3 | `requestModelRegistration` doesn't refund `msg.value` above the required fee — any overpayment is silently kept. | `DINModelRegistry.sol` L155-194 | +| L-4 | `rejectModel()` never refunds the fee paid in the corresponding `requestModelRegistration` — a rejected requester loses their fee permanently. Confirm this is the intended design (anti-spam fee) rather than an oversight. | `DINModelRegistry.sol` L244-254 | +| L-5 | `withdrawFees(address payable to)` has no zero-address check; calling it with `to == address(0)` silently burns the entire fee balance (owner-only footgun, not exploitable by a third party). | `DINModelRegistry.sol` L472-477 | +| L-6 | `DINTaskCoordinator`/`DINTaskAuditor` constructors accept `dinvalidatorStakeContract_address` / `dintaskcoordinator_contract_address` with no zero-address check. Self-inflicted misconfiguration risk only (deployer controls the args), not attacker-triggered. | `DINTaskCoordinator.sol` L87-92, `DINTaskAuditor.sol` L148-167 | +| L-7 | `totalDepositedRewards` and the `RewardDeposited` event are declared but never written/emitted anywhere; the contract also has no `receive()`/`fallback()`, so any ETH that ever reaches this contract (e.g. via a forced `selfdestruct` send) would be permanently unwithdrawable. Dead code / incomplete feature. | `DINTaskAuditor.sol` L16, L102 | +| L-8 | `updateDinPerEth()` / `updateValidatorStakeContract()` take effect immediately with no timelock, giving depositors a front-run/back-run arbitrage window around rate changes. Inherent to the documented "tentative workaround" centralized exchange-rate design — flagged for completeness, not a new issue. | `DinCoordinator.sol` L106-120 | + +--- + +## Proxy-Specific Checks — the 4 Upgradeable Platform Contracts + +`DinToken`, `DinCoordinator`, `DinValidatorStake`, `DINModelRegistry`. Checked per `Documentation/technical/upgradable-contracts/hardhat/README.md` and the task's minimum-coverage list. + +### Initializer protection — PASS, verified at runtime, not just in source + +The task explicitly asked whether `_disableInitializers()` is *effective at runtime*, not just present in the source. Verified with two independent PoCs per contract (8 tests total, all passing — `foundry/test/SecurityFindings.t.sol`): + +1. **Direct call to a freshly-deployed implementation's `initialize()` reverts** (`test_implementation_rejectsDirectInitialize_*`) — confirms `_disableInitializers()` in the constructor actually locks the implementation, closing the classic "attacker calls `initialize()` on the implementation and claims ownership of it" path. Not exploitable against the live protocol today regardless (the implementation holds no funds or routing of its own — only the proxy does), but this is exactly the kind of thing that's cheap to verify and expensive to get wrong. +2. **A second `initialize()` call through the proxy reverts** (`test_proxy_rejectsDoubleInitialize_*`) — confirms the `initializer` modifier's one-shot guard survives the full deploy-via-proxy path, not just in isolation. + +No initializer front-running window exists either: per the architecture doc, `initCalldata` is `delegatecall`ed atomically during the `TransparentUpgradeableProxy` constructor, so there's no block where the proxy exists uninitialized. + +### Storage layout — PASS, confirmed via `forge inspect storage-layout` + +All four contracts' *own* declared state starts at slot 0, and `uint256[50] private __gap` is correctly the last entry in every one: + +``` +DinToken: slot 0 coordinator → slot 1 __gap[50] +DinCoordinator: slot 0 dinToken, 1 dinValidatorStakeContract, 2 dinPerEth → slot 3 __gap[50] +DinValidatorStake: slot 0 DIN_TOKEN, 1 DIN_COORDINATOR, 2 slasherContracts, 3 validators → slot 4 __gap[50] +DINModelRegistry: slot 0 dinValidatorStake … slot 10 modelDisabled → slot 11 __gap[50] +``` + +This is cleaner than it would be for a typical upgradeable contract because OZ v5's upgradeable base contracts (`OwnableUpgradeable`, `ERC20Upgradeable`, `Initializable`) use ERC-7201 namespaced storage — their fields live at `keccak256`-derived slots, not sequential ones — so inherited base storage cannot collide with each contract's own declared variables regardless of gap placement. The `__gap[50]` still matters for future *same-contract* V2 additions (per the design doc's stated purpose), and its position is correct in all four. The pre-existing `foundry/test/UpgradeValidation.t.sol` (`Upgrades.validateImplementation`, all 4 passing at baseline) already covers this class of check going forward — this section is corroborating evidence, not a new test. + +### Access control on the upgrade path — no in-scope finding + +Transparent Proxy puts upgrade authority in the auto-deployed `ProxyAdmin`, entirely outside the implementation contracts reviewed here — a bad implementation can't touch its own upgrade authority (that's the documented reason Transparent Proxy was chosen over UUPS). Nothing in `foundry/src/` grants any address an upgrade capability; there is no `_authorizeUpgrade` or equivalent to audit. `ProxyAdmin` custody (currently the DIN-Representative EOA per the design doc) is a governance/key-management concern, not a contract-code one — out of scope for a source-code review. + +### Constructor logic silently dropped under the proxy pattern — no finding + +All four constructors contain exactly one line, `_disableInitializers()`, with `@custom:oz-upgrades-unsafe-allow constructor` annotations. There is no other constructor logic in any of the four contracts that could silently stop running once behind a proxy — all real setup was correctly moved into `initialize()` per the conversion pattern documented in the architecture README. Checked explicitly per the task's ask; nothing to flag. + +--- + +## Seeded Leads — Explicitly Confirmed or Refuted + +1. **`DINTaskAuditor.slashAuditors()` nested iteration (batches × auditors × models) may not fit in a block at production scale — is this a real DoS?** **Confirmed real.** See H-1. Not hypothetical: cheap to trigger deliberately via Sybil registration (≈0.00001 ETH per identity at the default exchange rate), no collusion needed. +2. **`DINTaskAuditor._activeAuditorPool()` unbounded iteration over all registered auditors.** **Confirmed real**, same root cause as #1 (no registration cap) — folded into H-1 along with its `DINTaskCoordinator._activeAggregatorPool()` counterpart, which has the identical shape and wasn't in the seed list but is exposed to the same attack. +3. **blockhash/timestamp shuffling in auditor selection — predictable by block producers; does slashing/selection fairness depend on unpredictability?** **Confirmed real.** See H-4. It matters more than block-producer-level MEV — the *caller* (model owner, who is not disinterested) can grind the timing of their own transaction against public, pre-known entropy, which is a lower bar than needing block-producer collusion. + +--- + +## What I'd do differently with more time + +- Extend the PoC suite with a real (not just closed-form) gas measurement at spec-scale parameters (`auditorsPerBatch=10`, `modelsPerBatch=100`, ~500 registered auditors) to get an exact gas number instead of the order-of-magnitude estimate in H-1 — the flow to reach `slashAuditors`/`slashAggregators` requires driving a GI through 8+ phase transitions per participant, which is straightforward but time-consuming to script at that scale. +- M-1 (copy-the-leader free-riding) and H-3 (missing quorum) interact: a full fix probably wants to land together (commit-reveal naturally gives you a place to also enforce "N-of-M revealed before finalize is callable"). +- Didn't attempt fuzzing `DinCoordinator`'s exchange-rate math or `DinValidatorStake`'s stake accounting (suggested in the task) — manual review didn't turn up an obvious overflow/precision target beyond L-2, and Solidity 0.8's built-in overflow checks close off the classic wraparound class. Would still run `forge fuzz` against `slash()`'s active/pending-withdrawal split accounting given more time, since that's the one place doing subtraction across two balances in the same function. diff --git a/foundry/test/SecurityFindings.t.sol b/foundry/test/SecurityFindings.t.sol new file mode 100644 index 0000000..d5f1e55 --- /dev/null +++ b/foundry/test/SecurityFindings.t.sol @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.28; + +// ───────────────────────────────────────────────────────────────────────────── +// PoC tests for the 2026-07 security review (Developer/audits/2026-07_foundry-src-security-review.md). +// Pinned to commit d136ff3. These are additive, read/observe only — no +// contract source under test is modified. +// Run: forge test --match-contract SecurityFindingsTest -vv +// ───────────────────────────────────────────────────────────────────────────── + +import {Test} from "forge-std/Test.sol"; +import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; + +import {DinToken} from "../src/DinToken.sol"; +import {DinCoordinator} from "../src/DinCoordinator.sol"; +import {DinValidatorStake} from "../src/DinValidatorStake.sol"; +import {DINModelRegistry} from "../src/DINModelRegistry.sol"; +import {DINTaskCoordinator} from "../src/DINTaskCoordinator.sol"; +import {DINTaskAuditor} from "../src/DINTaskAuditor.sol"; +import {GIstates} from "../src/DINShared.sol"; + +contract SecurityFindingsTest is Test { + // ───────────────────────────────────────────────────────────────────── + // Shared platform fixture (6-step deployment order from + // Documentation/technical/upgradable-contracts/hardhat/README.md §5) + // ───────────────────────────────────────────────────────────────────── + + DinToken tokenImpl; + DinCoordinator coordinatorImpl; + DinValidatorStake stakeImpl; + DINModelRegistry registryImpl; + + DinToken token; + DinCoordinator coordinator; + DinValidatorStake stake; + DINModelRegistry registry; + + address admin = makeAddr("admin"); + + function _deployPlatform() internal { + vm.startPrank(admin); + + // 1. DinToken proxy + tokenImpl = new DinToken(); + TransparentUpgradeableProxy tokenProxy = new TransparentUpgradeableProxy( + address(tokenImpl), + admin, + abi.encodeCall(DinToken.initialize, ()) + ); + token = DinToken(address(tokenProxy)); + + // 2. DinCoordinator proxy + coordinatorImpl = new DinCoordinator(); + TransparentUpgradeableProxy coordinatorProxy = new TransparentUpgradeableProxy( + address(coordinatorImpl), + admin, + abi.encodeCall(DinCoordinator.initialize, (address(token))) + ); + coordinator = DinCoordinator(address(coordinatorProxy)); + + // 3. one-shot wiring + token.setCoordinator(address(coordinator)); + + // 4. DinValidatorStake proxy + stakeImpl = new DinValidatorStake(); + TransparentUpgradeableProxy stakeProxy = new TransparentUpgradeableProxy( + address(stakeImpl), + admin, + abi.encodeCall( + DinValidatorStake.initialize, + (address(token), address(coordinator)) + ) + ); + stake = DinValidatorStake(address(stakeProxy)); + + // 5. wire stake into coordinator + coordinator.updateValidatorStakeContract(address(stake)); + + // 6. DINModelRegistry proxy + registryImpl = new DINModelRegistry(); + TransparentUpgradeableProxy registryProxy = new TransparentUpgradeableProxy( + address(registryImpl), + admin, + abi.encodeCall(DINModelRegistry.initialize, (address(stake))) + ); + registry = DINModelRegistry(address(registryProxy)); + + vm.stopPrank(); + } + + // ───────────────────────────────────────────────────────────────────── + // Proxy-specific: initializer protection + // + // Confirms _disableInitializers() is effective at RUNTIME, not just + // present in source — i.e. an attacker cannot call initialize() directly + // on the deployed implementation address and claim ownership of it. + // (Not exploitable against the real protocol today, since the + // implementation holds no funds/routing of its own — but see the + // finding writeup for why this still matters operationally.) + // ───────────────────────────────────────────────────────────────────── + + function test_implementation_rejectsDirectInitialize_DinToken() public { + DinToken impl = new DinToken(); + vm.expectRevert(); + impl.initialize(); + } + + function test_implementation_rejectsDirectInitialize_DinCoordinator() public { + DinCoordinator impl = new DinCoordinator(); + vm.expectRevert(); + impl.initialize(makeAddr("fakeToken")); + } + + function test_implementation_rejectsDirectInitialize_DinValidatorStake() public { + DinValidatorStake impl = new DinValidatorStake(); + vm.expectRevert(); + impl.initialize(makeAddr("fakeToken"), makeAddr("fakeCoordinator")); + } + + function test_implementation_rejectsDirectInitialize_DINModelRegistry() public { + DINModelRegistry impl = new DINModelRegistry(); + vm.expectRevert(); + impl.initialize(makeAddr("fakeStake")); + } + + function test_proxy_rejectsDoubleInitialize_DinToken() public { + _deployPlatform(); + vm.expectRevert(); + token.initialize(); + } + + function test_proxy_rejectsDoubleInitialize_DinCoordinator() public { + _deployPlatform(); + vm.expectRevert(); + coordinator.initialize(makeAddr("someToken")); + } + + function test_proxy_rejectsDoubleInitialize_DinValidatorStake() public { + _deployPlatform(); + vm.expectRevert(); + stake.initialize(makeAddr("t"), makeAddr("c")); + } + + function test_proxy_rejectsDoubleInitialize_DINModelRegistry() public { + _deployPlatform(); + vm.expectRevert(); + registry.initialize(makeAddr("s")); + } + + // ───────────────────────────────────────────────────────────────────── + // Finding: zero-CID sentinel collision permanently DoSes + // finalizeT1Aggregation / finalizeT2Aggregation for the whole GI. + // + // T1/T2 finalization uses `winningCID == bytes32(0)` to detect "no + // submissions for this batch" and revert. Any assigned aggregator can + // submit `bytes32(0)` as their own aggregation CID; if that CID ends up + // as the batch's plurality vote (e.g. the only submission in that + // batch), finalization reverts for the ENTIRE GI, indistinguishable + // from "nobody submitted" — even though a submission genuinely exists. + // Task contracts are not upgradeable, so this has no on-chain recovery + // path: the GI is bricked and the model owner must abandon the round. + // ───────────────────────────────────────────────────────────────────── + + DINTaskCoordinator tc; + DINTaskAuditor ta; + + address modelOwner = makeAddr("modelOwner"); + address auditor1 = makeAddr("auditor1"); + address auditor2 = makeAddr("auditor2"); + address client1 = makeAddr("client1"); + address client2 = makeAddr("client2"); + address client3 = makeAddr("client3"); + address agg1 = makeAddr("agg1"); + address agg2 = makeAddr("agg2"); + address agg3 = makeAddr("agg3"); + + function _fundAndStake(address who) internal { + vm.deal(who, 1 ether); + vm.prank(who); + coordinator.depositAndMint{value: 0.001 ether}(); + vm.startPrank(who); + token.approve(address(stake), type(uint256).max); + stake.stake(10 ether); // MIN_STAKE + vm.stopPrank(); + } + + function _deployTaskPair() internal { + vm.startPrank(modelOwner); + tc = new DINTaskCoordinator(address(stake)); + ta = new DINTaskAuditor(address(stake), address(tc)); + tc.setDINTaskAuditorContract(address(ta)); + vm.stopPrank(); + + // DIN-Representative authorises both as slashers + vm.startPrank(admin); + coordinator.addSlasherContract(address(tc)); + coordinator.addSlasherContract(address(ta)); + vm.stopPrank(); + + vm.startPrank(modelOwner); + tc.setDINTaskCoordinatorAsSlasher(); + tc.setDINTaskAuditorAsSlasher(); + tc.setGenesisModelIpfsHash(bytes32(uint256(1))); + tc.startGI(1); + vm.stopPrank(); + } + + function _runToT1AggregationStarted() internal { + _deployPlatform(); + _deployTaskPair(); + + _fundAndStake(auditor1); + _fundAndStake(auditor2); + _fundAndStake(agg1); + _fundAndStake(agg2); + _fundAndStake(agg3); + + vm.startPrank(modelOwner); + tc.startDINaggregatorsRegistration(1); + vm.stopPrank(); + + vm.prank(agg1); + tc.registerDINaggregator(1); + vm.prank(agg2); + tc.registerDINaggregator(1); + vm.prank(agg3); + tc.registerDINaggregator(1); + + vm.startPrank(modelOwner); + tc.closeDINaggregatorsRegistration(1); + tc.startDINauditorsRegistration(1); + vm.stopPrank(); + + // auditorsPerBatch defaults to 3 (demo Params in the DINTaskAuditor + // constructor) so all 3 auditors must register before the window closes. + address auditor3 = makeAddr("auditor3"); + _fundAndStake(auditor3); + + vm.prank(auditor1); + ta.registerDINAuditor(1); + vm.prank(auditor2); + ta.registerDINAuditor(1); + vm.prank(auditor3); + ta.registerDINAuditor(1); + + vm.startPrank(modelOwner); + tc.closeDINauditorsRegistration(1); + tc.startLMsubmissions(1); + vm.stopPrank(); + + vm.prank(client1); + ta.submitLocalModel(bytes32(uint256(100)), 1); + vm.prank(client2); + ta.submitLocalModel(bytes32(uint256(200)), 1); + vm.prank(client3); + ta.submitLocalModel(bytes32(uint256(300)), 1); + + vm.startPrank(modelOwner); + tc.closeLMsubmissions(1); + tc.createAuditorsBatches(1); + tc.setTestDataAssignedFlag(1, true); + tc.startLMsubmissionsEvaluation(1); + vm.stopPrank(); + + // Both models pass: all 3 auditors vote eligible + score 100 on both models. + (, address[] memory batchAuditors, uint[] memory modelIdxs,) = ta.getAuditorsBatch(1, 0); + for (uint i = 0; i < batchAuditors.length; i++) { + for (uint m = 0; m < modelIdxs.length; m++) { + vm.prank(batchAuditors[i]); + ta.setAuditScorenEligibility(1, 0, modelIdxs[m], 100, true); + } + } + + vm.startPrank(modelOwner); + tc.closeLMsubmissionsEvaluation(1); + tc.autoCreateTier1AndTier2(1); + tc.startT1Aggregation(1); + vm.stopPrank(); + } + + function test_finalizeT1Aggregation_zeroCID_bricksEntireGI() public { + _runToT1AggregationStarted(); + + (, address[] memory t1aggs,,,) = tc.getTier1Batch(1, 0); + assertEq(t1aggs.length, 3, "sanity: T1 batch should have 3 aggregators"); + + // Only ONE of the three assigned aggregators submits, and submits + // the zero CID. The other two never submit before the model owner + // closes the window (a routine "not everyone responded in time" + // scenario, not an edge case). + vm.prank(t1aggs[0]); + tc.submitT1Aggregation(1, 0, bytes32(0)); + + // This is the ONLY submission in the batch, so it is unambiguously + // the plurality winner (1 vote > 0). Finalization should therefore + // either succeed with finalCID == bytes32(0), or explicitly reject + // zero-CID submissions at submit time. Instead: + vm.expectRevert(); // TC_NoSubmissions — wrongly conflates "no one voted" with "the winning vote was 0x0" + tc.finalizeT1Aggregation(1); + + // The GI is now stuck: GIstate is still T1AggregationStarted, the + // task contracts are not upgradeable, and there is no admin + // function to skip/override a single batch. Every subsequent call + // to finalizeT1Aggregation reverts the same way, forever. + assertEq(uint256(tc.GIstate()), uint256(GIstates.T1AggregationStarted)); + } +}