Skip to content

InitializationFailed is not raised when the implementation does not implement ICloneableV2.initialize — caller gets bare 0x #61

Description

@thedavidmeister

Unit

LibICloneableFactoryV4.cloneAndInitialize — the mandatory ICloneableV2.initialize return check — reached through both cloneDeterministic and cloneDeterministicOpenSalt (src/lib/LibICloneableFactoryV4.sol:150-170, check at line 166).

Intent oracle

src/interface/ICloneableV2.sol:38-42, on the success sentinel:

If initialization is successful the ICloneableV2 MUST return the keccak256 hash of the string "ICloneableV2.initialize". This avoids false positives where a contract building a proxy, such as an ICloneableFactoryV2, may incorrectly believe that the clone has been initialized but the implementation doesn't support ICloneableV2.

src/lib/LibICloneableFactoryV4.sol:24-26, on the typed error:

Thrown when initialization fails: ICloneableV2.initialize on the fresh clone returned something other than ICLONEABLE_V2_SUCCESS.

src/interface/ICloneableFactoryV3.sol:41-43:

The factory MUST ONLY consider the clone successfully created if initialize returns the keccak256 hash of the string "ICloneableV2.initialize".

Violated property

The stated purpose of the sentinel is to catch "the implementation doesn't support ICloneableV2". For the ordinary shapes of exactly that case, the sentinel comparison at line 166 is never reached: the ICloneableV2(child).initialize(data) call itself fails first, and the caller receives empty revert data (0x) rather than the typed InitializationFailed (0x19b991a8) that the error's own NatSpec describes.

The gap is between "returned something other than ICLONEABLE_V2_SUCCESS" (which reads as covering any non-success outcome, and is the case the sentinel exists for) and what the code actually distinguishes: only a well-formed 32-byte return that is not the sentinel.

Practical consequence: an integrator that cloned a contract which is not an ICloneableV2 cannot tell that apart from an out-of-gas, an unrelated revert inside a legitimate initialize, or any other bare failure. The typed error exists to make that distinction and does not fire.

Verified repro

Run against TestCloneFactory (the pure-delegation concrete already in test/src/concrete/) at c1c2afd, three implementations that all have non-zero code so checkImplementationCode passes:

/// Has code, but no `initialize(bytes)` and no fallback.
contract NoInitialize { uint256 public x; }

/// Has code and a fallback that returns nothing at all.
contract SilentFallback {
    fallback() external { assembly { return(0, 0) } }
}

/// Returns 8 bytes instead of 32.
contract ShortReturn {
    fallback() external { assembly { mstore(0, 1) return(0, 8) } }
}

For each, a raw call so the revert data is observable rather than swallowed:

(bool ok, bytes memory ret) = address(I_FACTORY).call(
    abi.encodeWithSignature(
        "cloneDeterministicOpenSalt(address,bytes,bytes32)",
        address(impl), hex"", bytes32(0)
    )
);
assertFalse(ok);
emit log_named_bytes("revert data", ret);

Observed on all three:

NoInitialize   revert data: 0x
SilentFallback revert data: 0x
ShortReturn    revert data: 0x

InitializationFailed selector: 0x19b991a8

cloneDeterministic behaves identically — the check is in the shared tail.

Mechanism: SilentFallback/ShortReturn return fewer than 32 bytes, so solc's own returndatasize() < 32 guard reverts with no data before the comparison; NoInitialize has no matching selector and no fallback, so the delegatecall reverts and the EIP-1167 runtime bubbles the empty revert through.

Note the sentinel does still work for the case the existing tests cover (TestCloneableFailure returns a well-formed 32-byte non-sentinel word → InitializationFailed). The uncovered shapes are the ones above.

Triage framing

Flagging, not adjudicating. Both readings seem available and I am not the one to pick:

  • By design. The library is a thin factory; bubbling whatever the implementation did is the honest thing, and a bare revert still satisfies "MUST ONLY consider the clone created if initialize returns the hash" because nothing is created. Under this reading the NatSpec on InitializationFailed and on ICLONEABLE_V2_SUCCESS is what wants narrowing, not the code.
  • A defect. The sentinel's documented purpose is specifically "the implementation doesn't support ICloneableV2", which is precisely what produces 0x here, so the typed error should cover it — e.g. a low-level call with an explicit returndatasize() == 32 && word == ICLONEABLE_V2_SUCCESS test, reverting InitializationFailed otherwise.

Either way there is currently no test over any of these three shapes, so whichever behaviour is intended is unpinned.

Found by adversarial review during AMT group g3-libicloneablefactoryv4-clone.

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