Shared test helpers are re-declared per test contract instead of living in test/lib

[thedavidmeister]

Audit finding CQ4-03 — dimension 4, severity LOW. Whole-repo audit pass 1 at 440e90b5.

test/src/concrete/MigrationRegistryApplied.t.sol:25-28

Problem

The suite already has a home for shared test utilities — test/lib/LibAccountCode.sol — and does not use it for helpers that several contracts need, re-declaring them per contract instead.

assumeMigration(bytes32) (vm.assume(migration != bytes32(0)); vm.assume(migration != MIGRATION_HEAD_GENESIS);) is declared four times, byte-identical: test/src/concrete/MigrationRegistryApplied.t.sol:25-28, test/src/concrete/MigrationRegistryHead.t.sol:24-27, test/src/concrete/MigrationRegistryApplyMigration.t.sol:25-28 and test/src/lib/LibMigrationRegistry.t.sol:33-36. It is not a convenience: it encodes the interface rule that the head space reserves exactly two values a migration id may never be, so it is the fuzz DOMAIN of every migration test in the repo. Four copies is four places to update when that domain changes, and the copy that gets missed is a test that silently starts fuzzing values the contract is specified to refuse.

The same property shows in the membership predicate: holdsPath (test/src/lib/LibRainDeploySnapshot.t.sol:74-81) and holdsName (test/src/lib/GeneratedSnapshotShape.t.sol:284-291) are the same keccak-membership loop over string[] under two names.

Proposed fix

Move both to test/lib/, beside LibAccountCode, and have the test contracts call them.

New file test/lib/LibMigrationFuzz.sol:

// SPDX-License-Identifier: LicenseRef-DCL-1.0
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
pragma solidity ^0.8.25;

import {Vm} from "forge-std-1.16.1/src/Vm.sol";
import {MIGRATION_HEAD_GENESIS} from "../../src/interface/IMigrationRegistryV1.sol";

/// @title LibMigrationFuzz
/// @notice The fuzz domain of a migration id: every `bytes32` except the two
/// the head space reserves. Declared once, so a change to what a head may hold
/// changes what every migration test fuzzes.
library LibMigrationFuzz {
    /// @param vm The Vm instance, for `assume`.
    /// @param migration The fuzzed candidate.
    function assumeMigration(Vm vm, bytes32 migration) internal pure {
        vm.assume(migration != bytes32(0));
        vm.assume(migration != MIGRATION_HEAD_GENESIS);
    }
}

New file test/lib/LibStringSet.sol:

// SPDX-License-Identifier: LicenseRef-DCL-1.0
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
pragma solidity ^0.8.25;

/// @title LibStringSet
/// @notice Membership over a `string[]`, for tests that assert about a set
/// whose order is the filesystem's.
library LibStringSet {
    /// @param haystack The strings to search.
    /// @param needle The string to find.
    /// @return Whether it is present.
    function holds(string[] memory haystack, string memory needle) internal pure returns (bool) {
        for (uint256 i = 0; i < haystack.length; i++) {
            if (keccak256(bytes(haystack[i])) == keccak256(bytes(needle))) {
                return true;
            }
        }
        return false;
    }
}

Then in each of the four migration test contracts delete the local assumeMigration and replace calls, e.g. in test/src/concrete/MigrationRegistryApplied.t.sol:

import {LibMigrationFuzz} from "../../lib/LibMigrationFuzz.sol";
...
-        assumeMigration(migration);
+        LibMigrationFuzz.assumeMigration(vm, migration);

and in the two snapshot test contracts delete holdsPath / holdsName and call LibStringSet.holds(...).

---

Verification — this finding survived an adversarial refutation pass

SURVIVES (LOW stands), with two corrections to the write-up.

Verified against source at 440e90b:

• assumeMigration(bytes32) internal pure { vm.assume(migration != bytes32(0)); vm.assume(migration != MIGRATION_HEAD_GENESIS); } is declared four times with identical bodies, at exactly the cited lines: /home/gildlab/code/rain-deploy-audit/test/src/concrete/MigrationRegistryApplied.t.sol:25-28, test/src/concrete/MigrationRegistryHead.t.sol:24-27, test/src/concrete/MigrationRegistryApplyMigration.t.sol:25-28, test/src/lib/LibMigrationRegistry.t.sol:33-36 (only the docstrings differ — ApplyMigration adds one sentence). Total 60 call sites across the four files.
• holdsPath (test/src/lib/LibRainDeploySnapshot.t.sol:74-81) and holdsName are the same keccak-membership loop over string[] under two names.

The strongest refutation attempt fails on the repo's OWN precedent: test/lib/LibAccountCode.sol was created in the most recent commit (dd098d5) for exactly this category — a fuzz DOMAIN shared by two test files (test/src/lib/LibAddressRegistry.t.sol and test/src/lib/LibMigrationRegistry.t.sol), extracted with a documented rationale. assumeMigration is the same category with four consumers instead of two, so leaving it duplicated is inconsistent with a convention this repo established for a weaker case, not a deliberate one it upholds. CLAUDE.md's only test-placement rule is the mirror-tree rule (test/src/** mirrors src/**, abstracts under test/), which the fix does not violate — test/lib/ is precisely the non-mirror home. Nothing in audit/ has previously ruled on it. The proposed LibMigrationFuzz.assumeMigration(Vm vm, bytes32) compiles: Vm.assume is external pure (dependencies/forge-std-1.16.1/src/Vm.sol:1757), so internal pure is legal.

Corrections:

1. Wrong citation: holdsName is at test/src/lib/GeneratedSnapshotShape.t.sol:135-142, not 284-291 — that file is only 244 lines long.
2. The stated hazard is overstated. A missed copy would fuzz a reserved id into a contract specified to refuse it, which surfaces as a reverting/failing test (loud CI red), not as a test that "silently" passes. The real cost is four edit sites for one interface rule, not a silent correctness hole.
3. The holdsPath/holdsName half is materially weaker than the assumeMigration half: each is used in exactly one file, their docstrings encode different local reasoning (walk order is the filesystem's vs. name-matching candidates to snapshots), and collapsing them into a generic LibStringSet.holds discards that reasoning. It is real duplication, but as a secondary point.

Severity LOW is correct: test-only, zero value at risk in production; it is a maintenance/consistency defect against the repo's own test/lib convention, not a hazard.