Approved workflow

setBeneficiary is permissionless and one-shot, which is the approved design ('payments go to a beneficiary set by the first caller once'), so this is not an implementation bug and the contract should not be rewritten on my say-so. The gap is who wins that race.

ProjectFactory makes no initialization calls, the registry constructor takes only $token, launch.json has no field that can preset the beneficiary (the manifest node correctly says so in its notes), contributors are forbidden from broadcasting, and the deployer service (apps/deployer) deploys the launch transaction and stops.

README.md:26-29 assigns the call to 'the deployer/operator' but nothing in the accepted tree or in the described service chain performs it, so between the launch transaction and an unscheduled manual follow-up the role sits unclaimed on a public mempool. The outcome is permanent: there is no reassignment path and no refund path.

Resolving this requires a scope decision, not a local patch - either (a) an explicitly authorized post-deploy step with evidence that some service actually performs it in the same block range, or (b) changing the agreed brief so the beneficiary is a constructor argument (which the brief's 'constructor takes only the token address ($token)' currently forbids, and which would need $owner-style resolution from policy rather than a hard-coded wallet).

Flagging as a trust-assumption/scope finding rather than high, because the implementation matches the approved brief exactly and sending the source back would not by itself close it.

The guard rejects periods > type(uint256).max / PERIOD (2592000), i.e. it permits periods up to ~4.46e70. But the next two statements are start + periods * PERIOD and periods * PRICE_PER_PERIOD, and PRICE_PER_PERIOD is 1e19, so amount overflows for any periods above ~1.157e58 - more than twelve orders of magnitude below the value the guard accepts.

Checked arithmetic makes this fail closed (no wrong state is written), so this is an error-surface defect, not an accounting defect.

The consequences are that the InvalidPeriods branch is effectively unreachable for anything except periods == 0; the ABI at docs/abi/SubscriptionRegistry.json advertises InvalidPeriods as the validation error for this parameter; and README.md:16-18 describes only 'zero periods' reverting, so the web client specified for the next stage will decode an undecodable Panic for these inputs and surface it as an unknown failure.

The bound that actually matters economically is much smaller still: total supply is 1e27 and a period costs 1e19, so no account can ever fund more than 1e8 periods.

README.md:33-36 reads 'This repository intentionally does not include launch.json; the independent manifest assignment owns it.' Commit b3b4818 (the manifest assignment) then added launch.json at the repository root. Because that assignment's write scope is launch.json only, it could not correct the README, and the manifest itself is not wrong - the stale sentence is.

A reviewer or operator reading the accepted tree gets a direct contradiction about whether the manifest at the root is authoritative, which matters precisely at the admission step that compares source against manifest.

The same section is the natural home for the environment variables the protected suites consume (IMD_TOKEN_CREATION_CODE, IMD_TOKEN_DECIMALS, IMD_PROJECT_FACTORY, IMD_PROJECT_CHAIN_ID, IMD_EXPECTED_TOKEN, IMD_EXPECTED_SUPPLY, IMD_PROJECT_COUNT, IMD_PROJECT_CODE_n, IMD_PROJECT_SALT_n, IMD_PROJECT_ADDRESS_n); the README documents forge build, forge test and forge fmt --check but names none of them, so nothing in the tree tells an operator how to reproduce the policy floor.

SubscriptionRegistry never calls token.transfer - its interface IERC20Payment only declares transferFrom - and it has no withdraw, sweep or rescue function. That is consistent with the brief ('no fee, owner, admin' and 'no refunds') and is the right choice for a contract that should not custody funds, so the fix is not to add an admin escape hatch: doing so would introduce exactly the privileged role the brief forbids.

The reviewable gap is that the failure mode is undocumented. The intended user flow is approve-then-subscribe, and a user who instead transfers CDNC to the registry address - the most common ERC-20 user error, and one the next-stage web client will display the registry address for - loses it with no on-chain remedy.

README.md's protocol-behavior section states that the registry 'does not custody funds and provides no refunds' but does not say that tokens mistakenly sent to it are burned in practice, and the web UI acceptance criteria ('connect, set the beneficiary once, subscribe for N periods') give no place for that warning to appear unless it is written down here first.

The brief requires 'events for every state change' and 'thorough Foundry tests for every path, including wrong amounts'.

Neither BeneficiarySet nor Subscribed is asserted anywhere in test/SubscriptionRegistry.t.sol - no vm.expectEmit appears in the suite - so the four non-indexed Subscribed fields (periods, amount, previousExpiry, newExpiry) are entirely unverified by the accepted tests, even though the next-stage web client is specified to read expiry state and would be the first consumer of them.

On the payment side, testRejectsZeroPeriodsAndInsufficientPaymentAuthorization covers only the zero-allowance case (0xBAD has neither balance nor allowance, and Cadence checks allowance first, so the balance path is shadowed).

I confirmed by direct test that the two missing paths currently behave correctly, so this is a coverage finding rather than a live defect - but as accepted, a regression in the amount computation or in the expiry rollback on either path would not fail forge test.

subscribe() calls token.transferFrom(msg.sender, recipient, amount) with no check that msg.sender != recipient. When the beneficiary subscribes, Cadence._transfer runs with from == to: it caches balance, writes balanceOf[from] = balance - value, then reads balanceOf[to] fresh and adds value back, so the net balance change is zero (the token's self-transfer accounting is itself correct - I verified that separately).

The beneficiary therefore obtains an active subscription of any length at zero cost. Recording this as info rather than a defect: the beneficiary receives 100% of subscription revenue by design, so paying itself and being paid back is economically identical to a free subscription either way, and no other account can reach this path.

It is listed only because it is a privileged capability that is not stated in README.md or in launch.json's notes, and the stage guidance asks that material privileged powers be documented rather than discovered later.

Both suites import forge-std/Test.sol, but the vendored library (added in a8a4c90) is a 44-line Test contract and a 23-line Vm interface rather than upstream forge-std. It declares exactly the surface the protected floors need - assertTrue/assertFalse/assertEq(uint256)/assertEq(address)/assertGt/assertLe plus fourteen cheatcodes - and I confirmed it is sufficient to compile both .imd/reads/protected suites, so the vendoring choice is defensible for an offline verifier.

The reviewable consequence is that fuzzing, invariant testing, bound(), deal(), assertLt, assertNotEq and the log_named_* helpers are all unavailable, which is why the accepted suite contains fourteen fixed-input unit tests and no fuzz run. That is a real ceiling on the 'thorough tests for every path' requirement: the arithmetic boundary in subscribe(), for instance, is exactly the kind of property a fuzz test would have surfaced and a fixed-input suite did not.

Not a defect to fix here - lib/ is outside every assignment's write scope - but it should be recorded so downstream admission does not read 'forge test: 14 passed' as broader evidence than it is.