diff --git a/.gitignore b/.gitignore index d9e0058..810321e 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,5 @@ docs/ # Dotenv file .env + +history diff --git a/.gitmodules b/.gitmodules index 415e443..4d17ab7 100644 --- a/.gitmodules +++ b/.gitmodules @@ -10,3 +10,6 @@ [submodule "lib/openzeppelin-contracts-upgradeable"] path = lib/openzeppelin-contracts-upgradeable url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable +[submodule "lib/RuleEngine"] + path = lib/RuleEngine + url = https://github.com/CMTA/RuleEngine diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..95b69ad --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,176 @@ +# DocumentEngine — Agent Guide + +> **Note — keep in sync:** `AGENTS.md` and `CLAUDE.md` must always be **identical**. +> Any edit to one must be applied verbatim to the other. + +> **Note — commit messages:** After each group of modifications or each feature +> added, always provide a **one-line GitHub commit message** (Conventional-Commits +> style, e.g. `feat: add token binding`, `fix: correct event args`, `docs: update README`). + +## What this project is + +`DocumentEngine` is a standalone smart contract that manages documents on-chain +through **ERC-1643** on behalf of **several** other smart contracts (e.g. CMTAT +tokens). Using an external engine keeps each token small and lets one operator +manage documents for a whole fleet of tokens. + +A document is `{ string uri, bytes32 documentHash, uint256 lastModified }`, +addressed by a `bytes32` name. + +## Key concepts + +- **Two management paths (both active at once):** + - **Admin path** — `DOCUMENT_MANAGER_ROLE`. Address-scoped overloads + (`setDocument(address,...)`, `removeDocument(address,...)`, batch variants) + manage documents for any contract. + - **Bound-token path** — the standard single-arg `IERC1643` functions + (`setDocument(name,uri,hash)`, `removeDocument(name)`) let a bound token manage + its **own** namespace (`_msgSender()`). Bind via the shared `ITokenBinding` + surface: `bindToken(token)` / `unbindToken(token)` / `isTokenBound(token)`, + implemented **once** for both deployments by `TokenBindingModule` — a single + allowlist, NOT a role (there is no `TOKEN_CONTRACT_ROLE`). Binding is authorized + by each deployment's document-management hook (DOCUMENT_MANAGER_ROLE / owner). + NOTE: RuleEngine's `ERC3643ComplianceExtendedModule` is intentionally **not** + reused for binding — it is an `IERC3643Compliance`, which would drag in + transfer-compliance callbacks (`canTransfer`/`transferred`/`created`/`destroyed`) + irrelevant to a document engine. See the README rationale section. +- **Events (ERC-1643 emission responsibility):** this engine is a *shared, + multi-token* manager, so it emits **only** the address-carrying extension events + `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` (param `subject`) and + **not** the base `DocumentUpdated` / `DocumentRemoved` (those carry no address and + are the token contract's responsibility). Extension declared in + `src/interfaces/IERC1643MultiDocument.sol`; rationale in `ERC-1643-proposition.md`. +- **Errors live on interfaces, not on `DocumentEngineInvariant`.** Each specification + error is declared by the interface defining its condition — `ERC1643InvalidName` / + `ERC1643MissingDocument` on `IERC1643`, `MultiDocumentInvalidSubject` on + `IERC1643MultiDocument`, `TokenBindingInvalidToken` on `ITokenBinding` — so an ABI + generated from an interface carries its errors and each is obtained exactly once + (the multi-subject draft's "MUST NOT declare them twice", also a compile error). + `DocumentEngineInvariant` keeps only errors no interface defines. +- **Token binding is idempotent and rejects `address(0)`:** `bindToken` / `unbindToken` + write and emit `TokenBindingSet` only on an actual change, so every event is a real + transition; a repeat call succeeds silently. `address(0)` reverts + `TokenBindingInvalidToken()`. +- **ERC-1643 conformance:** `setDocument` reverts `ERC1643InvalidName()` on + `name == 0`; `removeDocument` reverts `ERC1643MissingDocument()` on a missing doc; + `supportsInterface` advertises `IERC1643` + `IERC1643MultiDocument` (both deployments). + Both errors are declared by `IERC1643` itself since CMTAT `v3.3.0-rc2` — do **not** + re-declare them in `DocumentEngineInvariant` (duplicate declaration = compile error, + and the multi-subject draft forbids it). +- **`getDocument` returns flat values**, `(string uri, bytes32 documentHash, + uint256 lastModified)`, never the `Document` struct — the struct is storage-only. + A struct return prepends an offset word to the returndata while leaving the selector + and `type(IERC1643).interfaceId` unchanged, so the mismatch is invisible to ERC-165 + and a spec-conformant consumer silently mis-decodes. Pinned by + `testGetDocumentReturnsFlatErc1643Abi`. +- **ERC-2771:** meta-transaction (gasless) support; `_msgSender()` is used everywhere. +- **Access control:** `DEFAULT_ADMIN_ROLE` implicitly has every role (see the + `hasRole` override). +- **Flexible access control (CMTAT / RuleEngine pattern):** restricted functions + use the `onlyDocumentManager` / `onlyBoundToken` modifiers, which delegate to + overridable `internal virtual` hooks `_authorizeDocumentManagement()` (per + deployment: `DOCUMENT_MANAGER_ROLE` / owner) and + `_authorizeBoundTokenDocumentManagement()` (implemented once by + `TokenBindingModule` → allowlist check). Keep the management implementation + separate from the authorization logic — change *who* is authorized via a hook, not by + editing the management functions. +- **CMTAT integration:** since CMTAT v3, a token uses the engine via CMTAT's + `DocumentEngineModule` and `setDocumentEngine(engine)` (reads/writes are forwarded + keyed by the token address). Standard CMTAT standalone tokens store documents + on-chain instead and do **not** use this engine. + +## File tree + +``` +src/ +├── DocumentEngineBase.sol # Abstract base: ERC-1643 document logic + storage, +│ # both management paths, batch functions, modifiers, +│ # and the ABSTRACT _authorize* hooks (no access control) +├── DocumentEngine.sol # Deployment #1: role-based access control +│ # (AccessControlEnumerable, DOCUMENT_MANAGER_ROLE, +│ # _authorizeDocumentManagement, hasRole), ERC-2771, +│ # supportsInterface, constructor +├── DocumentEngineOwnable.sol # Deployment #2: Ownable2Step (single owner) instead of +│ # roles; document mgmt + binding are owner-only +├── DocumentEngineInvariant.sol # Non-specification errors ONLY (InvalidInputLength, +│ # AdminWithAddressZeroNotAllowed). Every spec error is +│ # declared by its own interface — see the note below. +│ # NO access-control specifics +├── interfaces/ +│ ├── IERC8303.sol # ERC-8303 "Contract Version" interface (id 0x54fd4d50) +│ ├── IERC1643MultiDocument.sol # Multi-token ERC-1643 extension (address-scoped fns + +│ │ # DocumentUpdatedForSubject / DocumentRemovedForSubject + +│ │ # MultiDocumentInvalidSubject) +│ └── ITokenBinding.sol # Shared binding surface: bindToken / unbindToken / +│ # isTokenBound + TokenBindingSet + TokenBindingInvalidToken +└── modules/ + ├── VersionModule.sol # Version module: implements ERC-8303 version() + ERC-165, + │ # holds the VERSION constant (currently "0.4.0") + └── TokenBindingModule.sol # Shared token-binding allowlist (ITokenBinding) + NotBoundToken; + # wires the bound-token hook; used by both deployments + +script/ +├── DeployDocumentEngine.s.sol # Deploy role-based DocumentEngine (env: DOCUMENT_ENGINE_ADMIN, +│ # DOCUMENT_ENGINE_FORWARDER); run()=env, deploy(admin,fwd)=testable +└── DeployDocumentEngineOwnable.s.sol # Deploy Ownable variant (env: DOCUMENT_ENGINE_OWNER, _FORWARDER) + +test/ +├── DocumentEngine.t.sol # Foundry tests: deploy, access control, admin + bound-token +│ # paths, batch ops (incl. name==0 / missing-doc guards), +│ # ERC-8303 + interface discovery, event emission (asserts the +│ # base events are NOT emitted), msg.sender-scoped reads, +│ # enumeration, fuzz round-trip/isolation, CMTAT integration +│ # (CMTATDocumentEngineMock), flexible-auth override (OpenDocumentEngine) +├── DocumentEngineOwnable.t.sol # Tests for the Ownable2Step deployment (owner path, +│ # token binding, two-step ownership, ERC-8303) +└── Deploy.t.sol # Tests for the deployment scripts (deploy() state + run() env) +``` + +**Contract split (CMTAT module/deployment pattern):** `DocumentEngineBase` holds +the document logic and abstract `_authorize*` hooks; each deployment contract +supplies the concrete access control. There are two deployments — +`DocumentEngine` (role-based, `AccessControlEnumerable`) and +`DocumentEngineOwnable` (`Ownable2Step`). Add new management logic in the base; +change *who* is authorized in a deployment (implement the `_authorize*` hooks). + +Other important files: + +- `foundry.toml` — solc `0.8.34`, `evm_version = prague` (required by CMTAT v3). +- `remappings.txt` — `CMTAT/`, `RuleEngine/`, `OZ/`, `@openzeppelin/contracts-upgradeable/`. +- `CHANGELOG.md` — semver history; update on every release (current: `v0.4.0`). +- `ERC-1643-proposition.md` — proposed optional multi-token events / extension. +- `README.md` — full documentation and Surya schema. +- `doc/` — Surya diagrams/reports (`doc/script/`), coverage, and + `doc/audits/` — the security overview (`AUDIT_OVERVIEW.md`) plus versioned + static-analysis output under `doc/audits/tools/vX.Y.Z//`, each with a + `*-report.md` (summary table prepended) and a `*-report-feedback.md` triaging + every finding. Aderyn was run for `v0.4.0`; Slither has never been run here. +- `IMPROVEMENT.md` — the open items: deviations from the two ERC specifications, + with severity, effort and a recommendation for each. Update it when an item is + fixed (move the record to `CHANGELOG.md` and `doc/audits/AUDIT_OVERVIEW.md`) + or when review surfaces a new one. +- `lib/` — submodules: `CMTAT`, `RuleEngine`, `openzeppelin-contracts(-upgradeable)`, `forge-std`. + +## Dependencies (tested versions) + +- CMTAT `v3.3.0-rc2`, RuleEngine `v3.0.0-rc4` (binding-pattern reference only; compliance module not reused) +- OpenZeppelin Contracts / Contracts Upgradeable `v5.6.1` +- Solidity `0.8.34`, Foundry + +## Common commands + +```bash +forge build # compile +forge test # run the test suite +forge fmt # format +forge test --gas-report +``` + +## Conventions + +- The `VERSION` constant (in `src/modules/VersionModule.sol`, exposed via + ERC-8303 `version()`) must match the latest `CHANGELOG.md` entry on release. +- Bump `MAJOR` on incompatible proxy-storage / external-library or API changes, + `MINOR` for backward-compatible features, `PATCH` for backward-compatible fixes. +- A bound token can only ever affect its **own** document namespace — never break + that isolation. diff --git a/CHANGELOG.md b/CHANGELOG.md index 199d877..8d56bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,148 @@ # CHANGELOG -Please follow https://changelog.md/ conventions. +Please follow [https://changelog.md](https://changelog.md) conventions and the other conventions below + +## Semantic Version 2.0.0 + +Given a version number MAJOR.MINOR.PATCH, increment the: + +1. MAJOR version when the new version makes: + - Incompatible proxy **storage** change internally or through the upgrade of an external library (OpenZeppelin) + - A significant change in external APIs (public/external functions) or in the internal architecture +2. MINOR version when the new version adds functionality in a backward compatible manner +3. PATCH version when the new version makes backward compatible bug fixes + +See [https://semver.org](https://semver.org) + +## Type of changes + +- `Added` for new features. +- `Changed` for changes in existing functionality. +- `Deprecated` for soon-to-be removed features. +- `Removed` for now removed features. +- `Fixed` for any bug fixes. +- `Security` in case of vulnerabilities. + +Reference: [keepachangelog.com/en/1.1.0/](https://keepachangelog.com/en/1.1.0/) + +## Checklist + +> Before a new release, perform the following tasks + +- Code: Update the version name in the `Version` core module, variable VERSION +- Run the formatter + +> forge fmt + +- Documentation + - Perform a code coverage and update the files in the corresponding directory [./doc/general/test/coverage](./doc/general/test/coverage) + - Perform an audit with several audit tools (Aderyn and Slither), update the report in the corresponding directory [./doc/audits/tools](./doc/audits/tools) + - Update surya doc by running the 3 scripts in [./doc/script](./doc/script) + + - Update changelog + +## v0.4.0 + +Targets **CMTAT `v3.3.0-rc2`** — see the [compatibility matrix](./README.md#version-compatibility) +for which CMTAT release each version of this engine is built against. + +> **Versioning note.** `getDocument` changes shape relative to `v0.3.0`, which the convention above +> classifies as a MAJOR bump. `MINOR` is used because the project is still in its `0.x` line, where a +> `1.0.0` would wrongly signal a stable, audited release. Treat this release as breaking for any +> consumer decoding `getDocument`. + +### Changed + +- **Dependencies** + - Upgrade CMTAT `v2.5.0-rc0` → [`v3.3.0-rc2`](https://github.com/CMTA/CMTAT/releases/tag/v3.3.0-rc2) + (`lib/CMTAT` → `35d8940b40943828c5ea407dc6b22d559d92e4ae`). Development passed through + `v3.3.0-rc1`; that interim release is **not** compatible with the code as shipped here, because + it declares neither the ERC-1643 errors nor the flat `getDocument` return (see below). + - Upgrade OpenZeppelin Contracts (and Contracts Upgradeable) `v5.0.2` → `v5.6.1` + - Add [CMTA/RuleEngine](https://github.com/CMTA/RuleEngine) `v3.0.0-rc4` as a submodule (binding-pattern reference; see [Why not reuse RuleEngine's compliance module?](./README.md#why-not-reuse-ruleengines-erc-3643-compliance-module) — its `ERC3643ComplianceExtendedModule` is not reused) + - `foundry.lock` now records every submodule by tag; all five entries had gone stale since `v0.3.0`. +- **Toolchain**: bump Solidity `0.8.26` → `0.8.34` and `evm_version` `cancun` → `prague` to match CMTAT v3 (CMTAT uses `require(cond, CustomError())`, which needs solc ≥ 0.8.27) +- **`IERC1643` (CMTAT v3) breaking changes** + - `getDocument` keeps returning `(string uri, bytes32 documentHash, uint256 lastModified)` — the + flat ERC-1643 ABI — on **both** overloads, `getDocument(bytes32)` and + `getDocument(address subject, bytes32)`. CMTAT `v3.3.0-rc1` briefly replaced this with a + `Document` struct and `v3.3.0-rc2` reverted it; this engine follows rc2, so relative to `v0.3.0` + the external shape is unchanged. + + The distinction is worth recording because it is invisible to interface detection: return types + are not part of a function signature, so both shapes share the same selectors and the same + `type(IERC1643).interfaceId` (`0xecfecec8`). A consumer built from the specification ABI decodes + a struct return as garbage *without reverting* — `uri` becomes binary junk, `documentHash` + becomes `0x…60`, and `lastModified` becomes the real hash as a `uint256`. `getDocument` is now + covered by `testGetDocumentReturnsFlatErc1643Abi`, which inspects the returndata directly since + ERC-165 structurally cannot. + - The `Document` struct and the `DocumentUpdated`/`DocumentRemoved` events are now provided by `IERC1643`; the duplicate local declarations were removed from `DocumentEngineInvariant`. The struct is retained internally for storage only. + - `ERC1643InvalidName()` / `ERC1643MissingDocument()` are likewise declared by `IERC1643` as of + CMTAT `v3.3.0-rc2` and are **not** re-declared here. The multi-subject draft requires a contract + implementing both interfaces to obtain each error exactly once ("MUST NOT declare them twice"), + and re-declaring is a compile error. Selectors, and hence revert data, are unchanged. + The same principle was applied to every other error: `MultiDocumentInvalidSubject()` moved to + `IERC1643MultiDocument` and `TokenBindingInvalidToken()` is declared on `ITokenBinding`, so an + ABI generated from an interface carries its errors. `DocumentEngineInvariant` now holds only + `InvalidInputLength` and `AdminWithAddressZeroNotAllowed`, which no interface defines. + - Import path moved: `CMTAT/interfaces/engine/draft-IERC1643.sol` → `CMTAT/interfaces/tokenization/draft-IERC1643.sol`. + +- **`ERC1643InvalidSubject()` renamed to `MultiDocumentInvalidSubject()`** and moved from + `DocumentEngineInvariant` to `IERC1643MultiDocument`, matching the multi-subject draft. **This + changes the error selector**, so integrators decoding this revert must update. + + The draft's rule is that an error is prefixed by the proposal that *defines* its condition, not by + the one it sits next to. The null-`subject` condition cannot arise in ERC-1643 at all — its + `setDocument` has no `subject` argument, so the subject is implicitly the contract itself, which is + never the null address — so borrowing the `ERC1643` prefix named the error after a standard in + which it is unreachable. The two genuinely-shared errors keep their prefix for the opposite reason. +- **Token binding rejects `address(0)` and is idempotent.** `bindToken` / `unbindToken` now revert + `TokenBindingInvalidToken()` on the null address — which can never call the engine, so binding it + granted nothing while still emitting an event indexers key on — and write plus emit + `TokenBindingSet` **only when the binding actually changes**. A repeated call still succeeds, since + the caller's intent already holds, but emits nothing, so every event in the log is a real + transition and an indexer never has to de-duplicate. + +### Added + +- **Bound-token document management**: implement the now-mandatory `IERC1643.setDocument(name, uri, hash)` and `removeDocument(name)`, gated by the `onlyBoundToken` modifier and scoped to the caller (`_msgSender()`) own namespace. A token bound with `bindToken(token)` (see the shared binding module below) manages its own documents and can never affect another contract's documents. The admin overloads (explicit `address`, `DOCUMENT_MANAGER_ROLE`) are unchanged, so both systems work side by side. (RuleEngine's `ERC3643ComplianceExtendedModule` was evaluated for the binding but intentionally not reused — see the README.) +- **Optional multi-token events**: alongside the standard `IERC1643` events, the engine now also emits `DocumentUpdatedForContract` / `DocumentRemovedForContract`, which carry the `smartContract` (token) address so off-chain indexers can tell which contract a document belongs to during multi-contract operations. See [`ERC-1643-proposition.md`](./doc/ERCSpecification/ERC-1643-proposition.md) for the proposed optional standard extension. +- **Flexible access control (CMTAT / RuleEngine pattern)**: the restricted functions use the `onlyDocumentManager` / `onlyBoundToken` modifiers, which delegate to overridable `internal virtual` authorization hooks `_authorizeDocumentManagement()` / `_authorizeBoundTokenDocumentManagement()`. Each deployment implements the admin hook (`DOCUMENT_MANAGER_ROLE` or `owner`); the bound-token hook is implemented once by `TokenBindingModule` (the shared allowlist). This separates the document-management implementation from the authorization logic. +- **Split into a base contract and a deployment contract** (CMTAT module/deployment pattern): the document-management logic and storage now live in the new abstract `DocumentEngineBase` (with abstract `_authorize*` hooks), while `DocumentEngine` is the deployment contract that defines the access control (`AccessControl`, the concrete hooks and `hasRole`) and the ERC-2771 wiring. The deployable `DocumentEngine` API and behavior are unchanged. +- **Version module implementing ERC-8303**: the version is now exposed through a dedicated `VersionModule` (`src/modules/VersionModule.sol`) implementing the `IERC8303` interface (`src/interfaces/IERC8303.sol`). It adds a standard `version()` view function (in addition to the existing public `VERSION` constant) and advertises ERC-8303 via ERC-165 (`supportsInterface(0x54fd4d50) == true`). `DocumentEngine` combines the module's `supportsInterface` with the access-control base. +- **Second deployment `DocumentEngineOwnable`** (`src/DocumentEngineOwnable.sol`): an alternative deployment that uses OpenZeppelin `Ownable2Step` (single owner, two-step transfer) instead of role-based access control, reusing the same `DocumentEngineBase` logic and the shared `TokenBindingModule`. Both document management and token binding are restricted to the `owner`. + +### Changed (access control) + +- `DocumentEngine` now inherits **`AccessControlEnumerable`** instead of `AccessControl`, adding on-chain enumeration of role members (`getRoleMember`, `getRoleMemberCount`) and advertising `IAccessControlEnumerable` via ERC-165. Default authorization behavior is unchanged. +- Moved the `DOCUMENT_MANAGER_ROLE` constant out of the shared `DocumentEngineInvariant` and into the role-based `DocumentEngine`, so `DocumentEngineInvariant` (and the `DocumentEngineOwnable` deployment) no longer carry access-control-specific constants. The invariant now holds only the shared errors. + +### Fixed (ERC-1643 conformance) + +Aligned the implementation with the updated [ERC-1643](./doc/ERCSpecification/erc-1643.md) (which now folds in the multi-token extension and the emission-responsibility rules): + +- **Emission responsibility.** As a shared, multi-token manager the engine now emits **only** the address-carrying extension events and **no longer** emits the base `DocumentUpdated` / `DocumentRemoved` events (the spec's `MUST NOT` for a shared manager — those events carry no `subject` and belong on the token contract). +- **Extension events/interface.** Renamed the multi-token events to the standard `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` (parameter `subject`), and introduced the `IERC1643MultiDocument` interface (`src/interfaces/IERC1643MultiDocument.sol`) that the base now implements — the address-scoped `getDocument` / `getAllDocuments` / `setDocument` / `removeDocument`. +- **Input validation.** `setDocument` now reverts `ERC1643InvalidName()` when `name == bytes32(0)` and `MultiDocumentInvalidSubject()` when `subject == address(0)` (the multi-subject draft's null-namespace guard); `removeDocument` now reverts `ERC1643MissingDocument()` for a non-existent document (previously it silently emitted a spurious removal event). See [`erc-draft_multi_document_management.md`](./doc/ERCSpecification/erc-draft_multi_document_management.md) for the corresponding multi-subject draft. +- **ERC-165 discovery.** `supportsInterface` now returns `true` for `type(IERC1643).interfaceId` and `type(IERC1643MultiDocument).interfaceId` (both deployments). + +### Added (token binding) + +- **Shared `ITokenBinding` interface + `TokenBindingModule`.** `bindToken(token)` / `unbindToken(token)` / `isTokenBound(token)` + `TokenBindingSet` event (`src/interfaces/ITokenBinding.sol`), implemented once for both deployments by `src/modules/TokenBindingModule.sol` — a single **allowlist**, not a role. Both deployments now share the exact same binding mechanism (same functions, event, and `NotBoundToken` revert on an unbound write) and advertise `type(ITokenBinding).interfaceId` via ERC-165. The role deployment **no longer uses `TOKEN_CONTRACT_ROLE`** (removed) — binding is authorized by the document-management hook (`DOCUMENT_MANAGER_ROLE`, or the `owner` in `DocumentEngineOwnable`). + +### Notes / bottlenecks + +- **Subject-side emission is CMTAT `v3.3.0-rc2` or later.** rc2 made `DocumentEngineModule` re-emit + the standard `DocumentUpdated` / `DocumentRemoved` on the **token's own address** after forwarding + to the engine, and revert with `CMTAT_DocumentEngineModule_NoDocumentEngine` when no engine is set. + Combined with this engine emitting only the address-carrying `*ForSubject` events, the + subject-initiated call topology is fully conformant with the multi-subject draft's *Emission + 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 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 diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..95b69ad --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,176 @@ +# DocumentEngine — Agent Guide + +> **Note — keep in sync:** `AGENTS.md` and `CLAUDE.md` must always be **identical**. +> Any edit to one must be applied verbatim to the other. + +> **Note — commit messages:** After each group of modifications or each feature +> added, always provide a **one-line GitHub commit message** (Conventional-Commits +> style, e.g. `feat: add token binding`, `fix: correct event args`, `docs: update README`). + +## What this project is + +`DocumentEngine` is a standalone smart contract that manages documents on-chain +through **ERC-1643** on behalf of **several** other smart contracts (e.g. CMTAT +tokens). Using an external engine keeps each token small and lets one operator +manage documents for a whole fleet of tokens. + +A document is `{ string uri, bytes32 documentHash, uint256 lastModified }`, +addressed by a `bytes32` name. + +## Key concepts + +- **Two management paths (both active at once):** + - **Admin path** — `DOCUMENT_MANAGER_ROLE`. Address-scoped overloads + (`setDocument(address,...)`, `removeDocument(address,...)`, batch variants) + manage documents for any contract. + - **Bound-token path** — the standard single-arg `IERC1643` functions + (`setDocument(name,uri,hash)`, `removeDocument(name)`) let a bound token manage + its **own** namespace (`_msgSender()`). Bind via the shared `ITokenBinding` + surface: `bindToken(token)` / `unbindToken(token)` / `isTokenBound(token)`, + implemented **once** for both deployments by `TokenBindingModule` — a single + allowlist, NOT a role (there is no `TOKEN_CONTRACT_ROLE`). Binding is authorized + by each deployment's document-management hook (DOCUMENT_MANAGER_ROLE / owner). + NOTE: RuleEngine's `ERC3643ComplianceExtendedModule` is intentionally **not** + reused for binding — it is an `IERC3643Compliance`, which would drag in + transfer-compliance callbacks (`canTransfer`/`transferred`/`created`/`destroyed`) + irrelevant to a document engine. See the README rationale section. +- **Events (ERC-1643 emission responsibility):** this engine is a *shared, + multi-token* manager, so it emits **only** the address-carrying extension events + `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` (param `subject`) and + **not** the base `DocumentUpdated` / `DocumentRemoved` (those carry no address and + are the token contract's responsibility). Extension declared in + `src/interfaces/IERC1643MultiDocument.sol`; rationale in `ERC-1643-proposition.md`. +- **Errors live on interfaces, not on `DocumentEngineInvariant`.** Each specification + error is declared by the interface defining its condition — `ERC1643InvalidName` / + `ERC1643MissingDocument` on `IERC1643`, `MultiDocumentInvalidSubject` on + `IERC1643MultiDocument`, `TokenBindingInvalidToken` on `ITokenBinding` — so an ABI + generated from an interface carries its errors and each is obtained exactly once + (the multi-subject draft's "MUST NOT declare them twice", also a compile error). + `DocumentEngineInvariant` keeps only errors no interface defines. +- **Token binding is idempotent and rejects `address(0)`:** `bindToken` / `unbindToken` + write and emit `TokenBindingSet` only on an actual change, so every event is a real + transition; a repeat call succeeds silently. `address(0)` reverts + `TokenBindingInvalidToken()`. +- **ERC-1643 conformance:** `setDocument` reverts `ERC1643InvalidName()` on + `name == 0`; `removeDocument` reverts `ERC1643MissingDocument()` on a missing doc; + `supportsInterface` advertises `IERC1643` + `IERC1643MultiDocument` (both deployments). + Both errors are declared by `IERC1643` itself since CMTAT `v3.3.0-rc2` — do **not** + re-declare them in `DocumentEngineInvariant` (duplicate declaration = compile error, + and the multi-subject draft forbids it). +- **`getDocument` returns flat values**, `(string uri, bytes32 documentHash, + uint256 lastModified)`, never the `Document` struct — the struct is storage-only. + A struct return prepends an offset word to the returndata while leaving the selector + and `type(IERC1643).interfaceId` unchanged, so the mismatch is invisible to ERC-165 + and a spec-conformant consumer silently mis-decodes. Pinned by + `testGetDocumentReturnsFlatErc1643Abi`. +- **ERC-2771:** meta-transaction (gasless) support; `_msgSender()` is used everywhere. +- **Access control:** `DEFAULT_ADMIN_ROLE` implicitly has every role (see the + `hasRole` override). +- **Flexible access control (CMTAT / RuleEngine pattern):** restricted functions + use the `onlyDocumentManager` / `onlyBoundToken` modifiers, which delegate to + overridable `internal virtual` hooks `_authorizeDocumentManagement()` (per + deployment: `DOCUMENT_MANAGER_ROLE` / owner) and + `_authorizeBoundTokenDocumentManagement()` (implemented once by + `TokenBindingModule` → allowlist check). Keep the management implementation + separate from the authorization logic — change *who* is authorized via a hook, not by + editing the management functions. +- **CMTAT integration:** since CMTAT v3, a token uses the engine via CMTAT's + `DocumentEngineModule` and `setDocumentEngine(engine)` (reads/writes are forwarded + keyed by the token address). Standard CMTAT standalone tokens store documents + on-chain instead and do **not** use this engine. + +## File tree + +``` +src/ +├── DocumentEngineBase.sol # Abstract base: ERC-1643 document logic + storage, +│ # both management paths, batch functions, modifiers, +│ # and the ABSTRACT _authorize* hooks (no access control) +├── DocumentEngine.sol # Deployment #1: role-based access control +│ # (AccessControlEnumerable, DOCUMENT_MANAGER_ROLE, +│ # _authorizeDocumentManagement, hasRole), ERC-2771, +│ # supportsInterface, constructor +├── DocumentEngineOwnable.sol # Deployment #2: Ownable2Step (single owner) instead of +│ # roles; document mgmt + binding are owner-only +├── DocumentEngineInvariant.sol # Non-specification errors ONLY (InvalidInputLength, +│ # AdminWithAddressZeroNotAllowed). Every spec error is +│ # declared by its own interface — see the note below. +│ # NO access-control specifics +├── interfaces/ +│ ├── IERC8303.sol # ERC-8303 "Contract Version" interface (id 0x54fd4d50) +│ ├── IERC1643MultiDocument.sol # Multi-token ERC-1643 extension (address-scoped fns + +│ │ # DocumentUpdatedForSubject / DocumentRemovedForSubject + +│ │ # MultiDocumentInvalidSubject) +│ └── ITokenBinding.sol # Shared binding surface: bindToken / unbindToken / +│ # isTokenBound + TokenBindingSet + TokenBindingInvalidToken +└── modules/ + ├── VersionModule.sol # Version module: implements ERC-8303 version() + ERC-165, + │ # holds the VERSION constant (currently "0.4.0") + └── TokenBindingModule.sol # Shared token-binding allowlist (ITokenBinding) + NotBoundToken; + # wires the bound-token hook; used by both deployments + +script/ +├── DeployDocumentEngine.s.sol # Deploy role-based DocumentEngine (env: DOCUMENT_ENGINE_ADMIN, +│ # DOCUMENT_ENGINE_FORWARDER); run()=env, deploy(admin,fwd)=testable +└── DeployDocumentEngineOwnable.s.sol # Deploy Ownable variant (env: DOCUMENT_ENGINE_OWNER, _FORWARDER) + +test/ +├── DocumentEngine.t.sol # Foundry tests: deploy, access control, admin + bound-token +│ # paths, batch ops (incl. name==0 / missing-doc guards), +│ # ERC-8303 + interface discovery, event emission (asserts the +│ # base events are NOT emitted), msg.sender-scoped reads, +│ # enumeration, fuzz round-trip/isolation, CMTAT integration +│ # (CMTATDocumentEngineMock), flexible-auth override (OpenDocumentEngine) +├── DocumentEngineOwnable.t.sol # Tests for the Ownable2Step deployment (owner path, +│ # token binding, two-step ownership, ERC-8303) +└── Deploy.t.sol # Tests for the deployment scripts (deploy() state + run() env) +``` + +**Contract split (CMTAT module/deployment pattern):** `DocumentEngineBase` holds +the document logic and abstract `_authorize*` hooks; each deployment contract +supplies the concrete access control. There are two deployments — +`DocumentEngine` (role-based, `AccessControlEnumerable`) and +`DocumentEngineOwnable` (`Ownable2Step`). Add new management logic in the base; +change *who* is authorized in a deployment (implement the `_authorize*` hooks). + +Other important files: + +- `foundry.toml` — solc `0.8.34`, `evm_version = prague` (required by CMTAT v3). +- `remappings.txt` — `CMTAT/`, `RuleEngine/`, `OZ/`, `@openzeppelin/contracts-upgradeable/`. +- `CHANGELOG.md` — semver history; update on every release (current: `v0.4.0`). +- `ERC-1643-proposition.md` — proposed optional multi-token events / extension. +- `README.md` — full documentation and Surya schema. +- `doc/` — Surya diagrams/reports (`doc/script/`), coverage, and + `doc/audits/` — the security overview (`AUDIT_OVERVIEW.md`) plus versioned + static-analysis output under `doc/audits/tools/vX.Y.Z//`, each with a + `*-report.md` (summary table prepended) and a `*-report-feedback.md` triaging + every finding. Aderyn was run for `v0.4.0`; Slither has never been run here. +- `IMPROVEMENT.md` — the open items: deviations from the two ERC specifications, + with severity, effort and a recommendation for each. Update it when an item is + fixed (move the record to `CHANGELOG.md` and `doc/audits/AUDIT_OVERVIEW.md`) + or when review surfaces a new one. +- `lib/` — submodules: `CMTAT`, `RuleEngine`, `openzeppelin-contracts(-upgradeable)`, `forge-std`. + +## Dependencies (tested versions) + +- CMTAT `v3.3.0-rc2`, RuleEngine `v3.0.0-rc4` (binding-pattern reference only; compliance module not reused) +- OpenZeppelin Contracts / Contracts Upgradeable `v5.6.1` +- Solidity `0.8.34`, Foundry + +## Common commands + +```bash +forge build # compile +forge test # run the test suite +forge fmt # format +forge test --gas-report +``` + +## Conventions + +- The `VERSION` constant (in `src/modules/VersionModule.sol`, exposed via + ERC-8303 `version()`) must match the latest `CHANGELOG.md` entry on release. +- Bump `MAJOR` on incompatible proxy-storage / external-library or API changes, + `MINOR` for backward-compatible features, `PATCH` for backward-compatible fixes. +- A bound token can only ever affect its **own** document namespace — never break + that isolation. diff --git a/IMPROVEMENT.md b/IMPROVEMENT.md new file mode 100644 index 0000000..3eca0a4 --- /dev/null +++ b/IMPROVEMENT.md @@ -0,0 +1,306 @@ +# IMPROVEMENT — open items + +Known deviations, gaps and improvement opportunities in `DocumentEngine`, carried forward as of +**`v0.4.0`** (CMTAT `v3.3.0-rc2`). + +They come from a clause-by-clause conformance analysis of the implementation against the two +specifications this engine implements: + +| Spec | File | Role | +| --- | --- | --- | +| ERC-1643 — Document Management for Security Tokens | [`doc/ERCSpecification/erc-1643.md`](./doc/ERCSpecification/erc-1643.md) | Per-contract interface | +| Multi-Subject Document Management (unnumbered draft) | [`doc/ERCSpecification/erc-draft_multi_document_management.md`](./doc/ERCSpecification/erc-draft_multi_document_management.md) | Address-scoped companion interface | + +**None of these is an exploitable vulnerability.** Items already fixed are not repeated here — they +are recorded in [`CHANGELOG.md`](./CHANGELOG.md) and in +[`doc/audits/AUDIT_OVERVIEW.md`](./doc/audits/AUDIT_OVERVIEW.md). Static-analysis findings are +tracked separately under [`doc/audits/tools/`](./doc/audits/tools). + +> This project has not been audited. These items are the output of specification review and +> automated tooling, not of a formal security audit. + +## Summary + +| # | Item | Severity | Effort | Kind | +| --- | --- | --- | --- | --- | +| [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 | + +¹ 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 granularity is fixed at compile time; the hook cannot express per-`subject` rules + +**Severity:** Low — **Medium** for a deployment shared across unrelated issuers · **Effort:** Medium +· **Kind:** extensibility + deployment guidance + +**Where:** `src/DocumentEngineBase.sol:56`, `:111-186`; `src/DocumentEngine.sol:46-48`; +`src/DocumentEngineOwnable.sol:39-41` + +### This is not a conformance failure + +The draft's requirement is: + +> 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 + +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: + +> 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 + +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. + +### What is actually open + +The draft's Security Consideration is about **unrelated** subjects: + +> If writes are not authorized per `subject`, any caller permitted to write for one subject can +> modify another subject's legal or operational references. + +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() internal view virtual; // no subject parameter +``` + +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 + +**Severity:** Medium · **Effort:** Small–Medium · **Kind:** deviation from a specification `SHOULD` + +**Where:** `src/DocumentEngineBase.sol:71-84`; demonstrated at `test/DocumentEngine.t.sol:197` + +> A deployment in which an operator calls `setDocument(address subject, ...)` … directly, with no +> execution point in the subject, does **not** satisfy ERC-1643's emission requirement for that +> subject. +> — draft §Call Topology + +> a management contract SHOULD restrict its address-scoped writes to callers for which one of the two +> topologies above holds. + +Neither deployment applies such a restriction. The repository's own CMTAT integration test writes +through exactly the prohibited path: + +```solidity +// test/DocumentEngine.t.sol:196-197 +vm.prank(admin); +documentEngine.setDocument(address(cmtat), documentName, documentURI, documentHash); +``` + +The engine emits `DocumentUpdatedForSubject`; the CMTAT token emits nothing; anyone subscribed to the +token's address concludes its documents are unchanged. The failure is silent in both directions, as +the draft's Security Considerations describe. + +The **subject-initiated** topology, by contrast, became fully conformant with CMTAT `v3.3.0-rc2`, +which lists this under *Fixed* as "Delegating document token emits ERC-1643 events on its own address +(dual emission)". `DocumentEngineModule` forwards to the engine and then re-emits `DocumentUpdated` / +`DocumentRemoved` on the **token's** own address (`DocumentEngineModule.sol:91-92`, `:102-104`), +reading the metadata before removal so `DocumentRemoved` carries the removed values as the spec +requires; the engine emits the address-carrying events on its own address. Both mutators also revert +with `CMTAT_DocumentEngineModule_NoDocumentEngine` when no engine is set, closing a path where a +write would previously have been lost. + +So the gap is narrow but sharp: whether a CMTAT subject's documents are observable on its own address +depends entirely on **which door the operator uses**. Through the token (`cmtat.setDocument(...)`) it +is; straight to the engine (`engine.setDocument(address(cmtat), ...)`) it is not. Nothing in either +contract signals the difference. + +**Recommendation.** Pick one and state it: + +- *Documentation-only* (cheapest): a prominent README/NatSpec note that the address-scoped writes are + for subjects that either do not implement ERC-1643 or accept the loss of per-contract + observability; the bound-token path is the conformant route for ERC-1643 subjects. +- *Enforced*: gate the address-scoped writes on `isTokenBound(subject) == false`, forcing ERC-1643 + subjects through their own contract. +- *Callback*: add the draft's "manager-initiated with callback" topology — an optional permissioned + hook on the subject invoked after the write, so the subject emits. + +## 3 — The engine advertises `IERC1643` but is not a usable ERC-1643 endpoint + +**Severity:** Low · **Effort:** Trivial · **Kind:** documentation + +**Where:** `src/DocumentEngine.sol:88`; `src/DocumentEngineOwnable.sol:47-49` + +This is permitted — the draft's rule is that a contract may advertise `type(IERC1643).interfaceId` +*only if* it implements the base functions, and the engine does. But those functions are +`_msgSender()`-scoped (`DocumentEngineBase.sol:181-216`), so a consumer that ERC-165-detects +ERC-1643 on the **engine** address and then calls `getDocument(name)` receives empty values, no +revert, and never sees a base event. This is the deployment error the draft's Backwards Compatibility +section names as the one way to break a legacy consumer, and ERC-165 offers no way to detect it. + +**Recommendation.** Documentation, not code: state prominently in the README and in the NatSpec of +the no-argument functions that consumers must be pointed at the **subject**, never at the engine, and +that the base functions exist solely for bound subjects calling on their own behalf. + +## 4 — Enumeration cost and removal complexity + +**Severity:** Low · **Effort:** Medium · **Kind:** gas / scalability + +**Where:** `src/DocumentEngineBase.sol:236-245`, `:206-216`, `:150-186` + +- `_removeDocumentName` is a linear scan over the subject's name array. The draft's Reference + Implementation section expects "index tracking to support O(1) removals". `batchRemoveDocuments` + compounds this to O(n·m) and can plausibly exceed the block gas limit for a subject with a large + document set. +- `getAllDocuments` returns the entire array with no paginated alternative. ERC-1643's Security + Considerations explicitly call this out: "Implementations expecting large sets should consider + exposing an additional paginated accessor alongside this interface." + +Neither is a conformance failure. Independently corroborated by Aderyn — L-5 at +`DocumentEngineBase.sol:238`, reached from its "costly operation in a loop" heuristic +([triage](./doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md)). + +**Recommendation.** Add `mapping(address => mapping(bytes32 => uint256)) private _nameIndex` for +O(1) swap-and-pop, plus `getDocumentsPaginated(address subject, uint256 offset, uint256 limit)` and +`getDocumentCount(address subject)`. + +## 5 — The ERC-2771 forwarder is a universal write authority + +**Severity:** Info · **Effort:** Trivial · **Kind:** documentation + +**Where:** `src/DocumentEngine.sol:99-101`; `src/modules/TokenBindingModule.sol:84-88` + +`_msgSender()` drives both `_checkTokenBound()` and the role check. The trusted forwarder can +therefore present itself as any bound subject — writing into that subject's namespace — and as any +role holder. This is the ordinary ERC-2771 trust assumption, but it deserves stating explicitly given +the draft's framing: + +> Subjects should treat the choice of management contract as a permissioning decision, not merely a +> storage one. + +A subject binding to this engine is also trusting the engine's forwarder. `forwarderIrrevocable` is +immutable, which is the right call for predictability, but it also means a compromised forwarder +cannot be revoked — the only remedy is unbinding every subject and migrating. + +**Recommendation.** Document the forwarder as part of every bound subject's trust boundary, in the +README section on ERC-2771 and in the constructor NatSpec. + +## 6 — `_removeDocument` emits before the state change + +**Severity:** Info · **Effort:** Trivial · **Kind:** cosmetic + +**Where:** `src/DocumentEngineBase.sol:258` (emit) before `:260-261` (delete) + +The specs mandate "after state changes" for `setDocument` only, and the implementation complies there +(`:286`). For removal the metadata must be read before deletion, so the current ordering is +convenient; there are no external calls anywhere in the write path, so there is no reentrancy +exposure. + +**Recommendation.** Cosmetic only — the metadata is already cached in the `doc` local, so emitting +after the delete would align both paths at no cost. + +## 7 — Upstream: `IMultiDocumentSubject` manager discovery + +**Severity:** Info · **Kind:** upstream (CMTAT), not this repository + +Not implemented here, and correctly so: the draft is explicit that this interface is "**implemented +by the subject, not by the management contract**", and it is optional for both. No action is required +of the engine. + +There is, however, a near-miss upstream worth aligning. CMTAT's `DocumentEngineModule` already +exposes the same concept under a different shape: + +| draft `IMultiDocumentSubject` | CMTAT `DocumentEngineModule` | +| --- | --- | +| `documentManager() returns (address)` | `documentEngine() returns (IERC1643)` | +| `DocumentManagerUpdated(address previous, address new)` | `DocumentEngine(IERC1643 engine)` — no previous address | +| MUST return `address(0)` when not delegated | ✅ default zero | +| MUST NOT revert | ✅ | + +**Recommendation.** Adding the previous address to the CMTAT event, or an alias getter, would make +CMTAT tokens discoverable by any consumer implementing the draft. That is a change for CMTAT, not for +this repository. + +--- + +## Not open items + +Recorded so they are not re-raised. Verified conformant, several by explicit test: + +- **Subject isolation** — `mapping(address => mapping(bytes32 => Document))` with a per-subject name + array, covered by a fuzz test. A bound subject writes to `_msgSender()` and structurally cannot + reach another namespace. +- **Emission responsibility** — only `DocumentUpdatedForSubject` / `DocumentRemovedForSubject`, never + the base events, as the draft requires of a multi-subject manager. The suite asserts the *absence* + of the base events. +- **`lastModified == 0` as the sole absent-entry sentinel** — respected as both the read convention + and the internal existence check, keeping `uri` / `documentHash` free to be legitimately empty. +- **Null-subject guard on the write path only** — matching the draft's note that guarding + `setDocument` suffices, since removal then fails with `ERC1643MissingDocument()` anyway. +- **`IERC1643MultiDocument` neither inherits nor imports `IERC1643`** — the independence the draft + requires is a property of the file, not a convention. +- **`getDocument` returns the flat ERC-1643 ABI** — pinned by a test that inspects returndata, since + the interface id is identical for both shapes and ERC-165 cannot catch a regression. Both interface + ids are asserted as literals (`0xecfecec8`, `0xa2b1179b`). +- **Unstable ordering after removal** — explicitly permitted by ERC-1643; swap-and-pop is fine. diff --git a/README.md b/README.md index 380053e..7e71694 100644 --- a/README.md +++ b/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. @@ -13,37 +19,218 @@ The ERC-1643 defines a document with three attributes: - A generic URI (represented as a `string`) that could point to a website or other document portal. - The hash of the document contents associated with it on-chain. -A smart contract needs only to implement two functions from this standard, available in the interface [IERC1643](./contracts/interfaces/engined/draft-IERC1643.sol) to get the documents from the documentEngine. +A smart contract needs only to read documents from this standard through the interface [IERC1643](./lib/CMTAT/contracts/interfaces/tokenization/draft-IERC1643.sol) to get the documents from the documentEngine: ```solidity interface IERC1643 { -function getDocument(bytes32 _name) external view returns (string memory , bytes32, uint256); -function getAllDocuments() external view returns (bytes32[] memory); + error ERC1643InvalidName(); + error ERC1643MissingDocument(); + + function getDocument(bytes32 name) + external + view + returns (string memory uri, bytes32 documentHash, uint256 lastModified); + function getAllDocuments() external view returns (bytes32[] memory documentNames_); + function setDocument(bytes32 name, string calldata uri, bytes32 documentHash) external; + function removeDocument(bytes32 name) external; } ``` -Use an external contract for your smart contract provides two advantages: +> **Note — `getDocument` returns flat values.** CMTAT `v3.3.0-rc1` briefly returned a `Document` +> struct here; `v3.3.0-rc2` restored the three flat return values mandated by the ERC-1643 ABI, and +> this engine follows. The distinction matters because return types are not part of a function +> signature: both shapes have the same selector and the same `type(IERC1643).interfaceId`, so a +> struct return is undetectable through ERC-165 and a consumer built from the specification ABI +> would silently decode it as garbage. The `Document` struct is kept internally for storage only. +> `testGetDocumentReturnsFlatErc1643Abi` pins the wire format. + +Using an external contract for your smart contract provides two advantages: - Reduce code size of your smart contract - Allow to manage documents for several different smart contracts -Warning: +### Two ways to manage documents + +The engine supports **two management paths** at the same time: + +**1. Admin path (`DOCUMENT_MANAGER_ROLE`).** Since the engine manages documents +for several different smart contracts, the admin functions take one supplementary +`address smartContract` argument compared to the ERC-1643: -Since this engine allows to set documents for several different smart contracts, the functions to set documents take one supplementary arguments than defined in the ERC-1643. +```solidity +// DocumentEngine (admin overloads) +function setDocument(address smartContract, bytes32 name_, string memory uri_, bytes32 documentHash_) external; +function removeDocument(address smartContract, bytes32 name_) external; +``` -IERC1643 +**2. Bound-token path.** This implements the standard, single-argument ERC-1643 +functions. A token is *bound* to the engine through the shared **`ITokenBinding`** +surface — identical across both deployments, so integrators bind/query a token the +same way regardless of the access-control model: ```solidity -function setDocument(bytes32 _name, string _uri, bytes32 _documentHash) external; +documentEngine.bindToken(address(token)); // also: unbindToken(token), isTokenBound(token) ``` -DocumentEngine +Both deployments share the exact same binding mechanism — a single allowlist in +`TokenBindingModule` (`src/modules/TokenBindingModule.sol`), **not** a role. They +expose the same `bindToken` / `unbindToken` / `isTokenBound` functions, emit the +same `TokenBindingSet` event, and revert with the same `NotBoundToken` error when a +non-bound caller attempts a write. The only difference is *who* may bind: whoever +may manage documents in that deployment (the `DOCUMENT_MANAGER_ROLE` holder, or the +`owner`), since binding is authorized by the same document-management hook. + +Once bound, the token manages its **own** documents (`msg.sender` is the token); +it can never affect another contract's documents: + +```solidity +// DocumentEngine (standard ERC-1643, scoped to msg.sender) +function setDocument(bytes32 name_, string calldata uri_, bytes32 documentHash_) external; +function removeDocument(bytes32 name_) external; +``` + +> This mirrors the RuleEngine *binding* pattern without reusing its +> `ERC3643ComplianceExtendedModule` — see +> [Why not reuse RuleEngine's ERC-3643 compliance module?](#why-not-reuse-ruleengines-erc-3643-compliance-module) below. + +### Flexible access control + +Following the CMTAT / [RuleEngine](https://github.com/CMTA/RuleEngine) pattern, +the restricted functions do not hardcode a check. They carry a **modifier** +(`onlyDocumentManager` / `onlyBoundToken`) that delegates to an **overridable +`internal virtual` authorization hook**: + +- the **admin path** delegates to `_authorizeDocumentManagement()`, the one hook + each deployment implements (`_checkRole(DOCUMENT_MANAGER_ROLE)` for + `DocumentEngine`, `_checkOwner()` for `DocumentEngineOwnable`); +- the **bound-token path** delegates to `_authorizeBoundTokenDocumentManagement()`, + which `TokenBindingModule` implements once for both deployments (it checks the + shared binding allowlist). ```solidity -function setDocument(address smartContract,bytes32 name_,string memory uri_, bytes32 documentHash_) +// implemented per deployment (the only access-control hook they supply) +function _authorizeDocumentManagement() internal view virtual { + _checkRole(DOCUMENT_MANAGER_ROLE); // or _checkOwner() +} + +// implemented once in TokenBindingModule for both deployments +function _authorizeBoundTokenDocumentManagement() internal view virtual override { + _checkTokenBound(); // reverts NotBoundToken if msg.sender is not bound +} ``` +This separates the document-management implementation from the authorization +logic: a subclass changes *who* is authorized by overriding the hook, never by +touching the management functions. + +### Why not reuse RuleEngine's ERC-3643 compliance module? + +CMTA's [RuleEngine](https://github.com/CMTA/RuleEngine) (v3) ships an +`ERC3643ComplianceExtendedModule` that offers a ready-made token-binding registry +(`bindToken` / `unbindToken` / `isTokenBound` / `getTokenBounds`). It is tempting +to reuse it for the bound-token path, but we deliberately do **not**, because that +module is an **`IERC3643Compliance`** — a *transfer-compliance* contract. + +Inheriting it would force the DocumentEngine to also implement the ERC-3643 +transfer-compliance callbacks that come with that interface: + +```solidity +function canTransfer(address, address, uint256) external view returns (bool); +function transferred(address, address, uint256) external; +function created(address, uint256) external; +function destroyed(address, uint256) external; +``` +A document engine has **nothing to do with token transfers**, so these would have +to be stubbed as no-ops (`canTransfer` always returning `true`). That is +misleading: the contract would advertise a transfer-compliance surface it does +not honor, enlarging the ABI and inviting integrators to wire it where a real +compliance contract is expected. + +The binding concept we actually need is tiny — "is this caller a token allowed to +manage its own documents?" — so we implement just that: a `TOKEN_CONTRACT_ROLE` +in the role-based `DocumentEngine` (exactly the RuleEngine *binding* mechanism, +which is role-based, not the compliance module) and an owner-managed allowlist in +`DocumentEngineOwnable`. This keeps the engine's surface honest and minimal while +still mirroring the RuleEngine binding pattern. The RuleEngine submodule is kept +as a reference for that pattern. + +### Events + +This engine is a **shared, multi-token** document manager, so — per the ERC-1643 +["Emission Responsibility"](./doc/ERCSpecification/erc-1643.md) rules — it emits +**only** the address-carrying extension events +`DocumentUpdatedForSubject(address indexed subject, …)` / +`DocumentRemovedForSubject(…)`, and **not** the base `DocumentUpdated` / +`DocumentRemoved` events. The base events carry no address and so cannot identify +which token contract a change belongs to; they are the responsibility of the +token contract that exposes ERC-1643 to consumers (it re-emits them when +delegating). See +[ERC-1643-proposition.md](./doc/ERCSpecification/ERC-1643-proposition.md) and the +`IERC1643MultiDocument` extension. + +### Integration with CMTAT + +Since CMTAT v3, the shipped standalone tokens store documents on-chain +(`DocumentERC1643Module`) and do not consume an external engine through their +constructor. To use this engine, a CMTAT token relies on the +`DocumentEngineModule` and is wired at runtime with `setDocumentEngine(engine)`; +reads/writes are then forwarded to the engine keyed by the token address. + + + +## Architecture + +The engine is split into two contracts (CMTAT module/deployment pattern): + +- **`DocumentEngineBase`** (abstract) — holds the document storage and all the + ERC-1643 document-management functions, plus the `onlyDocumentManager` / + `onlyBoundToken` modifiers and the **abstract** `_authorize*` hooks. It is + agnostic to the access-control implementation. +- **`DocumentEngine`** (deployment) — the concrete, deployable contract. It + defines the **access control** (`AccessControlEnumerable`, the `_authorize*` + hook implementations and the `hasRole` override) and wires the ERC-2771 + (gasless) support. `AccessControlEnumerable` additionally allows enumerating + the members of each role on-chain. +- **`DocumentEngineOwnable`** (alternative deployment) — same base logic, but + access control is a single **owner** via `Ownable2Step` (two-step ownership + transfer) instead of roles. Both document management and token binding are + `owner`-only. +- **`TokenBindingModule`** (`src/modules/TokenBindingModule.sol`) — the shared + token-binding registry (an allowlist) implementing `ITokenBinding` + (`bindToken` / `unbindToken` / `isTokenBound` + `TokenBindingSet`). Both + deployments inherit it, so binding is identical (same functions, event, and + `NotBoundToken` revert) and ERC-165-discoverable regardless of the + access-control model; binding is authorized by each deployment's + document-management hook. + +`DocumentEngineInvariant` provides the errors shared by every deployment. +Access-control specifics are **not** defined there: the `DOCUMENT_MANAGER_ROLE` +constant lives in the role-based `DocumentEngine`, and the owner logic in +`DocumentEngineOwnable`. + +`VersionModule` (`src/modules/VersionModule.sol`) isolates the version concern +and implements [ERC-8303](https://ethereum-magicians.org/t/erc-8303-contract-version/28795) +(see below). + +## Version (ERC-8303) + +The contract version is exposed through the `VersionModule`, which implements +the [ERC-8303](https://ethereum-magicians.org/t/erc-8303-contract-version/28795) +`IERC8303` interface: + +```solidity +interface IERC8303 { + function version() external view returns (string memory); +} +``` + +- `version()` returns the current version string (e.g. `"0.4.0"`), following + Semantic Versioning 2.0.0. +- The public `VERSION` constant is kept for backward compatibility and returns + the same value. +- ERC-165 discovery is supported: `supportsInterface(0x54fd4d50)` (the ERC-8303 + interface id) returns `true`. ## Schema @@ -69,14 +256,16 @@ function setDocument(address smartContract,bytes32 name_,string memory uri_, byt | :----------------: | :------------------: | :----------------------------------------------: | :------------: | :-----------: | | └ | **Function Name** | **Visibility** | **Mutability** | **Modifiers** | | | | | | | -| **DocumentEngine** | Implementation | IERC1643, DocumentEngineInvariant, AccessControl | | | +| **DocumentEngine** | Implementation | DocumentEngineBase, VersionModule, AccessControlEnumerable, ERC2771Context | | | | └ | | Public ❗️ | 🛑 | NO❗️ | -| └ | setDocument | Public ❗️ | 🛑 | onlyRole | -| └ | removeDocument | External ❗️ | 🛑 | onlyRole | -| └ | batchSetDocuments | External ❗️ | 🛑 | onlyRole | -| └ | batchSetDocuments | External ❗️ | 🛑 | onlyRole | -| └ | batchRemoveDocuments | External ❗️ | 🛑 | onlyRole | -| └ | batchRemoveDocuments | External ❗️ | 🛑 | onlyRole | +| └ | setDocument | Public ❗️ | 🛑 | onlyDocumentManager | +| └ | removeDocument | External ❗️ | 🛑 | onlyDocumentManager | +| └ | setDocument | External ❗️ | 🛑 | onlyBoundToken | +| └ | removeDocument | External ❗️ | 🛑 | onlyBoundToken | +| └ | batchSetDocuments | External ❗️ | 🛑 | onlyDocumentManager | +| └ | batchSetDocuments | External ❗️ | 🛑 | onlyDocumentManager | +| └ | batchRemoveDocuments | External ❗️ | 🛑 | onlyDocumentManager | +| └ | batchRemoveDocuments | External ❗️ | 🛑 | onlyDocumentManager | | └ | getDocument | External ❗️ | | NO❗️ | | └ | getDocument | External ❗️ | | NO❗️ | | └ | getAllDocuments | External ❗️ | | NO❗️ | @@ -112,26 +301,77 @@ Please see the OpenGSN [documentation](https://docs.opengsn.org/contracts/#recei The toolchain includes the following components, where the versions are the latest ones that we tested: - Foundry -- Solidity 0.8.26 (via solc-js) -- OpenZeppelin Contracts (submodule) [v5.0.2](https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.0.2) +- Solidity 0.8.34 (via solc-js), `evm_version = prague` +- OpenZeppelin Contracts (submodule) [v5.6.1](https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v5.6.1) - Tests - - [CMTAT v2.5.0-rc0](https://github.com/CMTA/CMTAT/releases/tag/v2.5.0-rc0) - - OpenZeppelin Contracts Upgradeable(submodule) [v5.0.2](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v5.0.2) + - [CMTAT v3.3.0-rc2](https://github.com/CMTA/CMTAT/releases/tag/v3.3.0-rc2) + - [RuleEngine v3.0.0-rc4](https://github.com/CMTA/RuleEngine/releases/tag/v3.0.0-rc4) (binding-pattern reference only — its compliance module is [not reused](#why-not-reuse-ruleengines-erc-3643-compliance-module)) + - OpenZeppelin Contracts Upgradeable (submodule) [v5.6.1](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v5.6.1) + +### Version compatibility + +Each release of this engine is built and tested against one CMTAT release. CMTAT's `IERC1643` is +not stable across its own release candidates, so pairing a version of this engine with a different +CMTAT than the one below is not supported. + +| DocumentEngine | CMTAT | Solidity / `evm_version` | OpenZeppelin | `getDocument` returns | +| -------------- | ----- | ------------------------ | ------------ | --------------------- | +| **v0.4.0** (current) | [v3.3.0-rc2](https://github.com/CMTA/CMTAT/releases/tag/v3.3.0-rc2) | `0.8.34` / `prague` | v5.6.1 | `(string, bytes32, uint256)` | +| v0.3.0 | [v2.5.0-rc0](https://github.com/CMTA/CMTAT/releases/tag/v2.5.0-rc0) | `0.8.26` / `cancun` | v5.0.2 | `(string, bytes32, uint256)` | +| v0.2.0 | [v2.5.0-rc0](https://github.com/CMTA/CMTAT/releases/tag/v2.5.0-rc0) | `0.8.26` / `cancun` | v5.0.2 | `(string, bytes32, uint256)` | +| v0.1.0 | [v2.5.0-rc0](https://github.com/CMTA/CMTAT/releases/tag/v2.5.0-rc0) | `0.8.26` / `cancun` | v5.0.2 | `(string, bytes32, uint256)` | + +Notes on the CMTAT v2 → v3 jump at `v0.4.0`: + +- **CMTAT `v3.3.0-rc1` is not supported.** It is the one release in which `IERC1643.getDocument` + returns a `Document` struct rather than the three flat values; `v3.3.0-rc2` reverted that. rc1 also + does not declare `ERC1643InvalidName` / `ERC1643MissingDocument` on the interface. Building this + engine against rc1 fails to compile. +- The `IERC1643` import path moved in CMTAT v3, from + `CMTAT/interfaces/engine/draft-IERC1643.sol` to `CMTAT/interfaces/tokenization/draft-IERC1643.sol`. +- Document names became `bytes32` in CMTAT v3 (they were `string` up to v2.5.0-rc0). +- Solidity `≥ 0.8.27` is required from `v0.4.0` on, because CMTAT v3 uses + `require(cond, CustomError())`. + +Exact submodule revisions are pinned in [`foundry.lock`](./foundry.lock). ## Tools -### Prettier +### Formatting (forge fmt) + +`forge fmt` is the canonical formatter for this project (configured under `[fmt]` +in `foundry.toml`): ```bash -npx prettier --write --plugin=prettier-plugin-solidity 'src/**/*.sol' +forge fmt # format src/, test/, script/ +forge fmt --check # verify formatting (CI) ``` -### Slither +### Static analysis + +Reports are versioned under [`doc/audits/tools/`](./doc/audits/tools), one directory per release, +each with the raw tool output (prefixed by a summary table) and a feedback file triaging every +finding against the source. The security overview is +[`doc/audits/AUDIT_OVERVIEW.md`](./doc/audits/AUDIT_OVERVIEW.md). + +| Release | Tool | Result | Report | Triage | +| ------- | ---- | ------ | ------ | ------ | +| v0.4.0 | Aderyn `0.6.5` | 0 High · 6 Low — **nothing to fix** | [report](./doc/audits/tools/v0.4.0/aderyn/aderyn-report.md) | [feedback](./doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md) | +| v0.4.0 | Slither | not run | — | — | ```bash -slither . --checklist --filter-paths "openzeppelin-contracts|test|CMTAT|forge-std" > slither-report.md +# Aderyn — mocks excluded (this project's mocks live in test/, which Aderyn does not scan) +aderyn -x mocks --output doc/audits/tools/v0.4.0/aderyn/aderyn-report.md + +# Slither +slither . --checklist --filter-paths "node_modules,test,forge-std,CMTAT,openzeppelin-contracts" \ + > doc/audits/tools/v0.4.0/slither/slither-report.md ``` +> **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 [`IMPROVEMENT.md`](./IMPROVEMENT.md). + ### Surya See [./doc/script](./doc/script) @@ -197,10 +437,40 @@ $ anvil ##### Deploy +Two deployment scripts are provided in [`script/`](./script), one per access-control +variant. Both read their configuration from environment variables: + +| Variable | Used by | Default | Meaning | +| --- | --- | --- | --- | +| `DOCUMENT_ENGINE_ADMIN` | `DeployDocumentEngine` | `msg.sender` | account granted `DEFAULT_ADMIN_ROLE` | +| `DOCUMENT_ENGINE_OWNER` | `DeployDocumentEngineOwnable` | `msg.sender` | initial owner | +| `DOCUMENT_ENGINE_FORWARDER` | both | `address(0)` | ERC-2771 trusted forwarder (`address(0)` disables gasless) | + +> **Warning** +> +> These environment variables, and passing a raw key with `--private-key`, are +> intended for **local testing only — do not use them in production**. A private +> key supplied on the command line or through an environment variable is exposed +> in your shell history and process environment. For production deployments, use a +> secure signing method (encrypted keystore, hardware wallet, ...) as described in +> the Foundry Key Management documentation (getfoundry.sh) for securely +> broadcasting transactions through a script. + ```shell -$ forge script script/Counter.s.sol:CounterScript --rpc-url --private-key +# Role-based DocumentEngine (AccessControlEnumerable) +$ DOCUMENT_ENGINE_ADMIN=0xYourAdmin \ + forge script script/DeployDocumentEngine.s.sol \ + --rpc-url --private-key --broadcast + +# Owner-based DocumentEngineOwnable (Ownable2Step) +$ DOCUMENT_ENGINE_OWNER=0xYourOwner \ + forge script script/DeployDocumentEngineOwnable.s.sol \ + --rpc-url --private-key --broadcast ``` +Drop `--broadcast` (and `--rpc-url`) for a local dry-run. The scripts are covered by +[`test/Deploy.t.sol`](./test/Deploy.t.sol). + ##### Cast ```shell diff --git a/doc/ERCSpecification/erc-1643.md b/doc/ERCSpecification/erc-1643.md new file mode 100644 index 0000000..83af8c6 --- /dev/null +++ b/doc/ERCSpecification/erc-1643.md @@ -0,0 +1,180 @@ +--- +eip: 1643 +title: Document Management for Security Tokens +description: Interface to attach, update, remove, and enumerate legal or operational documents for token contracts. +author: Adam Dossa (@adamdossa), Pablo Ruiz (@pabloruiz55), Fabian Vogelsteller (@frozeman), Stephane Gosselin (@thegostep), Ryan Sauge (@rya-sge) +discussions-to: https://ethereum-magicians.org/t/erc-1643-document-management-standard-erc-1400/27437 +status: Draft +type: Standards Track +category: ERC +created: 2018-09-09 +--- + +## Abstract + +This ERC defines a standard interface for associating documents with a token contract and for notifying off-chain systems when those documents change. Documents can represent legal agreements, offering materials, disclosures, or other issuer-provided references needed for security token operations. + +## Motivation + +Security tokens commonly represent assets with legal rights and obligations that depend on external documents. Wallets, exchanges, custodians, and compliance tools need a predictable way to discover those documents and track updates. + +Without a standard, each implementation exposes different storage and retrieval methods, increasing integration cost and operational risk. A common interface allows ecosystem participants to read and monitor document metadata consistently. + +Within security token frameworks, the document management component highlights that security tokens usually have associated documentation such as offering documents and legend details. The ability to set, remove, and retrieve these documents, with events emitted on those actions, allows investors and integrators to remain up to date. + +This ERC intentionally does not define an on-chain mechanism for investors to attest they have read or agreed to any document. + +Although originally designed for security tokens built on [ERC-20](./eip-20.md) as part of a broader real-world asset token suite, this interface is not restricted to that context. It can be adopted by any token standard, including [ERC-721](./eip-721.md) non-fungible tokens and [ERC-1155](./eip-1155.md) multi-tokens, as well as by decentralized applications, vaults, and any other on-chain product that requires structured document management. + +Historically, this proposal was authored as part of a broader security token standards suite that had not yet been merged in this repository when this proposal was added. + +## Specification + +The key words "MUST", "MUST NOT", "REQUIRED", "SHOULD", and "MAY" in this document are to be interpreted as described in RFC 2119 and RFC 8174. + +Implementations MUST support querying and subscribing to updates on any relevant documentation for the security. + +A document entry is identified by a name (`bytes32`) and stores: + +- A URI (`string`) pointing to the document location. +- A content hash (`bytes32`) for integrity checks. +- A last-modified timestamp (`uint256`) set when the entry is written. + +### Interface + +```solidity +/// @title IERC1643 Document Management +interface IERC1643 { + /// @notice Emitted when a document is created or updated. + /// @param name Identifier of the document. + /// @param uri Document location. + /// @param documentHash Hash of the document contents. + event DocumentUpdated(bytes32 indexed name, string uri, bytes32 documentHash); + + /// @notice Emitted when a document is removed. + /// @param name Identifier of the document. + /// @param uri Document location at the time of removal. + /// @param documentHash Hash of the document contents at the time of removal. + event DocumentRemoved(bytes32 indexed name, string uri, bytes32 documentHash); + + /// @notice Reverts when `setDocument` is called with `name == bytes32(0)`. + error ERC1643InvalidName(); + + /// @notice Reverts when `removeDocument` is called for a missing document. + error ERC1643MissingDocument(); + + /// @notice Creates or updates a document entry. + /// @dev MUST emit `DocumentUpdated` on success. + /// @param name Identifier of the document. + /// @param uri Document location. + /// @param documentHash Hash of the document contents. + function setDocument(bytes32 name, string calldata uri, bytes32 documentHash) external; + + /// @notice Removes an existing document entry. + /// @dev MUST emit `DocumentRemoved` on success. + /// @param name Identifier of the document to remove. + function removeDocument(bytes32 name) external; + + /// @notice Returns metadata for a document identified by `name`. + /// @param name Identifier of the document. + /// @return uri Document location. + /// @return documentHash Hash of the document contents. + /// @return lastModified Last update timestamp. + function getDocument(bytes32 name) + external + view + returns (string memory uri, bytes32 documentHash, uint256 lastModified); + + /// @notice Returns all document names currently tracked by the contract. + /// @return documentNames Names of all documents that are currently set. + function getAllDocuments() external view returns (bytes32[] memory documentNames); +} +``` + +### Interface Detection ([ERC-165](./eip-165.md)) + +Implementations SHOULD support ERC-165 interface detection. + +When ERC-165 is implemented, `supportsInterface` SHOULD return `true` for `type(IERC1643).interfaceId` and for the ERC-165 interface id. + +### Function Requirements + +- `getDocument`: + - MUST return the latest values for the provided document name. + - MUST return empty values when the entry does not exist (`""`, `bytes32(0)`, `0`). + - MUST NOT revert solely because the entry does not exist. + - Implementations SHOULD ensure that a stored entry always has a non-zero `lastModified`, so that `lastModified == 0` identifies an absent entry. Since `uri` and `documentHash` MAY both be empty for a stored document, they cannot distinguish an absent entry from one stored with empty metadata, and `lastModified` is the only value that can. Integrators should treat this as advisory rather than guaranteed when interacting with deployments predating this guidance. + +- `setDocument`: + - MUST create a new entry when `name` is not present. + - MUST overwrite the existing entry when `name` already exists. + - MUST update the stored last-modified timestamp. + - MUST emit `DocumentUpdated` after state changes. + - MUST revert if the update cannot be persisted. + - SHOULD revert when `name == bytes32(0)` to avoid ambiguous/default-key usage. + - `uri` and `documentHash` MAY be empty (`""` and `bytes32(0)`), depending on issuer workflow and document lifecycle stage. + - Implementations MAY decide to reject empty `uri` and/or empty `documentHash` based on policy requirements. + - Implementations SHOULD use the custom error defined in the interface (`ERC1643InvalidName()`) when rejecting `name == bytes32(0)`. + - Implementations MAY use different error names/signatures than those shown in this specification. + +- `removeDocument`: + - MUST remove the entry identified by `name`. + - MUST emit `DocumentRemoved` with the removed metadata. + - MUST revert if removal cannot be completed. + - Implementations SHOULD use the custom error defined in the interface (`ERC1643MissingDocument()`) when the named document does not exist. + - Implementations MAY use different error names/signatures than those shown in this specification. + +- `getAllDocuments`: + - MUST include every document name added by `setDocument` and not removed by `removeDocument`. + - MUST NOT include removed document names. + - The order of the returned names is unspecified. Removing a document MAY change the position of unrelated names, so consumers MUST NOT rely on a stable ordering, and MUST NOT treat a change in position as a change to a document. + +## Rationale + +- The standard uses `bytes32` names to keep keys compact and deterministic, while leaving naming conventions to implementations. A URI-based pointer is used instead of on-chain document storage to avoid high gas costs and to support existing off-chain document systems. +- Including a document hash enables clients to verify that fetched off-chain content matches issuer-published metadata. +- Emitting update and removal events supports indexing and near-real-time monitoring without repeated full-state polling. +- While a human-readable document title cannot always be represented directly in `bytes32` without hashing or canonicalization, `bytes32` remains practical for on-chain identifiers because fixed-size values can be compared directly (`a == b`). By contrast, `string` comparisons generally require hashing (for example, `keccak256(bytes(s))`), which increases contract code size and gas usage when repeated comparisons are needed on-chain, such as locating and removing a document name from an array. + +## Backwards Compatibility + +This ERC is additive and does not alter base token transfer semantics. It can be implemented alongside existing token standards and permissioning systems without changing their core behavior. + +## Test Cases + +Implementations should verify at least the following: + +- Adding a new document and reading it through `getDocument`. +- Reading a name that was never set, confirming `getDocument` returns empty values (`""`, `bytes32(0)`, `0`) and does not revert. +- Updating an existing document and validating changed URI/hash/timestamp. +- Removing a document and ensuring it is no longer returned by `getAllDocuments`. +- Emission of `DocumentUpdated` on create/update and `DocumentRemoved` on delete. +- Enumeration consistency after multiple add/update/remove operations. + +## Reference Implementation + +The interface is provided in [the reference interface](../assets/eip-1643/src/erc-1643/IERC1643.sol). A reusable abstract module implementing the full interface is provided in [the reference module](../assets/eip-1643/src/erc-1643/ERC1643.sol). + +Example integrations attaching the module to [ERC-20](./eip-20.md) and [ERC-721](./eip-721.md) tokens are provided in [the ERC-20 example](../assets/eip-1643/src/ERC20DocumentToken.sol) and [the ERC-721 example](../assets/eip-1643/src/ERC721DocumentToken.sol). + +These examples use the OpenZeppelin library and restrict document mutation to the contract owner. They are provided for educational purposes only and have not been audited. + +The module maintains: + +- Mapping from `bytes32` name to document metadata. +- Array/set for enumeration of active names. +- Index tracking to support O(1) removals from the enumeration set. + +## Security Considerations + +- Document URIs may reference mutable off-chain content. Consumers are strongly encouraged to verify content using the published `documentHash` and trusted retrieval channels. +- Implementations should protect `setDocument` and `removeDocument` with appropriate authorization, otherwise unauthorized actors can modify legal or operational references. +- Applications should treat event streams as advisory and reconcile against on-chain state when correctness is critical. +- `getAllDocuments` returns the entire set of names in a single call, and its response grows with the number of stored documents. It is the only enumeration path defined here, so a contract holding a large document set may produce a call that exceeds the gas limit an `eth_call` provider applies, leaving no standard way to enumerate. Implementations expecting large sets should consider exposing an additional paginated accessor alongside this interface. +- Document names may not always fit cleanly into `bytes32`, especially for long legal titles. Implementations should avoid lossy truncation of human-readable names; using a deterministic hash-based identifier (for example, the document content hash or a hash of a canonical full title) as the `bytes32` name is a safer alternative. +- The custom errors `ERC1643InvalidName()` and `ERC1643MissingDocument()` are defined in this interface but were absent from the earlier draft text of this proposal. Older implementations may not define these errors and may instead revert with strings or implementation-specific error patterns. Integrators should not assume all [ERC-1643](./eip-1643.md) contracts expose identical revert data. +- ERC-165 interface detection was also not part of the earlier ERC-1643 draft text. Older implementations may not expose `supportsInterface` for ERC-165 or `IERC1643`, so integrators should treat ERC-165 support as optional when interacting with legacy deployments. + +## Copyright + +Copyright and related rights waived via [CC0](../LICENSE.md). diff --git a/doc/ERCSpecification/erc-8303-draft.md b/doc/ERCSpecification/erc-8303-draft.md new file mode 100644 index 0000000..15be396 --- /dev/null +++ b/doc/ERCSpecification/erc-8303-draft.md @@ -0,0 +1,131 @@ +--- +eip: 8303 +title: Contract Version +description: Interface for exposing a contract implementation version string +author: Ryan Sauge (@rya-sge) +discussions-to: https://ethereum-magicians.org/t/erc-8303-contract-version/28795 +status: Draft +type: Standards Track +category: ERC +created: 2026-02-12 +--- + +## Abstract + +This ERC defines a minimal interface to expose a contract version string through a standardized `version()` view function. The design is based on the version pattern used by [ERC-3643](./eip-3643.md), while remaining token-agnostic and applicable to other smart contract domains, including DeFi applications such as lending protocols. + +## Motivation + +Integrators frequently need a simple, on-chain way to identify which contract implementation they interact with. A standardized version function improves: + +- integration safety (feature gating by version), +- operations (faster incident triage), +- governance and migration tracking (upgrade visibility), +- ecosystem tooling interoperability. + +It is also useful for end-users, developers, and security auditors to identify which version of a codebase is currently used by a deployed contract. + +The same requirement appears in permissioned token systems ([ERC-3643](./eip-3643.md)) and in DeFi systems where contracts evolve over time. + +## Specification + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119) and [RFC 8174](https://www.rfc-editor.org/rfc/rfc8174). + +### Interface + +```solidity +interface IERC8303 { + /// @notice Returns the implementation version string. + /// @return The version value (for example "1.0.0"). + function version() external view returns (string memory); +} +``` + +### Required Behavior + +1. **Version read** + - `version()` MUST be a view function. + - `version()` MUST NOT revert under normal operation. + - `version()` MUST return a non-empty string. + +2. **Version meaning** + - Returned values SHOULD be stable and machine-comparable by off-chain tooling. + - Returned values SHOULD follow a Semantic Versioning 2.0.0-like format: `MAJOR.MINOR.PATCH` using decimal integers (for example `1.0.0`, `3.2.1`). + - The canonical recommended pattern is `^[0-9]+\.[0-9]+\.[0-9]+$`. + - Implementations MAY define their own versioning policy, but SHOULD document it publicly. + +3. **Deployment model compatibility** + - This interface is compatible with immutable deployments and proxy-based upgradeable deployments. + - In upgradeable systems, `version()` SHOULD reflect the active implementation seen by users and integrators. + +### [ERC-165](./eip-165.md) + +Implementations SHOULD support [ERC-165](./eip-165.md) interface discovery for this interface. + +If an implementation supports [ERC-165](./eip-165.md), `supportsInterface(type(IERC8303).interfaceId)` MUST return `true`. + +- The interface id for `IERC8303` is `0x54fd4d50`. + +### Compatibility Note for ERC-3643 Integrations + +Integrators MAY treat legacy ERC-3643 token contracts exposing a compatible `version()` function as implementing this ERC even if they do not advertise ERC-165 support. + +## Rationale + +- **Minimal scope**: A single function maximizes adoption and keeps gas/runtime complexity negligible. +- **ERC-3643 alignment**: Reuses a proven pattern already used in regulated token implementations. +- **Token-agnostic design**: The interface applies to token contracts and non-token contracts alike. +- **Optional ERC-165**: ERC-165 support is recommended but not required, lowering the adoption barrier for contracts that do not implement interface discovery. When ERC-165 is supported, advertising this interface is mandatory to ensure consistent detection by integrators. +- **`string` over `bytes32`**: A human-readable string is preferred to a fixed-size bytes32 for legibility in explorers and tooling, at the cost of marginally higher gas for the return value. + +## Backwards Compatibility + +This ERC is fully additive. Contracts already exposing `version()` are naturally compatible if they match the interface signature. + +## Test Cases + +The following test cases apply to any conforming implementation. + +1. `version()` MUST NOT revert. +2. `version()` MUST return a non-empty string. +3. `version()` MUST return the version string declared by the implementation (e.g. `"1.0.0"`). +4. If the contract supports [ERC-165](./eip-165.md), `supportsInterface(0x54fd4d50)` MUST return `true`. +5. If the contract supports [ERC-165](./eip-165.md), `supportsInterface(0xffffffff)` MUST return `false`. + +## Reference Implementation + +Reference implementations are provided in the assets folder: the [interface](../assets/erc-8303/src/IERC8303.sol) and a [base implementation](../assets/erc-8303/src/ERC8303.sol), along with usage examples for [ERC-20](../assets/erc-8303/src/examples/ERC20VersionedExample.sol) and [ERC-721](../assets/erc-8303/src/examples/ERC721VersionedExample.sol) tokens. These examples are provided for educational purposes only and are not audited. + +```solidity +// SPDX-License-Identifier: CC0-1.0 +pragma solidity ^0.8.0; + +import "./IERC8303.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; + +contract ERC8303Example is IERC8303, ERC165 { + function version() external pure override returns (string memory) { + return "1.0.0"; + } + + function supportsInterface(bytes4 interfaceId) + public + view + override + returns (bool) + { + return interfaceId == type(IERC8303).interfaceId + || super.supportsInterface(interfaceId); + } +} +``` + +## Security Considerations + +- `version()` is metadata and must not be used as a sole authorization primitive. +- In upgradeable systems, governance controls remain the trust anchor; version reporting does not prevent malicious upgrades. +- Integrators should combine version checks with other trust signals (governance model, audits, deployment provenance). + +## Copyright + +Copyright and related rights waived via [CC0](../LICENSE.md). diff --git a/doc/ERCSpecification/erc-draft_multi_document_management.md b/doc/ERCSpecification/erc-draft_multi_document_management.md new file mode 100644 index 0000000..47bc57c --- /dev/null +++ b/doc/ERCSpecification/erc-draft_multi_document_management.md @@ -0,0 +1,268 @@ +--- +title: Multi-Subject Document Management +description: Interface for a contract that attaches, updates, removes, and enumerates documents on behalf of multiple subject contracts. +author: Ryan Sauge (@rya-sge) +discussions-to: https://ethereum-magicians.org/t/erc-1643-document-management-standard-erc-1400/27437 +status: Draft +type: Standards Track +category: ERC +created: 2026-07-28 +requires: 165, 1643 +--- + +## Abstract + +This ERC defines an interface for a contract that stores and manages documents on behalf of **several other contracts**, called *subjects*. Every function is scoped by a `subject` address, and every event carries that address, so a single management contract can serve many subjects while remaining observable and operable per subject. + +It is a companion to [ERC-1643](./eip-1643.md), which defines the equivalent per-contract interface. A subject is not required to implement ERC-1643, or any other particular interface, to have documents managed on its behalf. + +## Motivation + +[ERC-1643](./eip-1643.md) associates documents with the contract that exposes the interface. Its `DocumentUpdated` and `DocumentRemoved` events carry only `(name, uri, documentHash)` — no address — so consumers attribute an event to the address that emitted it. This is unambiguous when a single contract both exposes the interface and stores its own documents. + +A common operational and gas optimization is to **delegate** document management to a separate contract. When that contract is dedicated to one subject, nothing new is needed: it effectively is that subject's ERC-1643 implementation, and its events are unambiguous. But when **one management contract serves many subjects**, the per-contract events break down — a consumer watching the management contract cannot tell which subject a change belongs to, and the management contract has no compliant way to report per-subject changes at all. + +Shared document management is worth supporting directly. 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. What the shared case needs is an address in the event and an address in the function signature. That is the whole of this proposal. + +The scope is deliberately broader than tokens. A subject is any contract that documents can belong to — an [ERC-20](./eip-20.md) or [ERC-721](./eip-721.md) token, an [ERC-1155](./eip-1155.md) multi-token, a vault, or any other on-chain product. Nothing in this interface inspects the subject or calls into it. + +## Specification + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174. + +A document entry is identified by a `subject` (`address`) together with a name (`bytes32`), and stores: + +- A URI (`string`) pointing to the document location. +- A content hash (`bytes32`) for integrity checks. +- A last-modified timestamp (`uint256`) set when the entry is written. + +This is the [ERC-1643](./eip-1643.md) data model, extended with the `subject` address. Entries under different subjects are independent: a `name` written for one subject MUST NOT affect the entry stored under the same `name` for any other subject. + +### Interface + +This interface is declared **independently of `IERC1643`** — it does not inherit it — so a management contract can implement the address-scoped surface without being forced to implement the per-contract single-argument functions. + +A contract MAY implement both, in which case it implements, and advertises, each interface separately. + +Two of the three errors below, `ERC1643InvalidName` and `ERC1643MissingDocument`, are **the same errors defined by [ERC-1643](./eip-1643.md)** — same names, same signatures, hence the same 4-byte selectors — reused deliberately so that a caller sees identical revert data for identical conditions whether it is talking to an ERC-1643 contract or to a management contract. They are restated here rather than inherited because this interface does not inherit `IERC1643`; a contract implementing both obtains each error once, from `IERC1643`, and MUST NOT declare them twice. + +`MultiDocumentInvalidSubject` has no ERC-1643 counterpart: the condition it reports cannot arise there, because ERC-1643's `setDocument` has no `subject` argument — its subject is implicitly the contract itself, which is never the null address. + +```solidity +/// @title IERC1643MultiDocument Multi-Subject Document Management +interface IERC1643MultiDocument { + /// @notice Emitted when a document is created or updated for `subject`. + /// @param subject Address of the contract the document belongs to. + /// @param name Identifier of the document. + /// @param uri Document location. + /// @param documentHash Hash of the document contents. + event DocumentUpdatedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); + + /// @notice Emitted when a document is removed for `subject`. + /// @param subject Address of the contract the document belonged to. + /// @param name Identifier of the document. + /// @param uri Document location at the time of removal. + /// @param documentHash Hash of the document contents at the time of removal. + event DocumentRemovedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); + + /// @notice Reverts when `setDocument` or `removeDocument` is called with `subject == address(0)`. + /// @dev Specific to this proposal; has no ERC-1643 counterpart. + error MultiDocumentInvalidSubject(); + + /// @notice Reverts when `setDocument` is called with `name == bytes32(0)`. + /// @dev Same error as defined by ERC-1643, reused unchanged. + error ERC1643InvalidName(); + + /// @notice Reverts when `removeDocument` is called for a missing document. + /// @dev Same error as defined by ERC-1643, reused unchanged. + error ERC1643MissingDocument(); + + /// @notice Creates or updates a document entry for `subject`. + /// @dev MUST emit `DocumentUpdatedForSubject` on success. + /// @param subject Address of the contract the document belongs to. + /// @param name Identifier of the document. + /// @param uri Document location. + /// @param documentHash Hash of the document contents. + function setDocument(address subject, bytes32 name, string calldata uri, bytes32 documentHash) external; + + /// @notice Removes an existing document entry for `subject`. + /// @dev MUST emit `DocumentRemovedForSubject` on success. + /// @param subject Address of the contract the document belongs to. + /// @param name Identifier of the document to remove. + function removeDocument(address subject, bytes32 name) external; + + /// @notice Returns metadata for the document identified by `name` belonging to `subject`. + /// @param subject Address of the contract the documents belong to. + /// @param name Identifier of the document. + /// @return uri Document location. + /// @return documentHash Hash of the document contents. + /// @return lastModified Last update timestamp. + function getDocument(address subject, bytes32 name) + external + view + returns (string memory uri, bytes32 documentHash, uint256 lastModified); + + /// @notice Returns all document names currently tracked for `subject`. + /// @param subject Address of the contract the documents belong to. + /// @return documentNames Names of all documents that are currently set for `subject`. + function getAllDocuments(address subject) external view returns (bytes32[] memory documentNames); +} +``` + +### Function Requirements + +- `getDocument`: + - MUST return the latest values for the provided `subject` and `name`. + - MUST return empty values when the entry does not exist (`""`, `bytes32(0)`, `0`). + - MUST NOT revert solely because the entry does not exist. + +- `setDocument`: + - MUST create a new entry when `name` is not present for `subject`. + - MUST overwrite the existing entry when `name` already exists for `subject`. + - MUST update the stored last-modified timestamp. + - MUST emit `DocumentUpdatedForSubject` after state changes. + - MUST revert if the update cannot be persisted. + - SHOULD revert when `name == bytes32(0)`, using `ERC1643InvalidName()`. + - `uri` and `documentHash` MAY be empty (`""` and `bytes32(0)`), depending on issuer workflow and document lifecycle stage. Implementations MAY reject empty values based on policy requirements. + +- `removeDocument`: + - MUST remove the entry identified by `subject` and `name`. + - MUST emit `DocumentRemovedForSubject` with the removed metadata. + - MUST revert if removal cannot be completed. + - SHOULD revert when the named document does not exist for `subject`, using `ERC1643MissingDocument()`. + +- `getAllDocuments`: + - MUST include every name added for `subject` by `setDocument` and not removed by `removeDocument`. + - MUST NOT include removed names, or names belonging to any other subject. + +- `setDocument` and `removeDocument` SHOULD revert when `subject == address(0)`, since the null address is never a valid document subject, using `MultiDocumentInvalidSubject()`. + - Guarding the write path is sufficient: when `setDocument` rejects `subject == address(0)`, no document can exist under that subject, so `removeDocument` there already fails with `ERC1643MissingDocument()`. + +- Implementations MAY use different error names/signatures than those shown in this specification. + +### Authorization + +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. A single management contract holding the document sets of unrelated subjects behind one address makes this the central security property of the proposal; see Security Considerations. + +### Interface Detection ([ERC-165](./eip-165.md)) + +Implementations SHOULD support ERC-165 interface detection. When ERC-165 is implemented, `supportsInterface` SHOULD return `true` for `type(IERC1643MultiDocument).interfaceId` and for the ERC-165 interface id. + +`type(IERC1643MultiDocument).interfaceId` is the XOR of only this interface's own address-scoped functions. Because this interface does not inherit `IERC1643`, and because Solidity excludes inherited selectors from `type(I).interfaceId` in any case, advertising it says nothing about whether the per-contract single-argument functions are implemented. Accordingly, a contract MUST return `true` for `type(IERC1643).interfaceId` only if it also implements those base functions; a management contract exposing only the address-scoped surface MUST NOT advertise `type(IERC1643).interfaceId`. + +Events are not part of any ERC-165 interface id, so `supportsInterface` reflects only which functions a contract implements, not whether it emits the events defined here. + +### Optional: Subject-Side Manager Discovery + +Nothing above lets a consumer that knows only a subject find the management contract holding that subject's documents; the address has to be learned out of band. Subjects MAY close this gap by implementing the following interface. It is **implemented by the subject, not by the management contract**, and is optional for both. + +```solidity +/// @title IMultiDocumentSubject Manager discovery (optional) +interface IMultiDocumentSubject { + /// @notice Emitted when the managing contract changes. + /// @param previousManager Address that previously managed this contract's documents. + /// @param newManager Address that manages this contract's documents from now on. + event DocumentManagerUpdated(address indexed previousManager, address indexed newManager); + + /// @notice Returns the contract managing this contract's documents. + /// @dev MUST NOT revert. MUST emit `DocumentManagerUpdated` when the returned value changes. + /// @return manager Address of the managing contract, or `address(0)` when documents are managed in-contract. + function documentManager() external view returns (address manager); +} +``` + +- `documentManager`: + - MUST return the address of the contract to which this contract's document management is delegated. + - MUST return `address(0)` when document management is not delegated. + - MUST NOT revert. +- A contract that changes the returned value MUST emit `DocumentManagerUpdated`, so a consumer that cached the address learns of the migration. Without this, discovery would be a one-shot read that silently goes stale. +- This interface is declared independently of both `IERC1643` and `IERC1643MultiDocument`, so its ERC-165 id is distinct and advertising it perturbs neither. A subject implementing it SHOULD return `true` for `type(IMultiDocumentSubject).interfaceId`. + +The returned address is an **assertion by the subject**, not a verified link: nothing requires the named contract to acknowledge the relationship, or to have any documents for that subject at all. Consumers MUST treat it as a discovery hint and not as evidence of authorization; see Security Considerations. + +### Relationship to [ERC-1643](./eip-1643.md) + +A subject need not implement ERC-1643. This section applies only when it does, and constrains such deployments; it places no requirement on ERC-1643 itself, which is unchanged by this proposal. + +#### Emission Responsibility + +ERC-1643's `DocumentUpdated` / `DocumentRemoved` carry no address, so consumers attribute them to the address that emitted them. Those events MUST therefore be emitted by the contract that exposes ERC-1643 to consumers — the address consumers are expected to subscribe to. When an ERC-1643 subject delegates to a management contract, the emitter MUST be chosen so per-contract observability is preserved: + +- A management contract **dedicated to a single** subject MAY be that subject's ERC-1643 implementation and MUST emit `DocumentUpdated` / `DocumentRemoved`; those events are unambiguous because only one subject is served. Such a contract does not need this proposal. +- A management contract **serving several** subjects MUST NOT report per-subject changes through ERC-1643's events, since those events cannot identify the subject. In this configuration each subject MUST emit ERC-1643's events for its own documents, and the management contract emits `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` instead, as those carry the `subject` address. +- Implementations SHOULD NOT emit ERC-1643's events from **both** the subject and the management contract; it is redundant and wastes gas. + +#### Call Topology + +A contract can only emit an event in a transaction in which it executes. The requirement that each subject emit ERC-1643's events for its own documents therefore constrains how a write reaches the management contract, not only which contract is nominally responsible for emitting. + +A write that creates, updates, or removes a document for an ERC-1643 subject MUST include an execution point in that subject at which the event is emitted. Two topologies satisfy this: + +- **Subject-initiated.** The subject calls the management contract's address-scoped write and emits ERC-1643's event itself. The management contract emits the corresponding event defined here. +- **Manager-initiated with callback.** An authorized operator calls the management contract, which calls back into the subject through an implementation-defined permissioned hook; the subject emits ERC-1643's event. This proposal does not define the signature of that hook. + +A deployment in which an operator calls `setDocument(address subject, ...)` or `removeDocument(address subject, bytes32 name)` directly, with no execution point in the subject, does **not** satisfy ERC-1643's emission requirement for that subject: a consumer subscribed to the subject never observes the change, so the subject does not support subscribing to updates on its documentation and is not a conformant ERC-1643 implementation in that deployment, even though it exposes the ERC-1643 functions. Note this is a consequence of ERC-1643's own requirements, not an additional obligation imposed here. + +Accordingly, a management contract SHOULD restrict its address-scoped writes to callers for which one of the two topologies above holds — the subject itself, or an operator whose write path calls back into the subject. This is narrower than, and consistent with, the per-`subject` authorization required above: authorization decides *who* may write for a subject, while call topology decides whether that write remains observable on the subject's own address. + +Consumers of this proposal's events are unaffected in either topology: `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` carry the `subject` address and are emitted by the management contract in every case. + +## Rationale + +The `subject` parameter is named generically rather than "token" because the contract documents belong to is not necessarily a token, and because the on-chain identifiers should not hard-code an assumption the interface does not enforce. The event names follow the parameter (`DocumentUpdatedForSubject`), keeping the event and its attribute aligned. + +The interface is declared independently of `IERC1643` rather than inheriting it. Inheritance would force every shared management contract to implement the per-contract single-argument functions, which have no meaningful subject in the shared case, and would invite contracts to advertise an ERC-165 interface id for functions they do not implement. + +Separate address-scoped functions are used instead of overloading the per-contract ones with a subject-carrying variant of the same name because the resulting selectors are distinct either way; declaring them here keeps this proposal self-contained and readable without reference to ERC-1643's interface. + +The data model is inherited from ERC-1643 unchanged — `bytes32` names for compact, directly comparable on-chain identifiers, a URI pointer instead of on-chain storage, and a content hash for integrity — so that a subject can migrate between self-managed and delegated document storage without changing what consumers read. + +Errors are prefixed by the proposal that defines the condition, not by the proposal that declares them. `ERC1643InvalidName` and `ERC1643MissingDocument` keep their ERC-1643 prefix because they are ERC-1643's errors, reused so revert data stays identical across both interfaces; renaming them here would fragment that. `MultiDocumentInvalidSubject` is defined by this proposal alone and is therefore named after it, rather than borrowing a prefix from a standard in which the condition cannot occur. + +Manager discovery is defined here, and as a separate optional interface, for three reasons. It is a function on the *subject*, so folding it into ERC-1643 would grow that proposal with a function about a delegation arrangement ERC-1643 does not itself define. It is meaningful only where delegation exists, which is the subject of this proposal. And keeping it out of `IERC1643MultiDocument` means a management contract is never asked to implement a getter about itself that only its subjects can answer, while the distinct ERC-165 id lets a consumer detect discovery support without inferring anything about either document interface. + +The names `IERC1643MultiDocument`, `MultiDocumentInvalidSubject` and `IMultiDocumentSubject` are all provisional. They record the companion relationship while this proposal is unnumbered, and SHOULD be revisited together to track this proposal's own number once an editor assigns one. The two reused ERC-1643 error names are **not** provisional and are expected to stay as they are. + +## Backwards Compatibility + +This proposal introduces a new interface and does not modify [ERC-1643](./eip-1643.md) or any other proposal. + +- **Function selectors.** `getDocument(address,bytes32)`, `getAllDocuments(address)`, `setDocument(address,bytes32,string,bytes32)` and `removeDocument(address,bytes32)` have different signatures — hence different 4-byte selectors — than their single-argument ERC-1643 counterparts. A contract implementing both exposes both sets side by side with no collision. +- **Events.** `DocumentUpdatedForSubject` / `DocumentRemovedForSubject` are new event topics. ERC-1643's `DocumentUpdated` / `DocumentRemoved` are untouched and keep their exact meaning. +- **ERC-165.** Advertising `type(IERC1643MultiDocument).interfaceId` does not disturb `supportsInterface(type(IERC1643).interfaceId)`. A consumer that only knows ERC-1643 detects it exactly as before. +- **Manager discovery.** `IMultiDocumentSubject` is optional and separate. It adds one selector and one event topic to a subject that chooses to implement it, and is declared independently of both document interfaces, so it perturbs neither interface id. A subject that does not implement it is unaffected, and a consumer that does not know it behaves exactly as before. + +A consumer that only knows ERC-1643 is unaffected: it continues to call the per-contract functions and subscribe to the per-contract events on the address it was directed to watch. The one way to break such a consumer is to direct it at a management contract that emits only the events defined here — a deployment error, addressed by the Emission Responsibility and Call Topology requirements above rather than by the interface itself. + +## Test Cases + +Implementations should verify at least the following: + +- Subject isolation: the same `name` written for two different subjects yields two independent entries, and `getAllDocuments` for one subject never returns the other's names. +- Adding, updating, and removing a document for a subject, and reading it back through `getDocument`. +- Emission of `DocumentUpdatedForSubject` on create/update and `DocumentRemovedForSubject` on delete, each carrying the correct `subject`. +- A caller not authorized for a subject failing to create, update, or remove that subject's documents. +- `setDocument` rejecting `subject == address(0)` and `name == bytes32(0)`. +- `supportsInterface` returning `true` for `type(IERC1643MultiDocument).interfaceId`, and returning `false` for `type(IERC1643).interfaceId` on a contract that implements only the address-scoped surface. +- For a subject implementing `IMultiDocumentSubject`: `documentManager` returning the delegate's address, returning `address(0)` when management is not delegated, and `DocumentManagerUpdated` being emitted with the correct previous and new addresses when the delegate changes. + +## Reference Implementation + +A reference implementation is not yet provided. It is expected to maintain, per subject, a mapping from `bytes32` name to document metadata, a set for enumeration of active names, and index tracking to support O(1) removals — the ERC-1643 reference module's structure, keyed additionally by `subject` — together with the per-`subject` authorization required in the Specification. + +## Security Considerations + +- **Per-subject authorization is the central risk.** A management contract holds the document sets of unrelated subjects behind a single address. If writes are not authorized per `subject`, any caller permitted to write for one subject can modify another subject's legal or operational references. This is a stronger requirement than in the per-contract case, where a contract's own access control naturally scopes to its own documents. +- **Delegation moves the subject's trust boundary.** A subject that delegates document management is only as protected as the management contract's access control: whoever can write for that subject there can change its legal or operational references, regardless of the subject's own permissioning. Delegating to a contract serving several subjects also concentrates the document sets of unrelated parties behind a single address, so a single access-control flaw there is not contained to one subject. Subjects should treat the choice of management contract as a permissioning decision, not merely a storage one. +- **The null subject.** Implementations should reject `subject == address(0)`. This is a data-integrity concern rather than a fund-safety one: subject namespaces are isolated, and the null address cannot call the contract to read documents registered under it, so such entries are inert. They do, however, let callers populate a namespace no contract can ever own, cluttering state and misleading off-chain indexers that key on `subject`. +- **Direct writes can silently bypass subject-side emission.** When a subject implements [ERC-1643](./eip-1643.md), a write sent directly to the management contract without an execution point in the subject leaves the subject's per-contract events unemitted. The failure is quiet in both directions: the write succeeds and the event defined here is emitted, so nothing reverts, while a consumer watching the subject sees no event and concludes the documents are unchanged. Integrators who must rely on the per-contract events should confirm the deployment's write path out of band. +- **Event emission is not discoverable.** Because events are outside ERC-165, `supportsInterface(type(IERC1643MultiDocument).interfaceId)` confirms only that the functions exist, not that the address-carrying events are actually emitted. Integrators relying on those events should confirm emission out of band. +- **`documentManager` is an unverified assertion.** The value is chosen entirely by the subject, and nothing requires the named contract to acknowledge the relationship or to hold any documents for that subject. A compromised or malicious subject can point consumers at an attacker-controlled contract serving fabricated documents, and calling `getAllDocuments(subject)` on the claimed manager does not disprove this — anyone can populate their own contract with entries for any subject. Manager discovery is a convenience for locating a feed, not an authorization or authenticity signal. Consumers making decisions that depend on document contents should verify against the published `documentHash` and, where the stakes justify it, confirm the manager address through the same out-of-band channel they would have used without this interface. +- **A stale cached manager address.** A consumer that reads `documentManager` once and does not watch `DocumentManagerUpdated` may keep following a superseded contract after a migration, seeing a frozen document set with no indication it is no longer current. +- **Mutable off-chain content.** Document URIs may reference content that changes independently of the chain. Consumers are strongly encouraged to verify content against the published `documentHash` and to use trusted retrieval channels. +- **Names may not fit `bytes32`.** Long legal titles should not be lossily truncated; a deterministic hash-based identifier (for example the document content hash, or a hash of a canonical full title) is a safer choice for the `bytes32` name. +- **Events are advisory.** Applications should reconcile event streams against on-chain state when correctness is critical. + +## Copyright + +Copyright and related rights waived via [CC0](../LICENSE.md). diff --git a/doc/audits/AUDIT_OVERVIEW.md b/doc/audits/AUDIT_OVERVIEW.md new file mode 100644 index 0000000..4676427 --- /dev/null +++ b/doc/audits/AUDIT_OVERVIEW.md @@ -0,0 +1,67 @@ +# Security & audit overview — DocumentEngine + +> **This project has not been audited.** No formal external security audit has been performed on any +> release. What follows is the record of the automated and AI-assisted analyses that *have* been run. +> These are **not** a substitute for an audit. Anyone deploying this engine in production must +> commission their own independent security assessment. + +## In scope + +The `src/` tree only — 9 files, 298 nSLOC as of `v0.4.0`: + +``` +src/DocumentEngine.sol src/interfaces/IERC1643MultiDocument.sol +src/DocumentEngineBase.sol src/interfaces/IERC8303.sol +src/DocumentEngineInvariant.sol src/interfaces/ITokenBinding.sol +src/DocumentEngineOwnable.sol src/modules/TokenBindingModule.sol + src/modules/VersionModule.sol +``` + +Out of scope: `lib/` (CMTAT, RuleEngine, OpenZeppelin — audited, or not, upstream), `test/`, +`script/`. + +## Analyses + +| Analysis | Version | Report | Triage | +| --- | --- | --- | --- | +| Aderyn `0.6.5` | `v0.4.0` | [report](./tools/v0.4.0/aderyn/aderyn-report.md) | [feedback](./tools/v0.4.0/aderyn/aderyn-report-feedback.md) | +| Slither | — | not run | — | +| ERC conformance analysis (AI-assisted) | `v0.4.0` | open items: [`IMPROVEMENT.md`](../../IMPROVEMENT.md) | — | + +## Static-analysis results + +| Tool | High | Medium | Low | Info | Anything to fix? | +| --- | --- | --- | --- | --- | --- | +| Aderyn `0.6.5` | 0 | — | 6 | 0 | **No.** 4 by design, 1 environment, 1 false positive; 1 of the "by design" instances overlaps a known scalability item (§4.7) | +| Slither | — | — | — | — | not run for `v0.4.0` | + +Aderyn reports no Medium or Info categories; it classifies only High and Low. + +## Substantive findings fixed in `v0.4.0` + +From the ERC conformance analysis (open items: [`IMPROVEMENT.md`](../../IMPROVEMENT.md)) rather than from the +static analyzers — neither tool can see these, since both are ABI- and specification-level: + +| Finding | Severity | Status | +| --- | --- | --- | +| `getDocument` returned a `Document` struct where ERC-1643 mandates three flat values. Same selector and same `type(IERC1643).interfaceId` either way, so ERC-165 detection could not distinguish them and a spec-conformant consumer silently decoded corrupt values | High | **Fixed** — flat return on both overloads, pinned by `testGetDocumentReturnsFlatErc1643Abi`, which inspects the returndata directly | +| `ERC1643InvalidName` / `ERC1643MissingDocument` declared both locally and by `IERC1643`, which the multi-subject draft forbids and the compiler rejects | Blocker | **Fixed** — local declarations removed | +| Null-subject error named `ERC1643InvalidSubject`, after a standard in which the condition cannot occur, and declared on an abstract contract rather than an interface | Low | **Fixed** — renamed `MultiDocumentInvalidSubject` and moved to `IERC1643MultiDocument`; every specification error now sits on the interface defining its condition | +| `bindToken(address(0))` accepted, and bind/unbind emitted `TokenBindingSet` even when the binding did not change | Low | **Fixed** — null address rejected with `TokenBindingInvalidToken()`; both are now idempotent and emit only on a real transition | + +## Known open items + +Not defects in the sense of being exploitable, but tracked deviations from the specifications. Full +detail, with a recommendation for each, in [`IMPROVEMENT.md`](../../IMPROVEMENT.md). + +| Item | Severity | Where | +| --- | --- | --- | +| `_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 | +| The ERC-2771 trusted forwarder can act as any bound subject and is immutable | Info | item 5 | + +## Reporting a vulnerability + +See the repository's security policy, or contact [admin@cmta.ch](mailto:admin@cmta.ch). diff --git a/doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md b/doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md new file mode 100644 index 0000000..3bfe2fb --- /dev/null +++ b/doc/audits/tools/v0.4.0/aderyn/aderyn-report-feedback.md @@ -0,0 +1,60 @@ +# Aderyn report — triage (DocumentEngine `v0.4.0`) + +| | | +| --- | --- | +| Report | [`aderyn-report.md`](./aderyn-report.md) | +| Command | `aderyn -x mocks --output doc/audits/tools/v0.4.0/aderyn/aderyn-report.md` | +| Tool version | `aderyn 0.6.5` | +| Scope | `src/` only — 9 files, 307 nSLOC. **Mocks and tests excluded.** This project keeps its mocks (`CMTATDocumentEngineMock`, `OpenDocumentEngine`) inside `test/DocumentEngine.t.sol`, which Aderyn does not scan, so `-x mocks` matched nothing and changed nothing. | +| Dependency | CMTAT `v3.3.0-rc2` (`35d8940b`) | +| Result | **0 High · 6 Low** | + +## Executive triage + +**Nothing to fix.** No finding is exploitable, and none blocks the `v0.4.0` release. + +Five of the six are the analyzer's standing advisories about deliberate design choices (a privileged +operator, a caret pragma, PUSH0, revert-in-loop, storage-writes-in-loop) and one is a false positive. + +The one result worth keeping in view is **L-5 at `DocumentEngineBase.sol:238`**, which is not a batch +loop but the linear scan in `_removeDocumentName`. Aderyn reached it from the "costly operation in a +loop" heuristic; it happens to land on the same code as `IMPROVEMENT.md` item 4, which flags the O(n) +removal against the multi-subject draft's expectation of "index tracking to support O(1) removals". +That is a scalability item, not a vulnerability — a subject with a large document set makes +`removeDocument` progressively more expensive, and `batchRemoveDocuments` compounds it to O(n·m). +Two independent routes arriving at the same line is a reasonable argument for doing the index-mapping +fix in a later release. It is deliberately **not** being done in `v0.4.0`, which is scoped to the +CMTAT upgrade. + +## Findings + +| 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 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. | +| L-5 | Costly operations inside loop | Low | 5 | **By design** ×4, **known item** ×1 | Four instances (`:124, 142, 156, 170`) are storage writes in the batch loops — unavoidable, and the reason the batch functions exist is to amortise the 21 000-gas transaction overhead across those writes. The fifth (`:238`) is `_removeDocumentName`'s linear scan with swap-and-pop; see the triage note above and `IMPROVEMENT.md` item 4. | +| L-6 | Unchecked Return | Low | 1 | **False positive** | `DocumentEngine.sol:35`, `_grantRole(DEFAULT_ADMIN_ROLE, admin);`. OpenZeppelin's `_grantRole` returns `false` only when the account already holds the role. This call is in the constructor of a freshly deployed contract, where no role has been granted yet, so it always returns `true`; `admin == address(0)` is already rejected on the preceding lines. There is no state to check and no recovery path to take. | + +## Delta + +This report was regenerated after the error-naming and token-binding fixes (error renaming and +relocation; `bindToken`/`unbindToken` null-address rejection and idempotence). **Nothing moved**: +the same six detectors fire with the same instance counts, and every cited line is unchanged. nSLOC +rose 298 → 307 for the added guard and its NatSpec. + +Worth noting explicitly, since it is a null result that is easy to misread as "not analysed": the new +`TokenBindingModule._setTokenBinding` — which adds a revert and an early return — triggered **no** +new finding, including no addition to L-4 (`revert` in a loop), because it contains no loop. + +## Delta from the previous version + +None — this is the **first** static-analysis run recorded for this repository. `doc/audits/` did not +exist before `v0.4.0`; the `CHANGELOG.md` release checklist referenced `doc/audits/tools` but no +report had been committed. There is therefore no baseline to diff against, and future runs should +diff against this one. + +Note for the next run: the `CLAUDE.md` file tree claims a Slither report exists under `doc/`. It does +not. Slither is installed (`slither --version` resolves) and was **not** run for `v0.4.0` — this +release re-ran Aderyn only. A Slither run would make the next delta meaningful across both tools. diff --git a/doc/audits/tools/v0.4.0/aderyn/aderyn-report.md b/doc/audits/tools/v0.4.0/aderyn/aderyn-report.md new file mode 100644 index 0000000..0044169 --- /dev/null +++ b/doc/audits/tools/v0.4.0/aderyn/aderyn-report.md @@ -0,0 +1,323 @@ +> **Summary — generated for DocumentEngine `v0.4.0` (CMTAT `v3.3.0-rc2`).** +> +> | | | +> | --- | --- | +> | Command | `aderyn -x mocks --output doc/audits/tools/v0.4.0/aderyn/aderyn-report.md` | +> | Tool version | `aderyn 0.6.5` | +> | Scope | `src/` only — 9 files, 307 nSLOC. **Mocks/tests excluded** (this project has no `src/mocks`; its mocks live in `test/`, which Aderyn does not scan). | +> | Result | **0 High · 6 Low · 0 Info** | +> | Verdict | **Nothing to fix.** No finding is exploitable. One (L-5 at `DocumentEngineBase.sol:238`) independently corroborates a known gas/scalability item already tracked as [`IMPROVEMENT.md`](../../../../IMPROVEMENT.md) item 4. | +> +> | ID | Detector | Sev | Instances | Assessment | +> | --- | --- | --- | --- | --- | +> | L-1 | Centralization Risk | Low | 2 | **By design** — a document manager is a privileged operator by definition | +> | L-2 | Unspecific Solidity Pragma | Low | 9 | **By design** — `^0.8.20` is deliberate; the deployed compiler is pinned in `foundry.toml` | +> | L-3 | PUSH0 Opcode | Low | 9 | **Environment** — `evm_version = prague`; only relevant on chains without PUSH0 | +> | L-4 | Loop Contains `require`/`revert` | Low | 4 | **By design** — batch operations are deliberately all-or-nothing | +> | L-5 | Costly operations inside loop | Low | 5 | **By design** (4 batch loops) + **1 known item** — `_removeDocumentName` is O(n), see §4.7 | +> | L-6 | Unchecked Return | Low | 1 | **False positive** — `_grantRole` in a constructor on a fresh contract cannot return `false` | +> +> Full triage, with the reasoning verified against each cited line: +> [`aderyn-report-feedback.md`](./aderyn-report-feedback.md). +> Security overview: [`doc/audits/AUDIT_OVERVIEW.md`](../../../AUDIT_OVERVIEW.md). + +# Aderyn Analysis Report + +This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a static analysis tool built by [Cyfrin](https://cyfrin.io), a blockchain security company. This report is not a substitute for manual audit or security review. It should not be relied upon for any purpose other than to assist in the identification of potential security vulnerabilities. +# Table of Contents + +- [Summary](#summary) + - [Files Summary](#files-summary) + - [Files Details](#files-details) + - [Issue Summary](#issue-summary) +- [Low Issues](#low-issues) + - [L-1: Centralization Risk](#l-1-centralization-risk) + - [L-2: Unspecific Solidity Pragma](#l-2-unspecific-solidity-pragma) + - [L-3: PUSH0 Opcode](#l-3-push0-opcode) + - [L-4: Loop Contains `require`/`revert`](#l-4-loop-contains-requirerevert) + - [L-5: Costly operations inside loop](#l-5-costly-operations-inside-loop) + - [L-6: Unchecked Return](#l-6-unchecked-return) + + +# Summary + +## Files Summary + +| Key | Value | +| --- | --- | +| .sol Files | 9 | +| Total nSLOC | 307 | + + +## Files Details + +| Filepath | nSLOC | +| --- | --- | +| src/DocumentEngine.sol | 52 | +| src/DocumentEngineBase.sol | 149 | +| src/DocumentEngineInvariant.sol | 5 | +| src/DocumentEngineOwnable.sol | 28 | +| src/interfaces/IERC1643MultiDocument.sol | 13 | +| src/interfaces/IERC8303.sol | 4 | +| src/interfaces/ITokenBinding.sol | 8 | +| src/modules/TokenBindingModule.sol | 36 | +| src/modules/VersionModule.sol | 12 | +| **Total** | **307** | + + +## Issue Summary + +| Category | No. of Issues | +| --- | --- | +| High | 0 | +| Low | 6 | + + +# Low Issues + +## L-1: Centralization Risk + +Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds. + +
2 Found Instances + + +- Found in src/DocumentEngine.sol [Line: 24](../../../../../src/DocumentEngine.sol#L24) + + ```solidity + contract DocumentEngine is TokenBindingModule, VersionModule, AccessControlEnumerable, ERC2771Context { + ``` + +- Found in src/DocumentEngineOwnable.sol [Line: 24](../../../../../src/DocumentEngineOwnable.sol#L24) + + ```solidity + contract DocumentEngineOwnable is TokenBindingModule, VersionModule, Ownable2Step, ERC2771Context { + ``` + +
+ + + +## L-2: Unspecific Solidity Pragma + +Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;` + +
9 Found Instances + + +- Found in src/DocumentEngine.sol [Line: 2](../../../../../src/DocumentEngine.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineBase.sol [Line: 2](../../../../../src/DocumentEngineBase.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineInvariant.sol [Line: 2](../../../../../src/DocumentEngineInvariant.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineOwnable.sol [Line: 2](../../../../../src/DocumentEngineOwnable.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/IERC1643MultiDocument.sol [Line: 2](../../../../../src/interfaces/IERC1643MultiDocument.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/IERC8303.sol [Line: 2](../../../../../src/interfaces/IERC8303.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/ITokenBinding.sol [Line: 2](../../../../../src/interfaces/ITokenBinding.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/modules/TokenBindingModule.sol [Line: 2](../../../../../src/modules/TokenBindingModule.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/modules/VersionModule.sol [Line: 2](../../../../../src/modules/VersionModule.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +
+ + + +## L-3: PUSH0 Opcode + +Solc compiler version 0.8.20 switches the default target EVM version to Shanghai, which means that the generated bytecode will include PUSH0 opcodes. Be sure to select the appropriate EVM version in case you intend to deploy on a chain other than mainnet like L2 chains that may not support PUSH0, otherwise deployment of your contracts will fail. + +
9 Found Instances + + +- Found in src/DocumentEngine.sol [Line: 2](../../../../../src/DocumentEngine.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineBase.sol [Line: 2](../../../../../src/DocumentEngineBase.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineInvariant.sol [Line: 2](../../../../../src/DocumentEngineInvariant.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/DocumentEngineOwnable.sol [Line: 2](../../../../../src/DocumentEngineOwnable.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/IERC1643MultiDocument.sol [Line: 2](../../../../../src/interfaces/IERC1643MultiDocument.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/IERC8303.sol [Line: 2](../../../../../src/interfaces/IERC8303.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/interfaces/ITokenBinding.sol [Line: 2](../../../../../src/interfaces/ITokenBinding.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/modules/TokenBindingModule.sol [Line: 2](../../../../../src/modules/TokenBindingModule.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +- Found in src/modules/VersionModule.sol [Line: 2](../../../../../src/modules/VersionModule.sol#L2) + + ```solidity + pragma solidity ^0.8.20; + ``` + +
+ + + +## L-4: Loop Contains `require`/`revert` + +Avoid `require` / `revert` statements in a loop because a single bad item can cause the whole transaction to fail. It's better to forgive on fail and return failed elements post processing of the loop + +
4 Found Instances + + +- Found in src/DocumentEngineBase.sol [Line: 124](../../../../../src/DocumentEngineBase.sol#L124) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 142](../../../../../src/DocumentEngineBase.sol#L142) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 156](../../../../../src/DocumentEngineBase.sol#L156) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 170](../../../../../src/DocumentEngineBase.sol#L170) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +
+ + + +## L-5: Costly operations inside loop + +Invoking `SSTORE` operations in loops may waste gas. Use a local variable to hold the loop computation result. + +
5 Found Instances + + +- Found in src/DocumentEngineBase.sol [Line: 124](../../../../../src/DocumentEngineBase.sol#L124) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 142](../../../../../src/DocumentEngineBase.sol#L142) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 156](../../../../../src/DocumentEngineBase.sol#L156) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 170](../../../../../src/DocumentEngineBase.sol#L170) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +- Found in src/DocumentEngineBase.sol [Line: 238](../../../../../src/DocumentEngineBase.sol#L238) + + ```solidity + for (uint256 i = 0; i < length; ++i) { + ``` + +
+ + + +## L-6: Unchecked Return + +Function returns a value but it is ignored. Consider checking the return value. + +
1 Found Instances + + +- Found in src/DocumentEngine.sol [Line: 35](../../../../../src/DocumentEngine.sol#L35) + + ```solidity + _grantRole(DEFAULT_ADMIN_ROLE, admin); + ``` + +
+ + + diff --git a/doc/slither-report.md b/doc/slither-report.md deleted file mode 100644 index cc8c1dd..0000000 --- a/doc/slither-report.md +++ /dev/null @@ -1,38 +0,0 @@ -**THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. -Summary - - [dead-code](#dead-code) (1 results) (Informational) - - [solc-version](#solc-version) (1 results) (Informational) -## dead-code - -> Acknowledge - -Impact: Informational -Confidence: Medium - - - [ ] ID-0 -[DocumentEngine._msgData()](src/DocumentEngine.sol#L265-L272) is never used and should be removed - -src/DocumentEngine.sol#L265-L272 - -## solc-version - -> Acknowledge - -Impact: Informational -Confidence: High - - [ ] ID-1 - Version constraint ^0.8.20 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html) - - VerbatimInvalidDeduplication - - FullInlinerNonExpressionSplitArgumentEvaluationOrder - - MissingSideEffectsOnSelectorAccess. - It is used by: - - lib/CMTAT/contracts/interfaces/engine/draft-IERC1643.sol#3 - - lib/openzeppelin-contracts/contracts/access/AccessControl.sol#4 - - lib/openzeppelin-contracts/contracts/access/IAccessControl.sol#4 - - lib/openzeppelin-contracts/contracts/metatx/ERC2771Context.sol#4 - - lib/openzeppelin-contracts/contracts/utils/Context.sol#4 - - lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol#4 - - lib/openzeppelin-contracts/contracts/utils/introspection/IERC165.sol#4 - - src/DocumentEngine.sol#2 - - src/DocumentEngineInvariant.sol#2 - diff --git a/foundry.lock b/foundry.lock new file mode 100644 index 0000000..8dfec2c --- /dev/null +++ b/foundry.lock @@ -0,0 +1,32 @@ +{ + "lib/CMTAT": { + "tag": { + "name": "v3.3.0-rc2", + "rev": "35d8940b40943828c5ea407dc6b22d559d92e4ae" + } + }, + "lib/RuleEngine": { + "tag": { + "name": "v3.0.0-rc4", + "rev": "66fcf2aafebd1f9d9de8a81dec92b88da071c9b3" + } + }, + "lib/forge-std": { + "tag": { + "name": "v1.7.1", + "rev": "f73c73d2018eb6a111f35e4dae7b4f27401e9421" + } + }, + "lib/openzeppelin-contracts": { + "tag": { + "name": "v5.6.1", + "rev": "5fd1781b1454fd1ef8e722282f86f9293cacf256" + } + }, + "lib/openzeppelin-contracts-upgradeable": { + "tag": { + "name": "v5.6.1", + "rev": "7bf4727aacdbfaa0f36cbd664654d0c9e1dc52bf" + } + } +} diff --git a/foundry.toml b/foundry.toml index acc6e5a..67b08dd 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,10 +1,19 @@ [profile.default] -solc = "0.8.26" +solc = "0.8.34" src = "src" out = "out" libs = ["lib"] optimizer = true optimizer_runs = 200 -evm_version = 'cancun' +evm_version = 'prague' + +# `forge fmt` is the canonical formatter for this project (run `forge fmt`). +[fmt] +line_length = 120 +tab_width = 4 +bracket_spacing = false +int_types = "long" +quote_style = "double" +number_underscore = "preserve" # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options diff --git a/lib/CMTAT b/lib/CMTAT index e8048d4..580d477 160000 --- a/lib/CMTAT +++ b/lib/CMTAT @@ -1 +1 @@ -Subproject commit e8048d43b0299afd83f150d3725ab299994b4271 +Subproject commit 580d4776e4cbb857b2da7d83fd79144ae7e47557 diff --git a/lib/RuleEngine b/lib/RuleEngine new file mode 160000 index 0000000..66fcf2a --- /dev/null +++ b/lib/RuleEngine @@ -0,0 +1 @@ +Subproject commit 66fcf2aafebd1f9d9de8a81dec92b88da071c9b3 diff --git a/lib/forge-std b/lib/forge-std index 1714bee..f73c73d 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 1714bee72e286e73f76e320d110e0eaf5c4e649d +Subproject commit f73c73d2018eb6a111f35e4dae7b4f27401e9421 diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts index dbb6104..5fd1781 160000 --- a/lib/openzeppelin-contracts +++ b/lib/openzeppelin-contracts @@ -1 +1 @@ -Subproject commit dbb6104ce834628e473d2173bbc9d47f81a9eec3 +Subproject commit 5fd1781b1454fd1ef8e722282f86f9293cacf256 diff --git a/lib/openzeppelin-contracts-upgradeable b/lib/openzeppelin-contracts-upgradeable index 723f8ca..7bf4727 160000 --- a/lib/openzeppelin-contracts-upgradeable +++ b/lib/openzeppelin-contracts-upgradeable @@ -1 +1 @@ -Subproject commit 723f8cab09cdae1aca9ec9cc1cfa040c2d4b06c1 +Subproject commit 7bf4727aacdbfaa0f36cbd664654d0c9e1dc52bf diff --git a/package.json b/package.json index a56a8e3..4ca5410 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,5 @@ { "devDependencies": { - "prettier-plugin-solidity": "^1.4.1", "solidity-docgen": "^0.6.0-beta.36", "surya": "^0.4.11" } diff --git a/remappings.txt b/remappings.txt index b0803f5..31bf2f2 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,3 +1,4 @@ CMTAT/=lib/CMTAT/contracts/ +RuleEngine/=lib/RuleEngine/src/ OZ/=lib/openzeppelin-contracts/contracts/ @openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/ \ No newline at end of file diff --git a/script/DeployDocumentEngine.s.sol b/script/DeployDocumentEngine.s.sol new file mode 100644 index 0000000..d76e75f --- /dev/null +++ b/script/DeployDocumentEngine.s.sol @@ -0,0 +1,42 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import "forge-std/Script.sol"; +import {DocumentEngine} from "../src/DocumentEngine.sol"; + +/** + * @title DeployDocumentEngine + * @notice Deploys the role-based {DocumentEngine} (AccessControlEnumerable). + * @dev Configuration via environment variables: + * - `DOCUMENT_ENGINE_ADMIN` : address granted `DEFAULT_ADMIN_ROLE` (default: `msg.sender`) + * - `DOCUMENT_ENGINE_FORWARDER` : ERC-2771 trusted forwarder, `address(0)` disables gasless (default: `address(0)`) + * + * Usage: + * forge script script/DeployDocumentEngine.s.sol \ + * --rpc-url $RPC_URL --private-key $PRIVATE_KEY --broadcast + * + * Warning: the environment variables above and passing a raw key with + * `--private-key` are for local testing only, not for production. For production + * use a secure signing method (encrypted keystore, hardware wallet, ...) as + * described in the Foundry Key Management documentation (getfoundry.sh). + */ +contract DeployDocumentEngine is Script { + function run() external returns (DocumentEngine documentEngine) { + address admin = vm.envOr("DOCUMENT_ENGINE_ADMIN", msg.sender); + address forwarder = vm.envOr("DOCUMENT_ENGINE_FORWARDER", address(0)); + + documentEngine = deploy(admin, forwarder); + + console2.log("DocumentEngine deployed at:", address(documentEngine)); + console2.log(" admin :", admin); + console2.log(" trusted forwarder:", forwarder); + console2.log(" version :", documentEngine.version()); + } + + /// @dev Broadcasted deployment, isolated from env parsing so it can be reused/tested. + function deploy(address admin, address forwarder) public returns (DocumentEngine documentEngine) { + vm.startBroadcast(); + documentEngine = new DocumentEngine(admin, forwarder); + vm.stopBroadcast(); + } +} diff --git a/script/DeployDocumentEngineOwnable.s.sol b/script/DeployDocumentEngineOwnable.s.sol new file mode 100644 index 0000000..e7b7e92 --- /dev/null +++ b/script/DeployDocumentEngineOwnable.s.sol @@ -0,0 +1,42 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import "forge-std/Script.sol"; +import {DocumentEngineOwnable} from "../src/DocumentEngineOwnable.sol"; + +/** + * @title DeployDocumentEngineOwnable + * @notice Deploys the owner-based {DocumentEngineOwnable} (Ownable2Step). + * @dev Configuration via environment variables: + * - `DOCUMENT_ENGINE_OWNER` : initial owner (default: `msg.sender`) + * - `DOCUMENT_ENGINE_FORWARDER` : ERC-2771 trusted forwarder, `address(0)` disables gasless (default: `address(0)`) + * + * Usage: + * forge script script/DeployDocumentEngineOwnable.s.sol \ + * --rpc-url $RPC_URL --private-key $PRIVATE_KEY --broadcast + * + * Warning: the environment variables above and passing a raw key with + * `--private-key` are for local testing only, not for production. For production + * use a secure signing method (encrypted keystore, hardware wallet, ...) as + * described in the Foundry Key Management documentation (getfoundry.sh). + */ +contract DeployDocumentEngineOwnable is Script { + function run() external returns (DocumentEngineOwnable documentEngine) { + address owner = vm.envOr("DOCUMENT_ENGINE_OWNER", msg.sender); + address forwarder = vm.envOr("DOCUMENT_ENGINE_FORWARDER", address(0)); + + documentEngine = deploy(owner, forwarder); + + console2.log("DocumentEngineOwnable deployed at:", address(documentEngine)); + console2.log(" owner :", owner); + console2.log(" trusted forwarder:", forwarder); + console2.log(" version :", documentEngine.version()); + } + + /// @dev Broadcasted deployment, isolated from env parsing so it can be reused/tested. + function deploy(address owner, address forwarder) public returns (DocumentEngineOwnable documentEngine) { + vm.startBroadcast(); + documentEngine = new DocumentEngineOwnable(owner, forwarder); + vm.stopBroadcast(); + } +} diff --git a/src/DocumentEngine.sol b/src/DocumentEngine.sol index 3f4fff9..7b1a17e 100644 --- a/src/DocumentEngine.sol +++ b/src/DocumentEngine.sol @@ -1,35 +1,34 @@ //SPDX-License-Identifier: MPL-2.0 pragma solidity ^0.8.20; -import "OZ/access/AccessControl.sol"; +import "OZ/access/extensions/AccessControlEnumerable.sol"; +import {IAccessControl} from "OZ/access/IAccessControl.sol"; +import {IERC1643} from "CMTAT/interfaces/tokenization/draft-IERC1643.sol"; +import {IERC1643MultiDocument} from "./interfaces/IERC1643MultiDocument.sol"; +import {ITokenBinding} from "./interfaces/ITokenBinding.sol"; import "OZ/metatx/ERC2771Context.sol"; -import "CMTAT/interfaces/engine/draft-IERC1643.sol"; -import "./DocumentEngineInvariant.sol"; +import "./modules/TokenBindingModule.sol"; +import "./modules/VersionModule.sol"; /** * @title DocumentEngine - * @notice contract to manage documents on-chain through ERC-1643 + * @notice Deployment contract to manage documents on-chain through ERC-1643. + * @dev Wires the document-management logic ({DocumentEngineBase}) with a + * concrete access-control implementation. The authorization hooks are defined + * here (role-based `AccessControlEnumerable`, which additionally allows + * enumerating role members), keeping the access control separate from the + * document-management logic (CMTAT / CMTA-RuleEngine pattern). The contract + * version is exposed through the {VersionModule} (ERC-8303), and it also wires + * the ERC-2771 (gasless) meta-transaction support. */ -contract DocumentEngine is - IERC1643, - DocumentEngineInvariant, - AccessControl, - ERC2771Context -{ - /** - * @notice - * Get the current version of the smart contract - */ - string public constant VERSION = "0.3.0"; - // Mapping from contract addresses to document names to their corresponding Document structs - mapping(address => mapping(bytes32 => Document)) private _documents; - mapping(address => bytes32[]) private _documentNames; +contract DocumentEngine is TokenBindingModule, VersionModule, AccessControlEnumerable, ERC2771Context { + // Role allowed to manage documents on behalf of any smart contract, and to + // bind/unbind tokens (admin path). Token binding uses the shared allowlist in + // {TokenBindingModule}, not a dedicated role. + bytes32 public constant DOCUMENT_MANAGER_ROLE = keccak256("DOCUMENT_MANAGER_ROLE"); // Constructor to initialize the admin role - constructor( - address admin, - address forwarderIrrevocable - ) ERC2771Context(forwarderIrrevocable) { + constructor(address admin, address forwarderIrrevocable) ERC2771Context(forwarderIrrevocable) { if (admin == address(0)) { revert AdminWithAddressZeroNotAllowed(); } @@ -37,221 +36,57 @@ contract DocumentEngine is } /*////////////////////////////////////////////////////////////// - PUBLIC/EXTERNAL FUNCTIONS + ACCESS CONTROL (implementation) //////////////////////////////////////////////////////////////*/ /** - * @notice Restricted function to set or update a document - */ - function setDocument( - address smartContract, - bytes32 name_, - string memory uri_, - bytes32 documentHash_ - ) public onlyRole(DOCUMENT_MANAGER_ROLE) { - _setDocument(smartContract, name_, uri_, documentHash_); - } - - /** - * @notice Restricted function to remove a document for a given smart contract and name - */ - function removeDocument( - address smartContract, - bytes32 name_ - ) external onlyRole(DOCUMENT_MANAGER_ROLE) { - _removeDocument(smartContract, name_); - } - - /** - * @notice Batch version of setDocument to handle multiple documents at once + * @dev Authorization for the admin document-management path. + * The caller must hold `DOCUMENT_MANAGER_ROLE`. Override to customize. */ - function batchSetDocuments( - address[] calldata smartContracts, - bytes32[] calldata names, - string[] calldata uris, - bytes32[] calldata hashes - ) external onlyRole(DOCUMENT_MANAGER_ROLE) { - if ( - smartContracts.length == 0 || - smartContracts.length != names.length || - names.length != uris.length || - uris.length != hashes.length - ) { - revert InvalidInputLength(); - } - for (uint256 i = 0; i < smartContracts.length; i++) { - _setDocument(smartContracts[i], names[i], uris[i], hashes[i]); - } - } - - /** - * @notice Batch version of setDocument to handle multiple documents at once - */ - function batchSetDocuments( - address smartContract, - bytes32[] calldata names, - string[] calldata uris, - bytes32[] calldata hashes - ) external onlyRole(DOCUMENT_MANAGER_ROLE) { - if ( - names.length == 0 || - names.length != uris.length || - uris.length != hashes.length - ) { - revert InvalidInputLength(); - } - for (uint256 i = 0; i < names.length; ++i) { - _setDocument(smartContract, names[i], uris[i], hashes[i]); - } + function _authorizeDocumentManagement() internal view virtual override { + _checkRole(DOCUMENT_MANAGER_ROLE); } /** - * @notice Batch version of removeDocument to handle multiple documents at once + * @dev Returns `true` if `account` has been granted `role`. The default admin + * (`DEFAULT_ADMIN_ROLE`) is treated as holding **every** role. + * + * Note: this virtual "admin has all roles" behavior is NOT reflected by + * {AccessControlEnumerable} enumeration. `getRoleMember` / `getRoleMemberCount` + * report only explicit grants, so a `DEFAULT_ADMIN_ROLE` holder satisfies + * `hasRole(anyRole, admin)` yet does not appear in `getRoleMember(anyRole, ...)`. */ - function batchRemoveDocuments( - address[] calldata smartContracts, - bytes32[] calldata names - ) external onlyRole(DOCUMENT_MANAGER_ROLE) { - if ( - smartContracts.length == 0 || - (smartContracts.length != names.length) - ) { - revert InvalidInputLength(); - } - - for (uint256 i = 0; i < smartContracts.length; ++i) { - _removeDocument(smartContracts[i], names[i]); - } - } - - /** - * @notice Batch version of removeDocument to handle multiple documents at once - */ - function batchRemoveDocuments( - address smartContract, - bytes32[] calldata names - ) external onlyRole(DOCUMENT_MANAGER_ROLE) { - if (names.length == 0) { - revert InvalidInputLength(); - } - - for (uint256 i = 0; i < names.length; ++i) { - _removeDocument(smartContract, names[i]); - } - } - - /** - * @notice Public function to get a document from msg.sender - */ - function getDocument( - bytes32 name_ - ) external view override returns (string memory, bytes32, uint256) { - return _getDocument(msg.sender, name_); - } - - /** - * @notice Public function to get a document for a specific contract address - */ - function getDocument( - address smartContract, - bytes32 name_ - ) external view returns (string memory, bytes32, uint256) { - return _getDocument(smartContract, name_); - } - - /** - * @notice Get all document names for msg.sender - */ - function getAllDocuments() - external + function hasRole(bytes32 role, address account) + public view - override - returns (bytes32[] memory) + virtual + override(AccessControl, IAccessControl) + returns (bool) { - return _documentNames[msg.sender]; - } - - /** - * @notice Get all document names for a specific smart contract - */ - function getAllDocuments( - address smartContract - ) external view returns (bytes32[] memory) { - return _documentNames[smartContract]; - } - - /* ============ ACCESS CONTROL ============ */ - /* - * @dev Returns `true` if `account` has been granted `role`. - */ - function hasRole( - bytes32 role, - address account - ) public view virtual override returns (bool) { // The Default Admin has all roles - if (AccessControl.hasRole(DEFAULT_ADMIN_ROLE, account)) { + if (super.hasRole(DEFAULT_ADMIN_ROLE, account)) { return true; } - return AccessControl.hasRole(role, account); - } - - /*////////////////////////////////////////////////////////////// - INTERNAL FUNCTIONS - //////////////////////////////////////////////////////////////*/ - - /** - * @dev Internal function to fetch a document - */ - function _getDocument( - address smartContract, - bytes32 name_ - ) internal view returns (string memory, bytes32, uint256) { - Document memory doc = _documents[smartContract][name_]; - return (doc.uri, doc.documentHash, doc.lastModified); + return super.hasRole(role, account); } /** - * @dev Internal helper to remove the document name from the list of document names + * @dev ERC-165 discovery: advertises ERC-1643 and its multi-token extension, + * plus the version module (ERC-8303) and `AccessControlEnumerable`. + * The engine implements the base single-argument functions, so it advertises + * `type(IERC1643).interfaceId`; it also implements the address-scoped + * extension, so it advertises `type(IERC1643MultiDocument).interfaceId`. + * See {IERC165-supportsInterface}. */ - function _removeDocumentName( - address smartContract, - bytes32 name_ - ) internal { - uint256 length = _documentNames[smartContract].length; - for (uint256 i = 0; i < length; ++i) { - if (_documentNames[smartContract][i] == name_) { - _documentNames[smartContract][i] = _documentNames[ - smartContract - ][length - 1]; - _documentNames[smartContract].pop(); - break; - } - } - } - - function _removeDocument(address smartContract, bytes32 name_) internal { - Document memory doc = _documents[smartContract][name_]; - emit DocumentRemoved(smartContract, name_, doc.uri, doc.documentHash); - - delete _documents[smartContract][name_]; - _removeDocumentName(smartContract, name_); - } - - function _setDocument( - address smartContract, - bytes32 name_, - string memory uri_, - bytes32 documentHash_ - ) internal { - Document storage doc = _documents[smartContract][name_]; - if (doc.lastModified == 0) { - // new document - _documentNames[smartContract].push(name_); - } - doc.uri = uri_; - doc.documentHash = documentHash_; - doc.lastModified = block.timestamp; - emit DocumentUpdated(smartContract, name_, uri_, documentHash_); + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(VersionModule, AccessControlEnumerable) + returns (bool) + { + return interfaceId == type(IERC1643).interfaceId || interfaceId == type(IERC1643MultiDocument).interfaceId + || interfaceId == type(ITokenBinding).interfaceId || super.supportsInterface(interfaceId); } /*////////////////////////////////////////////////////////////// @@ -261,36 +96,21 @@ contract DocumentEngine is /** * @dev This surcharge is not necessary if you do not use ERC2771 */ - function _msgSender() - internal - view - override(ERC2771Context, Context) - returns (address sender) - { + function _msgSender() internal view override(ERC2771Context, Context) returns (address sender) { return ERC2771Context._msgSender(); } /** * @dev This surcharge is not necessary if you do not use ERC2771 */ - function _msgData() - internal - view - override(ERC2771Context, Context) - returns (bytes calldata) - { + function _msgData() internal view override(ERC2771Context, Context) returns (bytes calldata) { return ERC2771Context._msgData(); } /** * @dev This surcharge is not necessary if you do not use the MetaTxModule */ - function _contextSuffixLength() - internal - view - override(ERC2771Context, Context) - returns (uint256) - { + function _contextSuffixLength() internal view override(ERC2771Context, Context) returns (uint256) { return ERC2771Context._contextSuffixLength(); } } diff --git a/src/DocumentEngineBase.sol b/src/DocumentEngineBase.sol new file mode 100644 index 0000000..018b563 --- /dev/null +++ b/src/DocumentEngineBase.sol @@ -0,0 +1,288 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import "OZ/utils/Context.sol"; +import "CMTAT/interfaces/tokenization/draft-IERC1643.sol"; +import {IERC1643MultiDocument} from "./interfaces/IERC1643MultiDocument.sol"; +import "./DocumentEngineInvariant.sol"; + +/** + * @title DocumentEngineBase + * @notice Document management logic (ERC-1643) for several smart contracts. + * @dev This abstract base holds the document storage and all the + * document-management functions, but it is **agnostic to the access-control + * implementation**. Authorization is delegated to the abstract hooks + * {_authorizeDocumentManagement} and {_authorizeBoundTokenDocumentManagement} + * (through the `onlyDocumentManager` / `onlyBoundToken` modifiers), which a + * deployment contract must implement (see {DocumentEngine}). + * + * This separation (base logic + deployment-defined access control) follows the + * CMTAT and CMTA/RuleEngine pattern. + */ +abstract contract DocumentEngineBase is IERC1643, IERC1643MultiDocument, DocumentEngineInvariant, Context { + // Mapping from contract addresses to document names to their corresponding Document structs + mapping(address => mapping(bytes32 => Document)) private _documents; + mapping(address => bytes32[]) private _documentNames; + + /*////////////////////////////////////////////////////////////// + ACCESS CONTROL (hooks) + //////////////////////////////////////////////////////////////*/ + + /** + * @dev Restricts a function to accounts allowed to manage documents on + * behalf of any smart contract (admin path). Delegates the authorization + * to {_authorizeDocumentManagement} so that the document-management + * implementation stays separate from the access-control logic. + */ + modifier onlyDocumentManager() { + _authorizeDocumentManagement(); + _; + } + + /** + * @dev Restricts a function to tokens bound to this engine, letting them + * manage their own documents (bound-token path). Delegates to + * {_authorizeBoundTokenDocumentManagement}. + */ + modifier onlyBoundToken() { + _authorizeBoundTokenDocumentManagement(); + _; + } + + /** + * @dev Authorization hook for the admin document-management path. + * Implemented by the deployment contract (e.g. a role check). + */ + function _authorizeDocumentManagement() internal view virtual; + + /** + * @dev Authorization hook for the bound-token document-management path. + * Implemented by the deployment contract (e.g. a role check). + */ + function _authorizeBoundTokenDocumentManagement() internal view virtual; + + /*////////////////////////////////////////////////////////////// + PUBLIC/EXTERNAL FUNCTIONS + //////////////////////////////////////////////////////////////*/ + + /** + * @notice Restricted function to set or update a document + */ + function setDocument(address subject, bytes32 name_, string memory uri_, bytes32 documentHash_) + public + override + onlyDocumentManager + { + _setDocument(subject, name_, uri_, documentHash_); + } + + /** + * @notice Restricted function to remove a document for a given smart contract and name + */ + function removeDocument(address subject, bytes32 name_) external override onlyDocumentManager { + _removeDocument(subject, name_); + } + + /* ============ ERC-1643 (bound token) ============ */ + + /** + * @notice ERC-1643 function to set or update a document for the caller. + * @dev The document is stored under the caller (`_msgSender()`) namespace. + * Restricted by the `onlyBoundToken` hook: the caller must be a token bound to + * this engine. How a token is bound is deployment-specific (see the + * {_authorizeBoundTokenDocumentManagement} implementations). A bound token can + * only manage its own documents; it can never affect another contract's documents. + */ + function setDocument(bytes32 name_, string calldata uri_, bytes32 documentHash_) external override onlyBoundToken { + _setDocument(_msgSender(), name_, uri_, documentHash_); + } + + /** + * @notice ERC-1643 function to remove a document for the caller. + * @dev See {setDocument}. Scoped to the caller (`_msgSender()`) namespace. + */ + function removeDocument(bytes32 name_) external override onlyBoundToken { + _removeDocument(_msgSender(), name_); + } + + /** + * @notice Batch version of setDocument to handle multiple documents at once + */ + function batchSetDocuments( + address[] calldata subjects, + bytes32[] calldata names, + string[] calldata uris, + bytes32[] calldata hashes + ) external onlyDocumentManager { + if ( + subjects.length == 0 || subjects.length != names.length || names.length != uris.length + || uris.length != hashes.length + ) { + revert InvalidInputLength(); + } + uint256 length = subjects.length; + for (uint256 i = 0; i < length; ++i) { + _setDocument(subjects[i], names[i], uris[i], hashes[i]); + } + } + + /** + * @notice Batch version of setDocument to handle multiple documents at once + */ + function batchSetDocuments( + address subject, + bytes32[] calldata names, + string[] calldata uris, + bytes32[] calldata hashes + ) external onlyDocumentManager { + if (names.length == 0 || names.length != uris.length || uris.length != hashes.length) { + revert InvalidInputLength(); + } + uint256 length = names.length; + for (uint256 i = 0; i < length; ++i) { + _setDocument(subject, names[i], uris[i], hashes[i]); + } + } + + /** + * @notice Batch version of removeDocument to handle multiple documents at once + */ + function batchRemoveDocuments(address[] calldata subjects, bytes32[] calldata names) external onlyDocumentManager { + if (subjects.length == 0 || (subjects.length != names.length)) { + revert InvalidInputLength(); + } + + uint256 length = subjects.length; + for (uint256 i = 0; i < length; ++i) { + _removeDocument(subjects[i], names[i]); + } + } + + /** + * @notice Batch version of removeDocument to handle multiple documents at once + */ + function batchRemoveDocuments(address subject, bytes32[] calldata names) external onlyDocumentManager { + if (names.length == 0) { + revert InvalidInputLength(); + } + + uint256 length = names.length; + for (uint256 i = 0; i < length; ++i) { + _removeDocument(subject, names[i]); + } + } + + /** + * @notice ERC-1643 function to get a document for the caller (`_msgSender()`) + * @dev Returns the three fields as flat values, matching the ERC-1643 ABI. The `Document` + * struct is kept for storage only: returning it would prepend a struct offset word to the + * returndata, so a consumer decoding per the ERC-1643 signature would silently mis-decode. + */ + function getDocument(bytes32 name_) + external + view + override + returns (string memory uri, bytes32 documentHash, uint256 lastModified) + { + return _getDocument(_msgSender(), name_); + } + + /** + * @notice Public function to get a document for a specific contract address + * @dev Flat return, see {getDocument(bytes32)}. + */ + function getDocument(address subject, bytes32 name_) + external + view + override + returns (string memory uri, bytes32 documentHash, uint256 lastModified) + { + return _getDocument(subject, name_); + } + + /** + * @notice Get all document names for msg.sender + */ + function getAllDocuments() external view override returns (bytes32[] memory) { + return _documentNames[_msgSender()]; + } + + /** + * @notice Get all document names for a specific smart contract + */ + function getAllDocuments(address subject) external view override returns (bytes32[] memory) { + return _documentNames[subject]; + } + + /*////////////////////////////////////////////////////////////// + INTERNAL FUNCTIONS + //////////////////////////////////////////////////////////////*/ + + /** + * @dev Internal function to fetch a document, as flat values + */ + function _getDocument(address subject, bytes32 name_) + internal + view + returns (string memory uri, bytes32 documentHash, uint256 lastModified) + { + Document storage doc = _documents[subject][name_]; + return (doc.uri, doc.documentHash, doc.lastModified); + } + + /** + * @dev Internal helper to remove the document name from the list of document names + */ + function _removeDocumentName(address subject, bytes32 name_) internal { + uint256 length = _documentNames[subject].length; + for (uint256 i = 0; i < length; ++i) { + if (_documentNames[subject][i] == name_) { + _documentNames[subject][i] = _documentNames[subject][length - 1]; + _documentNames[subject].pop(); + break; + } + } + } + + function _removeDocument(address subject, bytes32 name_) internal { + Document memory doc = _documents[subject][name_]; + // ERC-1643: reverts when the named document does not exist + if (doc.lastModified == 0) { + revert ERC1643MissingDocument(); + } + + // This engine is a shared, multi-subject manager: per the ERC-1643 + // "Emission Responsibility" rules it emits only the address-carrying + // extension event (the base `DocumentRemoved` is the token contract's + // responsibility). See doc/ERCSpecification. + emit DocumentRemovedForSubject(subject, name_, doc.uri, doc.documentHash); + + delete _documents[subject][name_]; + _removeDocumentName(subject, name_); + } + + function _setDocument(address subject, bytes32 name_, string memory uri_, bytes32 documentHash_) internal { + // Multi-token guard: `subject` must be a real contract address, never the + // null namespace. (The bound-token path passes `_msgSender()`, never zero.) + if (subject == address(0)) { + revert MultiDocumentInvalidSubject(); + } + // ERC-1643: reject the null name (ambiguous / default key) + if (name_ == bytes32(0)) { + revert ERC1643InvalidName(); + } + + Document storage doc = _documents[subject][name_]; + if (doc.lastModified == 0) { + // new document + _documentNames[subject].push(name_); + } + doc.uri = uri_; + doc.documentHash = documentHash_; + doc.lastModified = block.timestamp; + + // Shared, multi-subject manager: emit only the address-carrying extension + // event (see {_removeDocument} note and doc/ERCSpecification). + emit DocumentUpdatedForSubject(subject, name_, uri_, documentHash_); + } +} diff --git a/src/DocumentEngineInvariant.sol b/src/DocumentEngineInvariant.sol index b3836d8..81e718b 100644 --- a/src/DocumentEngineInvariant.sol +++ b/src/DocumentEngineInvariant.sol @@ -1,31 +1,25 @@ //SPDX-License-Identifier: MPL-2.0 pragma solidity ^0.8.20; -contract DocumentEngineInvariant { - error DocumentNotFound(address smartContract, bytes32 name); +/** + * @title DocumentEngineInvariant + * @notice Shared errors for the DocumentEngine, common to every deployment + * regardless of its access-control model. + * @dev Access-control specifics (roles, owner, ...) are intentionally NOT + * defined here; they belong to the deployment contract (e.g. the role constants + * live in {DocumentEngine}, the owner logic in {DocumentEngineOwnable}). This + * contract is only ever used as a base, never deployed on its own. + */ +abstract contract DocumentEngineInvariant { error InvalidInputLength(); error AdminWithAddressZeroNotAllowed(); - event DocumentUpdated( - address smartContract, - bytes32 name, - string uri, - bytes32 documentHash - ); - event DocumentRemoved( - address smartContract, - bytes32 name, - string uri, - bytes32 documentHash - ); - - // Document structure - struct Document { - string uri; - bytes32 documentHash; - uint256 lastModified; - } - - bytes32 public constant DOCUMENT_MANAGER_ROLE = - keccak256("DOCUMENT_MANAGER_ROLE"); + // Only errors that no interface defines belong here. Every specification error is declared by + // the interface that defines its condition, so that an ABI generated from the interface carries + // it and a contract implementing several interfaces obtains each error exactly once — the + // multi-subject draft's "MUST NOT declare them twice", which the compiler also enforces: + // - `ERC1643InvalidName()` / `ERC1643MissingDocument()` → `IERC1643` (since CMTAT v3.3.0-rc2) + // - `MultiDocumentInvalidSubject()` → `IERC1643MultiDocument` + // - `NotBoundToken(address)` → `ITokenBinding` + // - `TokenBindingInvalidToken()` → `ITokenBinding` } diff --git a/src/DocumentEngineOwnable.sol b/src/DocumentEngineOwnable.sol new file mode 100644 index 0000000..e8227cc --- /dev/null +++ b/src/DocumentEngineOwnable.sol @@ -0,0 +1,76 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import {Ownable} from "OZ/access/Ownable.sol"; +import {Ownable2Step} from "OZ/access/Ownable2Step.sol"; +import {IERC1643} from "CMTAT/interfaces/tokenization/draft-IERC1643.sol"; +import {IERC1643MultiDocument} from "./interfaces/IERC1643MultiDocument.sol"; +import {ITokenBinding} from "./interfaces/ITokenBinding.sol"; +import "OZ/metatx/ERC2771Context.sol"; +import "./modules/TokenBindingModule.sol"; +import "./modules/VersionModule.sol"; + +/** + * @title DocumentEngineOwnable + * @notice Alternative deployment of the DocumentEngine that uses a single owner + * ({Ownable2Step}) instead of role-based access control. + * @dev Reuses the same document-management logic ({DocumentEngineBase}) and token + * binding ({TokenBindingModule}), swapping only the access-control implementation: + * document management and token binding are both restricted to the `owner`, and a + * bound token manages only its own documents. Ownership uses the two-step transfer + * flow for safety, and the contract also exposes its version through ERC-8303 + * ({VersionModule}) and wires ERC-2771. + */ +contract DocumentEngineOwnable is TokenBindingModule, VersionModule, Ownable2Step, ERC2771Context { + /** + * @param owner_ initial owner of the contract + * @param forwarderIrrevocable address of the ERC-2771 forwarder (gasless support) + */ + constructor(address owner_, address forwarderIrrevocable) Ownable(owner_) ERC2771Context(forwarderIrrevocable) {} + + /*////////////////////////////////////////////////////////////// + ACCESS CONTROL (implementation) + //////////////////////////////////////////////////////////////*/ + + /** + * @dev Authorization for the admin document-management path (and, via + * {TokenBindingModule}, for token binding): only the owner. + */ + function _authorizeDocumentManagement() internal view virtual override { + _checkOwner(); + } + + /** + * @dev ERC-165 discovery: advertises ERC-1643 and its multi-token extension, + * plus the version module (ERC-8303). See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override(VersionModule) returns (bool) { + return interfaceId == type(IERC1643).interfaceId || interfaceId == type(IERC1643MultiDocument).interfaceId + || interfaceId == type(ITokenBinding).interfaceId || super.supportsInterface(interfaceId); + } + + /*////////////////////////////////////////////////////////////// + ERC2771 + //////////////////////////////////////////////////////////////*/ + + /** + * @dev This surcharge is not necessary if you do not use ERC2771 + */ + function _msgSender() internal view override(ERC2771Context, Context) returns (address sender) { + return ERC2771Context._msgSender(); + } + + /** + * @dev This surcharge is not necessary if you do not use ERC2771 + */ + function _msgData() internal view override(ERC2771Context, Context) returns (bytes calldata) { + return ERC2771Context._msgData(); + } + + /** + * @dev This surcharge is not necessary if you do not use the MetaTxModule + */ + function _contextSuffixLength() internal view override(ERC2771Context, Context) returns (uint256) { + return ERC2771Context._contextSuffixLength(); + } +} diff --git a/src/interfaces/IERC1643MultiDocument.sol b/src/interfaces/IERC1643MultiDocument.sol new file mode 100644 index 0000000..9be7e28 --- /dev/null +++ b/src/interfaces/IERC1643MultiDocument.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +/** + * @title IERC1643MultiDocument — optional multi-token ERC-1643 extension + * @notice Address-scoped document management for a contract that manages + * documents on behalf of several `subject` contracts. + * @dev Declared **independently of `IERC1643`** (it does not inherit it), so a + * shared management contract can implement the address-scoped surface without + * being forced to implement the base single-argument functions. `subject` is the + * address of the contract the documents belong to (typically a token contract, + * but the reasoning applies to any ERC-721/ERC-1155 token, vault, or other + * on-chain product). See `doc/ERCSpecification/erc-draft_multi_document_management.md`. + */ +interface IERC1643MultiDocument { + /// @notice Reverts when `setDocument` or `removeDocument` is called with `subject == address(0)`. + /// @dev Specific to this proposal; it has no ERC-1643 counterpart, because ERC-1643's + /// `setDocument` has no `subject` argument — its subject is implicitly the contract itself, + /// which is never the null address. Named after the proposal that defines the condition, not + /// after one in which the condition cannot occur; the two errors this interface shares with + /// ERC-1643 (`ERC1643InvalidName`, `ERC1643MissingDocument`) keep their prefix for the opposite + /// reason, and are declared by `IERC1643`, never here. + error MultiDocumentInvalidSubject(); + + /// @notice Returns metadata for the document `name` belonging to `subject`. + /// @dev Returns the three fields as flat values, matching the specification ABI. A missing + /// document yields empty values (`""`, `bytes32(0)`, `0`) and does not revert. + /// @return uri Document location. + /// @return documentHash Hash of the document contents. + /// @return lastModified Last update timestamp. + function getDocument(address subject, bytes32 name) + external + view + returns (string memory uri, bytes32 documentHash, uint256 lastModified); + + /// @notice Returns all document names currently tracked for `subject`. + function getAllDocuments(address subject) external view returns (bytes32[] memory documentNames); + + /// @notice Creates or updates a document entry for `subject`. + /// @dev MUST emit {DocumentUpdatedForSubject} on success. + function setDocument(address subject, bytes32 name, string calldata uri, bytes32 documentHash) external; + + /// @notice Removes an existing document entry for `subject`. + /// @dev MUST emit {DocumentRemovedForSubject} on success. + function removeDocument(address subject, bytes32 name) external; + + /// @notice Emitted when a document is created or updated for `subject`. + event DocumentUpdatedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); + + /// @notice Emitted when a document is removed for `subject`. + event DocumentRemovedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); +} diff --git a/src/interfaces/IERC8303.sol b/src/interfaces/IERC8303.sol new file mode 100644 index 0000000..a52a77c --- /dev/null +++ b/src/interfaces/IERC8303.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +/** + * @title IERC8303 - Contract Version + * @notice Interface for exposing a contract implementation version string. + * @dev ERC-8303 (Draft) — https://ethereum-magicians.org/t/erc-8303-contract-version/28795 + * The interface id is `0x54fd4d50` (the `version()` selector). + */ +interface IERC8303 { + /// @notice Returns the implementation version string. + /// @return The version value, for example "1.0.0". + function version() external view returns (string memory); +} diff --git a/src/interfaces/ITokenBinding.sol b/src/interfaces/ITokenBinding.sol new file mode 100644 index 0000000..67cb940 --- /dev/null +++ b/src/interfaces/ITokenBinding.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +/** + * @title ITokenBinding + * @notice Common token-binding surface shared by every DocumentEngine deployment, + * so integrators bind, unbind and query a token the same way regardless of the + * underlying access-control model (role-based or owner-based). + * @dev A *bound* token is allowed to manage its own documents through the standard + * single-argument ERC-1643 functions (`msg.sender` is the token). Binding is a + * privileged operation; the exact authorization (a role, the owner, ...) and the + * revert raised when a non-bound caller attempts a write are deployment-specific. + */ +interface ITokenBinding { + /// @notice Emitted when a token is bound (`bound = true`) or unbound (`bound = false`). + /// @dev Emitted only when the binding actually changes, so the event stream contains no + /// no-op entries and an indexer can replay it as a sequence of transitions. + event TokenBindingSet(address indexed token, bool bound); + + /// @notice Thrown when a binding operation targets the null address. + error TokenBindingInvalidToken(); + + /// @notice Binds `token`, allowing it to manage its own documents. + /// @dev Idempotent: binding an already-bound token succeeds and emits nothing. + /// Reverts {TokenBindingInvalidToken} when `token` is the null address. + function bindToken(address token) external; + + /// @notice Unbinds `token`. + /// @dev Idempotent: unbinding a token that is not bound succeeds and emits nothing. + /// Reverts {TokenBindingInvalidToken} when `token` is the null address. + function unbindToken(address token) external; + + /// @notice Returns whether `token` is currently bound. + function isTokenBound(address token) external view returns (bool); +} diff --git a/src/modules/TokenBindingModule.sol b/src/modules/TokenBindingModule.sol new file mode 100644 index 0000000..a65596b --- /dev/null +++ b/src/modules/TokenBindingModule.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import {DocumentEngineBase} from "../DocumentEngineBase.sol"; +import {ITokenBinding} from "../interfaces/ITokenBinding.sol"; + +/** + * @title TokenBindingModule + * @notice Shared token-binding registry (an allowlist) implementing {ITokenBinding}, + * used by every DocumentEngine deployment so binding behaves identically — same + * functions, same event, same revert — regardless of the access-control model. + * @dev A *bound* token may manage its own documents through the standard + * single-argument ERC-1643 functions (`msg.sender` is the token). This module: + * - stores the allowlist and implements `bindToken` / `unbindToken` / `isTokenBound`; + * - wires the base bound-token hook ({_authorizeBoundTokenDocumentManagement}) to + * the allowlist ({_checkTokenBound}); + * - gates binding management with the deployment's document-management + * authorization ({_authorizeDocumentManagement}), so whoever may manage + * documents may also decide bindings. It is therefore access-control agnostic: + * the deployment only implements {_authorizeDocumentManagement}. + */ +abstract contract TokenBindingModule is DocumentEngineBase, ITokenBinding { + /// @dev Tokens bound to the engine, allowed to manage their own documents. + mapping(address => bool) private _boundTokens; + + /// @notice Thrown when a non-bound caller attempts a bound-token operation. + error NotBoundToken(address caller); + + /** + * @inheritdoc ITokenBinding + * @dev Authorized by the deployment's document-management check. + */ + function bindToken(address token) external virtual override { + _authorizeDocumentManagement(); + _setTokenBinding(token, true); + } + + /** + * @inheritdoc ITokenBinding + * @dev Authorized by the deployment's document-management check. + */ + function unbindToken(address token) external virtual override { + _authorizeDocumentManagement(); + _setTokenBinding(token, false); + } + + /** + * @dev Shared bind/unbind implementation. + * + * Rejects the null address: `address(0)` can never call the engine, so binding it grants + * nothing, but it would still emit a {TokenBindingSet} that off-chain indexers key on — the + * same data-integrity argument the multi-subject draft makes for rejecting a null `subject`. + * + * Writing and emitting only on an actual change makes both functions idempotent and keeps the + * event stream free of no-op entries, so an indexer can treat every {TokenBindingSet} as a real + * transition rather than having to de-duplicate. The repeated call still succeeds, since the + * caller's intent — "this token is (not) bound" — already holds. + */ + function _setTokenBinding(address token, bool bound) internal { + if (token == address(0)) { + revert TokenBindingInvalidToken(); + } + if (_boundTokens[token] == bound) { + return; + } + _boundTokens[token] = bound; + emit TokenBindingSet(token, bound); + } + + /// @inheritdoc ITokenBinding + function isTokenBound(address token) public view virtual override returns (bool) { + return _boundTokens[token]; + } + + /** + * @dev Bound-token document-management authorization: the caller + * (`_msgSender()`) must be a bound token. + */ + function _authorizeBoundTokenDocumentManagement() internal view virtual override { + _checkTokenBound(); + } + + /// @dev Reverts {NotBoundToken} if the caller (`_msgSender()`) is not bound. + function _checkTokenBound() internal view { + if (!_boundTokens[_msgSender()]) { + revert NotBoundToken(_msgSender()); + } + } +} diff --git a/src/modules/VersionModule.sol b/src/modules/VersionModule.sol new file mode 100644 index 0000000..e0957f6 --- /dev/null +++ b/src/modules/VersionModule.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import {ERC165} from "OZ/utils/introspection/ERC165.sol"; +import {IERC8303} from "../interfaces/IERC8303.sol"; + +/** + * @title VersionModule + * @notice Exposes the current contract version through ERC-8303 (`version()`), + * with optional ERC-165 interface discovery. + * @dev Implements ERC-8303 (Draft). The version string is defined here so the + * version concern is isolated in a dedicated module (CMTAT pattern). A deployment + * contract that also implements ERC-165 must combine this module's + * {supportsInterface} with the others it inherits. + */ +abstract contract VersionModule is IERC8303, ERC165 { + /** + * @notice Get the current version of the smart contract. + * @dev Follows Semantic Versioning 2.0.0 (`MAJOR.MINOR.PATCH`). + */ + string public constant VERSION = "0.4.0"; + + /** + * @inheritdoc IERC8303 + */ + function version() public view virtual override(IERC8303) returns (string memory version_) { + return VERSION; + } + + /** + * @dev Advertises ERC-8303 support (interface id `0x54fd4d50`). + * See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(IERC8303).interfaceId || super.supportsInterface(interfaceId); + } +} diff --git a/test/Deploy.t.sol b/test/Deploy.t.sol new file mode 100644 index 0000000..815f1cd --- /dev/null +++ b/test/Deploy.t.sol @@ -0,0 +1,78 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import "forge-std/Test.sol"; +import {DeployDocumentEngine} from "../script/DeployDocumentEngine.s.sol"; +import {DeployDocumentEngineOwnable} from "../script/DeployDocumentEngineOwnable.s.sol"; +import {DocumentEngine} from "../src/DocumentEngine.sol"; +import {DocumentEngineOwnable} from "../src/DocumentEngineOwnable.sol"; + +contract DeployDocumentEngineTest is Test { + DeployDocumentEngine internal deployer; + address internal admin = makeAddr("admin"); + address internal forwarder = makeAddr("forwarder"); + + function setUp() public { + deployer = new DeployDocumentEngine(); + } + + function testDeploySetsAdminAndForwarder() public { + DocumentEngine engine = deployer.deploy(admin, forwarder); + + assertTrue(engine.hasRole(engine.DEFAULT_ADMIN_ROLE(), admin)); + assertTrue(engine.isTrustedForwarder(forwarder)); + assertEq(engine.version(), "0.4.0"); + } + + function testDeployWithoutForwarder() public { + DocumentEngine engine = deployer.deploy(admin, address(0)); + + assertTrue(engine.hasRole(engine.DEFAULT_ADMIN_ROLE(), admin)); + assertFalse(engine.isTrustedForwarder(forwarder)); + } + + function testRunReadsEnv() public { + vm.setEnv("DOCUMENT_ENGINE_ADMIN", vm.toString(admin)); + vm.setEnv("DOCUMENT_ENGINE_FORWARDER", vm.toString(forwarder)); + + DocumentEngine engine = deployer.run(); + + assertTrue(engine.hasRole(engine.DEFAULT_ADMIN_ROLE(), admin)); + assertTrue(engine.isTrustedForwarder(forwarder)); + } +} + +contract DeployDocumentEngineOwnableTest is Test { + DeployDocumentEngineOwnable internal deployer; + address internal owner = makeAddr("owner"); + address internal forwarder = makeAddr("forwarder"); + + function setUp() public { + deployer = new DeployDocumentEngineOwnable(); + } + + function testDeploySetsOwnerAndForwarder() public { + DocumentEngineOwnable engine = deployer.deploy(owner, forwarder); + + assertEq(engine.owner(), owner); + assertTrue(engine.isTrustedForwarder(forwarder)); + assertEq(engine.version(), "0.4.0"); + } + + function testDeployWithoutForwarder() public { + DocumentEngineOwnable engine = deployer.deploy(owner, address(0)); + + assertEq(engine.owner(), owner); + assertFalse(engine.isTrustedForwarder(forwarder)); + } + + function testRunReadsEnv() public { + vm.setEnv("DOCUMENT_ENGINE_OWNER", vm.toString(owner)); + vm.setEnv("DOCUMENT_ENGINE_FORWARDER", vm.toString(forwarder)); + + DocumentEngineOwnable engine = deployer.run(); + + assertEq(engine.owner(), owner); + assertTrue(engine.isTrustedForwarder(forwarder)); + } +} diff --git a/test/DocumentEngine.t.sol b/test/DocumentEngine.t.sol index fda9890..9ea7d81 100644 --- a/test/DocumentEngine.t.sol +++ b/test/DocumentEngine.t.sol @@ -5,7 +5,40 @@ import "forge-std/Test.sol"; import "../src/DocumentEngine.sol"; import "../src/DocumentEngineInvariant.sol"; import "OZ/access/AccessControl.sol"; -import "CMTAT/CMTAT_STANDALONE.sol"; +import {IERC165} from "OZ/utils/introspection/IERC165.sol"; +import {IERC8303} from "../src/interfaces/IERC8303.sol"; +import {IERC1643MultiDocument} from "../src/interfaces/IERC1643MultiDocument.sol"; +import {ITokenBinding} from "../src/interfaces/ITokenBinding.sol"; +import {TokenBindingModule} from "../src/modules/TokenBindingModule.sol"; +import {DocumentEngineModule} from "CMTAT/modules/wrapper/options/DocumentEngineModule.sol"; + +/** + * @dev Minimal token wired to CMTAT's official `DocumentEngineModule`. + * + * Since CMTAT v3, the shipped standalone tokens store documents on-chain + * (`DocumentERC1643Module`) and no longer consume an external document engine + * through their constructor. Integration with an external `DocumentEngine` now + * goes through `DocumentEngineModule`, which this mock exercises with real + * CMTAT code: reads/writes are forwarded to the engine keyed by `msg.sender`. + */ +contract CMTATDocumentEngineMock is DocumentEngineModule { + // No access restriction for the mock: document management is authorized for anyone. + function _authorizeDocumentManagement() internal override {} +} + +/** + * @dev Demonstrates the flexible access control: overriding the authorization + * hook opens the admin document-management path to anyone, without touching the + * document-management implementation. + */ +contract OpenDocumentEngine is DocumentEngine { + constructor(address admin, address forwarder) DocumentEngine(admin, forwarder) {} + + function _authorizeDocumentManagement() internal view override { + // no access restriction (custom authorization) + } +} + contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { DocumentEngine public documentEngine; address public admin = address(0x1); @@ -17,45 +50,41 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { string public documentURI = "https://example.com/doc1"; bytes32 public documentHash = keccak256("doc1Hash"); bytes32 public constant DOCUMENT_ROLE = keccak256("DOCUMENT_ROLE"); + // Roles are defined on the role-based deployment (DocumentEngine), not on the + // shared DocumentEngineInvariant; mirrored here for the assertions. + bytes32 public constant DOCUMENT_MANAGER_ROLE = keccak256("DOCUMENT_MANAGER_ROLE"); address AddressZero = address(0); - CMTAT_STANDALONE cmtat; + + // Local copies of the extension events, so `vm.expectEmit` can emit and match them. + event DocumentUpdatedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); + event DocumentRemovedForSubject(address indexed subject, bytes32 indexed name, string uri, bytes32 documentHash); + event TokenBindingSet(address indexed token, bool bound); + // Base ERC-1643 event signatures (this shared engine must NOT emit them). + bytes32 internal constant BASE_UPDATED_SIG = keccak256("DocumentUpdated(bytes32,string,bytes32)"); + bytes32 internal constant BASE_REMOVED_SIG = keccak256("DocumentRemoved(bytes32,string,bytes32)"); + + /** + * @dev Since CMTAT `v3.3.0-rc2`, `getDocument` returns the three ERC-1643 fields as flat + * values instead of a `Document` struct. These helpers repack them so the assertions below + * stay readable; {testGetDocumentReturnsFlatErc1643Abi} pins the wire format itself. + */ + function _doc(IERC1643MultiDocument engine_, address subject, bytes32 name_) + internal + view + returns (IERC1643.Document memory document) + { + (document.uri, document.documentHash, document.lastModified) = engine_.getDocument(subject, name_); + } + + /// @dev See {_doc(IERC1643MultiDocument,address,bytes32)}; caller-scoped ERC-1643 read. + function _doc(IERC1643 engine_, bytes32 name_) internal view returns (IERC1643.Document memory document) { + (document.uri, document.documentHash, document.lastModified) = engine_.getDocument(name_); + } + function setUp() public { documentEngine = new DocumentEngine(admin, AddressZero); vm.prank(admin); - documentEngine.setDocument( - testContract, - documentName, - documentURI, - documentHash - ); - - // CMTAT - ICMTATConstructor.ERC20Attributes - memory erc20Attributes = ICMTATConstructor.ERC20Attributes( - "CMTA Token", - "CMTAT", - 0 - ); - ICMTATConstructor.BaseModuleAttributes - memory baseModuleAttributes = ICMTATConstructor - .BaseModuleAttributes( - "CMTAT_ISIN", - "https://cmta.ch", - "CMTAT_info" - ); - ICMTATConstructor.Engine memory engines = ICMTATConstructor.Engine( - IRuleEngine(AddressZero), - IDebtEngine(AddressZero), - IAuthorizationEngine(AddressZero), - IERC1643(AddressZero) - ); - cmtat = new CMTAT_STANDALONE( - AddressZero, - admin, - erc20Attributes, - baseModuleAttributes, - engines - ); + documentEngine.setDocument(testContract, documentName, documentURI, documentHash); } /*////////////////////////////////////////////////////////////// @@ -69,9 +98,7 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { // Forwarder assertEq(documentEngine.isTrustedForwarder(forwarder), true); // admin - vm.expectRevert( - abi.encodeWithSelector(AdminWithAddressZeroNotAllowed.selector) - ); + vm.expectRevert(abi.encodeWithSelector(AdminWithAddressZeroNotAllowed.selector)); documentEngine = new DocumentEngine(AddressZero, forwarder); } @@ -82,28 +109,15 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { function testCannotNonAdminSetDocument() public { vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) - ); - documentEngine.setDocument( - testContract, - documentName, - documentURI, - documentHash + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); + documentEngine.setDocument(testContract, documentName, documentURI, documentHash); } function testCannotNonAdminRemoveDocument() public { vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); documentEngine.removeDocument(testContract, documentName); } @@ -127,21 +141,13 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); documentEngine.batchSetDocuments(smartContracts, names, uris, hashes); vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); documentEngine.batchSetDocuments(testContract, names, uris, hashes); } @@ -157,58 +163,286 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); documentEngine.batchRemoveDocuments(smartContracts, names); vm.prank(attacker); vm.expectRevert( - abi.encodeWithSelector( - AccessControlUnauthorizedAccount.selector, - attacker, - DOCUMENT_MANAGER_ROLE - ) + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) ); documentEngine.batchRemoveDocuments(testContract, names); } /*////////////////////////////////////////////////////////////// - Get + Get //////////////////////////////////////////////////////////////*/ - function testGetAllDocuments() public view { + function testGetAllDocuments() public { bytes32[] memory docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 1); assertEq(docs[0], documentName); } + /*////////////////////////////////////////////////////////////// + CMTAT integration (external engine via DocumentEngineModule) + //////////////////////////////////////////////////////////////*/ + function testCanReturnCMTATDocument() public { - // Arrange + // Arrange: a CMTAT-style token bound to the engine + CMTATDocumentEngineMock cmtat = new CMTATDocumentEngineMock(); + cmtat.setDocumentEngine(documentEngine); + uint256 lastModif = block.timestamp; vm.prank(admin); - documentEngine.setDocument( - address(cmtat), - documentName, - documentURI, - documentHash - ); - vm.prank(admin); - cmtat.setDocumentEngine(documentEngine); + documentEngine.setDocument(address(cmtat), documentName, documentURI, documentHash); - // Call from CMTAT, return document + // Call from CMTAT, forwarded to the engine bytes32[] memory docs = cmtat.getAllDocuments(); assertEq(docs.length, 1); assertEq(docs[0], documentName); - (string memory uri, bytes32 hash, uint256 lastModified) = cmtat - .getDocument(documentName); + IERC1643.Document memory doc = _doc(cmtat, documentName); + assertEq(doc.uri, documentURI); + assertEq(doc.documentHash, documentHash); + assertEq(doc.lastModified, lastModif); + } + + /*////////////////////////////////////////////////////////////// + Bound token (shared ITokenBinding allowlist / TokenBindingModule) + //////////////////////////////////////////////////////////////*/ + + function testBoundTokenCanManageOwnDocument() public { + // Bind the token to the engine (shared ITokenBinding surface) + vm.prank(admin); + documentEngine.bindToken(testContract); + assertTrue(documentEngine.isTokenBound(testContract)); + + // The bound token manages its own document namespace (msg.sender) + bytes32 selfName = keccak256("self-doc"); + string memory selfURI = "https://example.com/self"; + bytes32 selfHash = keccak256("selfHash"); + + vm.prank(testContract); + documentEngine.setDocument(selfName, selfURI, selfHash); + + IERC1643.Document memory doc = _doc(documentEngine, testContract, selfName); + assertEq(doc.uri, selfURI); + assertEq(doc.documentHash, selfHash); + assertEq(doc.lastModified, block.timestamp); + + // and can remove it + vm.prank(testContract); + documentEngine.removeDocument(selfName); + doc = _doc(documentEngine, testContract, selfName); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); + } + + function testNonAdminCannotBindToken() public { + vm.prank(attacker); + vm.expectRevert( + abi.encodeWithSelector(AccessControlUnauthorizedAccount.selector, attacker, DOCUMENT_MANAGER_ROLE) + ); + documentEngine.bindToken(testContract); + } + + function testAdminCanUnbindToken() public { + vm.prank(admin); + documentEngine.bindToken(testContract); + assertTrue(documentEngine.isTokenBound(testContract)); + + vm.prank(admin); + documentEngine.unbindToken(testContract); + assertFalse(documentEngine.isTokenBound(testContract)); + } + + function testCannotBindZeroAddress() public { + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(ITokenBinding.TokenBindingInvalidToken.selector)); + documentEngine.bindToken(AddressZero); + + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(ITokenBinding.TokenBindingInvalidToken.selector)); + documentEngine.unbindToken(AddressZero); + + assertFalse(documentEngine.isTokenBound(AddressZero)); + } + + /** + * @dev Binding is idempotent: the repeated call succeeds, because the caller's intent already + * holds, but emits nothing — so every {TokenBindingSet} in the log is a real transition and an + * indexer never has to de-duplicate. + */ + function testBindTokenIsIdempotentAndDoesNotReEmit() public { + vm.prank(admin); + vm.expectEmit(true, false, false, true); + emit TokenBindingSet(testContract, true); + documentEngine.bindToken(testContract); + + // second bind: succeeds, changes nothing, emits nothing + vm.recordLogs(); + vm.prank(admin); + documentEngine.bindToken(testContract); + assertEq(vm.getRecordedLogs().length, 0, "re-binding must not emit"); + assertTrue(documentEngine.isTokenBound(testContract)); + } + + function testUnbindTokenIsIdempotentAndDoesNotReEmit() public { + // unbinding a token that was never bound: succeeds, emits nothing + vm.recordLogs(); + vm.prank(admin); + documentEngine.unbindToken(testContract); + assertEq(vm.getRecordedLogs().length, 0, "unbinding an unbound token must not emit"); + assertFalse(documentEngine.isTokenBound(testContract)); + + vm.prank(admin); + documentEngine.bindToken(testContract); + + vm.prank(admin); + vm.expectEmit(true, false, false, true); + emit TokenBindingSet(testContract, false); + documentEngine.unbindToken(testContract); + + vm.recordLogs(); + vm.prank(admin); + documentEngine.unbindToken(testContract); + assertEq(vm.getRecordedLogs().length, 0, "re-unbinding must not emit"); + assertFalse(documentEngine.isTokenBound(testContract)); + } + + function testUnboundContractCannotSetOwnDocument() public { + bytes32 selfName = keccak256("self-doc"); + vm.prank(attacker); + vm.expectRevert(abi.encodeWithSelector(TokenBindingModule.NotBoundToken.selector, attacker)); + documentEngine.setDocument(selfName, documentURI, documentHash); + } + + function testUnboundContractCannotRemoveOwnDocument() public { + bytes32 selfName = keccak256("self-doc"); + vm.prank(attacker); + vm.expectRevert(abi.encodeWithSelector(TokenBindingModule.NotBoundToken.selector, attacker)); + documentEngine.removeDocument(selfName); + } + + /*////////////////////////////////////////////////////////////// + Flexible access control (overridable authorization hook) + //////////////////////////////////////////////////////////////*/ + + function testFlexibleAuthorizationCanBeOverridden() public { + OpenDocumentEngine openEngine = new OpenDocumentEngine(admin, AddressZero); + + // attacker holds no role, yet can manage documents because the + // authorization hook was overridden to allow anyone. + vm.prank(attacker); + openEngine.setDocument(testContract, documentName, documentURI, documentHash); + + IERC1643.Document memory doc = _doc(openEngine, testContract, documentName); + assertEq(doc.uri, documentURI); + assertEq(doc.documentHash, documentHash); + } + + /*////////////////////////////////////////////////////////////// + Version (ERC-8303) + //////////////////////////////////////////////////////////////*/ + + function testVersionReturnsNonEmptyString() public { + string memory v = documentEngine.version(); + assertGt(bytes(v).length, 0); + assertEq(v, "0.4.0"); + // the public VERSION constant matches version() + assertEq(documentEngine.VERSION(), v); + } + + function testSupportsInterfaceERC8303() public { + // interface id declared by ERC-8303 + assertEq(type(IERC8303).interfaceId, bytes4(0x54fd4d50)); + assertTrue(documentEngine.supportsInterface(type(IERC8303).interfaceId)); + } + + function testSupportsERC1643Interfaces() public { + // Pinned literals: an interface id is the XOR of the selectors, which depend only on the + // function names and argument types. The CMTAT `v3.3.0-rc2` change of the `getDocument` + // return shape therefore moved neither id — which is exactly why that change was + // undetectable through ERC-165 (see {testGetDocumentReturnsFlatErc1643Abi}). + assertEq(type(IERC1643).interfaceId, bytes4(0xecfecec8)); + assertEq(type(IERC1643MultiDocument).interfaceId, bytes4(0xa2b1179b)); + + // implements the base single-argument functions... + assertTrue(documentEngine.supportsInterface(type(IERC1643).interfaceId)); + // ...and the address-scoped multi-token extension + assertTrue(documentEngine.supportsInterface(type(IERC1643MultiDocument).interfaceId)); + // ...and the shared token-binding surface + assertTrue(documentEngine.supportsInterface(type(ITokenBinding).interfaceId)); + } + + /** + * @dev Pins the `getDocument` wire format to the flat ERC-1643 ABI. + * + * Return types do not take part in a function signature, so returning a `Document` struct + * instead of the three flat values leaves both the selector and `type(IERC1643).interfaceId` + * unchanged: ERC-165 discovery cannot catch the difference, and a consumer built from the + * specification ABI would silently decode a struct return as garbage. The only way to catch a + * regression is to inspect the returndata, so assert the first word is the string offset + * (`0x60`) of a flat `(string,bytes32,uint256)` and not the `0x20` struct offset. + */ + function testGetDocumentReturnsFlatErc1643Abi() public { + (bool okSubject, bytes memory subjectScoped) = address(documentEngine) + .staticcall(abi.encodeWithSignature("getDocument(address,bytes32)", testContract, documentName)); + assertTrue(okSubject); + assertEq(_firstWord(subjectScoped), 0x60, "getDocument(address,bytes32) must return flat values"); + + vm.prank(testContract); + (bool okSelf, bytes memory selfScoped) = + address(documentEngine).staticcall(abi.encodeWithSignature("getDocument(bytes32)", documentName)); + assertTrue(okSelf); + assertEq(_firstWord(selfScoped), 0x60, "getDocument(bytes32) must return flat values"); + + // The decoded values must round-trip through the specification's own signature. + (string memory uri, bytes32 hash_, uint256 lastModified) = abi.decode(subjectScoped, (string, bytes32, uint256)); assertEq(uri, documentURI); - assertEq(hash, documentHash); - assertEq(lastModif, lastModified); + assertEq(hash_, documentHash); + assertEq(lastModified, block.timestamp); + } + + function _firstWord(bytes memory data) private pure returns (uint256 word) { + assembly { + word := mload(add(data, 0x20)) + } + } + + /*////////////////////////////////////////////////////////////// + ERC-1643 input validation + //////////////////////////////////////////////////////////////*/ + + function testCannotSetDocumentWithZeroName() public { + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643.ERC1643InvalidName.selector)); + documentEngine.setDocument(testContract, bytes32(0), documentURI, documentHash); + } + + function testCannotRemoveMissingDocument() public { + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643.ERC1643MissingDocument.selector)); + documentEngine.removeDocument(testContract, keccak256("does-not-exist")); + } + + function testBoundTokenCannotSetZeroName() public { + vm.prank(admin); + documentEngine.bindToken(testContract); + vm.prank(testContract); + vm.expectRevert(abi.encodeWithSelector(IERC1643.ERC1643InvalidName.selector)); + documentEngine.setDocument(bytes32(0), documentURI, documentHash); + } + + function testSupportsInterfaceERC165AndAccessControl() public { + assertTrue(documentEngine.supportsInterface(type(IERC165).interfaceId)); + assertTrue(documentEngine.supportsInterface(type(IAccessControl).interfaceId)); + } + + function testDoesNotSupportInvalidInterface() public { + assertFalse(documentEngine.supportsInterface(bytes4(0xffffffff))); } /*////////////////////////////////////////////////////////////// @@ -217,29 +451,18 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { function testAdminCanSetDocument() public { uint256 lastModif = block.timestamp; vm.prank(admin); - documentEngine.setDocument( - testContract, - documentName, - documentURI, - documentHash - ); + documentEngine.setDocument(testContract, documentName, documentURI, documentHash); - (string memory uri, bytes32 hash, uint256 lastModified) = documentEngine - .getDocument(testContract, documentName); - assertEq(uri, documentURI); - assertEq(hash, documentHash); - assertEq(lastModif, lastModified); + IERC1643.Document memory doc = _doc(documentEngine, testContract, documentName); + assertEq(doc.uri, documentURI); + assertEq(doc.documentHash, documentHash); + assertEq(doc.lastModified, lastModif); } function testAdminCanSetDocumentAgain() public { // Arrange vm.prank(admin); - documentEngine.setDocument( - testContract, - documentName, - documentURI, - documentHash - ); + documentEngine.setDocument(testContract, documentName, documentURI, documentHash); bytes32[] memory docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 1); assertEq(docs[0], documentName); @@ -248,19 +471,13 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { string memory documentURIV2 = "https://example.com/doc1"; bytes32 documentHashV2 = keccak256("doc1Hash"); vm.prank(admin); - documentEngine.setDocument( - testContract, - documentName, - documentURIV2, - documentHashV2 - ); + documentEngine.setDocument(testContract, documentName, documentURIV2, documentHashV2); // Assert - (string memory uri, bytes32 hash, uint256 lastModified) = documentEngine - .getDocument(testContract, documentName); - assertEq(uri, documentURIV2); - assertEq(hash, documentHashV2); - assertEq(lastModif, lastModified); + IERC1643.Document memory doc = _doc(documentEngine, testContract, documentName); + assertEq(doc.uri, documentURIV2); + assertEq(doc.documentHash, documentHashV2); + assertEq(doc.lastModified, lastModif); docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 1); assertEq(docs[0], documentName); @@ -287,24 +504,16 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { documentEngine.batchSetDocuments(smartContracts, names, uris, hashes); // Check the first document - ( - string memory uri1, - bytes32 hash1, - uint256 lastModified1 - ) = documentEngine.getDocument(testContract, documentName); - assertEq(uri1, documentURI); - assertEq(hash1, documentHash); - assertEq(lastModified1, block.timestamp); + IERC1643.Document memory doc1 = _doc(documentEngine, testContract, documentName); + assertEq(doc1.uri, documentURI); + assertEq(doc1.documentHash, documentHash); + assertEq(doc1.lastModified, block.timestamp); // Check the second document - ( - string memory uri2, - bytes32 hash2, - uint256 lastModified2 - ) = documentEngine.getDocument(anotherSmartContract, names[1]); - assertEq(uri2, uris[1]); - assertEq(hash2, hashes[1]); - assertEq(lastModified2, block.timestamp); + IERC1643.Document memory doc2 = _doc(documentEngine, anotherSmartContract, names[1]); + assertEq(doc2.uri, uris[1]); + assertEq(doc2.documentHash, hashes[1]); + assertEq(doc2.lastModified, block.timestamp); } function testAdminCanBatchSetDocumentsForTheSameContract() public { @@ -328,24 +537,16 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { documentEngine.batchSetDocuments(smartContracts, names, uris, hashes); // Check the first document - ( - string memory uri1, - bytes32 hash1, - uint256 lastModified1 - ) = documentEngine.getDocument(testContract, documentName); - assertEq(uri1, documentURI); - assertEq(hash1, documentHash); - assertEq(lastModified1, block.timestamp); + IERC1643.Document memory doc1 = _doc(documentEngine, testContract, documentName); + assertEq(doc1.uri, documentURI); + assertEq(doc1.documentHash, documentHash); + assertEq(doc1.lastModified, block.timestamp); // Check the second document - ( - string memory uri2, - bytes32 hash2, - uint256 lastModified2 - ) = documentEngine.getDocument(testContract, names[1]); - assertEq(uri2, uris[1]); - assertEq(hash2, hashes[1]); - assertEq(lastModified2, block.timestamp); + IERC1643.Document memory doc2 = _doc(documentEngine, testContract, names[1]); + assertEq(doc2.uri, uris[1]); + assertEq(doc2.documentHash, hashes[1]); + assertEq(doc2.lastModified, block.timestamp); } function testCannotAddBatchDocumentIfLengthMismatch_A() public { @@ -412,11 +613,10 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { // Check that both documents are removed // Check the second document - (string memory uri, bytes32 hash, uint256 lastModified) = documentEngine - .getDocument(testContract, documentName); - assertEq(uri, ""); - assertEq(hash, ""); - assertEq(lastModified, 0); + IERC1643.Document memory doc = _doc(documentEngine, testContract, documentName); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); bytes32[] memory docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 0); } @@ -439,22 +639,17 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { // Check that both documents are removed // Check the second document - (string memory uri, bytes32 hash, uint256 lastModified) = documentEngine - .getDocument(testContract, documentName); - assertEq(uri, ""); - assertEq(hash, ""); - assertEq(lastModified, 0); + IERC1643.Document memory doc = _doc(documentEngine, testContract, documentName); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); bytes32[] memory docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 0); - ( - string memory uri2, - bytes32 hash2, - uint256 lastModified2 - ) = documentEngine.getDocument(anotherSmartContract, names[1]); - assertEq(uri2, ""); - assertEq(hash2, ""); - assertEq(lastModified2, 0); + IERC1643.Document memory doc2 = _doc(documentEngine, anotherSmartContract, names[1]); + assertEq(doc2.uri, ""); + assertEq(doc2.documentHash, ""); + assertEq(doc2.lastModified, 0); docs = documentEngine.getAllDocuments(anotherSmartContract); assertEq(docs.length, 0); } @@ -500,24 +695,16 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { documentEngine.batchSetDocuments(testContract, names, uris, hashes); // Check the first document - ( - string memory uri1, - bytes32 hash1, - uint256 lastModified1 - ) = documentEngine.getDocument(testContract, documentName); - assertEq(uri1, documentURI); - assertEq(hash1, documentHash); - assertEq(lastModified1, block.timestamp); + IERC1643.Document memory doc1 = _doc(documentEngine, testContract, documentName); + assertEq(doc1.uri, documentURI); + assertEq(doc1.documentHash, documentHash); + assertEq(doc1.lastModified, block.timestamp); // Check the second document - ( - string memory uri2, - bytes32 hash2, - uint256 lastModified2 - ) = documentEngine.getDocument(testContract, names[1]); - assertEq(uri2, uris[1]); - assertEq(hash2, hashes[1]); - assertEq(lastModified2, block.timestamp); + IERC1643.Document memory doc2 = _doc(documentEngine, testContract, names[1]); + assertEq(doc2.uri, uris[1]); + assertEq(doc2.documentHash, hashes[1]); + assertEq(doc2.lastModified, block.timestamp); } function testAdminCanBatchRemoveDocumentsForOnlyOneContract() public { @@ -534,30 +721,216 @@ contract DocumentEngineTest is Test, DocumentEngineInvariant, AccessControl { // Check that both documents are removed // Check the second document - (string memory uri, bytes32 hash, uint256 lastModified) = documentEngine - .getDocument(testContract, documentName); - assertEq(uri, ""); - assertEq(hash, ""); - assertEq(lastModified, 0); + IERC1643.Document memory doc = _doc(documentEngine, testContract, documentName); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); bytes32[] memory docs = documentEngine.getAllDocuments(testContract); assertEq(docs.length, 0); - ( - string memory uri2, - bytes32 hash2, - uint256 lastModified2 - ) = documentEngine.getDocument(testContract, names[1]); - assertEq(uri2, ""); - assertEq(hash2, ""); - assertEq(lastModified2, 0); - } - - function testCannotRemoveBatchDocumentIfEmptyLengthForOnlyOneContract() - public - { + IERC1643.Document memory doc2 = _doc(documentEngine, testContract, names[1]); + assertEq(doc2.uri, ""); + assertEq(doc2.documentHash, ""); + assertEq(doc2.lastModified, 0); + } + + function testCannotRemoveBatchDocumentIfEmptyLengthForOnlyOneContract() public { bytes32[] memory names = new bytes32[](0); vm.expectRevert(abi.encodeWithSelector(InvalidInputLength.selector)); vm.prank(admin); documentEngine.batchRemoveDocuments(testContract, names); } + + /*////////////////////////////////////////////////////////////// + Events (emission responsibility) + //////////////////////////////////////////////////////////////*/ + + function testSetDocumentEmitsForSubjectEvent() public { + bytes32 name = keccak256("evt-doc"); + vm.expectEmit(true, true, false, true, address(documentEngine)); + emit DocumentUpdatedForSubject(testContract, name, documentURI, documentHash); + vm.prank(admin); + documentEngine.setDocument(testContract, name, documentURI, documentHash); + } + + function testRemoveDocumentEmitsForSubjectEvent() public { + // `documentName` for `testContract` was registered in setUp + vm.expectEmit(true, true, false, true, address(documentEngine)); + emit DocumentRemovedForSubject(testContract, documentName, documentURI, documentHash); + vm.prank(admin); + documentEngine.removeDocument(testContract, documentName); + } + + function testSetDocumentDoesNotEmitBaseEvent() public { + vm.recordLogs(); + vm.prank(admin); + documentEngine.setDocument(testContract, keccak256("evt-doc"), documentURI, documentHash); + _assertBaseEventNotEmitted(BASE_UPDATED_SIG); + } + + function testRemoveDocumentDoesNotEmitBaseEvent() public { + vm.recordLogs(); + vm.prank(admin); + documentEngine.removeDocument(testContract, documentName); + _assertBaseEventNotEmitted(BASE_REMOVED_SIG); + } + + /// @dev Asserts no recorded log emitted by the engine carries the base ERC-1643 signature. + function _assertBaseEventNotEmitted(bytes32 baseSig) internal { + Vm.Log[] memory logs = vm.getRecordedLogs(); + for (uint256 i = 0; i < logs.length; ++i) { + if (logs[i].emitter == address(documentEngine)) { + assertTrue(logs[i].topics[0] != baseSig, "base ERC-1643 event must not be emitted"); + } + } + } + + /*////////////////////////////////////////////////////////////// + msg.sender-scoped reads (base ERC-1643) + //////////////////////////////////////////////////////////////*/ + + function testMsgSenderScopedReads() public { + // setUp registered `documentName` for `testContract`; read it as that caller + vm.prank(testContract); + IERC1643.Document memory doc = _doc(documentEngine, documentName); + assertEq(doc.uri, documentURI); + assertEq(doc.documentHash, documentHash); + + vm.prank(testContract); + bytes32[] memory names = documentEngine.getAllDocuments(); + assertEq(names.length, 1); + assertEq(names[0], documentName); + } + + function testMsgSenderScopedReadReturnsEmptyForOther() public { + // `attacker` has no documents of its own + vm.prank(attacker); + IERC1643.Document memory doc = _doc(documentEngine, documentName); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); + + vm.prank(attacker); + assertEq(documentEngine.getAllDocuments().length, 0); + } + + /*////////////////////////////////////////////////////////////// + Batch edge cases (name==0 / missing doc) + //////////////////////////////////////////////////////////////*/ + + function testCannotSetDocumentForZeroSubject() public { + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643MultiDocument.MultiDocumentInvalidSubject.selector)); + documentEngine.setDocument(AddressZero, documentName, documentURI, documentHash); + } + + function testBatchSetRevertsOnZeroSubject() public { + address[] memory subjects = new address[](1); + subjects[0] = AddressZero; + bytes32[] memory names = new bytes32[](1); + names[0] = documentName; + string[] memory uris = new string[](1); + uris[0] = documentURI; + bytes32[] memory hashes = new bytes32[](1); + hashes[0] = documentHash; + + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643MultiDocument.MultiDocumentInvalidSubject.selector)); + documentEngine.batchSetDocuments(subjects, names, uris, hashes); + } + + function testBatchSetRevertsOnZeroName() public { + address[] memory subjects = new address[](1); + subjects[0] = testContract; + bytes32[] memory names = new bytes32[](1); + names[0] = bytes32(0); + string[] memory uris = new string[](1); + uris[0] = documentURI; + bytes32[] memory hashes = new bytes32[](1); + hashes[0] = documentHash; + + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643.ERC1643InvalidName.selector)); + documentEngine.batchSetDocuments(subjects, names, uris, hashes); + } + + function testBatchRemoveRevertsOnMissingDocument() public { + address[] memory subjects = new address[](1); + subjects[0] = testContract; + bytes32[] memory names = new bytes32[](1); + names[0] = keccak256("never-set"); + + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(IERC1643.ERC1643MissingDocument.selector)); + documentEngine.batchRemoveDocuments(subjects, names); + } + + /*////////////////////////////////////////////////////////////// + Enumeration & fuzz + //////////////////////////////////////////////////////////////*/ + + function testEnumerationAfterMixedOps() public { + address subj = address(0xBEEF); + bytes32 n1 = keccak256("n1"); + bytes32 n2 = keccak256("n2"); + bytes32 n3 = keccak256("n3"); + + vm.startPrank(admin); + documentEngine.setDocument(subj, n1, "u1", bytes32(0)); + documentEngine.setDocument(subj, n2, "u2", bytes32(0)); + documentEngine.setDocument(subj, n3, "u3", bytes32(0)); + assertEq(documentEngine.getAllDocuments(subj).length, 3); + + // overwrite does not add a new entry + documentEngine.setDocument(subj, n2, "u2-updated", bytes32(0)); + assertEq(documentEngine.getAllDocuments(subj).length, 3); + + // removal shrinks the set (swap-and-pop) + documentEngine.removeDocument(subj, n2); + vm.stopPrank(); + + bytes32[] memory names = documentEngine.getAllDocuments(subj); + assertEq(names.length, 2); + assertTrue( + (names[0] == n1 && names[1] == n3) || (names[0] == n3 && names[1] == n1), + "remaining names must be n1 and n3" + ); + } + + function testFuzzSetGetRemoveRoundTrip(address subject, bytes32 name, string calldata uri, bytes32 hash) public { + vm.assume(name != bytes32(0)); // the null name reverts by design + vm.assume(subject != AddressZero); // the null subject reverts by design + + vm.prank(admin); + documentEngine.setDocument(subject, name, uri, hash); + + IERC1643.Document memory doc = _doc(documentEngine, subject, name); + assertEq(doc.uri, uri); + assertEq(doc.documentHash, hash); + assertEq(doc.lastModified, block.timestamp); + + vm.prank(admin); + documentEngine.removeDocument(subject, name); + + doc = _doc(documentEngine, subject, name); + assertEq(doc.uri, ""); + assertEq(doc.documentHash, ""); + assertEq(doc.lastModified, 0); + } + + function testFuzzDocumentsAreIsolatedPerSubject(address subjectA, address subjectB, bytes32 name) public { + vm.assume(name != bytes32(0)); + vm.assume(subjectA != subjectB); + vm.assume(subjectA != AddressZero); // the null subject reverts by design + // `testContract` is pre-populated in setUp; exclude it from the "untouched" subject + vm.assume(subjectB != testContract); + + vm.prank(admin); + documentEngine.setDocument(subjectA, name, documentURI, documentHash); + + // subjectB is unaffected + IERC1643.Document memory docB = _doc(documentEngine, subjectB, name); + assertEq(docB.lastModified, 0); + assertEq(documentEngine.getAllDocuments(subjectB).length, 0); + } } diff --git a/test/DocumentEngineOwnable.t.sol b/test/DocumentEngineOwnable.t.sol new file mode 100644 index 0000000..b53e7ae --- /dev/null +++ b/test/DocumentEngineOwnable.t.sol @@ -0,0 +1,156 @@ +//SPDX-License-Identifier: MPL-2.0 +pragma solidity ^0.8.20; + +import "forge-std/Test.sol"; +import "../src/DocumentEngineOwnable.sol"; +import {Ownable} from "OZ/access/Ownable.sol"; +import {IAccessControl} from "OZ/access/IAccessControl.sol"; +import {IERC165} from "OZ/utils/introspection/IERC165.sol"; +import {IERC8303} from "../src/interfaces/IERC8303.sol"; +import {IERC1643} from "CMTAT/interfaces/tokenization/draft-IERC1643.sol"; +import {IERC1643MultiDocument} from "../src/interfaces/IERC1643MultiDocument.sol"; +import {ITokenBinding} from "../src/interfaces/ITokenBinding.sol"; +import {TokenBindingModule} from "../src/modules/TokenBindingModule.sol"; + +contract DocumentEngineOwnableTest is Test { + DocumentEngineOwnable public engine; + address public owner = address(0x1); + address public newOwner = address(0x2); + address public attacker = address(0x3); + address private testContract = address(0x4); + bytes32 public documentName = keccak256("doc1"); + string public documentURI = "https://example.com/doc1"; + bytes32 public documentHash = keccak256("doc1Hash"); + address AddressZero = address(0); + + /** + * @dev Since CMTAT `v3.3.0-rc2`, `getDocument` returns the three ERC-1643 fields as flat + * values instead of a `Document` struct; this helper repacks them so the assertions below + * stay readable. The wire format is pinned by `DocumentEngineTest`. + */ + function _doc(IERC1643MultiDocument engine_, address subject, bytes32 name_) + internal + view + returns (IERC1643.Document memory document) + { + (document.uri, document.documentHash, document.lastModified) = engine_.getDocument(subject, name_); + } + + function setUp() public { + engine = new DocumentEngineOwnable(owner, AddressZero); + } + + /* ============ DEPLOYMENT ============ */ + + function testDeploySetsOwner() public { + assertEq(engine.owner(), owner); + } + + function testDeployRevertsWithZeroOwner() public { + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableInvalidOwner.selector, AddressZero)); + new DocumentEngineOwnable(AddressZero, AddressZero); + } + + /* ============ ADMIN PATH (owner) ============ */ + + function testOwnerCanSetAndRemoveDocument() public { + vm.prank(owner); + engine.setDocument(testContract, documentName, documentURI, documentHash); + + IERC1643.Document memory doc = _doc(engine, testContract, documentName); + assertEq(doc.uri, documentURI); + assertEq(doc.documentHash, documentHash); + assertEq(doc.lastModified, block.timestamp); + + vm.prank(owner); + engine.removeDocument(testContract, documentName); + doc = _doc(engine, testContract, documentName); + assertEq(doc.lastModified, 0); + } + + function testNonOwnerCannotSetDocument() public { + vm.prank(attacker); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, attacker)); + engine.setDocument(testContract, documentName, documentURI, documentHash); + } + + /* ============ BOUND-TOKEN PATH ============ */ + + function testOwnerCanBindToken() public { + vm.prank(owner); + engine.bindToken(testContract); + assertTrue(engine.isTokenBound(testContract)); + } + + function testNonOwnerCannotBindToken() public { + vm.prank(attacker); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, attacker)); + engine.bindToken(testContract); + } + + function testBoundTokenCanManageOwnDocument() public { + vm.prank(owner); + engine.bindToken(testContract); + + vm.prank(testContract); + engine.setDocument(documentName, documentURI, documentHash); + + IERC1643.Document memory doc = _doc(engine, testContract, documentName); + assertEq(doc.uri, documentURI); + + vm.prank(testContract); + engine.removeDocument(documentName); + doc = _doc(engine, testContract, documentName); + assertEq(doc.lastModified, 0); + } + + function testUnbindTokenRevokesSelfManagement() public { + vm.prank(owner); + engine.bindToken(testContract); + assertTrue(engine.isTokenBound(testContract)); + + vm.prank(owner); + engine.unbindToken(testContract); + assertFalse(engine.isTokenBound(testContract)); + + // once unbound, the token can no longer self-manage + vm.prank(testContract); + vm.expectRevert(abi.encodeWithSelector(TokenBindingModule.NotBoundToken.selector, testContract)); + engine.setDocument(documentName, documentURI, documentHash); + } + + function testUnboundTokenCannotSelfManage() public { + vm.prank(attacker); + vm.expectRevert(abi.encodeWithSelector(TokenBindingModule.NotBoundToken.selector, attacker)); + engine.setDocument(documentName, documentURI, documentHash); + } + + /* ============ TWO-STEP OWNERSHIP ============ */ + + function testTwoStepOwnershipTransfer() public { + vm.prank(owner); + engine.transferOwnership(newOwner); + // ownership not transferred until accepted + assertEq(engine.owner(), owner); + assertEq(engine.pendingOwner(), newOwner); + + vm.prank(newOwner); + engine.acceptOwnership(); + assertEq(engine.owner(), newOwner); + assertEq(engine.pendingOwner(), AddressZero); + } + + /* ============ VERSION (ERC-8303) ============ */ + + function testVersionAndInterface() public { + assertEq(engine.version(), "0.4.0"); + assertTrue(engine.supportsInterface(type(IERC8303).interfaceId)); + assertTrue(engine.supportsInterface(type(IERC165).interfaceId)); + assertTrue(engine.supportsInterface(type(IERC1643).interfaceId)); + assertTrue(engine.supportsInterface(type(IERC1643MultiDocument).interfaceId)); + assertTrue(engine.supportsInterface(type(ITokenBinding).interfaceId)); + // no role-based access control here + assertFalse(engine.supportsInterface(type(IAccessControl).interfaceId)); + assertFalse(engine.supportsInterface(bytes4(0xffffffff))); + } +}