Skip to content

NewClone is declared by three published interfaces under two signatures; ICloneableFactoryV1 and V2 share one topic0 #65

Description

@thedavidmeister

Unit

src/interface/ICloneableFactoryV3.sol:31 — the NewClone event declaration — read against the rest of the published interface family (src/interface/ICloneableFactoryV2.sol:15, src/interface/deprecated/ICloneableFactoryV1.sol:15).

Intent oracle

ICloneableFactoryV3's NatSpec on NewClone:

Carries the full deterministic deploy so an indexer can reconstruct it from the event alone — without reading calldata or relying on the implementation to emit its own init event.

and on the interface itself:

Creates EIP-1167 proxy clones … and emits events so indexers can discover them.

ICloneableFactoryV4 extends that with an explicit instruction to indexers:

An indexer that wants to verify the address rather than take the emitted one MUST pick the derivation: the two cannot both produce the emitted clone, so trying both and keeping the match is well defined.

So the event is designed as the primary, self-sufficient discovery channel for third-party indexers.

Violated property

This package publishes three interfaces that each declare an event named NewClone, under two different signatures:

interface signature topics[0]
deprecated/ICloneableFactoryV1 NewClone(address,address,address) A
ICloneableFactoryV2 NewClone(address,address,address) A (identical)
ICloneableFactoryV3 (and V4 by inheritance) NewClone(address,address,address,bytes32,bytes) B

Two consequences:

  1. ICloneableFactoryV1 and ICloneableFactoryV2 are indistinguishable on the wire. Their NewClone is byte-for-byte the same topic with the same data layout, so an indexer that has both ABIs cannot tell which interface a log came from. Their normative text differs (V1 promises only "EIP1167 proxy as used by Open Zeppelin is recommended"; V2 adds the ICloneableV2 success-hash requirement), so the two are not interchangeable specifications even though they are interchangeable logs.

  2. Name-keyed tooling is ambiguous. A contract may legally implement ICloneableFactoryV2 and ICloneableFactoryV3 at once, and the resulting ABI then carries two entries called NewClone. Common indexer/client idioms key on the name (contract.on("NewClone", …), contract.filters.NewClone, a subgraph eventHandlers entry) and are ambiguous or error out on such an ABI.

Related, and part of the same published-surface question: ICloneableFactoryV2.clone carries normative text ("MUST call ICloneableV2.initialize atomically … MUST emit NewClone") for a function no code in this repo implements — LibICloneableFactoryV4 has no clone — so the V2 spec is published without an implementation on this half of the split.

Verified repro

Against c1c2afd in a clean checkout, forge test:

// Both interfaces can be carried by one contract; this compiles.
abstract contract DualFactory is ICloneableFactoryV2, ICloneableFactoryV3 {}

function testV1V2SameTopic() external pure {
    assertEq(ICloneableFactoryV1.NewClone.selector, ICloneableFactoryV2.NewClone.selector);
    assertTrue(ICloneableFactoryV2.NewClone.selector != ICloneableFactoryV3.NewClone.selector);
}

testV1V2SameTopic passes: V1 and V2 are the same topic, V3 is a different one.

DualFactory compiles, and its compiled ABI contains two NewClone entries:

$ grep -o '"type":"event","name":"NewClone"' out/…/DualFactory.json | wc -l
2
$ grep -o 'NewClone(address,address,address[^"]*' out/…/DualFactory.json | sort -u
NewClone(address,address,address)
NewClone(address,address,address,bytes32,bytes)

(The wc -l above counts 2 in the abi array; the artifact repeats the ABI in rawMetadata, hence 4 matches across the whole file.)

Triage framing

Flagging rather than adjudicating. This may be entirely intended — the interfaces are versioned separately, topics[0] does separate V3/V4 from V1/V2, and V1/V2 sharing a topic may be a deliberate consequence of V2 being a pure re-specification of V1's shape. Equally it may be an accident that has to be lived with, since all three are already published and cannot be renamed without breaking existing consumers.

What a maintainer might want to decide:

  • Whether the V1/V2 topic identity should be documented on both interfaces, so a consumer knows the topic alone does not identify the spec.
  • Whether ICloneableFactoryV3's NatSpec should warn that a factory implementing V2 as well produces a name-ambiguous ABI, given the interface leans on indexers as its consumer.
  • Whether ICloneableFactoryV2's implementation-free normative text belongs in this half of the library/deploy split at all.

Found by adversarial review during the AMT campaign on g4-icloneablefactoryv3-newclone. No behaviour of LibICloneableFactoryV4 is wrong; this is about the published surface the library's event is part of.

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