From fa796ae902a2752419f276c55ce44987f7ee770a Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Mon, 5 Oct 2026 13:31:18 +0100 Subject: [PATCH 1/2] docs(backlog,audit): link BL-27..31 to new issues, mark stale findings fixed BL-27..31 said "no issue yet" -- filed #221 (BL-29, DinFeeRouter fund lock), #222 (BL-30, stalled-GI no recovery), #223 (BL-27+BL-28, dincli model-owner deploy/release-slots gaps), #224 (BL-31 plus the audit doc's M-2/L-4, bundled as "needs a product decision" findings). Added BL-34/BL-35 for M-2/L-4 since they predated the backlog's BL-numbering and had no row of their own. Also closed the loop on the 2026-07 security review (foundry-src-security-review.md): M-3, M-4, L-1, L-5, L-7 are already fixed in current develop (GI-state gate in slashAuditors, a real jail() mechanism, correct stake() CEI order, withdrawFees removed entirely, giRewardPool replacing totalDepositedRewards) but the doc was never updated to say so -- re-verified each against develop @ 740a613 before marking. M-2 and L-4 re-confirmed still open and now point at #224. Co-Authored-By: Claude Sonnet 5 --- Developer/BACK_LOG.md | 12 +++++++----- .../audits/foundry-src-security-review.md | 14 ++++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/Developer/BACK_LOG.md b/Developer/BACK_LOG.md index fe772f3..bcde3d3 100644 --- a/Developer/BACK_LOG.md +++ b/Developer/BACK_LOG.md @@ -33,10 +33,12 @@ | BL-24 | DP / Privacy (cache_model_0) | F4 — Laplace noise calibrated against an L2 clip, not the L1 sensitivity it's normally calibrated against | `post_training_laplace`'s `laplace_scale` is scaled by the same `clipping_norm` as the Gaussian mechanisms (`clip_state_dict`'s L2-norm clip), but the Laplace mechanism's textbook privacy guarantee is calibrated against L1 sensitivity, not L2. [PR #109](https://github.com/InfiniteZeroFoundation/DevNet/pull/109) flagged this explicitly rather than silently assuming it's fine (`Documentation/technical/services/clients.md:210-212`: "Laplace noise calibrated against an L2 clip is an open question tracked separately"), and [#99](https://github.com/InfiniteZeroFoundation/DevNet/issues/99)'s F4 (`Documentation/technical/audits/dp-mechanism-review.md` §F4) called the structural mismatch confirmed but left the exact magnitude/severity for a DP specialist's sign-off — explicitly excluded from #99's scope rather than fixed. [#119](https://github.com/InfiniteZeroFoundation/DevNet/pull/119) (which closed #99's remaining F8 items) left F4 untouched, so nothing currently tracks it now that #99 is closed. Needs either a DP specialist's opinion that L2-calibrated Laplace is acceptable as-is (and the open-question note downgraded to an explanation), or a fix (e.g. recalibrating against an actual L1 clip, or reclassifying `post_training_laplace` as experimental alongside F5's `per_layer` treatment) before any ε is claimed for it. | P3/P4 (needs DP specialist input first) | [#99](https://github.com/InfiniteZeroFoundation/DevNet/issues/99) F4 (excluded from scope), `Documentation/technical/audits/dp-mechanism-review.md` §F4, Sep 10, 2026 | 🆕 New — unaddressed; `Documentation/technical/services/clients.md`'s open-question note on L2-calibrated Laplace noise is unchanged since PR #109/#119 | | BL-25 | Tooling / reference model (cache_model_0) | `cache_model_0/abis/` task-contract artifacts are stale vs. `foundry/src` | `cache_model_0/abis/DINTaskCoordinator.json` and `DINTaskAuditor.json` are full hardhat artifacts (abi + bytecode) last regenerated 2026-06-09, missing 67 ABI entries present in `foundry/src` for the coordinator (104 vs 170) and 96 for the auditor (68 vs 158), with pre-foundry constructors. [PR #171](https://github.com/InfiniteZeroFoundation/DevNet/pull/171) regenerated the six bundled `dincli/abis/` files from `foundry/out` but deliberately left these alone: [cache_model_0/manifest.json](../cache_model_0/manifest.json)'s `task_contracts` block pins each artifact to an IPFS CID, records `"type": "hardhat-artifact"` and a hardhat/cancun/optimizer-off `compilation` block, and carries deployed addresses, and dincli resolves task artifacts by CID. Regenerating the files locally would leave the manifest describing artifacts it no longer points to. Needs one pass: regenerate both from `foundry/out` (with bytecode, e.g. `dincli system dump-abi --bytecode --output cache_model_0/abis`), update the manifest's artifact type and compilation metadata to foundry, re-pin to IPFS and update the CIDs. Addresses only change on a redeploy. | P3/P4 | PR #171 merge (2026-09-26): out of scope for the PR's ABI refresh | 🆕 New — unaddressed | | BL-26 | Solidity/foundry (task contracts) | Batch-assignment seed lock (H-2) shares BL-11's re-roll residual, plus a new pool-reshaping gap | `DINTaskCoordinator.autoCreateTier1AndTier2`/`createAuditorsBatches` (task_240926_18 Part B, issue #156 H-2 aggregation side) reuse BL-11's future-block-seed + permissionless-lock pattern (`lockAggSeed`/`lockAuditSeed`), so they inherit its residual (2): the model owner can decline to lock a seed they dislike and wait out the ~256-block `blockhash` window for a fresh draw — repeatable, since nothing locks automatically today. A second, distinct gap not present in BL-11's `_assignFreshSubgroup` (which shuffles a fixed pool and only lets an exiting validator drop itself, not re-roll the rest): `_activeAggregatorPool`/`_activeAuditorPool` are evaluated at `autoCreateTier1AndTier2`/`createAuditorsBatches` call time, *after* the seed is already public, and Fisher-Yates re-rolls the whole assignment on any pool-size change — so a validator can unstake between lock and create to reshape everyone else's batch. Cost to the attacker is real but modest (their own Sybils sit out the GI and enter unbonding). Ready-made fix for the pool-reshaping gap: adopt `_assignFreshSubgroup`'s pattern — shuffle the fixed per-GI registration list (`dinAggregators[_GI]` / the auditor equivalent) with the locked seed, then skip inactive validators while filling seats, so an unstake only forfeits the leaver's own seat instead of re-rolling everyone else's. | P3/P4 | [PR #191](https://github.com/InfiniteZeroFoundation/DevNet/pull/191) review, Sep 29, 2026 | 🟡 Partially mitigated — the decline-to-lock re-roll is narrowed by a validator-side lock in dincli, added at the PR #191 merge (`dincli aggregator lock-seed` / `dincli auditor lock-seed`, and automatically in `show-t1-batches`/`show-t2-batches`/`lms-evaluation show-batch` before batches exist): any validator online after the seed block locks it, so the owner only gets a re-roll if nobody else is. A contract-side cap on re-anchors was considered and deferred: with no GI-abort path, a GI capped out would be stuck forever (`endGI` needs `AggregatorsSlashed`, `startGI` needs `GIended`), locking validators' registration slots and the reward pool — it needs a GI-abort design (who may abort, where the reward pool goes) first. Pool-reshaping gap unaddressed. Documented as residuals in the PR (`DINTaskCoordinator.md` §6.3). Sequencer-trust residual (shared with BL-11) tracked in [issue #178](https://github.com/InfiniteZeroFoundation/DevNet/issues/178); the re-roll-by-declining-to-lock and pool-reshaping gaps above have no dedicated issue yet, same as BL-11's own un-issued residual (2) | -| BL-27 | dincli (model owner) | `dincli model-owner deploy` still uses the pre-foundry task-contract constructors | `dincli/cli/modelownerd/deploy.py` sends `DINTaskCoordinator.constructor(stake)` (`:35`) and `DINTaskAuditor.constructor(stake, coordinator)` (`:77`), but `foundry/src` takes `(stake, modelId)` and `(stake, coordinator, modelId)`. Against foundry artifacts, which are the bundled `dincli/abis/` since PR #171, the deploy fails at ABI encoding, so model owners can't deploy task contracts from dincli. The `tests/dincli/` harness hides this because it deploys task contracts from **hardhat** artifacts. The fix also has to settle where `modelId` comes from: the registry assigns it only at approval, after the contracts exist (`DINTaskCoordinator.md` §10 No. 4). | P3 (before any model-owner onboarding on DevNet 3.0) | PR No. 204 review, Sep 30, 2026 (caveats `DINTaskCoordinator.md` §10 No. 10, `DINTaskAuditor.md` §13 No. 8) | 🆕 New — unaddressed; no issue yet | -| BL-28 | dincli (model owner) | No dincli command for `releaseGIRegistrationSlots` | `DINTaskCoordinator.releaseGIRegistrationSlots(gi)` (owner-only, once per ended GI) is the only thing that decrements validators' `activeRegistrationCount`; `endGI` deliberately doesn't (BL-10). dincli ships the function in its ABI but has no command that calls it, so under a non-zero `maxConcurrentRegistrationsPerStakeUnit` every GI permanently consumes its validators' concurrent-registration slots until they hit `TC_/TA_ConcurrentRegistrationCapReached`. Harmless while the cap is 0 (the default). | P3 (before the cap is set on any network) | task_100926_12 / issue #37 fast-follow; PR No. 204 review | 🆕 New — unaddressed; no issue yet | -| BL-29 | Solidity/foundry (fees) | `DinFeeRouter`'s non-treasury shares accrue with no way out | `routeFeeETH`/`routeFeeDIN` pay the treasury share out immediately but only *count* the `validatorPool`, `storage` and `publicGoods` shares in `accruedEth`/`accruedDin`. No function withdraws or distributes them, so with the default 95% validator-pool split, 95% of every swept faucet/registry fee stays in the router until an upgrade adds a path. Needs a decision on who can pull each bucket and where the validator-pool share goes (reward pools? `DinEmission`?). | P3/P4 | PR No. 204 review (`DinCoordinator.md` §14 No. 2), Sep 30, 2026; related to issue #43's open items | 🆕 New — unaddressed; no issue yet | -| BL-30 | Solidity/foundry (task contracts) | No recovery from a stalled GI | If a T1/T2 batch never reaches the reveal quorum, `finalizeT1Aggregation`/`finalizeT2Aggregation` revert forever (`TC_NoSubmissions`/`TC_InsufficientSubmissions`), and there is no owner path to skip the batch or abort the GI. `endGI` needs `AggregatorsSlashed` and `startGI` needs `GIended`, so the model is stuck: its reward pool and its validators' registration slots stay locked. This is also why BL-26's contract-side re-anchor cap was deferred. Needs a GI-abort design: who may abort, when, and where the reward pool goes. | P3/P4 | PR No. 191 review (BL-26); PR No. 204 review (`DINTaskCoordinator.md` §10 No. 6) | 🆕 New — unaddressed; no issue yet | -| BL-31 | Solidity/foundry (slashing) | S6 no-participation slashing is implemented but not wired | `DinValidatorStake.recordNoParticipation` (escalating 10%-per-breach slash past `s6NoParticipationThreshold`) exists and is tested, but no task contract calls it. They deliberately skip it where `slashPartial` (S1/S2, with S5 escalation) already penalises the same missed work, to avoid stacking past `MIN_STAKE`. So S6 never fires. Decide whether S6 should cover something S1/S2 don't (e.g. registered but never assigned or never active), or whether to remove it and its parameters. | P3 | PR No. 204 review (`DinValidatorStake.md` §14 No. 1), Sep 30, 2026; slashing taxonomy S6 (issue #38) | 🆕 New — unaddressed; no issue yet | +| BL-27 | dincli (model owner) | `dincli model-owner deploy` still uses the pre-foundry task-contract constructors | `dincli/cli/modelownerd/deploy.py` sends `DINTaskCoordinator.constructor(stake)` (`:35`) and `DINTaskAuditor.constructor(stake, coordinator)` (`:77`), but `foundry/src` takes `(stake, modelId)` and `(stake, coordinator, modelId)`. Against foundry artifacts, which are the bundled `dincli/abis/` since PR #171, the deploy fails at ABI encoding, so model owners can't deploy task contracts from dincli. The `tests/dincli/` harness hides this because it deploys task contracts from **hardhat** artifacts. The fix also has to settle where `modelId` comes from: the registry assigns it only at approval, after the contracts exist (`DINTaskCoordinator.md` §10 No. 4). | P3 (before any model-owner onboarding on DevNet 3.0) | PR No. 204 review, Sep 30, 2026 (caveats `DINTaskCoordinator.md` §10 No. 10, `DINTaskAuditor.md` §13 No. 8) | 🆕 New — unaddressed; tracked in [#223](https://github.com/InfiniteZeroFoundation/DevNet/issues/223) | +| BL-28 | dincli (model owner) | No dincli command for `releaseGIRegistrationSlots` | `DINTaskCoordinator.releaseGIRegistrationSlots(gi)` (owner-only, once per ended GI) is the only thing that decrements validators' `activeRegistrationCount`; `endGI` deliberately doesn't (BL-10). dincli ships the function in its ABI but has no command that calls it, so under a non-zero `maxConcurrentRegistrationsPerStakeUnit` every GI permanently consumes its validators' concurrent-registration slots until they hit `TC_/TA_ConcurrentRegistrationCapReached`. Harmless while the cap is 0 (the default). | P3 (before the cap is set on any network) | task_100926_12 / issue #37 fast-follow; PR No. 204 review | 🆕 New — unaddressed; tracked in [#223](https://github.com/InfiniteZeroFoundation/DevNet/issues/223) | +| BL-29 | Solidity/foundry (fees) | `DinFeeRouter`'s non-treasury shares accrue with no way out | `routeFeeETH`/`routeFeeDIN` pay the treasury share out immediately but only *count* the `validatorPool`, `storage` and `publicGoods` shares in `accruedEth`/`accruedDin`. No function withdraws or distributes them, so with the default 95% validator-pool split, 95% of every swept faucet/registry fee stays in the router until an upgrade adds a path. Needs a decision on who can pull each bucket and where the validator-pool share goes (reward pools? `DinEmission`?). | P3/P4 | PR No. 204 review (`DinCoordinator.md` §14 No. 2), Sep 30, 2026; related to issue #43's open items | 🆕 New — unaddressed; tracked in [#221](https://github.com/InfiniteZeroFoundation/DevNet/issues/221) | +| BL-30 | Solidity/foundry (task contracts) | No recovery from a stalled GI | If a T1/T2 batch never reaches the reveal quorum, `finalizeT1Aggregation`/`finalizeT2Aggregation` revert forever (`TC_NoSubmissions`/`TC_InsufficientSubmissions`), and there is no owner path to skip the batch or abort the GI. `endGI` needs `AggregatorsSlashed` and `startGI` needs `GIended`, so the model is stuck: its reward pool and its validators' registration slots stay locked. This is also why BL-26's contract-side re-anchor cap was deferred. Needs a GI-abort design: who may abort, when, and where the reward pool goes. | P3/P4 | PR No. 191 review (BL-26); PR No. 204 review (`DINTaskCoordinator.md` §10 No. 6) | 🆕 New — unaddressed; tracked in [#222](https://github.com/InfiniteZeroFoundation/DevNet/issues/222) | +| BL-31 | Solidity/foundry (slashing) | S6 no-participation slashing is implemented but not wired | `DinValidatorStake.recordNoParticipation` (escalating 10%-per-breach slash past `s6NoParticipationThreshold`) exists and is tested, but no task contract calls it. They deliberately skip it where `slashPartial` (S1/S2, with S5 escalation) already penalises the same missed work, to avoid stacking past `MIN_STAKE`. So S6 never fires. Decide whether S6 should cover something S1/S2 don't (e.g. registered but never assigned or never active), or whether to remove it and its parameters. | P3 | PR No. 204 review (`DinValidatorStake.md` §14 No. 1), Sep 30, 2026; slashing taxonomy S6 (issue #38) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | | BL-32 | Tooling / onboarding | Contributor environment preflight check | No single command checks a dev machine before work starts. Each of these has already cost time: (1) Python ≥ 3.12; (2) `forge`/`anvil` installed and solc 0.8.28 available; (3) `foundry/node_modules` present. Without `npm ci`, `forge test`'s parallel tests race to fetch `@openzeppelin/upgrades-core` over npx (see [CLAUDE.md](../CLAUDE.md)); (4) `eth-account` and `py-multibase` import; (5) `torch` imports in the active venv, which the full `pytest` run needs; (6) `.env` / `.env.` exist, the RPC endpoint answers, and `resolve_ipfs_config()` resolves. Proposal: `scripts/preflight.sh` or `dincli system doctor`, one line per check, `OK` or `MISSING → `, non-zero exit on any failure. Also usable as the first CI step and in the onboarding docs. Read-only: it reports missing packages and never installs them. | P3/P4 (onboarding; small, ~half a day) | `preflight.sh` pattern from [Osomudeya/devops-scripting-labs](https://github.com/Osomudeya/devops-scripting-labs) (reviewed 2026-10-02); no existing equivalent (the only "preflight" in `dincli/cli/system.py` is the bridge's tx preflight) | 💭 Idea | | BL-33 | Tooling / deploy | On-chain deployment drift check | Nothing verifies that `foundry/deployments/.json` (and the `din_info` that `dincli system import-deployments` writes) still matches the chain. Proposal: a read-only check, either a forge script or `dincli system verify-deployment`, that for each recorded contract compares (1) the proxy's EIP-1967 implementation and admin slots against the recorded implementation/`proxyAdmin*` addresses; (2) the deployed runtime code hash against the current `forge build` artifact (immutables masked); (3) owner / DIN-Representative addresses; (4) wiring: `DinCoordinator` ↔ `DinToken`/`DinValidatorStake`/`DinModelRegistry`/fee router/treasury links and slasher authorizations on `DinValidatorStake`. Report per-item `OK` / `DRIFT` with expected vs. actual. Caveat: it only covers what the deployments JSON records, so a clean run doesn't prove nothing else was deployed or authorized. | P3 (before the DevNet 3.0 deploy; re-run after every `UpgradePlatform.s.sol` run) | Terraform state-vs-live drift pattern from [Osomudeya/devops-scripting-labs](https://github.com/Osomudeya/devops-scripting-labs) `03-drift-detection` (reviewed 2026-10-02); no existing code reads EIP-1967 slots or code hashes in `dincli/` or `foundry/script/` | 💭 Idea | +| BL-34 | Solidity/foundry (task contracts) | `DINModelRegistry.disableModel()` kill-switch doesn't reach the live task contracts | `modelDisabled[modelId]` only gates `requestModelRegistration`/`requestManifestUpdate` inside the registry itself. `DINTaskCoordinator`/`DINTaskAuditor` have no dependency on, or awareness of, the registry (confirmed: zero functional references, only two doc comments in the auditor). A model the DIN-Representative has disabled — e.g. because its task contracts are wrongfully slashing honest validators — keeps running full GIs completely unaffected. The task contracts' own doc table calls this a "kill-switch," which it isn't. Needs a decision: wire the task contracts to check `modelDisabled` before state-changing calls (requires threading the model ID + registry address into contracts that don't know about the registry today), or document `disableModel` as registry-metadata-only. | P3/P4 | 2026-07 security review M-2 (`Documentation/technical/audits/foundry-src-security-review.md`) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | +| BL-35 | Solidity/foundry (registry) | `rejectModel()`'s non-refund of the registration fee was never confirmed as intentional | `feePaid` (set at `requestModelRegistration`) is never refunded on `rejectModel()`, but it's also never refunded on `approveModel()` either — the fee appears to be a deliberate pay-to-apply cost regardless of outcome (anti-spam), not an oversight specific to rejection. Never written down as a deliberate choice anywhere. Needs confirmation and a doc note in `DINModelRegistry.md` / the dinrep role docs, so a future reader doesn't "fix" it into an inconsistent partial-refund state. | P4 (doc-only once confirmed) | 2026-07 security review L-4 (`Documentation/technical/audits/foundry-src-security-review.md`) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | diff --git a/Documentation/technical/audits/foundry-src-security-review.md b/Documentation/technical/audits/foundry-src-security-review.md index 14dc09d..90f9045 100644 --- a/Documentation/technical/audits/foundry-src-security-review.md +++ b/Documentation/technical/audits/foundry-src-security-review.md @@ -165,6 +165,8 @@ This doesn't cause direct fund loss, but it undermines the core assumption that ### M-2. `DINModelRegistry.disableModel()` kill-switch doesn't reach the live task contracts +**Still open — tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) / BL-34** (re-confirmed against `develop` @ `740a613`, 2026-10). + **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. @@ -177,6 +179,8 @@ This doesn't cause direct fund loss, but it undermines the core assumption that ### M-3. `DINTaskAuditor.slashAuditors()` has no internal GI-state gate +**Fixed — commit `216527c`** (same security batch as C-1/C-2/H-1, discussion #88). `slashAuditors()` now independently reverts with `TA_CannotSlashAuditors` unless `dintaskcoordinatorContract.GIstate() == GIstates.T2AggregationDone`, consistent with every other function in the file. + **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. @@ -189,6 +193,8 @@ Today this isn't independently exploitable (the coordinator's gate holds), but i ### M-4. `DinValidatorStake`'s `Jailed` status is dead code +**Fixed — [issue #37](https://github.com/InfiniteZeroFoundation/DevNet/issues/37), commit `aee404f`** ("governable stake params, jail/reactivate, inert bounds storage"). A real `jail()`/reactivate path now writes `jailedUntil` and `status = ValidatorStatus.Jailed`, and emits `ValidatorJailed`; the mechanism this finding flagged as unreachable is live. + **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. @@ -203,13 +209,13 @@ This isn't exploitable, but it either indicates a missing feature (a `jail()` fu | # | 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-1 | **Fixed** (current `stake()` updates `activeStake` before calling `DIN_TOKEN.safeTransferFrom` — correct checks-effects-interactions order). | `DinValidatorStake.sol` L113-126 | | L-2 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `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. *(Fix note: the zero-mint is only reachable when `dinPerEth < 1e18`, i.e. `msg.value * dinPerEth < 1e18` — any `dinPerEth >= 1e18` mints at least 1 wei for `msg.value >= 1`, clean multiple or not.)* | `DinCoordinator.sol` L92-101 | | L-3 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `requestModelRegistration` doesn't refund `msg.value` above the required fee — any overpayment is silently kept. *(Fix note: `requestManifestUpdate` had the same gap and is fixed the same way — `feePaid` records the required fee and the excess is refunded.)* | `DINModelRegistry.sol` L166-211 (`requestManifestUpdate` L281-320) | -| 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-4 | `rejectModel()` never refunds the fee paid in the corresponding `requestModelRegistration` — a rejected requester loses their fee permanently. `approveModel()` doesn't touch `feePaid` either, so this looks like an intentional pay-to-apply design rather than an oversight, but it was never confirmed/documented as such. Tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) / BL-35. | `DINModelRegistry.sol` L244-254 | +| L-5 | **Fixed/obsolete** — `withdrawFees` no longer exists in `DINModelRegistry.sol`; superseded by `setFees`/`sweepFeesToRouter` ([#203](https://github.com/InfiniteZeroFoundation/DevNet/pull/203)), neither of which has this gap. | ~~`DINModelRegistry.sol` L472-477~~ | | L-6 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `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` L256-264 (+ `setDINTaskAuditorContract` L272-283), `DINTaskAuditor.sol` L399-424 | -| 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-7 | **Fixed** (task_210726_6 §3) — `totalDepositedRewards` replaced by a real per-GI `giRewardPool` mapping ("funded per-GI, settled per-GI" model), and `RewardDeposited` is now actually emitted. No `receive()`/`fallback()` still, but the reward asset model changed enough that the original "stray ETH via forced selfdestruct" framing may no longer apply — not re-verified here. | `DINTaskAuditor.sol` | | 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 | --- From 250d5164b56391802c69d7a8e4012dcf4f39b6aa Mon Sep 17 00:00:00 2001 From: Abidoyesimze Date: Tue, 6 Oct 2026 05:48:48 +0100 Subject: [PATCH 2/2] fix: PR No. 225 review follow-ups (L-5 source, L-7 status, two nits) L-5: cite commit 06190e8 ("scope DINModelRegistry fee path to ETH-only, accumulate-then-sweep") instead of issue #203 -- #203 is an issue, not a PR, and only removed dincli's dead withdraw-fees command; 06190e8 is the actual commit that removed withdrawFees from the contract. L-7: downgrade "Fixed" to "Partly fixed". Re-verified against develop: the dead-code half (totalDepositedRewards -> giRewardPool, RewardDeposited now emitted) is fixed, but the stray-ETH half still applies exactly as originally written -- no receive()/fallback() or ETH withdrawal path exists. Nits: M-4's fix note said jail(); the actual function is jailValidator(). BL-34 said "only two doc comments" in the auditor naming DINModelRegistry -- re-counted: two name it explicitly (L31, L434), two more reference "model registry" generically without naming the type (L393, L405). Co-Authored-By: Claude Sonnet 5 --- Developer/BACK_LOG.md | 2 +- .../technical/audits/foundry-src-security-review.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Developer/BACK_LOG.md b/Developer/BACK_LOG.md index bcde3d3..dbc9550 100644 --- a/Developer/BACK_LOG.md +++ b/Developer/BACK_LOG.md @@ -40,5 +40,5 @@ | BL-31 | Solidity/foundry (slashing) | S6 no-participation slashing is implemented but not wired | `DinValidatorStake.recordNoParticipation` (escalating 10%-per-breach slash past `s6NoParticipationThreshold`) exists and is tested, but no task contract calls it. They deliberately skip it where `slashPartial` (S1/S2, with S5 escalation) already penalises the same missed work, to avoid stacking past `MIN_STAKE`. So S6 never fires. Decide whether S6 should cover something S1/S2 don't (e.g. registered but never assigned or never active), or whether to remove it and its parameters. | P3 | PR No. 204 review (`DinValidatorStake.md` §14 No. 1), Sep 30, 2026; slashing taxonomy S6 (issue #38) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | | BL-32 | Tooling / onboarding | Contributor environment preflight check | No single command checks a dev machine before work starts. Each of these has already cost time: (1) Python ≥ 3.12; (2) `forge`/`anvil` installed and solc 0.8.28 available; (3) `foundry/node_modules` present. Without `npm ci`, `forge test`'s parallel tests race to fetch `@openzeppelin/upgrades-core` over npx (see [CLAUDE.md](../CLAUDE.md)); (4) `eth-account` and `py-multibase` import; (5) `torch` imports in the active venv, which the full `pytest` run needs; (6) `.env` / `.env.` exist, the RPC endpoint answers, and `resolve_ipfs_config()` resolves. Proposal: `scripts/preflight.sh` or `dincli system doctor`, one line per check, `OK` or `MISSING → `, non-zero exit on any failure. Also usable as the first CI step and in the onboarding docs. Read-only: it reports missing packages and never installs them. | P3/P4 (onboarding; small, ~half a day) | `preflight.sh` pattern from [Osomudeya/devops-scripting-labs](https://github.com/Osomudeya/devops-scripting-labs) (reviewed 2026-10-02); no existing equivalent (the only "preflight" in `dincli/cli/system.py` is the bridge's tx preflight) | 💭 Idea | | BL-33 | Tooling / deploy | On-chain deployment drift check | Nothing verifies that `foundry/deployments/.json` (and the `din_info` that `dincli system import-deployments` writes) still matches the chain. Proposal: a read-only check, either a forge script or `dincli system verify-deployment`, that for each recorded contract compares (1) the proxy's EIP-1967 implementation and admin slots against the recorded implementation/`proxyAdmin*` addresses; (2) the deployed runtime code hash against the current `forge build` artifact (immutables masked); (3) owner / DIN-Representative addresses; (4) wiring: `DinCoordinator` ↔ `DinToken`/`DinValidatorStake`/`DinModelRegistry`/fee router/treasury links and slasher authorizations on `DinValidatorStake`. Report per-item `OK` / `DRIFT` with expected vs. actual. Caveat: it only covers what the deployments JSON records, so a clean run doesn't prove nothing else was deployed or authorized. | P3 (before the DevNet 3.0 deploy; re-run after every `UpgradePlatform.s.sol` run) | Terraform state-vs-live drift pattern from [Osomudeya/devops-scripting-labs](https://github.com/Osomudeya/devops-scripting-labs) `03-drift-detection` (reviewed 2026-10-02); no existing code reads EIP-1967 slots or code hashes in `dincli/` or `foundry/script/` | 💭 Idea | -| BL-34 | Solidity/foundry (task contracts) | `DINModelRegistry.disableModel()` kill-switch doesn't reach the live task contracts | `modelDisabled[modelId]` only gates `requestModelRegistration`/`requestManifestUpdate` inside the registry itself. `DINTaskCoordinator`/`DINTaskAuditor` have no dependency on, or awareness of, the registry (confirmed: zero functional references, only two doc comments in the auditor). A model the DIN-Representative has disabled — e.g. because its task contracts are wrongfully slashing honest validators — keeps running full GIs completely unaffected. The task contracts' own doc table calls this a "kill-switch," which it isn't. Needs a decision: wire the task contracts to check `modelDisabled` before state-changing calls (requires threading the model ID + registry address into contracts that don't know about the registry today), or document `disableModel` as registry-metadata-only. | P3/P4 | 2026-07 security review M-2 (`Documentation/technical/audits/foundry-src-security-review.md`) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | +| BL-34 | Solidity/foundry (task contracts) | `DINModelRegistry.disableModel()` kill-switch doesn't reach the live task contracts | `modelDisabled[modelId]` only gates `requestModelRegistration`/`requestManifestUpdate` inside the registry itself. `DINTaskCoordinator`/`DINTaskAuditor` have no dependency on, or awareness of, the registry (confirmed: zero functional references; the auditor has two doc comments naming `DINModelRegistry` by name (L31, L434) plus two more referencing "model registry" generically without naming the type (L393, L405) — none are functional checks). A model the DIN-Representative has disabled — e.g. because its task contracts are wrongfully slashing honest validators — keeps running full GIs completely unaffected. The task contracts' own doc table calls this a "kill-switch," which it isn't. Needs a decision: wire the task contracts to check `modelDisabled` before state-changing calls (requires threading the model ID + registry address into contracts that don't know about the registry today), or document `disableModel` as registry-metadata-only. | P3/P4 | 2026-07 security review M-2 (`Documentation/technical/audits/foundry-src-security-review.md`) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | | BL-35 | Solidity/foundry (registry) | `rejectModel()`'s non-refund of the registration fee was never confirmed as intentional | `feePaid` (set at `requestModelRegistration`) is never refunded on `rejectModel()`, but it's also never refunded on `approveModel()` either — the fee appears to be a deliberate pay-to-apply cost regardless of outcome (anti-spam), not an oversight specific to rejection. Never written down as a deliberate choice anywhere. Needs confirmation and a doc note in `DINModelRegistry.md` / the dinrep role docs, so a future reader doesn't "fix" it into an inconsistent partial-refund state. | P4 (doc-only once confirmed) | 2026-07 security review L-4 (`Documentation/technical/audits/foundry-src-security-review.md`) | 🆕 New — unaddressed; tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) | diff --git a/Documentation/technical/audits/foundry-src-security-review.md b/Documentation/technical/audits/foundry-src-security-review.md index 90f9045..aced80c 100644 --- a/Documentation/technical/audits/foundry-src-security-review.md +++ b/Documentation/technical/audits/foundry-src-security-review.md @@ -193,7 +193,7 @@ Today this isn't independently exploitable (the coordinator's gate holds), but i ### M-4. `DinValidatorStake`'s `Jailed` status is dead code -**Fixed — [issue #37](https://github.com/InfiniteZeroFoundation/DevNet/issues/37), commit `aee404f`** ("governable stake params, jail/reactivate, inert bounds storage"). A real `jail()`/reactivate path now writes `jailedUntil` and `status = ValidatorStatus.Jailed`, and emits `ValidatorJailed`; the mechanism this finding flagged as unreachable is live. +**Fixed — [issue #37](https://github.com/InfiniteZeroFoundation/DevNet/issues/37), commit `aee404f`** ("governable stake params, jail/reactivate, inert bounds storage"). A real `jailValidator()`/reactivate path now writes `jailedUntil` and `status = ValidatorStatus.Jailed`, and emits `ValidatorJailed`; the mechanism this finding flagged as unreachable is live. **Contract / function:** `DinValidatorStake.sol` — `ValidatorStatus.Jailed` (L46), `jailedUntil` field (L54), read at L269 and L324-325. @@ -213,9 +213,9 @@ This isn't exploitable, but it either indicates a missing feature (a `jail()` fu | L-2 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `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. *(Fix note: the zero-mint is only reachable when `dinPerEth < 1e18`, i.e. `msg.value * dinPerEth < 1e18` — any `dinPerEth >= 1e18` mints at least 1 wei for `msg.value >= 1`, clean multiple or not.)* | `DinCoordinator.sol` L92-101 | | L-3 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `requestModelRegistration` doesn't refund `msg.value` above the required fee — any overpayment is silently kept. *(Fix note: `requestManifestUpdate` had the same gap and is fixed the same way — `feePaid` records the required fee and the excess is refunded.)* | `DINModelRegistry.sol` L166-211 (`requestManifestUpdate` L281-320) | | L-4 | `rejectModel()` never refunds the fee paid in the corresponding `requestModelRegistration` — a rejected requester loses their fee permanently. `approveModel()` doesn't touch `feePaid` either, so this looks like an intentional pay-to-apply design rather than an oversight, but it was never confirmed/documented as such. Tracked in [#224](https://github.com/InfiniteZeroFoundation/DevNet/issues/224) / BL-35. | `DINModelRegistry.sol` L244-254 | -| L-5 | **Fixed/obsolete** — `withdrawFees` no longer exists in `DINModelRegistry.sol`; superseded by `setFees`/`sweepFeesToRouter` ([#203](https://github.com/InfiniteZeroFoundation/DevNet/pull/203)), neither of which has this gap. | ~~`DINModelRegistry.sol` L472-477~~ | +| L-5 | **Fixed/obsolete** — `withdrawFees` no longer exists in `DINModelRegistry.sol`; superseded by `setFees`/`sweepFeesToRouter` (commit `06190e8`, "scope DINModelRegistry fee path to ETH-only, accumulate-then-sweep"), neither of which has this gap. | ~~`DINModelRegistry.sol` L472-477~~ | | L-6 | **Fixed — [PR #176](https://github.com/InfiniteZeroFoundation/DevNet/pull/176).** `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` L256-264 (+ `setDINTaskAuditorContract` L272-283), `DINTaskAuditor.sol` L399-424 | -| L-7 | **Fixed** (task_210726_6 §3) — `totalDepositedRewards` replaced by a real per-GI `giRewardPool` mapping ("funded per-GI, settled per-GI" model), and `RewardDeposited` is now actually emitted. No `receive()`/`fallback()` still, but the reward asset model changed enough that the original "stray ETH via forced selfdestruct" framing may no longer apply — not re-verified here. | `DINTaskAuditor.sol` | +| L-7 | **Partly fixed** (task_210726_6 §3) — the dead-code half: `totalDepositedRewards` replaced by a real per-GI `giRewardPool` mapping ("funded per-GI, settled per-GI" model), and `RewardDeposited` is now actually emitted (`DINTaskAuditor.sol:487`). The stray-ETH half still applies as originally written: the contract still has no `receive()`/`fallback()` or any ETH withdrawal path, so ETH forced into it (e.g. via `selfdestruct`) is still permanently unrecoverable (re-verified against `develop`). | `DINTaskAuditor.sol` | | 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 | ---