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

Fix: Resolve Module Address Collisions #690

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

marvinkruse
Copy link
Member

@marvinkruse marvinkruse commented Nov 15, 2024

Why does this PR exist?
I noticed that the original idea - when we switched to create2-based deployments in the factories - was to allow users to deploy workflows at the same address across networks. What we did not think about back then: the ModuleFactory_v1 is not just called by end users, but mainly be the OrchestratorFactory_v1, which makes the following salt calculation not unique per user:

function _createSalt() internal returns (bytes32) {
    return keccak256(
        abi.encodePacked(_msgSender(), _deploymentNonces[_msgSender()]++)
    );
}

So essentially, the system only works for OrchestratorFactory_v1, where the user calls the contract.

How did I fix this?
We decided, that it's enough if we guarantee folks that their Orchestrator can be at the same address across networks, and the modules don't matter really. So to resolve the underlying issue, I added the chainId into the salt calculation in the ModuleFactory_v1. This makes it unique at all times and the address can never be griefed (also not on accident).

@marvinkruse marvinkruse changed the base branch from main to dev November 15, 2024 11:42
@marvinkruse marvinkruse self-assigned this Nov 15, 2024
@marvinkruse marvinkruse force-pushed the resolve-module-address-collisions branch from fe38301 to b323e21 Compare November 15, 2024 11:50
@marvinkruse marvinkruse requested a review from FHieser November 26, 2024 12:53
@FHieser
Copy link
Contributor

FHieser commented Nov 26, 2024

9boukq

Copy link
Contributor

@FHieser FHieser left a comment

Choose a reason for hiding this comment

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

Need tests

@Zitzak
Copy link
Collaborator

Zitzak commented Jan 16, 2025

How to move forward with this PR? How does having these changes un-merged effect our deployments? @marvinkruse @FHieser @0xNuggan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants