Fix: Resolve Deployment Issues #751
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refines the deployment logic to ensure module addresses remain unique and secure while also enabling external contract deployment with initialization calls in a single transaction.
- Updated CREATE2-based deployment logic in ModuleFactory_v1 with improved salt generation.
- Introduced external contract deployment in OrchestratorFactory_v1 with support for subsequent initialization calls.
- Added a new unit test in OrchestratorFactoryV1Test to validate external contract deployment and post-deployment behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/unit/factories/OrchestratorFactory_v1.t.sol | Added a unit test for external contract deployment and event expectations. |
| src/factories/OrchestratorFactory_v1.sol | Introduced deployExternalContract using CREATE2 and post-deployment calls. |
| src/factories/ModuleFactory_v1.sol | Modified salt generation to improve nonce uniqueness using orchestrator address. |
| { | ||
| deploymentAddress = Create2.deploy(0, _createSalt(), code); | ||
| for (uint i; i < calls.length; ++i) { | ||
| (bool success,) = deploymentAddress.call(calls[i]); |
There was a problem hiding this comment.
Consider adding reentrancy protection or employing a checks-effects-interactions pattern for the external calls to mitigate potential security vulnerabilities, especially in production scenarios.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Yeah, that's exactly what I mentioned above. Will add something for this once we verified whether this works at all and is a good solution.
There was a problem hiding this comment.
@fabianschu and @FHieser: have some ideas to prevent abuse here? I just have one idea atm which is to have a list of allowed bytecode hashes that can be deployed via this (i.e. the 4 issuance token versions we have), so no one can build an exploiter contract (which would be annoying, as we'd need to whitelist them each every time there is something new, but doable).
Any other cool ideas? Or would a regular reentrancy-protection work already. I'm a bit concerned that users may be able to call other contracts "as the orchestrator factory" but not sure if it's being too careful atm.
There was a problem hiding this comment.
Not directly abuse, but we had some assumptions in the past regarding "What users will expect when they see a contract that was deployed via our factories". We never defined this properly, but one might argue that if contracts can be freely deployed via our factories it might give the feeling of "Yes this is a Inverter contract" which might be misleading.
Correct me but the purpose of this function is to salt deploy Token contracts or Singletons that are needed for a workflow correct?
As they are technically not modules I would be against just slapping this function in here.
Instead I would propose to do it in a library style deployment similar to the modulefactory but for singletons. That way we can restrict what users can deploy and ensure the quality of deployments in the first place. The deployments also can happen via Clones then too making them way cheaper.
|
@FHieser @fabianschu just tagging you for a review, this isn't "done" yet, as you can see in the warning, but the idea would be to get some feedback in general before I continue on this. Please reach out to me if you need any information. |
| { | ||
| deploymentAddress = Create2.deploy(0, _createSalt(), code); | ||
| for (uint i; i < calls.length; ++i) { | ||
| (bool success,) = deploymentAddress.call(calls[i]); |
There was a problem hiding this comment.
Not directly abuse, but we had some assumptions in the past regarding "What users will expect when they see a contract that was deployed via our factories". We never defined this properly, but one might argue that if contracts can be freely deployed via our factories it might give the feeling of "Yes this is a Inverter contract" which might be misleading.
Correct me but the purpose of this function is to salt deploy Token contracts or Singletons that are needed for a workflow correct?
As they are technically not modules I would be against just slapping this function in here.
Instead I would propose to do it in a library style deployment similar to the modulefactory but for singletons. That way we can restrict what users can deploy and ensure the quality of deployments in the first place. The deployments also can happen via Clones then too making them way cheaper.
| /// @dev Maps the hash of the callers address and the corresponding | ||
| /// orchestrator address to a nonce used for the create2-based | ||
| /// deployment. | ||
| mapping(bytes32 => uint) private _deploymentNonces; |
There was a problem hiding this comment.
Does this change the storage layout in any way?
There was a problem hiding this comment.
So, mapping storage works in the sense that the slot/address where this mapping is and the key(s) of the mapping are used to determine the storage address of the value stored. So, in general, this is just one storage slot in terms of the order that we have. As I changed the key from address to bytes32, it's basically generating totally new storage locations, and there won't be any overlap.
So short form: This is fine and doesn't change the layout in any way. There are also no "old" values in there.
Yeah, I know! Will be done after we confirmed that the solution works 🤝 |
|
Damn just saw Mehmets solution. |
This PR aims to address the following issues:
Below is the GitHub AI summary of the changes, be wary
This pull request introduces changes to enhance the deployment logic for CREATE2-based flows, improve nonce handling, and add support for deploying external contracts. It also includes a new test to validate the external contract deployment functionality. Below is a breakdown of the most important changes:
Enhancements to CREATE2-based deployment logic:
_deploymentNoncesmapping inModuleFactory_v1to use a hash of the caller's address and orchestrator address as the key, improving nonce uniqueness. (src/factories/ModuleFactory_v1.sol, src/factories/ModuleFactory_v1.solL115-R118)_createSaltfunction to generate a salt using the hash of the caller's address and orchestrator, ensuring better handling of orchestrator-specific deployments. (src/factories/ModuleFactory_v1.sol, src/factories/ModuleFactory_v1.solL289-R302)_createSalt, aligning with the new salt generation approach. (src/factories/ModuleFactory_v1.sol, src/factories/ModuleFactory_v1.solL212-R226)Support for external contract deployment:
deployExternalContractfunction inOrchestratorFactory_v1to deploy contracts using CREATE2 and execute initialization calls post-deployment. (src/factories/OrchestratorFactory_v1.sol, src/factories/OrchestratorFactory_v1.solR249-R264)Create2utility from OpenZeppelin to facilitate CREATE2-based deployments. (src/factories/OrchestratorFactory_v1.sol, src/factories/OrchestratorFactory_v1.solR40-R41)New test for external contract deployment:
OrchestratorFactoryV1Testto validate the deployment of an external ERC20 contract and the execution of post-deployment calls. (test/unit/factories/OrchestratorFactory_v1.t.sol, test/unit/factories/OrchestratorFactory_v1.t.solR345-R388)