Skip to content

TestCloneable, the only worked ICloneableV2 fixture, violates the interface's once-only initialize MUST that ICloneableFactoryV4's open-salt safety argument rests on #67

Description

@thedavidmeister

Unit

test/src/concrete/TestCloneable.sol — the reference ICloneableV2 fixture used by ~16 of the repo's flow tests — read against src/interface/ICloneableV2.sol:11-48.

Intent oracle

ICloneableV2, verbatim:

The ICloneableV2 contract MUST ensure that initialize can NOT be called more than once.

and

It is possible for someone to directly deploy an ICloneableV2 and fail to call initialize before other functions are called, and end users MAY NOT realise or know how to confirm a safe deployment state. The ICloneableV2 MUST take appropriate measures to ensure that functions called before initialize are safe to do so, or revert.

ICloneableFactoryV4 then builds its central security argument on that MUST for the open-salt derivation:

Without the msg.sender namespacing, anybody can deploy at this address before the party that intended to, and since clone-and-initialize is atomic and initialize runs exactly once, whoever gets there first sets the clone's state permanently.

and

A front-runner passing the SAME data produces the contract that was intended, initialized with the bytes that were intended, and has done nothing but pay the gas.

Both sentences are only true while the implementation honours the once-only MUST.

Violated property

TestCloneable, the fixture this repo ships as its worked ICloneableV2, does not honour it:

contract TestCloneable is ICloneableV2 {
    bytes public sData;

    function initialize(bytes memory data) external returns (bytes32) {
        sData = data;
        return ICLONEABLE_V2_SUCCESS;
    }
}

There is no once-only guard, so after the factory's atomic clone-and-initialize any account can call initialize again, in any later transaction, and overwrite the clone's state. Under that implementation the V4 open-salt argument inverts: a front-runner who deploys with the intended data has not "done nothing but pay the gas", because the state they set is not permanent and neither is anybody else's.

Before this campaign nothing in the suite exercised the once-only MUST at all, so a non-conformant implementation was both the only fixture and an untested one.

Same file family: TestCloneableFailure also implements ICloneableV2 with no once-only guard, and neither fixture implements the RECOMMENDED typed initialize overload that ICloneableV2 requires to revert InitializeSignatureFn — the error was referenced by no source or test file in the repo at c1c2afd.

Verified repro

Against c1c2afd, forge test:

function testTestCloneableReinitializable() external {
    TestCloneFactory factory = new TestCloneFactory();
    TestCloneable implementation = new TestCloneable();
    address child = factory.cloneDeterministicOpenSalt(address(implementation), hex"1234", bytes32(uint256(1)));
    assertEq(TestCloneable(child).sData(), hex"1234");

    // Anyone at all, in a later transaction.
    vm.prank(address(0xdeadbeef));
    TestCloneable(child).initialize(hex"beef");
    assertEq(TestCloneable(child).sData(), hex"beef");
}

Passes. The clone the factory produced is re-initialized by an unrelated account and its stored data changes from 0x1234 to 0xbeef.

Triage framing

Flagging rather than adjudicating; there are at least two readings and they lead to different fixes.

  • Reading A — the fixture is fine. TestCloneable is deliberately the minimum a factory flow test needs, the MUST is an obligation on implementations rather than on the factory, and the library correctly enforces nothing about it. Then the finding is a documentation one: the fixture should say in its NatSpec that it is deliberately non-conformant, so it is not mistaken for a reference implementation, and the V4 NatSpec's "runs exactly once" should be stated as a condition on the implementation rather than as a fact.
  • Reading B — the fixture is a reference. It is the only worked ICloneableV2 this package publishes and downstream implementers will copy it. Then it should carry the guard.

PR #TBD on branch 2026-08-24-amt-g4-icloneablefactoryv3-newclone adds TestCloneableConformant — a fixture that does honour both MUSTs — and tests that exercise them end to end on a factory-produced clone, without changing TestCloneable. That closes the coverage half either way and leaves the question above open.

Found by adversarial review during the AMT campaign on g4-icloneablefactoryv3-newclone.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    adversarialFound by adversarial reviewauditAudit finding

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions