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

Update EIP-7873: Creator Contract - revert reason & magic value #9391

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

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.


let tx_initcode_hash := calldataload(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be mload(0). Is there any reason to prefer one over the other? Are there any planned gas schedule changes that would impact here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calldataload(0) is simpler/safer, as it reads from an immutable buffer, independent from code that wrote that data into memory.

Not sure about gas schedule changes, but anyway I'd prefer simplicity today over performance in the future.

Co-authored-by: Francisco Giordano <fg@frang.io>
@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?

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