docs: regrade IMPROVEMENT item 1 — a global DOCUMENT_MANAGER_ROLE satisfies the draft's per-subject MUST

Author: rya-sge

CHANGELOG.md

@@ -140,8 +140,8 @@ Aligned the implementation with the updated [ERC-1643](./doc/ERCSpecification/er
   Responsibility* rules. The **admin path remains non-conformant by construction** — a write sent
   straight to the engine has no execution point in the subject, so the subject emits nothing.
   See [`IMPROVEMENT.md`](./IMPROVEMENT.md) item 2.
-- Open conformance gaps are tracked in [`IMPROVEMENT.md`](./IMPROVEMENT.md): per-`subject`
-  authorization (item 1), admin-path call topology (item 2), and enumeration cost (item 4).
+- Open items are tracked in [`IMPROVEMENT.md`](./IMPROVEMENT.md): the most severe is admin-path call
+  topology (item 2); also authorization granularity (item 1) and enumeration cost (item 4).
 - CMTAT v3 no longer ships a *standalone* token that consumes an external document engine through its constructor; the standard token stores documents on-chain (`DocumentERC1643Module`). External-engine integration now goes through CMTAT's `DocumentEngineModule` (`setDocumentEngine`). The test suite was updated to exercise this real integration path via a minimal token built on `DocumentEngineModule`.
 
 ## v0.3.0

IMPROVEMENT.md

@@ -23,80 +23,117 @@ tracked separately under [`doc/audits/tools/`](./doc/audits/tools).
 
 | # | Item | Severity | Effort | Kind |
 | --- | --- | --- | --- | --- |
-| [1](#1--authorization-is-not-per-subject-and-the-hook-cannot-express-it) | Authorization is not per-`subject`, and the hook cannot express it | **High** | Medium | Spec `MUST` |
+| [1](#1--authorization-granularity-is-fixed-at-compile-time-the-hook-cannot-express-per-subject-rules) | Authorization granularity is fixed at compile time; the hook cannot express per-`subject` rules | Low¹ | Medium | Extensibility |
 | [2](#2--the-admin-path-bypasses-subject-side-erc-1643-emission) | Admin path bypasses subject-side ERC-1643 emission | **Medium** | Small–Medium | Spec `SHOULD` |
 | [3](#3--the-engine-advertises-ierc1643-but-is-not-a-usable-erc-1643-endpoint) | Engine advertises `IERC1643` but is not a usable ERC-1643 endpoint | Low | Trivial | Docs |
 | [4](#4--enumeration-cost-and-removal-complexity) | Enumeration cost and removal complexity | Low | Medium | Gas |
 | [5](#5--the-erc-2771-forwarder-is-a-universal-write-authority) | ERC-2771 forwarder is a universal write authority | Info | Trivial | Docs |
 | [6](#6--_removedocument-emits-before-the-state-change) | `_removeDocument` emits before the state change | Info | Trivial | Cosmetic |
 | [7](#7--upstream-imultidocumentsubject-manager-discovery) | Upstream: `IMultiDocumentSubject` manager discovery | Info | — | Upstream |
 
-Item 1 is the only one that changes what the contract can express; everything else is documentation,
-gas, or cosmetic. Item 1 is also **source-compatible** for existing deployments — the default hook
-bodies would ignore the new argument — so it does not have to wait for a breaking release.
+¹ Low for the single-issuer fleet this engine targets, which is the model the draft sets out to
+support. **Medium** only for a deployment shared by unrelated issuers — see item 1 for why that
+configuration is not supportable today.
+
+**Item 2 is the most severe open item.** Item 1 is the only one that changes what the contract can
+*express*, and it is source-compatible for existing deployments (the default hook bodies would ignore
+the new argument), so it need not wait for a breaking release. Everything else is documentation, gas,
+or cosmetic.
 
 ---
 
-## 1 — Authorization is not per-`subject`, and the hook cannot express it
+## 1 — Authorization granularity is fixed at compile time; the hook cannot express per-`subject` rules
 
-**Severity:** High · **Effort:** Medium · **Kind:** deviation from a specification `MUST`
+**Severity:** Low — **Medium** for a deployment shared across unrelated issuers · **Effort:** Medium
+· **Kind:** extensibility + deployment guidance
 
-**Where:** `src/DocumentEngineBase.sol:56`, `:71-84`, `:111-186`; `src/DocumentEngine.sol:46-48`;
+**Where:** `src/DocumentEngineBase.sol:56`, `:111-186`; `src/DocumentEngine.sol:46-48`;
 `src/DocumentEngineOwnable.sol:39-41`
 
-> Implementations **MUST** authorize writes per `subject`, so that a caller cannot create, update, or
-> remove documents for a `subject` it is not permitted to manage.
-> — draft §Authorization
+### This is not a conformance failure
 
-`DOCUMENT_MANAGER_ROLE` is a single global permission: every holder may write for *every* subject.
-`DocumentEngineOwnable` is the same with `owner`. This is precisely the risk the draft's Security
-Considerations open with — "a management contract holds the document sets of unrelated subjects
-behind a single address" — and the engine's design is, by default, the unmitigated case.
+The draft's requirement is:
 
-That alone might be defensible as a deployment choice, since the project's stated model is a single
-operator managing a fleet it owns. The structural problem is that the escape hatch does not work:
+> Implementations MUST authorize writes per `subject`, so that a caller cannot create, update, or
+> remove documents for a `subject` **it is not permitted to manage**.
+> — draft §Authorization
 
-```solidity
-function _authorizeDocumentManagement() internal view virtual;   // no subject parameter
-```
+The operative words are "not permitted to manage", and what a caller is permitted to manage is
+defined by the deployment's own access control. `DOCUMENT_MANAGER_ROLE` is a specific, granted role
+whose permission covers every subject the engine serves — so there is no subject its holder is *not*
+permitted to manage, and the clause is satisfied. Same for `owner` in `DocumentEngineOwnable`.
+
+This is the case the draft explicitly sets out to support:
 
-A deployment cannot override this hook to implement per-subject rules, because at the point it runs
-the subject is not available. In the batch functions the situation is worse — the modifier fires
-**once** for the whole call, before any element is read, so even a subject-aware hook would be
-bypassed for `batchSetDocuments` / `batchRemoveDocuments`.
+> An issuer operating many tokens, funds, or vaults typically maintains one document library and
+> **one set of operators**, and duplicating that storage and access-control logic into every subject
+> contract is redundant and expensive.
+> — draft §Motivation
 
-Consequently the draft's own test case —
+A single global operator role over a fleet one issuer controls is that design, not a departure from
+it. The draft's test case — "A caller not authorized for a subject failing to create, update, or
+remove that subject's documents" — is covered by `testCannotNonAdminSetDocument` and its siblings: an
+account without the role is authorized for no subject, and its write reverts.
 
-> A caller not authorized for a subject failing to create, update, or remove that subject's documents.
+### What is actually open
 
-— is untestable against this codebase, and is indeed absent from the suite. The existing
-`testNonAdmin*` tests cover the globally-unauthorized case only.
+The draft's Security Consideration is about **unrelated** subjects:
 
-**Mitigating factor.** The **bound-token path is already per-subject** and cannot be escaped: the
-namespace is `_msgSender()`, structurally. A deployment that overrides
-`_authorizeDocumentManagement()` to always revert is fully conformant today — but that removes the
-admin path entirely rather than scoping it.
+> If writes are not authorized per `subject`, any caller permitted to write for one subject can
+> modify another subject's legal or operational references.
 
-**Recommendation.** Change the hook signature and call it per subject:
+That bites only when one engine instance is shared by parties that do not trust each other — two
+issuers, or a service operator hosting documents for external clients. In that deployment a global
+role does breach the property, and **this engine cannot currently express the alternative**, because
+the authorization hook receives no subject:
 
 ```solidity
-function _authorizeDocumentManagement(address subject) internal view virtual;
-
-function batchSetDocuments(address[] calldata subjects, ...) external {
-    for (uint256 i = 0; i < length; ++i) {
-        _authorizeDocumentManagement(subjects[i]);
-        _setDocument(subjects[i], names[i], uris[i], hashes[i]);
-    }
-}
+function _authorizeDocumentManagement() internal view virtual;   // no subject parameter
 ```
 
-The default implementations stay exactly as they are (`_checkRole(DOCUMENT_MANAGER_ROLE)` /
-`_checkOwner()`, ignoring `subject`), so behaviour and gas are effectively unchanged, but a
-deployment gains the ability to be conformant — for example a per-subject role
-`keccak256("DOCUMENT_MANAGER", subject)`. Token binding needs a separate hook
-(`_authorizeTokenBinding()`), since binding has no subject.
-
-Add the draft's missing test case once the hook can express it.
+So a deployer cannot subclass their way to per-subject rules; they would have to edit
+`DocumentEngineBase`. Two consequences:
+
+1. **A multi-tenant deployment is not supportable today.** The only conformant option is one engine
+   instance per trust domain — which is fine, and cheap, but is a deployment constraint that should
+   be written down rather than discovered.
+2. **It contradicts the project's own advertised extension model.** The README and `CLAUDE.md`
+   promise that a deployment changes *who* is authorized by overriding a hook, "not by editing the
+   management functions". That holds for swapping roles for an owner; it does not hold for making the
+   decision depend on the subject. The hook is the documented seam, and this is the one axis it
+   cannot turn.
+
+Related detail, relevant only if the hook ever gains a subject: in the batch functions the modifier
+fires **once** for the whole call, before any element is read, so a subject-aware hook would have to
+be invoked inside the loops rather than via the modifier.
+
+### Recommendation
+
+Low priority, and **not** required for the single-issuer model this engine targets. Two options:
+
+- *Documentation only* (sufficient today): state in the README that one engine instance serves one
+  trust domain, and that unrelated issuers should each deploy their own rather than share one.
+- *Enable the axis*, if multi-tenant support is ever wanted:
+
+  ```solidity
+  function _authorizeDocumentManagement(address subject) internal view virtual;
+
+  function batchSetDocuments(address[] calldata subjects, ...) external {
+      for (uint256 i = 0; i < length; ++i) {
+          _authorizeDocumentManagement(subjects[i]);
+          _setDocument(subjects[i], names[i], uris[i], hashes[i]);
+      }
+  }
+  ```
+
+  The default implementations stay exactly as they are (`_checkRole(DOCUMENT_MANAGER_ROLE)` /
+  `_checkOwner()`, ignoring `subject`), so behaviour and gas are unchanged and no existing deployment
+  is affected — a subclass simply gains the option of a per-subject rule such as
+  `keccak256("DOCUMENT_MANAGER", subject)`. Token binding would need a separate hook
+  (`_authorizeTokenBinding()`), since binding has no subject.
+
+Note also that the bound-token path is already per-subject and cannot be escaped: the namespace is
+`_msgSender()`, structurally. A subject that manages its own documents is unaffected by any of this.
 
 ## 2 — The admin path bypasses subject-side ERC-1643 emission
 

README.md

@@ -2,6 +2,12 @@
 
 > This project has not been audited yet, please use at your own risk. For any questions, please contact [admin@cmta.ch](mailto:admin@cmta.ch).
 >
+> **Known open items** are tracked in **[`IMPROVEMENT.md`](./IMPROVEMENT.md)**. None is an
+> exploitable vulnerability, but integrators should read it before relying on the engine — in
+> particular item 2 (a write sent straight to the engine leaves an ERC-1643 subject's own events
+> unemitted) and item 1 (one engine instance serves **one trust domain**: `DOCUMENT_MANAGER_ROLE`
+> covers every subject, so unrelated issuers should each deploy their own engine rather than share
+> one).
 
 The `DocumentEngine` is an external contract to manage documents through [*ERC-1643*](https://github.com/ethereum/EIPs/issues/1643), a standard proposition to manage document on-chain. This standard is notably used by [ERC-1400](https://github.com/ethereum/eips/issues/1411) from Polymath. 
 
@@ -364,7 +370,7 @@ slither . --checklist --filter-paths "node_modules,test,forge-std,CMTAT,openzepp
 
 > **Static-analysis output is leads, not findings.** Every dismissal in the feedback files was
 > verified against the cited `file:line`, and neither tool can see the specification-level issues
-> that matter most here — those are in [`ERC_RESULT.md`](./ERC_RESULT.md).
+> that matter most here — those are in [`IMPROVEMENT.md`](./IMPROVEMENT.md).
 
 ### Surya
 

doc/audits/AUDIT_OVERVIEW.md

@@ -56,7 +56,7 @@ detail, with a recommendation for each, in [`IMPROVEMENT.md`](../../IMPROVEMENT.
 
 | Item | Severity | Where |
 | --- | --- | --- |
-| Authorization is not per-`subject`, and `_authorizeDocumentManagement()` takes no `subject`, so a deployment cannot make it per-subject by overriding the hook | High | item 1 |
+| `_authorizeDocumentManagement()` takes no `subject`, so a deployment cannot make authorization per-subject by overriding the hook. Conformant for the single-issuer fleet the engine targets — `DOCUMENT_MANAGER_ROLE` is permitted to manage every subject — but it means one instance serves one trust domain | Low (Medium if shared across unrelated issuers) | item 1 |
 | Admin write path has no execution point in the subject, so an ERC-1643 subject emits nothing for writes sent straight to the engine | Medium | item 2 |
 | Engine advertises `IERC1643` but its base functions are `_msgSender()`-scoped, so it is not a usable endpoint for an external consumer | Low | item 3 |
 | `_removeDocumentName` is O(n); no paginated enumeration | Low | item 4 — also surfaced by Aderyn L-5 |

doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md

@@ -30,7 +30,7 @@ CMTAT upgrade.
 
 | ID | Detector | Sev | Instances | Disposition | Reason (verified against the cited lines) |
 | --- | --- | --- | --- | --- | --- |
-| L-1 | Centralization Risk | Low | 2 | **By design** | `DocumentEngine.sol:24`, `DocumentEngineOwnable.sol:24`. The whole premise of the contract is that a trusted operator manages documents for a fleet of subjects; `DOCUMENT_MANAGER_ROLE` (and `owner`) are that operator. Documented in the README and analysed in `IMPROVEMENT.md` item 1, which argues the privilege should be *narrowed to per-subject*, not removed. Aderyn cannot express that distinction. |
+| L-1 | Centralization Risk | Low | 2 | **By design** | `DocumentEngine.sol:24`, `DocumentEngineOwnable.sol:24`. The whole premise of the contract is that a trusted operator manages documents for a fleet of subjects; `DOCUMENT_MANAGER_ROLE` (and `owner`) are that operator. Documented in the README and analysed in `IMPROVEMENT.md` item 1, which concludes the global role is the correct model for the single-issuer fleet this engine targets. Aderyn cannot express that distinction. |
 | L-2 | Unspecific Solidity Pragma | Low | 9 | **By design** | Every file uses `pragma solidity ^0.8.20;`. The caret is intentional so the sources stay consumable as a library by projects on a different `0.8.x`; the compiler actually used for the deployed bytecode is pinned to `0.8.34` in `foundry.toml`, and `foundry.lock` pins every dependency. Verified: no file uses a construct that behaves differently across the allowed range. |
 | L-3 | PUSH0 Opcode | Low | 9 | **Environment** | Consequence of `^0.8.20` plus `evm_version = prague`: the compiler emits `PUSH0`, which is unavailable on chains that have not adopted Shanghai. Not a source defect. A deployer targeting such a chain must lower `evm_version` in `foundry.toml` — but CMTAT v3 itself requires `prague`, so that configuration is out of scope for this engine. |
 | L-4 | Loop Contains `require`/`revert` | Low | 4 | **By design** | `DocumentEngineBase.sol:124, 142, 156, 170` — the four batch loops. The reverts are raised inside `_setDocument` / `_removeDocument` (`ERC1643InvalidName`, `MultiDocumentInvalidSubject`, `ERC1643MissingDocument`). Batch operations are deliberately **all-or-nothing**: a batch containing one bad entry must not half-apply, since partial application would leave the operator unable to tell which documents were written without re-reading every entry. Skipping bad entries instead would silently drop them. |