deployToNetworks reports success without deploying when expectedAddress is stale relative to creationCode

[thedavidmeister]

Found by an adversarial mutation-testing pass over the whole repo at 4422e29
(2 commits past sol-v0.1.4).

Summary

LibRainDeploy.deployToNetworks decides whether to deploy purely from
expectedAddress.code.length == 0. It never checks that expectedAddress is
the address the supplied creationCode would actually deploy to.

The Zoltu factory is CREATE2 with a zero salt — disassembling
ZOLTU_FACTORY_BYTECODE gives CALLDATACOPY then
CREATE2(callvalue, 0, calldatasize, 0) — so the deployed address is a pure
function of the creation code:

address = keccak256(0xff ++ ZOLTU_FACTORY ++ bytes32(0) ++ keccak256(creationCode))[12:]

That value is computable in-library, deterministically, on every path. Because
it is never computed, a consumer whose pinned expectedAddress /
expectedCodeHash constants have drifted behind their contract's
creationCode gets a clean, silent no-op: the OLD contract is still at the OLD
address with the OLD code hash, so every network takes the skip branch, the
post-hoc code-hash check passes against the old contract, and the script
exits 0 having deployed nothing.

This contradicts the guarantee the README states:

Pre-calculated addresses asserted post-deploy: silent failures fail loudly.

Why the inputs can drift

creationCode is live (type(X).creationCode, recompiled on every run), while
expectedAddress / expectedCodeHash are committed constants in the consumer
repo. Those two can disagree, and that is exactly what happens when a
contract's bytecode changes and the pinned constants are not regenerated. On
the deploy path the mismatch is caught loudly (UnexpectedDeployedAddress); on
the skip path it is not caught at all.

rain.deploy is inherited across the org (raindex, rainlang, rain.factory,
rain.metadata, rain.math.float, rain.extrospection, rain.tofu.erc20-decimals,
st0x.deploy), so this sits on the path of every deploy script.

Repro

Verified against 4422e29. Add to test/src/lib/:

// A different contract from MockDeployable, standing in for a bytecode bump.
contract MockDeployableV2 {
    uint256 public value = 43;
    uint256 public other = 99;
}

function zoltuCreate2Address(bytes memory creationCode) internal pure returns (address) {
    return address(
        uint160(
            uint256(
                keccak256(
                    abi.encodePacked(
                        bytes1(0xff), LibRainDeploy.ZOLTU_FACTORY, bytes32(0), keccak256(creationCode)
                    )
                )
            )
        )
    );
}

/// The Zoltu factory is CREATE2 with a zero salt, so the deployed address is
/// derivable from the creation code alone.
function testZoltuIsCreate2SaltZero() external pure {
    assertEq(zoltuCreate2Address(type(MockDeployable).creationCode), MOCK_DEPLOYABLE_ADDRESS);
}

function testStaleExpectedAddressSilentlySkipsDeploy() external {
    vm.makePersistent(address(this));
    vm.createSelectFork(LibRainDeploy.ARBITRUM_ONE);
    address deployed = this.externalDeployZoltu(type(MockDeployable).creationCode);
    assertEq(deployed, MOCK_DEPLOYABLE_ADDRESS);
    vm.makePersistent(deployed);

    string[] memory networks = new string[](1);
    networks[0] = LibRainDeploy.ARBITRUM_ONE;
    address[] memory dependencies = new address[](0);

    address result = this.externalDeployToNetworks(
        networks,
        address(this),
        // The NEW contract's creation code...
        type(MockDeployableV2).creationCode,
        "",
        // ...paired with the OLD contract's address and code hash.
        MOCK_DEPLOYABLE_ADDRESS,
        MOCK_DEPLOYABLE_CODEHASH,
        dependencies
    );

    // Reports success.
    assertEq(result, MOCK_DEPLOYABLE_ADDRESS);
    // But the new contract does not exist anywhere.
    assertEq(zoltuCreate2Address(type(MockDeployableV2).creationCode).code.length, 0);
    // And the address it reported does not hold the requested code.
    assertTrue(result.codehash != keccak256(type(MockDeployableV2).runtimeCode));
}

where

address constant MOCK_DEPLOYABLE_ADDRESS = 0xC24016f209562fc151e5Ab7F88694ED5775feb36;
bytes32 constant MOCK_DEPLOYABLE_CODEHASH = 0xc1a263a0b50505687a5140c7964ec5c947329e7d03410306fee68cc3620c5483;

Both pass. testStaleExpectedAddressSilentlySkipsDeploy logs:

Deploying to network: arbitrum
 - Code already exists at expected address, skipping deployment
 - Final Address: 0xC24016f209562fc151e5Ab7F88694ED5775feb36
 - Verifying code hash

No revert, no warning, and MockDeployableV2 was never deployed to any
network.

Proposed fix

Derive the CREATE2 address from creationCode once and assert it equals
expectedAddress before branching on expectedAddress.code.length, so the
mismatch is caught on the skip path as well as the deploy path, and before any
broadcast is simulated:

address derived = address(
    uint160(
        uint256(
            keccak256(
                abi.encodePacked(bytes1(0xff), ZOLTU_FACTORY, bytes32(0), keccak256(creationCode))
            )
        )
    )
);
if (derived != expectedAddress) {
    revert UnexpectedDeployedAddress(expectedAddress, derived);
}

Related, for the same triage

The skip branch also bypasses the dependency and Zoltu-factory checks entirely
(documented in the deployToNetworks NatSpec and covered by
testDeployToNetworksSkipsAlreadyDeployedWithMissingDependency). That is a
deliberate choice for idempotency, but it means the README question "Are the
dependencies of the current deployment available on this network?" is silently
not answered for any already-deployed network. Worth confirming that is still
the intent alongside the fix above.

[thedavidmeister]

Fixed on main by #26 — closing.

deployToNetworks now derives the address from the creation code and refuses to proceed when it disagrees with the recorded one, before any fork:

address derivedAddress = zoltuAddress(creationCode);
if (derivedAddress != expectedAddress) {
    revert UnexpectedDeployedAddress(expectedAddress, derivedAddress);
}

The comment above it states the failure mode this issue reported: "before any fork, because otherwise a network that already has some other contract at the expected address takes the skip branch and reports success without ever deploying."

So the silent no-op is gone — a stale pin can no longer reach the skip branch at all, and therefore cannot reach the post-hoc code-hash check that would have passed against the old contract.

Covered by two tests on main: testDeployToNetworksStaleExpectedAddressReverts and testDeployToNetworksStaleExpectedAddressRevertsBeforeForking — the second pins that it fails without an RPC round trip.

zoltuAddress is also now the parameter the whole deploy-verification layer is built on (src/abstract/RainDeployVerify*), so the derivation this issue asked for is load-bearing well beyond this one call site.