Skip to content

Fix: Resolve Deployment Issues #751

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.

Sign up for GitHub

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

Draft
marvinkruse wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Fix: Resolve Deployment Issues #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef85639
Feat: Add external contract deployment option
marvinkruse May 20, 2025
b7916ea
Fix: Properly test event emissions
marvinkruse May 20, 2025
38a1c94
Fix: Add new function to interface
marvinkruse May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/factories/OrchestratorFactory_v1.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import {
Initializable
} from "@oz-up/access/Ownable2StepUpgradeable.sol";

import {Create2} from "@oz/utils/Create2.sol";

/**
* @title Inverter Orchestrator Factory
*
Expand Down Expand Up @@ -244,6 +246,18 @@ contract OrchestratorFactory_v1 is
return _orchestratorIdCounter;
}

/// @inheritdoc IOrchestratorFactory_v1
function deployExternalContract(bytes calldata code, bytes[] calldata calls)
Copy link
Contributor

@FHieser FHieser Jun 4, 2025

Choose a reason for hiding this comment

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

We agreed internally to move this into its own PR
@marvinkruse you can remove it here and then Merge

external
returns (address deploymentAddress)
{
deploymentAddress = Create2.deploy(0, _createSalt(), code);
for (uint i; i < calls.length; ++i) {
(bool success,) = deploymentAddress.call(calls[i]);
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

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.
Copy link
Member Author

@marvinkruse marvinkruse May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@marvinkruse marvinkruse May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@FHieser FHieser May 20, 2025

Choose a reason for hiding this comment

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

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.

require(success, "External contract deployment failed");
}
}

//--------------------------------------------------------------------------
// Internal Functions

Expand Down
9 changes: 9 additions & 0 deletions src/factories/interfaces/IOrchestratorFactory_v1.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ interface IOrchestratorFactory_v1 {
ModuleConfig[] memory moduleConfigs
) external returns (IOrchestrator_v1);

/// @notice Deploys an external contract using the CREATE2 opcode.
/// @dev Any further calls to the contract that serve its initialization
/// can be provided via the calls array and will be executed after.
/// @param code The creation code of the contract.
/// @param calls Additional calls to be made to the deployed contract.
function deployExternalContract(bytes calldata code, bytes[] calldata calls)
external
returns (address deploymentAddress);

/// @notice Returns the {IOrchestrator_v1} {IInverterBeacon_v1} address.
/// @return OrchestratorImplementationBeacon The {IInverterBeacon_v1} of the {Orchestrator_v1} Implementation.
function beacon() external view returns (IInverterBeacon_v1);
Expand Down
46 changes: 46 additions & 0 deletions test/unit/factories/OrchestratorFactory_v1.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,52 @@ contract OrchestratorFactoryV1Test is Test {
assertEq(address(orchestrator), address(orchestrator_retry_alice));
}

function testExternalContractDeployment() public {
// Prepare external contract to be deployed
// As this will mostly be used for IssuanceToken deployments, we will
// use a simple ERC20 contract.
bytes memory constructorArgs = abi.encode("Test Token", "TT", 18);
bytes memory bytecode = abi.encodePacked(
vm.getCode("ERC20Mock.sol:ERC20Mock"), constructorArgs
);

// Create encoded calls to the two function calls we want to do
// into a bytes array
address transferTarget = makeAddr("Target");

// 1. Calling the mint function with the address of the IssuanceToken
bytes memory mintCall = abi.encodeWithSelector(
ERC20Mock.mint.selector, address(factory), 100
);

// 2. Calling the transfer function to transfer the minted tokens
bytes memory transferCall = abi.encodeWithSelector(
ERC20Mock.transfer.selector, address(transferTarget), 100
);

// Assemble the calls array
bytes[] memory calls = new bytes[](2);
calls[0] = mintCall;
calls[1] = transferCall;

vm.expectEmit(true, true, true, true);
emit IERC20.Transfer(address(0), address(factory), 100);

vm.expectEmit(true, true, true, true);
emit IERC20.Transfer(address(factory), address(transferTarget), 100);

address deployedAddress =
factory.deployExternalContract(bytecode, calls);

// Verify that the deployed contract exists and the post-deployment
// calls were executed
assertTrue(ERC20Mock(deployedAddress).balanceOf(address(factory)) == 0);
assertTrue(
ERC20Mock(deployedAddress).balanceOf(address(transferTarget)) == 100
);
assertTrue(ERC20Mock(deployedAddress).decimals() == 18);
}

function _deployOrchestrator() private returns (address) {
// Create Empty ModuleConfig
IOrchestratorFactory_v1.ModuleConfig[] memory moduleConfigs =
Expand Down