Skip to content

The "nothing to check does not touch five RPC endpoints" early return is asserted by nothing #63

Description

@thedavidmeister

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

src/abstract/RainDeployVerifyChain.sol:110-129

Problem

checkDeployedOnSupportedNetworks returns early when derived.length == 0, and the comment gives the reason: forking to check nothing turns an outage into the failure of an assertion that has no subject, which is the one failure this contract is supposed to be legible against.

Nothing asserts it. RegistryDeployChainTest inherits the contract with an empty releasedSuites(), so the branch is TAKEN on every CI run, but the test passes identically if the early return is deleted — it would simply fork all five networks, find nothing to check on each, and pass. The property under test is the absence of the forks, and no test observes that.

That also makes this the repo's own state today: its only chain-anchored contract exercises this branch and asserts nothing about it, while five RPC endpoints are the stated failure mode it exists to keep separate.

Proposed fix

Add to test/src/abstract/RegistryDeployChain.t.sol (which already inherits the empty declaration this is about):

    /// Nothing to check MUST NOT touch an RPC endpoint. This repo has released
    /// nothing, so this is the branch every CI run takes: forking five networks
    /// to check nothing turns an outage into the failure of an assertion with
    /// no subject, which is the one failure this contract exists to stay
    /// legible against.
    ///
    /// The absence of a fork is what is asserted, because the pass is identical
    /// either way. `vm.activeFork()` reverts when nothing is selected, so the
    /// low-level call failing IS "no network was reached".
    function testChainWithNothingToCheckForksNothing() external {
        (bool activeBefore,) = address(vm).call(abi.encodeWithSignature("activeFork()"));
        assertFalse(activeBefore, "a fork was selected before the call");

        // The inherited matrix, over this repo's own (empty) released set.
        this.testSuitesLiveOnEverySupportedNetwork();

        (bool activeAfter,) = address(vm).call(abi.encodeWithSignature("activeFork()"));
        assertFalse(activeAfter, "the matrix forked a network with nothing to check");
    }

    /// And the early return is about having NOTHING to check, not about the
    /// networks: handed one derivation, the matrix does fork. Without this the
    /// test above is satisfied by a matrix that never forks at all.
    function testChainWithASubjectDoesFork() external {
        DerivedDeploy[] memory derived = new DerivedDeploy[](1);
        derived[0] = DerivedDeploy({
            suite: "forks-something",
            deployedAddress: address(this),
            bytecodeHash: address(this).codehash
        });
        vm.makePersistent(address(this));

        checkDeployedOnSupportedNetworks(derived);

        (bool active,) = address(vm).call(abi.encodeWithSignature("activeFork()"));
        assertTrue(active, "the matrix checked a subject without forking");
    }

DerivedDeploy must be imported from ../../../src/abstract/RainDeployVerifyBase.sol, and the contract needs Test's assertions, which it already inherits through RainDeployVerifyBase.


Verification — this finding survived an adversarial refutation pass

Verified against source and could not refute it.

Code reads as claimed. src/abstract/RainDeployVerifyChain.sol:110-129 is checkDeployedOnSupportedNetworks, whose first statement is if (derived.length == 0) { return; } under a comment that states the reason ("Nothing to check is not a reason to touch five RPC endpoints... turns an outage into the failure of an assertion that has no subject"). The cited lines say exactly what the finding says they say.

Coverage: exactly three contracts inherit RainDeployVerifyChain (grep -rn RainDeployVerifyChain --include=*.sol): RainDeployVerifyChainTest (test/src/abstract/RainDeployVerifyChain.t.sol, ExampleDeploySuites — 2 released suites), RainDeployVerifyChainCandidateTest (1 released suite), and RegistryDeployChainTest (test/src/abstract/RegistryDeployChain.t.sol, an empty body over RegistryDeploySuites). The first two never reach the branch; the third takes it on every run — RegistryDeploySuites.releasedSuites() concatenates LibAddressRegistryReleased.releasedSuites() and LibMigrationRegistryReleased.releasedSuites(), both generated as new DeploySuite[](0), so deriveDeployments returns length 0.

Nothing observes the absence of the forks. grep -rn "activeFork\|forks nothing\|derived.length" test/ src/ script/ returns only the source line itself, the loop bound, and a doc comment in RegistryDeployChain.t.sol asserting the property in prose. Deleting the early return leaves the entire suite passing (all five networks fork, find nothing to check, and return), so the mutant survives with no test killing it.

Refutation attempts that failed:

  • "Already covered elsewhere": no. The two chain contracts that do fork have non-empty released sets; testChainMatrixReachesTheLastSupportedNetwork and testChainMatrixCoversEverySupportedNetwork assert the loop DOES run, which is the opposite branch.
  • "Deliberate documented convention": CLAUDE.md and the RegistryDeployChainTest natspec both state "it forks nothing and passes" as a repo property — they document the behaviour, they do not document a decision to leave it unasserted. A documented property with no assertion is the gap, not a restatement of a convention.
  • "Untestable property": vm.activeFork() reverts when no fork is selected, so absence of a fork is observable; the proposed mechanism works.
  • "No value": weakest refutation, but it fails too — this abstract is published as the package's product and inherited by consumer repos that have released nothing, where the guard is the difference between chain verification needing five RPC endpoints and needing none.

Severity: LOW is correct. Nothing deployed depends on this; the value at risk is CI legibility and an unnecessary five-endpoint dependency for pre-release consumer repos, not on-chain value.

Fix caveat (does not affect validity): the proposed testChainWithASubjectDoesFork makes RegistryDeployChainTest itself fork all five networks, which contradicts the very identity that contract's natspec claims ("this forks nothing") and re-imposes the RPC dependency the guard exists to remove from it. The "does fork with a subject" half belongs in RainDeployVerifyChainTest (which already forks) or a dedicated empty-suites fixture, leaving only the no-fork assertion in RegistryDeployChain.t.sol.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

auditAudit findingpass1Audit pass 1 (whole-repo, 2026-08-15)severity:lowAudit severity: LOW

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions