run() is only ever tested on paths that fail before broadcasting — nothing pins what it would deploy, or where

[thedavidmeister]

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

src/abstract/RainDeployBroadcast.sol:77-92

Problem

RainDeployBroadcastTest's own docstring states it: "Nothing here broadcasts. Every case is one that fails before deployAndBroadcast is reached." The two cases are both UnknownDeploymentSuite. So no test drives run() through a successful suite selection, and these properties of the repo's only state-changing script (script/Deploy.sol, which is run() and nothing else) are unasserted:

• that the fields of the SELECTED suite are the ones handed to deployAndBroadcast. suite.artifactPath and suite.suite are both string and adjacent in the call, so swapping them compiles and would send forge verify-contract a suite key instead of a contract path; nothing would notice.
• that deployNetworks() — the documented override point for repos that bootstrap one chain per dispatch — is what is broadcast to. testDeployNetworksDefaultsToSupportedNetworks asserts the default value of that function in isolation, never that run() calls it rather than LibRainDeploy.supportedNetworks() directly. An override that run() ignored would broadcast a suite to five chains when the repo asked for one.

testSelectedSuiteCarriesTheRecordedPins asserts things about suiteByName, not about run().

Proposed fix

The env vars are process-global and this repo deliberately confines every vm.setEnv to one test function to avoid the race documented there, so the success leg is appended to that same function rather than given its own.

1. New fixture test/concrete/ExampleDeploySingleNetwork.sol:

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

import {RainDeployBroadcast} from "../../src/abstract/RainDeployBroadcast.sol";
import {LibRainDeploy} from "../../src/lib/LibRainDeploy.sol";
import {ExampleDeploySuites} from "../abstract/ExampleDeploySuites.sol";

/// @title ExampleDeploySingleNetwork
/// A repo that bootstraps one chain per dispatch, which is what
/// `deployNetworks()` is overridable for. A broadcast that reached a network
/// this does not name would be a suite deployed somewhere the repo did not ask
/// for.
contract ExampleDeploySingleNetwork is ExampleDeploySuites, RainDeployBroadcast {
    /// @inheritdoc RainDeployBroadcast
    function deployNetworks() internal pure override returns (string[] memory networks) {
        networks = new string[](1);
        networks[0] = LibRainDeploy.ARBITRUM_ONE;
    }
}

2. Append to RainDeployBroadcastTest.testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults (and rename it testRunSelectsTheSuiteFromTheEnvBeforeTheKeyNeverDefaultsAndBroadcastsIt), after the existing two legs:

        // ## Then the suite it names, actually broadcast
        //
        // The half above decides WHAT would be deployed; this is the half that
        // says the decision is carried through. Sequenced in the same test for
        // the same reason the first two are: `vm.setEnv` writes the forge
        // PROCESS' environment and forge runs tests concurrently.
        ExampleDeploySingleNetwork single = new ExampleDeploySingleNetwork();
        address expectedAddress = LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode);
        bytes32 expectedCodeHash = keccak256(type(MockDeployableV2).runtimeCode);

        vm.setEnv("DEPLOYMENT_SUITE", "second-address-candidate");
        vm.setEnv("DEPLOYMENT_KEY", "0xa11ce");

        single.run();

        // The suite `DEPLOYMENT_SUITE` named is the suite on chain, at the
        // address its own creation code derives and with the code hash that
        // creation code produces — so the selection reached the broadcast
        // rather than some other entry of the registry.
        assertEq(expectedAddress.code.length > 0, true);
        assertEq(expectedAddress.codehash, expectedCodeHash);

        // The OVERRIDE is what was broadcast to. A completed run leaves the
        // last network of `deployNetworks()` selected, so the chain id here is
        // arbitrum for the override and polygon for `supportedNetworks()` — the
        // only observation that tells an honoured override from an ignored one.
        assertEq(block.chainid, 42161);

        // And the suite the OTHER declaration selects is untouched, so the run
        // deployed the one key it was given and not the registry.
        assertEq(
            LibRainDeploy.zoltuAddress(sDeploy.externalSuiteByName("address-registry-candidate").creationCode).code.length,
            0
        );

with MockDeployableV2 and ExampleDeploySingleNetwork added to the imports. A follow-up test in the same function can assert suite.artifactPath reaches the verification command by asserting on the console output is not available — instead pin it structurally by asserting single.externalSuiteByName("second-address-candidate").artifactPath equals "test/concrete/MockDeployableV2.sol:MockDeployableV2" immediately before the run, so the value forwarded is named in the same test.

---

Verification — this finding survived an adversarial refutation pass

SURVIVES (tried hard to refute; could not on the substance).

Facts verified against source:

• /home/gildlab/code/rain-deploy-audit/src/abstract/RainDeployBroadcast.sol:77-92 — run() is suiteByName(envOr(DEPLOYMENT_SUITE)), envUint(DEPLOYMENT_KEY), then one LibRainDeploy.deployAndBroadcast(vm, deployNetworks(), key, suite.creationCode, suite.artifactPath, suite.storedDeployedAddress, suite.storedBytecodeHash, suite.dependencies). Nine lines of pure wiring.
• grep -rn "run()" test/ gives exactly two call sites, both in /home/gildlab/code/rain-deploy-audit/test/src/abstract/RainDeployBroadcast.t.sol (lines 94, 105), both preceded by vm.expectRevert(UnknownDeploymentSuite...). The file's own docstring states the gap. So the finding's core factual claim is exact: no test reaches deployAndBroadcast through run().
• The two tests offered as substitutes do not cover the wiring. testSelectedSuiteCarriesTheRecordedPins asserts on externalSuiteByName(...) fields; testDeployNetworksDefaultsToSupportedNetworks asserts on externalDeployNetworks() (an ExampleDeploy wrapper). Neither observes run(). deployNetworks() is never overridden anywhere in the repo, so the virtual's only reason to exist — the documented one-chain-per-dispatch override — has no test that it is what run() consults. That mutant (run() calling LibRainDeploy.supportedNetworks() directly) survives the whole suite today.
• The gap is not "untestable". The test file's stated rationale ("a key, an RPC and real money") is contradicted by its own sibling: /home/gildlab/code/rain-deploy-audit/test/src/lib/LibRainDeploy.t.sol drives the full success path — testDeployToNetworksMultipleNetworks and testDeployAndBroadcastUsesDeployerFromPrivateKey fork real Base/Arbitrum, use key 0xA11CE, and run vm.startBroadcast/deployZoltu for real on the fork. forge test broadcasts nothing off-machine, so no money is at risk. A documented gap is still a gap, and this one's documented justification is false in this repo's own terms — that does not qualify as the "deliberate documented convention" exemption.

Partial refutations that do NOT sink it, but that the fix should absorb:

• The suite.artifactPath / suite.suite swap claim is materially weaker than presented. contractPath is consumed by deployToNetworks in exactly one place (LibRainDeploy.sol:448-453), a console2.log of a manual forge verify-contract line. Nothing on chain depends on it, and no success test can pin it — the proposed fix concedes this and falls back to asserting externalSuiteByName(...).artifactPath, which is a verbatim restatement of the existing testSelectedSuiteCarriesTheRecordedPins and pins nothing about run(). That leg of the fix is vacuous and should be dropped.
• A wrong-field combination is partly guarded downstream: deployToNetworks (LibRainDeploy.sol:397-400) reverts UnexpectedDeployedAddress before forking whenever the passed creation code and recorded address disagree, so only a wholly-consistent wrong-suite substitution reaches a chain.
• Minor inaccuracy in the description: script/Deploy.sol is not the repo's only state-changing script (script/Build.sol has a run() that rewrites the generated snapshots); it is the only chain-state-changing one.

Severity left at MEDIUM. Value at risk is the org's only broadcast entry point, inherited by every downstream deploy repo, with zero success-path coverage; the unguarded residue is an override that run() could silently ignore (a consumer asking for one chain getting five, with a mid-run revert leaving a partial dispatch) and a wrong verify-contract path in the operator's log. Bounded by CREATE2 deploys being deterministic, idempotent and permissionless, and by the pin guards inside deployToNetworks — which is why it is not HIGH.