Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🛠 CreateXDeployer Specifications (Old Title: RFC: Switch Contract Deployment to CREATE2 Deploy) #129

Closed

Conversation

mdehoog
Copy link

@mdehoog mdehoog commented Jul 24, 2023

The Base team is truly sorry for what happened in #128. We've since added some information about this footgun in https://docs.base.org/tools/bridges. However, that doesn't solve the issue for this library.

This PR proposes somewhat of a new beginning for this library: using CREATE2 to deploy it (using Zoltu's deterministic-deployment-proxy). This means that anybody can deploy it to the canonical address 0xF49600926c7109BD66Ab97a2c036bf696e58Dbc2 on any EVM chain. It also removes the ability to accidentally bump the nonce of the deployment address, for whatever reason.

I've deployed and verified it to 5 chains as examples, linked in the README (we'd be happy to rally the community to get them all done if there's alignment that this is a good idea).

The caveat to this is, because the canonical address is changed, this is a breaking change to all existing users of this library. I'd suggest bumping the major version, or even spinning this (and https://github.com/pcaversaccio/xdeployer/) out into a new library, and keeping the old ones around but marking them as deprecated.

I've also removed the Pausable and Ownable features of this contract to make it more credibly neutral. Would love thoughts / feedback on this.

@pcaversaccio pcaversaccio added enhancement New feature or request help wanted Extra attention is needed labels Jul 24, 2023
@pcaversaccio pcaversaccio self-assigned this Jul 24, 2023
@pcaversaccio pcaversaccio changed the title RFC: switch contract deployment to CREATE2 deploy 🛠 RFC: Switch Contract Deployment to CREATE2 Deploy Jul 24, 2023
@pcaversaccio
Copy link
Owner

pcaversaccio commented Jul 24, 2023

@mdehoog thanks for starting this discussion. The recommended approach has at least 2 flaws:

  • 0x7A0D94F55792C434d74a40883C6ed8545E406D12 (Deterministic Deployment Proxy) cannot be replayed on newer EVM chains such as Canto (due to EIP-155; also see here). This reduces the powerfulness of this approach since it requires 0x7A0D94F55792C434d74a40883C6ed8545E406D12 to be deployed.
  • If we go that route, we should think about a more powerful Create2Deployer setup that includes also an initialisation possibility, create3 and redeployment protections (the redeployment efforts must be worth it). I have something like the following in mind:
    /*//////////////////////////////////////////////////////////////
                                 CREATE
    //////////////////////////////////////////////////////////////*/

/// @dev Deploys a contract using `CREATE`.
function deployCreate(bytes calldata initCode) external payable returns (address addr);

/// @dev Deploys a contract using `CREATE` and initialises it.
function deployCreateAndInit(bytes calldata initCode, bytes calldata data) external payable returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate` or `deployCreateAndInit` by `address(this)`.
function computeCreateAddress(uint256 nonce) external view returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate` or `deployCreateAndInit` by `deployer`.
function computeCreateAddress(address deployer, uint256 nonce) external view returns (address addr);

    /*//////////////////////////////////////////////////////////////
                                 CREATE2
    //////////////////////////////////////////////////////////////*/

/// @dev Deploys a contract using `CREATE2`.
function deployCreate2(bytes32 salt, bytes calldata initCode) external payable returns (address addr);

/// @dev Deploys a contract using `CREATE2` and initialises it.
function deployCreate2AndInit(bytes32 salt, bytes calldata initCode, bytes calldata data) external payable returns (address addr);

/// @dev Deploys a contract using `CREATE2` and a chain-specific guard.
function deployCreate2Guarded(bytes32 salt, bytes calldata initCode) external payable guard(salt) returns (address addr);

/// @dev Deploys a contract using `CREATE2` and a chain-specific guard, and initialises it.
function deployCreate2AndInitGuarded(bytes32 salt, bytes calldata initCode, bytes calldata data) external payable guard(salt) returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate2`, `deployCreate2AndInit`, `deployCreate2Guarded`, or `deployCreate2AndInitGuarded` by `address(this)`.
function computeCreate2Address(bytes32 salt, bytes32 codeHash) external view returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate2`, `deployCreate2AndInit`, `deployCreate2Guarded`, or `deployCreate2AndInitGuarded` by `deployer`.
function computeCreate2AddressWithDeployer(bytes32 salt, bytes32 codeHash, address deployer) external pure returns (address addr);

    /*//////////////////////////////////////////////////////////////
                                 CREATE3
    //////////////////////////////////////////////////////////////*/

/// @dev Deploys a contract using `CREATE3` and a frontrunning guard.
function deployCreate3(bytes32 salt, bytes calldata initCode) external payable onlyMsgSender(salt) returns (address addr);

/// @dev Deploys a contract using `CREATE3` and a frontrunning guard, and initialises it.
function deployCreate3AndInit(bytes32 salt, bytes calldata initCode, bytes calldata data) external payable onlyMsgSender(salt) returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate3` or `deployCreate3AndInit` by `address(this)`.
function computeCreate3Address(bytes32 salt, bytes32 codeHash) external view returns (address addr);

/// @dev Returns the address where a contract will be stored if deployed via `deployCreate3` or `deployCreate3AndInit` by `deployer`.
function computeCreate3AddressWithDeployer(bytes32 salt, bytes32 codeHash, address deployer) external pure returns (address addr);

    /*//////////////////////////////////////////////////////////////
                                 ERRORS
    //////////////////////////////////////////////////////////////*/

/// @dev Error that occurs when an invalid salt parameter is provided.
error InvalidSalt(address emitter);

    /*//////////////////////////////////////////////////////////////
                                 MODIFIERS
    //////////////////////////////////////////////////////////////*/

/// @dev Modifier that prevents redeploying a specific contract to another chain at the same address.
modifier guard(bytes32 salt) {
    salt = keccak256(abi.encode(msg.sender, block.chainid, salt));
    _;
}

/// @dev Modifier that ensures that the first 20 bytes of a submitted salt match those of the calling account.
modifier onlyMsgSender(bytes32 salt) {
    if (address(bytes20(salt)) != msg.sender) revert InvalidSalt(address(this));
    _;
}

I would like to add @mds1 to this discussion since part of the above ideas were shared by him. Please let me know your thoughts @mdehoog and @mds1.

@mds1
Copy link

mds1 commented Jul 24, 2023

Thanks for looping me in @pcaversaccio. Love to see the support from @mdehoog and the Base team about getting the community to rally around a new deployer.

If we go that route, we should think about a more powerful Create2Deployer setup that includes also an initialisation possibility, create3 and redeployment protections (the redeployment efforts must be worth it).

I agree with this. There's a set of features we can include in a deterministic deployer that will satisfy the vast majority of use cases, and no current universal deployer has this full set. So if we can write/deploy one and rally the community around it I think that'd be a really great public good, and save contract devs lots of time/effort. This set of features is a set of deployment options for users:

  • Deployment method: Create2 or Create3
  • Initialization: data: Option to provide data to call on the contract after deployment
  • Contract type: Standard contract, EIP-1167 minimal proxy, minimal proxy with immutables (1, 2)
  • Salt source: No salt, user provided salt, pseudo-random (e.g. keccak256(block.timestamp, msg.sender)). There's an argument to be made for only supporting user-provided salt and making this the responsibility of the integrator, and I lean towards that option to simplify the deployer. Main counterargument is that you can reduce L2 calldata by 32 bytes if you don't need to provide a salt, and since L2 calldata is pricey it might be worth just having salt/no-salt overloads
  • Salt options: Don't modify, hash with chain.id, hash with msg.sender, hash with both chain.id and msg.sender

This covers everything I've seen protocols need

There's three main challenges here:

  • Admittedly this is a lot of options, so coming up with a clean interface might be a bit tricky, but should be doable.
  • Create3 and Clones with immutables aren't well-audited yet. Hopefully they get audited by the Cantina Solady audit, because these are things I've seen a lot of projects want, but not use due to the lack of audit for them
  • Deployment method: My approach would be to have a private key and publish a pre-signed tx. This lets anyone deploy, and for chains that are unique (Arbitrum's gas metering, Celo's EIP-155 requirement, etc.) we still have the private key to fall back on. An alternative is deploying through an existing deterministic deployer, but this makes it hard to deploy on non-standard chains since those deployers use one-time keys (nick's method) so you need to convince the chain to administratively place the contract. (This does happen though, Arbitrum and Celo both placed 0x4e59b44847b379578588920ca78fbf26c0b4956c)

@mdehoog
Copy link
Author

mdehoog commented Jul 25, 2023

Thank you both for the thoughtful comments. Agree on the tradeoffs on private key deployments vs not. Benefit is avoiding bricking the deployment address, but if that comes with too many compatibility issues then that's a problem.

Specifically regarding the Canto example: I believe Canto actually supports pre-155 Txs, but there's a bug in ethermint that means nodes cannot opt-in to accepting them. I'm running a modified Canto node that is accepting these transactions. Somebody has prefunded the Arachnid proxy address on Canto. Unfortunately this transaction was signed with a gas price of 100 gwei, but Canto passed a governance vote to set the minimum to 1000 gwei, so the tx is erroring with provided fee < minimum global fee (10000000000000000 < 100000000000000000).

I imagine that many chains are similar in that they reject pre-155 txs at the RPC layer but not the P2P layer, so there's usually a way of including them (or falling back to a governance / admin vote if absolutely required).

I'm hopeful that we can get a version of the deployment proxy across most chains if we change the parameters slightly for maximal compatibility, but it's likely never going to be as compatible as the ability to fallback on a signature.

@pcaversaccio
Copy link
Owner

pcaversaccio commented Jul 26, 2023

So if we can write/deploy one and rally the community around it I think that'd be a really great public good, and save contract devs lots of time/effort.

100%.

Deployment method: Create2 or Create3

For the sake of completeness, we should also offer a CREATE-based deployment method. I personally sometimes use it, if I want to redeploy an init code from e.g. an attack to see whether there are similar contracts (via Etherscan) on eg testnets. I've actually built a repo here some time ago already. The use case is limited but if we build something universal, it should be as complete as possible imo. And the small additional deployment costs are a non-issue.

Initialization: data: Option to provide data to call on the contract after deployment

Agreed, but this will lead to a reentrancy possibility. We should not add a mutex lock to save runtime costs and inform the folks that leverage the universal deployer to ensure at the protocol level that potentially malicious reentrant calls do not affect their smart contract system. Something like here:

    /**
     * @dev Deploys and initialises a new contract via calling the `CREATE2` opcode and
     * using the salt value `salt`, the creation bytecode `initCode`, `msg.value`, and
     * initialisation code `data` as inputs. In order to save deployment costs, we do not
     * sanity check the `initCode` length. Note that if `msg.value` is non-zero, `initCode`
     * must have a `payable` constructor.
     * @param salt The 32-byte random value used to create the contract address.
     * @param initCode The creation bytecode.
     * @param data The initialisation code that is passed to the deployed contract.
     * @return newContract The 20-byte address where the contract was deployed.
     * @custom:security This function allows for reentrancy, however we refrain from adding
     * a mutex lock to keep it as use-case agnostic as possible. Please ensure at the protocol
     * level that potentially malicious reentrant calls do not affect your smart contract system.
     */
    function deployCreate2AndInit(bytes32 salt, bytes memory initCode, bytes calldata data)
        public
        payable
        returns (address newContract)
    {
        // solhint-disable-next-line no-inline-assembly
        assembly ("memory-safe") {
            newContract := create2(callvalue(), add(initCode, 0x20), mload(initCode), salt)
        }
        if (newContract == address(0)) revert FailedContractCreation(address(this));
        emit ContractCreation(newContract);

        // solhint-disable-next-line avoid-low-level-calls
        (bool success,) = newContract.call(data);
        if (!success) revert FailedContractInitialisation(address(this));
    }

Contract type: Standard contract, EIP-1167 minimal proxy, minimal proxy with immutables (1, 2)

I'm not yet sure about the minimal proxies (including 0age's version) since certain chains are not able to support it due to bytecode hash format. See e.g. zkSync, where we have the following requirements that are not satisfied by the minimal proxies (reference):
Each zkEVM bytecode must adhere to the following format:

  • Its length must be divisible by 32.
  • Its length in words (32-byte chunks) should be odd. In other words, bytecodeLength % 64 == 32.

Also, for these chains the CREATE behaviour differs from fully equivalent chains (see my thread here; TL;DR: no RLP encoding of address and nonce happens and a prefix is added before keccak256 hashing), thus a deployment of the universal deployer wouldn't lead to the same address as on all other chains. Maybe the best solution for these issues is simply to take the following assumption: "We only care about fully EVM equivalent chains and make it work there."

Salt source: No salt, user provided salt, pseudo-random (e.g. keccak256(block.timestamp, msg.sender)). There's an argument to be made for only supporting user-provided salt and making this the responsibility of the integrator, and I lean towards that option to simplify the deployer. Main counterargument is that you can reduce L2 calldata by 32 bytes if you don't need to provide a salt, and since L2 calldata is pricey it might be worth just having salt/no-salt overloads

A simple overload seems reasonable since the goal is to provide a universal deployer.

Salt options: Don't modify, hash with chain.id, hash with msg.sender, hash with both chain.id and msg.sender

Why not using additionally (to generate further some pseudorandomness), PREVRANDAO?

Create3 and Clones with immutables aren't well-audited yet.

If this is an issue we could maybe start a community effort for a public review? Or we apply for Optimism retroPGF round3 and use the funds for an audit?

Deployment method: My approach would be to have a private key and publish a pre-signed tx.

So the worst thing that can happen here is that the private key gets leaked, somebody deploys a malicious contract with the same function selectors on the chain using the nonce 0, and social engineer people into using it. I agree on publishing a presigned tx, but maybe, for additional security, we should use Shamir Secret Sharing to split the private key across multiple people (e.g. 3), with a recovery possibility if 2 people come together. Thoughts?

Thank you both for the thoughtful comments. Agree on the tradeoffs on private key deployments vs not. Benefit is avoiding bricking the deployment address, but if that comes with too many compatibility issues then that's a problem.

After deploying on around 70 chains, I have a certain experience to claim that there will always be compatibility issues. We wanted full EVM equivalence, but we got quasi-EVM equivalence unfortunately for many chains...

I imagine that many chains are similar in that they reject pre-155 txs at the RPC layer but not the P2P layer, so there's usually a way of including them (or falling back to a governance / admin vote if absolutely required).

I agree but don't underestimate the complexity to include it on the P2P layer. If you got a permissioned chain with only a few nodes running you have to contact them first (at least that's my experience)... the overhead can be quite big and we want to keep the deployment KISS imo.

I'm hopeful that we can get a version of the deployment proxy across most chains if we change the parameters slightly for maximal compatibility, but it's likely never going to be as compatible as the ability to fallback on a signature.

I feel fine with sacrificing the beauty of a keyless deployment (in that case) for having a fallback possibility, particularly to be future-proof.

@mdehoog
Copy link
Author

mdehoog commented Jul 26, 2023

Makes sense, we're aligned on keeping it a non-keyless deployment. For the current iteration of the contract, Base will attempt to add it at 0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2 in a future hardfork if possible.

Feel free to close this PR, but given the useful discussion here will leave that to your discretion 🙌

@pcaversaccio
Copy link
Owner

For the current iteration of the contract, Base will attempt to add it at 0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2 in a future hardfork if possible.

Oh, that's awesome news! Lmk if there is anything that I can do to support this process. Also, for my understanding, would you add the same contract as on Base Testnet (0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2) to 0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2 on Base mainnet? Or the one proposed in this PR?

Feel free to close this PR, but given the useful discussion here will leave that to your discretion 🙌

I will keep this PR open (a really fruitful discussion tbh) in order to further discuss the specs with @mds1 (and others that I will bring in). If you have anything to add wrt the specs, please keep sharing them as you have already done until now (or maybe someone else from the Base team wants to participate as well?).

@pcaversaccio pcaversaccio changed the title 🛠 RFC: Switch Contract Deployment to CREATE2 Deploy 🛠 CreateXDeployer Specifications (Old Title: ~RFC: Switch Contract Deployment to CREATE2 Deploy~) Jul 26, 2023
@pcaversaccio pcaversaccio changed the title 🛠 CreateXDeployer Specifications (Old Title: ~RFC: Switch Contract Deployment to CREATE2 Deploy~) 🛠 CreateXDeployer Specifications (_Old Title:_ ~~RFC: Switch Contract Deployment to CREATE2 Deploy~~) Jul 26, 2023
@pcaversaccio pcaversaccio changed the title 🛠 CreateXDeployer Specifications (_Old Title:_ ~~RFC: Switch Contract Deployment to CREATE2 Deploy~~) 🛠 CreateXDeployer Specifications (Old Title: RFC: Switch Contract Deployment to CREATE2 Deploy) Jul 26, 2023
@mdehoog
Copy link
Author

mdehoog commented Jul 26, 2023

Also, for my understanding, would you add the same contract as on Base Testnet (0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2) to 0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2 on Base mainnet? Or the one proposed in this PR?

Initial thought was the version with the Ownable, Pausable removed, as it's a predeploy and would provide the same functionality, unless there's a particular reason you need to retain the ability to pause deploys?

@pcaversaccio
Copy link
Owner

Initial thought was the version with the Ownable, Pausable removed, as it's a predeploy and would provide the same functionality, unless there's a particular reason you need to retain the ability to pause deploys?

Nah, that's fine. The reason why I have these functionalities in the contract is due to a legacy reason and I wanted to keep the consistency. But it's totally fine to remove them for the predeploy on Base.

@pcaversaccio
Copy link
Owner

Closing this PR as exciting things are happening:

@pcaversaccio
Copy link
Owner

FWIW, I just deployed (am probably the first one on this chain 😄) Create2Deployer to Base Sepolia (the block explorer doesn't load it yet properly but I pinged the blockscout team already): 0x13b0D85CcB8bf860b6b79AF3029fCA081AE9beF2. @mds1 if you want to do the same for multicall, the Base docs are not updated yet, but the proxy address for bridging is 0x49f53e41452C74589E85cA1677426Ba426459e85. DON'T use your deployer account for the bridging as otherwise you will have the same issue I had with the Base mainnet as the bridge transaction will be a self transfer.

@pcaversaccio
Copy link
Owner

pcaversaccio commented Oct 21, 2023

Nvm, I just deployed and verified it myself :-D via 0x07471adfe8f4ec553c1199f495be97fc8be8e0626ae307281c22534460184ed1: 0xcA11bde05977b3631167028862bE2a173976CA11. Opened the PR here mds1/multicall#166.

@pcaversaccio
Copy link
Owner

We're live with 56 EVM chain deployments of CreateX including Base Sepolia and Base Mainnet: https://twitter.com/pcaversaccio/status/1738153235678998855!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants