Reentrancy row 18 untested, and a stale rain-factory-0.1.1 remapping hides dependency drift

[thedavidmeister]

Two things left over from #325, filed together because they share a root: the reentrancy audit table has one uncovered row, and the dependency drift that broke #325 for two commits is still latent in the tree.

1. Row 18 of #316 has no regression test

#316's call-site table is 22 rows: 16 rated none (nothing to regression-test), 4 low, 1 low (benign), 1 minor. #325 covers every non-none row except one:

row	site	reachable	risk	test
11	ETH refund, ReceiptVault.sol:220/237/301/326/348	outside the guard	low (benign)	testDepositEthRefundReentryIsBenign
12	authorizeReceiptTransfer3	not nonReentrant	low	testDepositSwappedAuthorizerReceiptTransferGuardFires
14	certify	not nonReentrant	low	testCertifyReentrantAuthorizerObservesWrittenCertification
15	share _update	not nonReentrant	low	testShareTransferSwappedAuthorizerCannotOverspend
20	oracle _nextId	pre-guard	low	testDepositOracleReentryIsBenignWithFreshPrice
18	_setAuthorizer → IERC165(newAuthorizer).supportsInterface	could	minor	none

The mitigation the table records for row 18 is onlyOwner context — i.e. the argument is that the caller is trusted, not that re-entry is structurally impossible. That is exactly the kind of claim worth pinning, because it survives only as long as _setAuthorizer stays owner-gated. A test would set a newAuthorizer whose supportsInterface re-enters, and assert whatever the intended outcome is.

Alternatively, if minor + onlyOwner is judged not worth a fork-free unit test, say so on #316 and close the row explicitly. Either resolution is fine; the current state — a table row with no test and no recorded decision — is the one that rots.

Note #325 says Closes #316. If row 18 is meant to be covered before #316 closes, that reference should have been Refs.

2. A stale remapping is hiding a dependency version drift

remappings.txt on main carries both prefixes:

rain-factory-0.1.1/=dependencies/rain-factory-0.1.1/
rain-factory-0.1.5/=dependencies/rain-factory-0.1.5/

while foundry.toml declares only "rain-factory" = "0.1.5" and soldeer.lock carries only the 0.1.5 archive. So the 0.1.1 line resolves to a directory soldeer never installs.

That dangling remapping is what let #325 sit for five weeks looking fine. Its OffchainAssetReceiptVaultPaymentMint.reentrant.t.sol imported through the 0.1.1 prefix; the import looked legitimate because the remapping existed, and the failure only appeared when the branch was updated onto the current pin — as two errors in sequence:

Error (6275): Source "dependencies/rain-factory-0.1.1/src/concrete/CloneFactory.sol" not found
Error (9582): Member "clone" not found or not visible after argument-dependent lookup in contract CloneFactory

The second is the substantive half: 0.1.5 removed clone() in favour of cloneDeterministic() with an explicit salt. Any other branch or fixture still written against clone() will break the same way, and nothing in the tree says so.

Proposed fix

• Delete the rain-factory-0.1.1/ line from remappings.txt. An import through a prefix with no installed dependency should fail immediately and obviously, not five weeks later on someone else's merge-update.
• Add a check that every prefix in remappings.txt corresponds to an installed dependency — a small test or CI step. A remapping is a promise that a path resolves; nothing currently enforces it.
• Sweep for any remaining first-party use of the removed clone() API. At the time of writing there is none — the only surviving .clone( call sites are OpenZeppelin's Clones.clone(...), which is unrelated — but the sweep is what makes that a fact rather than a hope.

Why the check matters more than the deletion

Deleting the line fixes today. The check is what stops the next version bump from leaving another prefix pointing at nothing, which is the shape that cost this PR two CI rounds and very nearly got merged on a misread — the compile failure produced zero [FAIL lines, so "no failing tests" was true and meaningless at the same time.