A post-deploy check with zero reads passes on every network and no test pins it

[thedavidmeister]

Audit finding cov-08 — dimension 2, severity MEDIUM. Whole-repo audit pass 1 at 440e90b5.

src/lib/LibRainDeploy.sol:272-349

Problem

checkResolvedAddresses refuses a length mismatch and then loops; with readCalls.length == expectedAddresses.length == 0 the mismatch guard passes and the loop asserts nothing. checkResolvedAddressesOnNetworks forks every network and reports success having read nothing.

No test covers it: testCheckResolvedAddressesLengthMismatchReverts fuzzes uint8 readCallsLength, uint8 expectedLength under vm.assume(readCallsLength != expectedLength), so the equal-and-zero case is excluded by construction, and every other test passes a one- or two-element list.

This is the same hazard NoNetworks exists for one argument along — testCheckResolvedAddressesOnNetworksNoNetworksReverts says "an empty target set can never be mistaken for every read checking out" — and the read list has no equivalent guard. A consumer that builds readCalls from a list that came back empty (a config, a loop over a set of resolved names) gets a green "deployment verified across every network" that verified nothing, immediately before migrating onto that deployment.

Proposed fix

1. Fail closed on the empty read set, mirroring NoNetworks (src/lib/LibRainDeploy.sol):

    /// Thrown when a post-deploy check is given no reads. A check with nothing
    /// to read passes on every network having asserted nothing, which is
    /// indistinguishable from every read checking out.
    error NoResolvedAddressReads(address target);

and at the top of checkResolvedAddresses, before the pairing check:

        if (readCalls.length == 0) {
            revert NoResolvedAddressReads(target);
        }

2. Add to test/src/lib/LibRainDeploy.t.sol:

    /// A check with no reads MUST be refused rather than pass. It is the same
    /// hazard `NoNetworks` covers one argument along: an empty read set is
    /// indistinguishable from every read checking out, and it is what a
    /// consumer that built its read list from an empty config hands in — right
    /// before it migrates onto the deployment this was supposed to verify.
    function testCheckResolvedAddressesNoReadsReverts(address target) external {
        vm.expectRevert(abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, target));
        this.externalCheckResolvedAddresses("test_network", target, new bytes[](0), new address[](0));
    }

    /// And refused before any network is forked, so an empty read set is
    /// reported without an RPC round trip and cannot be masked by an outage.
    function testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking() external {
        string[] memory networks = new string[](1);
        // Not a configured RPC alias, so forking it is itself an error.
        networks[0] = "unconfigured_network";

        vm.expectRevert(abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, address(this)));
        this.externalCheckResolvedAddressesOnNetworks(networks, address(this), new bytes[](0), new address[](0));
    }

    /// The refusal is about the READS, not about the pairing: an empty pair is
    /// the one length mismatch cannot see, which is why it is its own error.
    function testCheckResolvedAddressesEmptyPairIsNotALengthMismatch(bytes32 name, address account) external {
        vm.assume(account != address(0));
        (, MockResolvedOwner consumer) = deployRegistryAndConsumer(name, account);

        vm.expectRevert(
            abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, address(consumer))
        );
        this.externalCheckResolvedAddresses("test_network", address(consumer), new bytes[](0), new address[](0));

        // One read still passes, so the refusal is about emptiness alone.
        LibRainDeploy.checkResolvedAddresses("test_network", address(consumer), ownerReadCalls(), expected(account));
    }

Update checkResolvedAddressesOnNetworks to run the new guard before the fork loop (it already runs the pairing check there for the same reason).

---

Verification — this finding survived an adversarial refutation pass

Verified against source and tests; could not refute.

Behaviour exists as described. /home/gildlab/code/rain-deploy-audit/src/lib/LibRainDeploy.sol:272-309: checkResolvedAddresses guards only readCalls.length != expectedAddresses.length, then for (uint256 i = 0; i < readCalls.length; i++). With both lengths zero the guard passes and the loop body never runs, so the function returns having asserted nothing. checkResolvedAddressesOnNetworks (lines 326-349) guards networks.length == 0 with NoNetworks and repeats only the pairing check before the fork loop, then forks every network, logs "Checking resolved addresses on network: …" per network and returns success. Errors list at lines 17-55 contains no empty-read error; no natspec, README (README.md:173) or CLAUDE.md text declares an empty read set deliberately allowed.

No test covers it. test/src/lib/LibRainDeploy.t.sol:959 testCheckResolvedAddressesLengthMismatchReverts(uint8, uint8) opens with vm.assume(readCallsLength != expectedLength), so equal-and-zero is excluded by construction. Every other call site uses ownerReadCalls() (1 element) or a hand-built 2-element pair (line 857 onwards). Grep across test/ and src/ shows the only callers of checkResolvedAddresses* are the lib itself and this test file — nothing passes new bytes[](0), so nothing pins either the pass or a refusal. The NoNetworks sibling at line 975 is exactly the analogous guard the finding names, and its own comment ("an empty target set can never be mistaken for every read checking out") plus the NoDeployCandidates convention in CLAUDE.md ("refusing an empty list, because a candidate the loop never reaches is a contract whose snapshot nothing anywhere anchors") make fail-closed-on-empty the stated house rule rather than an invented one.

Two caveats that do not refute it but bear on the fix. (1) The proposed fix as written breaks the existing fuzz test: placing if (readCalls.length == 0) revert NoResolvedAddressReads(target) above the pairing check makes fuzz cases like (0, 5) revert with NoResolvedAddressReads where testCheckResolvedAddressesLengthMismatchReverts expects ResolvedAddressesLengthMismatch. The guard must run after the pairing check (or the existing test must exclude readCallsLength == 0). (2) checkResolvedAddresses is internal view and does not take target ordering into account for the on-networks variant — the new guard must also be added explicitly before the fork loop in checkResolvedAddressesOnNetworks, as the fix says.

Severity corrected MEDIUM -> LOW. Value-at-risk: this is a post-deploy verification helper published for consumer deploy scripts; reaching the vacuous pass requires a consumer to hand in an empty read list it built dynamically, and the deployment's primary anchors (derived address, codehash and runtime-code pins across RainDeployVerifySnapshot/RainDeployVerifyChain) are unaffected — the false green is confined to the supplementary "does the deployed contract hold the addresses it resolved" assertion. No in-repo caller can hit it today.