task_061026_22 — Security Fixes: DinEmission.fundGI Mint/Drain (#226) + Manifest Path Traversal in dincli (#227) (Similoluwa)

[umeradl]

cc @abrahamnash: this task is scoped and ready for your approval / HR-side pre-processing. @Abidoyesimze, it's assigned to you.

Full spec: Developer/tasks/task_061026_22.md

The task file has the full specification: exact file/line references, coordination with the open plans, and the deliverables checklists. This post is the summary and the decisions that need flagging.

Timeline

No fixed deadline. Parts A and B are independent and can both start immediately. Post progress here so the reviews can be scheduled.

Summary

Similoluwa opened six issues on 2026-10-05 from a codebase scan. Each now has a status update with what can be done now. Two are confirmed security bugs with clear fixes, and they're this task: two PRs into develop.

• Part A: #226, DinEmission.fundGI mint/drain (status update).
  • fundGI is permissionless and mints, approves and calls into whatever taskAuditor the caller passes. With mintCap = 0 by default, anyone can mint DIN to themselves.
  • Fix: DinEmission gets a DINModelRegistry reference (a new slot before a 49-slot gap, set in initialize plus an owner setter, wired in DeployPlatform.s.sol). fundGI then reverts unless getModelIdByTaskAuditor returns exists.
  • Correction to the issue: the lookup returns (bool exists, uint256 modelId), and model ID 0 is valid, so the check is exists, not modelId != 0.
• Part B: #227, manifest path traversal in dincli (status update).
  • Manifest path fields are joined onto the cache dir, and an absolute path or .. lets a malicious model owner write any file on a participant's machine before any Docker sandbox runs.
  • Fix: one shared resolve_manifest_path helper that rejects absolute, .. and symlink escapes, used at all ~10 call sites, plus a backstop check in ensure_file_exists, with pytest.

Your other issues (not in this task)

Issue	Status
#223	Covered by umermjd11's task-plan-051026-2 TP-3 (PR #229)
#221	Needs a decision on where each fee-router bucket goes (with #43 / #155)
#222	Needs a GI-abort design, ideally before the DevNet 2.0 deploy
#224	Three maintainer decisions; options posted on the issue

Sequencing

Part	Overlap	Resolution
A	PR #229's planned dincli model-owner rewards fund-emission calls fundGI	Start now. Note the new revert in the PR so the command can pre-check it
B	PR #229 TP-3 also edits dincli/cli/auditor.py and context.py	Start now. Whichever lands second rebases

Decisions to flag, not guess

1. Part A, disabled models: whether fundGI also refuses a model with modelDisabled == true. The recommendation is yes. Say which in the PR. This doesn't settle Three dormant/no-op mechanisms awaiting a product decision: disableModel kill-switch, rejectModel fee handling, S6 slashing (M-2, L-4, BL-31) #224 M-2.
2. Part A, storage: DinEmission is a proxy. Append the new slot before __gap, shrink the gap to 49, and update storage_layout.md. Flag it for review.
3. Part A, alternative designs: if you think emission should be keyed by modelId instead of auditor address, raise it on security: DinEmission.fundGI lets anyone mint and drain DIN via an unvalidated taskAuditor address #226 before building it. That changes storage meaning and PR docs(tasks-plan): umermjd11 task plan 051026-2 — full-GI alignment: contracts check + richer GI states, dincli commands, integration suite on foundry, GI docs #229's planned command.
4. Part B, symlinks: a symlink inside the cache dir that points outside is rejected (resolve() follows it). Say so in the PR.

Reference material

• Developer/tasks/task_061026_22.md: full spec
• #226, #227: tracking issues, with their status updates
• Documentation/technical/storage_layout.md: proxy storage reference for Part A
• Documentation/technical/audits/foundry-src-security-review.md: your July review; record security: DinEmission.fundGI lets anyone mint and drain DIN via an unvalidated taskAuditor address #226 there once fixed

Post progress updates, questions and PR links here, same convention as prior task-tracking discussions. If anything in the spec doesn't feel right, is ambiguous, or needs clarification, comment here and cc @umeradl.

[abrahamnash]

Thanks for scoping this out, @umeradl. No issues on my side @Abidoyesimze.

[Abidoyesimze]

Part A done: PR #234, closes #226.

DinEmission now holds a DINModelRegistry reference (new proxy storage slot, __gap shrunk 50→49, confirmed storage-safe via test_validateImplementation_DinEmission). fundGI reverts UnregisteredTaskAuditor unless the registry maps taskAuditor to an approved model, and ModelDisabledForEmission if that model's disabled — A2 decision: refuse disabled models, per the issue's own reasoning (hard to justify, one extra view call). Both PoCs from the issue are now committed regression tests, break-then-fix confirmed, full suite green (498/498 including UpgradeValidation.t.sol).

Starting Part B (#227) now.

[Abidoyesimze]

Part B done: PR #237, closes #227.

New resolve_manifest_path(base_dir, manifest_path) helper rejects absolute paths, .. escapes, and symlink escapes (raises rather than re-anchoring). Every manifest-driven call site goes through it now — client, aggregator (×2), auditor (×3), modelownerd/{aggregation,auditor_batches,gi,model} (×2), plus the task-contract artifact path in context.py. Also caught modelownerd/gi.py's call site via the git grep sweep — it wasn't in the issue's original list but had the identical pattern. ensure_file_exists gets its own backstop containment check too (defaults to CACHE_DIR), so a future call site that forgets the helper still can't escape.

12/14 new tests pass locally; the other 2 (aggregator/auditor per-role) are blocked by this sandbox's Python 3.10 (pre-existing, unrelated — same reason test_dintoken.py is already excluded everywhere). Written against the identical, verified pattern the other two use. Break-then-fix confirmed, full suite green otherwise (199 passed, no new regressions beyond the known baseline).

Both parts of task_061026_22 are now up: #234 (Part A, #226) and #237 (Part B, #227).

[umeradl]

Both Parts of task_061026_22 are merged into develop, and issues No. 226 and No. 227 are closed as completed (226 closing comment, 227 closing comment). Thanks @Abidoyesimze.

Checked on origin/develop @ 920c93e. The develop push CI is green.

Part A, PR No. 234: merged as 2fa8ce8, with a follow-up fix in 07fd597.

Deliverable	Evidence
fundGI refuses unregistered auditors (A2: disabled models)	DinEmission.sol:214-216. It reverts UnregisteredTaskAuditor unless getModelIdByTaskAuditor returns exists, and reverts ModelDisabledForEmission for a disabled model.
Registry reference: initialize + owner setter, before __gap; storage_layout.md	modelRegistry is slot 9 and __gap[50] is slot 10. It's zero-checked in initialize, and setModelRegistry is onlyOwner with a zero check and an event. storage_layout.md ## DinEmission is updated.
DeployPlatform.s.sol wiring + deploy tests	DeployPlatform.s.sol:285-289 passes dinModelRegistryProxy. DeployPlatform.t.sol and DeployPlatformScript.t.sol are updated.
PoC regression tests, break-then-fix	EmissionTests.t.sol has test_fundGI_unregisteredTaskAuditor_revertsBeforeAnyStateChange and test_fundGI_repeatedFreshUnregisteredAuditors_allRevert, plus disabled-model and setter tests. Break-then-fix is in the PR description.
Full forge suite green, incl. UpgradeValidation.t.sol	It's green in CI. The local run at the PR No. 237 merge gave 501 passed, 0 failed.
ABIs; security-review note	There is no bundled dincli/abis ABI for DinEmission, so nothing changed. foundry-src-security-review.md records No. 226 as fixed.

• Deviation: 07fd597 keeps __gap at 50 rather than 49. Nothing on develop is deployed, so per storage_layout.md's core rules new slots go above the gap without shrinking it. This was recorded as the A1 amendment in the task file.

Part B, PR No. 237: merged as 0be3d4a, with a follow-up fix in 920c93e.

Deliverable	Evidence
Shared resolve_manifest_path	dincli/cli/utils.py. It rejects absolute paths, .. escapes and symlink escapes, and raises ManifestPathEscapesBaseError naming the key.
Every call site + ensure_file_exists backstop	All 19 manifest path joins go through the helper (modelownerd/gi.py was caught by the sweep). ensure_file_exists checks containment before any fetch.
B4 tests, break-then-fix	tests/test_manifest_path_traversal.py has 16 tests. Break-then-fix is in the PR description.
pytest -m "not integration" green	468 passed, 0 failed.
manifest.md sentence	The path row says it must be relative and stay inside the model's cache or task directory.

• Deviation: 920c93e replaces the CACHE_DIR default for base_dir with a required, role-scoped base. The default had broken the model owner's local task flow.

  • Participants use get_model_base_dir(model_id).
  • The model owner before registration uses get_task_dir(coordinator).

  Details are in the PR No. 237 outcome comment.

No out-of-scope follow-ups were raised. All deliverables are met, so I'm closing this as resolved and ticking the task file's checkboxes.