Skip to content

test: pin that a clone can clone through the same factory during initialize - #263

Open
thedavidmeister wants to merge 2 commits into
mainfrom
2026-09-21-issue-121-nested-clone-reentrancy
Open

thedavidmeister wants to merge 2 commits into
mainfrom
2026-09-21-issue-121-nested-clone-reentrancy

Conversation

@thedavidmeister

Copy link
Copy Markdown
Contributor

Closes #121

cloneAndInitialize makes one external call, child.call(initialize), after
the clone exists and after NewClone is emitted. That call can re-enter the
factory, and an orchestrator clone deploying its own parts during initialization
is the ordinary Rain shape. No fixture could reach the path: every one of them
only stores, emits, returns or reverts, so nothing exercised a nested CREATE2,
a nested occupancy check running while the outer clone already has code, or the
interleaved NewClone logs an indexer would see. No mutation operator can
author a fixture that calls back, so the gap was invisible to the campaign.

  • test/concrete/TestCloneableNestedClone.sol — new: clones a second
    implementation through the factory that is initializing it.
  • testNestedCloneDuringInitialize — pins both addresses against their own
    predictions, both initializations, and the two NewClone logs in order, the
    nested one sent by the outer clone.
  • testNestedCloneAtOccupiedAddressUnwindsOuterDeploy — a nested revert takes
    the whole outer deploy with it, and the outer (deployer, salt) is still free
    afterwards: a retry at a free inner salt lands at the address the outer always
    predicted. The outer runs through the NAMESPACED entry point, whose derivation
    excludes data, which is what lets the retry change the inner salt and keep
    the outer address.

QA

  • Discriminating tests: testNestedCloneDuringInitialize and
    testNestedCloneAtOccupiedAddressUnwindsOuterDeploy
    (test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol).
  • Mutations applied: 1 — a transient-storage re-entrancy guard on the shared
    tail (tload/revert/tstore(0,1) before checkImplementationCode,
    tstore(0,0) before return child), the defensive edit this PR exists to
    make fail. On main it SURVIVES: all 8 suites green, 78 passed / 0 failed. On
    this branch the cloneAndInitialize file goes 9 passed / 2 failed, and the
    two failures are exactly these two tests. Unmutated, the whole suite is 80
    passed / 0 failed.
  • Oracle: ICloneableFactoryV4's clauses on cloneDeterministic /
    cloneDeterministicOpenSalt, which say nothing that forbids re-entry, against
    a library that holds no storage of its own — so the second deploy is governed
    by the same occupancy check as the first, evaluated fresh. The open-salt
    derivation is caller-independent, which is what lets the test predict the
    nested address from outside.
  • Category check: the category is "factory behaviour reachable only from
    inside initialize". Its members are a nested clone that succeeds (first
    test), a nested clone that reverts (second test), and a nested call to a
    predict function — which reads no state the tail mutates and returns the
    same value inside the call as outside, so there is nothing there to break.

Touches test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol, as does
the PR for #120 — both branch off main and append to the same file.

🤖 Generated with Claude Code

thedavidmeister and others added 2 commits September 21, 2026 11:25
…ialize

`cloneAndInitialize` calls the clone after the clone exists and after
`NewClone` is emitted, and the library holds no state, so that call may
re-enter the factory. No fixture called out during `initialize`, so nothing
reached the re-entrant path at all: every existing fixture stores, emits,
returns or reverts.

`TestCloneableNestedClone` clones a second implementation through the factory
that is initializing it. One test pins that both clones land at their own
derivation's address with their own bytes and that the two `NewClone` logs
interleave as an indexer would see them, the outer first and the nested one
sent by the outer clone. The other pins that a nested revert takes the whole
outer deploy with it and leaves its `(deployer, salt)` free, by deploying
there again rather than by reading a code length a revert has already
rolled back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thedavidmeister thedavidmeister self-assigned this Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 29 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 544299f7-de33-4dd6-ad9f-ec5a6f3872f6

📥 Commits

Reviewing files that changed from the base of the PR and between f2d9e5a and cd3d5a4.

📒 Files selected for processing (2)
  • test/concrete/TestCloneableNestedClone.sol
  • test/src/lib/LibICloneableFactoryV4.cloneAndInitialize.t.sol

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

1 participant