Closed mdehoog closed 1 year ago
@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.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.
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:
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 overloadschain.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:
0x4e59b44847b379578588920ca78fbf26c0b4956c
)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.
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:
32
.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.
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 π
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?).
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?
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.
Closing this PR as exciting things are happening:
Create2Deployer
will be added as a predeploy to Base mainnet; see https://github.com/ethereum-optimism/op-geth/pull/126. Thanks a lot for pushing this @mdehoog!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.
Nvm, I just deployed and verified it myself :-D via 0x07471adfe8f4ec553c1199f495be97fc8be8e0626ae307281c22534460184ed1
: 0xcA11bde05977b3631167028862bE2a173976CA11
. Opened the PR here https://github.com/mds1/multicall/pull/166.
We're live with 56 EVM chain deployments of CreateX
including Base Sepolia and Base Mainnet: https://twitter.com/pcaversaccio/status/1738153235678998855!
The Base team is truly sorry for what happened in https://github.com/pcaversaccio/create2deployer/issues/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
andOwnable
features of this contract to make it more credibly neutral. Would love thoughts / feedback on this.