Skip to content

Conversation

@pdobacz
Copy link
Contributor

@pdobacz pdobacz commented Feb 20, 2025

Updates to the Creator Contract as done in ipsilon/eof#177 :

  • remove 0xff magic value from the final_salt calculation (CC @gumb0 @frangio)
  • pass on as return data the entire revert return data (presumably revert reason) in case deployment fails (CC @frangio )

@github-actions github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Feb 20, 2025
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 20, 2025

✅ All reviewers have approved.

Copy link
Contributor

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two questions:

  • Some use cases require a creation-and-initialise (can only happened after the bytecode is live) call; should this feature (i.e. the initialise call) also be offered as part of the creator contract? I offer this possibility in my CreateX factory for example. This would also complicate the callvalue handling, however, as there are now two native values to be considered: one for the creation tx as well as one for the initalise call.
  • How should we deal with ETH forced into the creator contract itself (can happen via selfdestruct send, set blockfee recipient address to CREATOR_CONTRACT_ADDRESS, or set withdrawal address on the Beacon chain to CREATOR_CONTRACT_ADDRESS). Should we just keep it locked or should we think about logic to handle it?

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

  • Some use cases require a creation-and-initialise (can only happened after the bytecode is live) call; should this feature (i.e. the initialise call) also be offered as part of the creator contract? I offer this possibility in my CreateX factory for example. This would also complicate the callvalue handling, however, as there are now two native values to be considered: one for the creation tx as well as one for the initalise call.

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

  • How should we deal with ETH forced into the creator contract itself (can happen via selfdestruct send, set blockfee recipient address to CREATOR_CONTRACT_ADDRESS, or set withdrawal address on the Beacon chain to CREATOR_CONTRACT_ADDRESS). Should we just keep it locked or should we think about logic to handle it?

The same as with other system contracts, it will be locked.

@pcaversaccio
Copy link
Contributor

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

InitcodeTransaction, unlike legacy creation transaction, has a separate data field for initialization data, so why couldn't this all create-and-initialize be done by a single TXCREATE, that takes initcode from one of tx.initcodes, and init data from tx.data ?

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Feb 21, 2025

If this initilize call can be achieved with a separate EXTCALL to deployed address, I don't see the reason to include this as a feature of Creator Contract.

This can't happen from an EOA tho - this would require another contract to interact with CREATOR_CONTRACT_ADDRESS. Use case is: I have an EOA as a deployer, some init code and some data to initialise the contract.

InitcodeTransaction, unlike legacy creation transaction, has a separate data field for initialization data, so why couldn't this all create-and-initialize be done by a single TXCREATE, that takes initcode from one of tx.initcodes, and init data from tx.data ?

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2025

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

If it cannot be called in the constructor (why?), then it won't work.

If it's rarely used optional feature, I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

@pcaversaccio
Copy link
Contributor

Maybe I'm misunderstanding something, but how can you call initialise on a contract that is in construction via TXCREATE but the initialise function itself of that contract requires existence of the bytecode itself (i.e. cannot be called in the constructor)?

If it cannot be called in the constructor (why?), then it won't work.

If it's rarely used optional feature, I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

So any use case that requires an external callback into the contract you create, would first need to be created otherwise it fails. An example would be using flashloans. Another use case are upgradeable contracts where you can't call constructors but you need to move it to an external initialiser function.

@frangio
Copy link
Contributor

frangio commented Feb 21, 2025

any use case that requires an external callback into the contract you create

This is a legitimate use case. Another one that we've already identified is creating a contract where the address is independent of some of the initialization parameters. However, I think the creator contract needs to be as simple as possible to achieve its goal and so I don't think it should accomodate any of these use cases. I think this would be the right choice even if they were the majority.

I'd say this should be part of a different TXCREATE-factory contract, which users are free to deploy.

This would be the answer. Note that once the creator contract is in place the guarantees about deterministic addresses can extend to any new factories, this is the only goal we should optimize for IMO.

@pdobacz
Copy link
Contributor Author

pdobacz commented Feb 24, 2025

  • How should we deal with ETH forced into the creator contract itself (can happen via selfdestruct send, set blockfee recipient address to CREATOR_CONTRACT_ADDRESS, or set withdrawal address on the Beacon chain to CREATOR_CONTRACT_ADDRESS). Should we just keep it locked or should we think about logic to handle it?

The same as with other system contracts, it will be locked.

This made me think about whether the Creator Contract doesn't accidentally implement standards we would not like it to. It seems we're ok for some examples I have in mind:

  • it doesn't accidentally implement ERC-165, as it will fail on the standard ERC-165 query because of input length 32 bytes
  • it won't accidentally accept an ERC-721 or ERC-1155 safeTransfer

Can you think of any other?


Whoops. I think it allows for reentrancy - the initcode can call into the Creator. One potential issue I can identify is that it could attempt to deploy at the same new_address again. CREATE/CREATE2 circumvent this by setting the nonce of the new_address account to 1 before starting execution of the initcode (EIP-161), I think both here and in EIP-7620 we should do the same.

Otherwise, the reentrancy can be ok and useful in order for multiple dependent contracts be deployed out of transaction data, rather than EOFCREATE.


Related to the above, I'm thinking whether we shouldn't include callvalue() in the final_salt. Malicious initcode may use it as encoded init_data_2 and alter its behavior and possibly the deployed code, while retaining the same new_address 😬 . It does require someone to interact with malicious initcode to fall victim to such attack, but still, it allows for malleability we are otherwise not accepting.

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Feb 24, 2025

Can you think of any other?

  • ERC-777 via IERC777Recipient(to).tokensReceived(...) (but e.g. OpenZeppelin deprecated this implementation fwiw)

Whoops. I think it allows for reentrancy - the initcode can call into the Creator. One potential issue I can identify is that it could attempt to deploy at the same new_address again. CREATE/CREATE2 circumvent this by setting the nonce of the new_address account to 1 before starting execution of the initcode (EIP-161), I think both here and in EIP-7620 we should do the same.

I think reentrancy is a feature here (I also allow for reentrancy in CreateX) - a rare case where it is tbh. Also, EIP-684 prevents collisions.

@frangio
Copy link
Contributor

frangio commented Feb 24, 2025

CREATE/CREATE2 circumvent this by setting the nonce of the new_address account to 1 before starting execution of the initcode (EIP-161)

What exactly is circumvented?

Related to the above, I'm thinking whether we shouldn't include callvalue() in the final_salt. Malicious initcode may use it as encoded init_data_2 and alter its behavior and possibly the deployed code, while retaining the same new_address 😬 . It does require someone to interact with malicious initcode to fall victim to such attack, but still, it allows for malleability we are otherwise not accepting.

There are other forms of malleability, from reading block and transaction parameters to reading other accounts' state. Ultimately to trust a contract deployed at a deterministic address you still need to check the code hash or to inspect the source code to determine it's not malleable. How should we decide which ones should impact on the address?

I actually think the more serious issue is that a deployment transaction can be frontrun and sent from a different account. Most of the motivating use cases for this feature need the address to be independent from the caller, but I think most users in general today do care about deployment frontrunning and assume it cannot happen.

@pdobacz
Copy link
Contributor Author

pdobacz commented Feb 24, 2025

CREATE/CREATE2 circumvent this by setting the nonce of the new_address account to 1 before starting execution of the initcode (EIP-161)

What exactly is circumvented?

Sorry, I might have ended up conflating 2 subjects:

1/ Creator Contract reentrancy - as pcaversaccio notes, it's OK
2/ explicitly setting nonce of new_address to 1 before initcode, as EIP-161 says - which it seems we need to add to the EIPs and tests.

There are other forms of malleability, from reading block and transaction parameters to reading other accounts' state. Ultimately to trust a contract deployed at a deterministic address you still need to check the code hash or to inspect the source code to determine it's not malleable. How should we decide which ones should impact on the address?

Yes, you're right. I'd risk to draw that line at "ones which one normally uses to variate the deployed contract", so code and init_data are ok to be in the hash, callvalue and other - not. A factory which factors in callvalue can always be deployed later, if it's useful. As long as we can ensure that deployment will happen using the Creator Contract, we're fine (noted by chfast). So I guess we're good here.

I actually think the more serious issue is that a deployment transaction can be frontrun and sent from a different account. Most of the motivating use cases for this feature need the address to be independent from the caller, but I think most users in general today do care about deployment frontrunning and assume it cannot happen.

A factory which factors in msg.sender in the final_salt can be deployed from the Creator Contract and then its deployments will not be frontrunnable.

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Feb 24, 2025

A factory which factors in msg.sender in the final_salt can be deployed from the Creator Contract and then its deployments will not be frontrunnable.

That's exactly what CreateX does fyi: https://github.com/pcaversaccio/createx/tree/main#special-features. The requirement is that msg.sender is trusted and access-controlled (e.g. an EOA). If it's another contract that is publicly callable, you can still frontrun with a different tx.origin (but same msg.sender).

@frangio
Copy link
Contributor

frangio commented Feb 24, 2025

A factory which factors in msg.sender in the final_salt can be deployed from the Creator Contract and then its deployments will not be frontrunnable.

Yes, what I was thinking was that "transaction to creator contract" would become the new default "deployment transaction", replacing those with to = null, which are not frontrunnable. Would we consider including a flag to toggle including CALLER in the salt?

@pcaversaccio
Copy link
Contributor

This is a legitimate use case. Another one that we've already identified is creating a contract where the address is independent of some of the initialization parameters. However, I think the creator contract needs to be as simple as possible to achieve its goal and so I don't think it should accomodate any of these use cases. I think this would be the right choice even if they were the majority.

I was just checking this morning the CreateX deployments, and I saw two deployCreate2AndInit(...) transactions:

image

image

The reason I want to share this is to highlight that the create-and-init feature is used in CreateX.

@pdobacz
Copy link
Contributor Author

pdobacz commented Feb 25, 2025

The reason I want to share this is to highlight that the create-and-init feature is used in CreateX.

Thank you. Assuming we want a "minimal" and simple Creator Contract to bootstrap everything else EOF - is it not enough if such more specialized create-and-init factory is deployed using that and not predeployed at fork activation?

@pcaversaccio
Copy link
Contributor

Thank you. Assuming we want a "minimal" and simple Creator Contract to bootstrap everything else EOF - is it not enough if such more specialized create-and-init factory is deployed using that and not predeployed at fork activation?

I think it's fine to keep it KISS for now - I just wanted to share real-world txs using this feature.

@pdobacz
Copy link
Contributor Author

pdobacz commented Feb 25, 2025

Thank you. Assuming we want a "minimal" and simple Creator Contract to bootstrap everything else EOF - is it not enough if such more specialized create-and-init factory is deployed using that and not predeployed at fork activation?

I think it's fine to keep it KISS for now - I just wanted to share real-world txs using this feature.

Just to be extra clear - we do want to accommodate all the deployment flows and use cases legacy supports, and wherever possible improve on that, in EOFv1 Osaka. We just don't want them to be directly available by calling into the EIP-7873's Creator Contract - where business logic is at a high premium. We need them to be available via factories, which will be deployed from the Creator, while maintaining all the useful traits like cross-chain, deterministic, counterfactual, AA-friendly etc etc.

In other words - it's crucial we're confident you'd be able to deploy an "EOF-CreateX" using the Creator Contract (or a factory deployed using the Creator Contract). Possibly the Creator Contract will be made obsolete by some superior ERC-based factory it deploys.

@pdobacz
Copy link
Contributor Author

pdobacz commented Feb 26, 2025

A factory which factors in msg.sender in the final_salt can be deployed from the Creator Contract and then its deployments will not be frontrunnable.

Yes, what I was thinking was that "transaction to creator contract" would become the new default "deployment transaction", replacing those with to = null, which are not frontrunnable. Would we consider including a flag to toggle including CALLER in the salt?

I was thinking about this, and actually I'm inclined to see it the other way around - not expect the Creator Contract would last long as the default deployment tool. Its business logic being at a premium (a flag would be quite a bit of new logic), I'd see it more as a boostrapping device for better-UX'ed ERC-factories. So unless non-front-runnability must be available for the deployment of such factories to work, I'd leave it up to the ERC-factories to provide it.

We can add it to the agenda to discuss on the next EOF impl call maybe?

@frangio
Copy link
Contributor

frangio commented Feb 26, 2025

My argument against ERC-factories was that it forces tools to deal with the edge case that they're not deployed on whatever network one is on.

Let's discuss this on the next call yeah.

@pdobacz
Copy link
Contributor Author

pdobacz commented Mar 6, 2025

On the EOF impl call 68 we discussed this and we're still open to do the flag. We need to seek input from tooling / compilers to gauge impact of not including the caller flag and not having the Creator Contract as a long-running default factory.

I'll merge these changes and we'll prepare a follow-up pr for the flag thingy

@pdobacz pdobacz closed this Mar 6, 2025
@pdobacz pdobacz reopened this Mar 6, 2025
@pdobacz pdobacz marked this pull request as ready for review March 6, 2025 13:54
@pdobacz pdobacz requested a review from eth-bot as a code owner March 6, 2025 13:54
@eth-bot eth-bot enabled auto-merge (squash) March 6, 2025 13:55
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 3fc4f87 into ethereum:master Mar 6, 2025
18 of 24 checks passed
@pdobacz pdobacz deleted the update-7873 branch March 6, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-update Modifies an existing proposal s-draft This EIP is a Draft t-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants