Skip to content

ExampleDeploySuites documents AddressRegistry as the only concrete in the repo, which is no longer true #75

Description

@thedavidmeister

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

test/abstract/ExampleDeploySuites.sol:28-30

Problem

The contract doc justifies borrowing MockDeployableV2 for the fixture's second address with "AddressRegistry is the only concrete in this repo, so the second address comes from MockDeployableV2". The repo now deploys two concretes — src/concrete/AddressRegistry.sol and src/concrete/MigrationRegistry.sol — both declared as candidates in RegistryDeploySuites and both carrying generated snapshots under src/generated/candidate/. The stated reason for the fixture's shape is therefore false, which matters more than an ordinary stale line: the paragraph exists precisely to tell the next reader why the fixture does not use a second real contract, and as written it invites removing MockDeployableV2 in favour of MigrationRegistry on the strength of a premise the file itself asserts.

Proposed fix

Restate the reason as the one that still holds: the second address must be a contract whose deployment the chain fixtures can construct and destroy freely, not a second real registry whose pinned address the chain matrix also asserts about.

test/abstract/ExampleDeploySuites.sol:26-31, before:

/// The `MockDeployableV2` entries exist for one reason: every loop here runs
/// over a list, and proving a loop does not stop at the first entry requires a
/// second entry at a DIFFERENT address. `AddressRegistry` is the only concrete
/// in this repo, so the second address comes from `MockDeployableV2`, which is
/// already on main for `LibRainDeploy`'s own tests. Without it, "the matrix
/// silently checks only the first suite" is undetectable — the failure mode

after:

/// The `MockDeployableV2` entries exist for one reason: every loop here runs
/// over a list, and proving a loop does not stop at the first entry requires a
/// second entry at a DIFFERENT address. It comes from `MockDeployableV2`
/// rather than from this repo's other concrete, `MigrationRegistry`: the chain
/// fixtures etch and empty every address they name, and an address this repo
/// really deploys to is one the real chain group asserts about too. Without a
/// second address, "the matrix silently checks only the first suite" is
/// undetectable — the failure mode

Verification — this finding survived an adversarial refutation pass

Could not refute — the cited text says exactly what the finding claims and is factually false at this commit.

Read in full: /home/gildlab/code/rain-deploy-audit/test/abstract/ExampleDeploySuites.sol. Lines 28-30 read verbatim: "AddressRegistry is the only concrete in this repo, so the second address comes from MockDeployableV2, which is already on main for LibRainDeploy's own tests."

That premise is false at HEAD:

  • /home/gildlab/code/rain-deploy-audit/src/concrete/ contains BOTH AddressRegistry.sol and MigrationRegistry.sol.
  • /home/gildlab/code/rain-deploy-audit/src/generated/candidate/ contains snapshots for both.
  • /home/gildlab/code/rain-deploy-audit/src/abstract/RegistryDeploySuites.sol declares two candidates: addressRegistryCandidate() ("address-registry") and migrationRegistryCandidate() ("migration-registry"), each with its own artifactPath into src/concrete/.
  • CLAUDE.md itself states the scoped exception covers "src/concrete/AddressRegistry.sol and src/concrete/MigrationRegistry.sol" as ordinary deployed contracts, and that script/Build.sol generates "one deploy record per deployed contract — AddressRegistry.sol and MigrationRegistry.sol". So repo conventions contradict the comment rather than excuse it.
  • git log shows MigrationRegistry landed in its own feature commits (e314b96, 7807d08, 5d8c40a) after the fixture's comment was written; the comment was simply not updated.

Refutation angles checked and rejected:

  • Not a misread: grep -rn "only concrete" hits exactly this one line, and it is the load-bearing justification sentence, not an aside.
  • Not correct-as-written under a narrower reading: there is no reading of "the only concrete in this repo" that survives two files in src/concrete/ both declared as deploy candidates.
  • Not already handled elsewhere: nothing in test/ or src/ asserts anything about comment accuracy, and no other doc restates the correct reason.
  • The replacement rationale the finding proposes is grounded in real code, not invented: test/src/abstract/RainDeployVerifyChainCandidate.t.sol:82-97 and RainDeployVerifyChain.t.sol:25-61 show the chain fixtures vm.etch runtime code and vm.etch(addr, hex"") the same addresses the suites name, which is precisely why the second address must be a freely-etchable mock rather than a second real registry the chain group also asserts about.

Severity: correct at LOW. It is documentation-only, in a test-side fixture abstract, with zero deployed-bytecode or on-chain value at risk; its cost is a future reader acting on a false premise and collapsing the loop-does-not-stop-at-first-entry coverage. That is above nothing (the sentence actively argues for a change that would weaken the matrix) but far below anything with production value at risk, so LOW stands.

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