task_061026_22.md

Task: Security Fixes for Your Own Findings — DinEmission.fundGI Mint/Drain (#226) + Manifest Path Traversal in dincli (#227)

ID: task_061026_22 Assigned to: Similoluwa Abidoye (@Abidoyesimze) Created: 2026-10-06 Status: Open (assigned) Dates: No fixed deadline. Parts A and B are independent and can both start immediately. Post progress in the tracking discussion so the review can be scheduled. Repo: https://github.com/InfiniteZeroFoundation/DevNet Base branch: develop. The spec was written against commit a1fcce2 (2026-10-05). Re-pull before starting, whenever you begin. Roadmap ref: P3-6 (security hardening before audit). DevNet 2.0 launch readiness: both findings affect every network the platform is deployed to. Tracking:

• Part A: #226 (closes, pending review). Scope agreed in the #226 status update.
• Part B: #227 (closes, pending review). Scope agreed in the #227 status update.
• Discussion #233 (progress updates, questions, PR links)
• Your other open issues, not in this task:
  • #223: covered by umermjd11's task-plan-051026-2 TP-3, PR #229.
  • #221, #222, #224: blocked on maintainer decisions. Each has a status update listing the open questions.

---

Status check at a1fcce2

Neither finding is fixed on develop:

Issue	State at a1fcce2
#226	DinEmission.fundGI(gi, taskAuditor) (foundry/src/DinEmission.sol:174) is external with no caller or taskAuditor check beyond != address(0). It mints to itself, calls safeIncreaseAllowance(taskAuditor, amount), then calls IDINTaskAuditor(taskAuditor).depositRewards. emissionState/giEmissionFunded are keyed by taskAuditor, so a fresh address restarts the decay schedule. DinCoordinator.mintCap defaults to 0 (uncapped). DinEmission holds no registry reference (storage: coordinator, dinToken, schedule params, the two mappings, uint256[50] private __gap at :115).
#227	DinContext.ensure_file_exists (dincli/cli/context.py:490) does file_path.parent.mkdir(parents=True, exist_ok=True) and writes the fetched file, with no containment check. dincli/services/ipfs.py:81 _normalize_path only logs a warning for /etc, /boot, /dev, /proc. Manifest-driven call sites build <base> / Path(manifest[...]["path"]), so an absolute path replaces the base and .. escapes it.

Correction to #226's recommendation: DINModelRegistry.getModelIdByTaskAuditor(taskAuditor) (foundry/src/DINModelRegistry.sol:422) returns (bool exists, uint256 modelId). Model ID 0 is a real model (modelId = models.length at approval), so the check must use exists, not modelId != 0.

---

Sequencing and coordination

Part	Overlaps with	Resolution
A	task-plan-051026-2 (PR #229) TP-3 adds dincli model-owner rewards fund-emission, which calls fundGI	Start now. TP-3 follows whatever check lands here. Mention the new revert in your PR so the command can check it first
A	task-plan-051026-1 (PR #219) changes DINTaskAuditor, not DinEmission	None
B	task-plan-051026-2 TP-3 adds commands to dincli/cli/auditor.py and dincli/cli/context.py	Start now. Whichever PR lands second rebases. Keep the helper small and in one place so the rebase is mechanical

Both Parts are independent of each other. One PR each, from separate branches into develop.

---

Part A — DinEmission.fundGI only funds registered task auditors (#226) — PR 1

A1. Registry check

• Add a DINModelRegistry reference to DinEmission:
  • Storage: declare it after the existing state and before __gap, and shrink __gap from 50 to 49. DinEmission is behind a transparent proxy, so the existing slot order must not change. (Amended at the PR No. 234 merge: __gap stays at 50. Nothing on develop is deployed yet, so per the "Core rules" in storage_layout.md new variables go above __gap without shrinking it.)
  • Set it in initialize (a new argument, InvalidAddress on zero, like the other two) and through an onlyOwner setter with an event, for an already-initialised proxy. The setter also rejects zero.
  • Interface: add a minimal getModelIdByTaskAuditor(address) returns (bool, uint256) (plus modelDisabled(uint256) returns (bool) if you take A2's option) to the interface block at the top of the file. Don't import the whole registry.
• In fundGI, before any state change: (bool exists, ) = registry.getModelIdByTaskAuditor(taskAuditor); if (!exists) revert UnregisteredTaskAuditor(taskAuditor); with a new custom error. Keep the existing zero-address check first.
• Keep fundGI permissionless. Its NatSpec argument (the amount is fixed by the schedule) holds once the recipient is constrained. Update the NatSpec to say the recipient must be an approved model's task auditor, and why (#226).

A2. Decision to flag in the PR: disabled models

Whether fundGI also refuses a model with modelDisabled[modelId] == true. Recommended: refuse. Emission for a model the DIN-Representative disabled is hard to justify, and the check is one extra view call. Say which you chose. This doesn't settle #224 M-2 (task contracts ignoring disableModel), which stays open.

A3. Deploy script and docs

• foundry/script/DeployPlatform.s.sol: DINModelRegistry is deployed at step 10, before DinEmission at step 13, so pass the registry proxy into DinEmission.initialize. Update DeployPlatform.t.sol / DeployPlatformScript.t.sol assertions that read the emission wiring.
• Documentation/technical/storage_layout.md ## DinEmission: add the new slot and the gap change.
• Documentation/technical/contracts/ doc for DinEmission (if present) and foundry-src-security-review.md: record #226 as a post-review finding, fixed, in the same format as the other fix notes.
• dincli/abis/: regenerate any bundled ABI that changed (dincli system dump-abi --official --artifact foundry/out/<C>.sol/<C>.json --output dincli/abis).

A4. Tests

• Your two PoCs become regression tests, now expecting the revert:
  • test_fundGI_arbitraryTaskAuditor_drainsMintedDIN → expects UnregisteredTaskAuditor, with no mint (totalMinted unchanged).
  • test_fundGI_repeatedWithFreshFakeAuditors_eachGetsUndecayedAmount → each fresh address reverts.
• The existing EmissionTests.t.sol happy paths keep passing. Their MockTaskAuditorReal has to be registered in a real (or minimal mock) registry, so the fixture changes and the assertions don't.
• Setter: owner-only, zero-address revert, event emitted.
• Upgrade safety: UpgradeValidation.t.sol green. If there's an upgrade-path test for DinEmission, the new slot doesn't collide.
• Break-then-fix: remove the registry check, confirm both PoC regression tests fail, then restore.

Deliverables (Part A)

• fundGI reverts for any taskAuditor the registry doesn't map to an approved model (plus the disabled-model decision from A2, stated in the PR)
• Registry reference set in initialize and through an owner setter, appended before __gap (kept at 50, see the A1 amendment); storage_layout.md updated
• DeployPlatform.s.sol wires it; deploy tests updated
• Both PoC tests committed as regression tests; existing emission tests pass; break-then-fix shown in the PR
• cd foundry && npm ci && forge build && forge test green across the full suite, including UpgradeValidation.t.sol
• Bundled ABIs refreshed where they changed; security-review doc notes #226 as fixed

---

Part B — Manifest paths can't leave the model's directory (#227) — PR 2

B1. One shared helper

• Add resolve_manifest_path(base_dir: Path, manifest_path: str | Path) -> Path in one shared place (dincli/cli/utils.py or next to ensure_file_exists). It:
  • rejects an absolute manifest_path (Path(manifest_path).is_absolute());
  • resolves (base_dir / manifest_path).resolve() and rejects it unless is_relative_to(base_dir.resolve());
  • raises a clear error naming the manifest key and the offending path. It never re-anchors silently.
• Symlinks: resolve() follows them, so a symlink inside the cache dir that points outside is rejected. Keep it that way and say so in the PR.

B2. Every call site goes through it

Replace each <base> / Path(manifest[...]["path"]) with the helper:

• dincli/cli/client.py:129-130
• dincli/cli/aggregator.py:336-337, :557-558
• dincli/cli/auditor.py:450-453
• dincli/cli/modelownerd/aggregation.py:399-400
• dincli/cli/modelownerd/auditor_batches.py:144
• dincli/cli/modelownerd/model.py:103-104, :219-220
• the task-contract artifact path in dincli/cli/context.py:316 (model_base_path / artifact_rel_path)

Grep for any others (git grep -n '\["path"\]' dincli) and list what you changed in the PR.

B3. Backstop in ensure_file_exists

ensure_file_exists gets a containment check too. Give it the trusted base directory, or check against CACHE_DIR and the task directory it's called for, so a future call site that forgets the helper still can't write outside. Leave _normalize_path's warning in place. It covers non-manifest paths and isn't the fix.

B4. Tests

pytest, in the existing monkeypatch style (see tests/test_ipfs_config.py, tests/test_dintoken.py). No network, IPFS or Docker.

• Helper:
  • a normal relative path resolves inside the base;
  • an absolute path is rejected;
  • ../../x is rejected;
  • a symlink inside the base pointing outside is rejected.
• Call sites: at least one per role (client, auditor, aggregator, model owner) plus the artifact path. A manifest with "path": "/tmp/…/outside" raises before retrieve_from_ipfs is called (monkeypatch it and assert it's never reached).
• ensure_file_exists backstop: a direct call with an outside path raises.
• Break-then-fix: bypass the helper at one call site, confirm its test fails, then restore.

Deliverables (Part B)

• One shared resolve_manifest_path helper rejecting absolute paths, .. escapes and symlink escapes
• Every manifest-driven call site uses it (list them in the PR); ensure_file_exists has the backstop check
• Tests as in B4, with break-then-fix shown in the PR
• pytest -m "not integration" green
• Documentation/public/manifest.md: one sentence that path fields must be relative and stay inside the model's directory

---

Submission

Two PRs from separate branches into develop:

• PR 1 (Part A): closes #226. Include the disabled-model decision, the storage-layout diff, and the break-then-fix output.
• PR 2 (Part B): closes #227. Include the call-site list and the break-then-fix output.

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

Notes

• Flag the DinEmission.sol storage change for Umer's review. It's a proxy contract, and storage layout is the thing to get right.
• Nothing on develop is deployed to a live network (Sepolia runs the older DevNet 1.0 contracts), so Part A needs no migration script. The owner setter is for completeness on already-initialised proxies.
• If a cleaner check than the registry mapping turns up for Part A (for example, emission keyed by modelId rather than auditor address), raise it on #226 before building it. Re-keying emissionState changes storage meaning and touches PR #229's planned command.
• Keep each PR to its issue. Other findings you spot along the way go in new issues, as you did with #221–#227.