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

SafeECDSASignerLaunchpad has unclear support for multi-owner accounts #372

Closed
mmv08 opened this issue Apr 9, 2024 · 4 comments · Fixed by #432
Closed

SafeECDSASignerLaunchpad has unclear support for multi-owner accounts #372

mmv08 opened this issue Apr 9, 2024 · 4 comments · Fixed by #432

Comments

@mmv08
Copy link
Member

mmv08 commented Apr 9, 2024

Description

When working on #360, we wanted to add two signers to the newly created Safe account: the user's connected wallet and the created passkey signer. However, this turned out to be non-trivial because SafeECDSASignerLaunchpad has unclear/limited support for multi-owner accounts.

  1. There's no straightforward way to specify multiple owners for the Safe configuration.
    When setting up the Safe, the contract hardcodes the signer/threshold parameters, leaving the only option to extend the configuration in the call executed later in the function or the optional delegatecall in the Safe setup phase.
    {
    address[] memory owners = new address[](1);
    owners[0] = ICustomECDSASignerFactory(signerFactory).createSigner(signerX, signerY, signerVerifier);
    ISafe(address(this)).setup(owners, 1, setupTo, setupData, fallbackHandler, address(0), 0, payable(address(0)));
    }
  2. During the validation phase, it only validates the signature from one (Passkey) owner. This is a potential security risk since a single signer has full control over a Safe.

Additional context

This issue was created to explore potential solutions and decide where we want to improve this situation. While briefly discussing it in a standup meeting, we agreed that solving it would be complex because it would require duplicating the whole Safe signature check logic in the contract (one of the solutions).

@nlordell
Copy link
Collaborator

nlordell commented Apr 9, 2024

Correct me if I'm wrong, but would you say that the expected outcome would be to:

  1. evaluate the security implications of the current setup? (Basically, what you said in the standup where we a single owner has full control of the multisig)
  2. evaluate with the product team what the requirements are for multi-owner use cases
  3. propose changes to the API to adjust for potential multi-owner use cases?

@mmv08
Copy link
Member Author

mmv08 commented Apr 9, 2024

Correct me if I'm wrong, but would you say that the expected outcome would be to:

  1. evaluate the security implications of the current setup? (Basically, what you said in the standup where we a single owner has full control of the multisig)
  2. propose changes to the API to adjust for potential multi-owner use cases?

Bang on!

Something that came to me while writing the issue: IIRC, the current launchpad contract was your experiment to make Passkey signers work with 4337, and we never defined any formal requirements (product, technical, etc) for the contract. Perhaps this is the moment to do this, sync with Niharika and formalize the requirements, etc

nlordell added a commit that referenced this issue Apr 10, 2024
This PR does a rework of some of the implementation details of the
`SafeECDSASignerLaunchpad` contract in light of some observations from
the previous PR #376.

Namely, this changes the initialization process to work in a slightly
different way:
1. Set the target singleton to a special slot when the entry point
   executes the `initCode` for the account
2. Signature verification checks that the account is an owner. This has
   the side-effect that you can initialize an account with multiple
   ECDSA owners and use any of them to sign the first user operation.
3. Promote the Safe to the singleton that was previously in storage.

The main difference with the previous flow is that we no longer have two
separate `setup` initializers that we `DELEGATECALL` to. Additionally,
we added checks that prevent double initialization as well as reentrency
issues in the execution function.

In addition, this also opens up a pretty clear path for supporting
multiple owners with the launchpad as the account has already undergone
"regular" Safe setup. This is relevant for #372.
nlordell added a commit that referenced this issue May 5, 2024
This PR does a rework of some of the implementation details of the
`SafeECDSASignerLaunchpad` contract in light of some observations from
the previous PR #376.

Namely, this changes the initialization process to work in a slightly
different way:
1. Set the target singleton to a special slot when the entry point
   executes the `initCode` for the account
2. Signature verification checks that the account is an owner. This has
   the side-effect that you can initialize an account with multiple
   ECDSA owners and use any of them to sign the first user operation.
3. Promote the Safe to the singleton that was previously in storage.

The main difference with the previous flow is that we no longer have two
separate `setup` initializers that we `DELEGATECALL` to. Additionally,
we added checks that prevent double initialization as well as reentrency
issues in the execution function.

In addition, this also opens up a pretty clear path for supporting
multiple owners with the launchpad as the account has already undergone
"regular" Safe setup. This is relevant for #372.
nlordell added a commit that referenced this issue May 10, 2024
This PR does a rework of some of the implementation details of the
`SafeECDSASignerLaunchpad` contract in light of some observations from
the previous PR #376.

Namely, this changes the initialization process to work in a slightly
different way:
1. Set the target singleton to a special slot when the entry point
   executes the `initCode` for the account
2. Signature verification checks that the account is an owner. This has
   the side-effect that you can initialize an account with multiple
   ECDSA owners and use any of them to sign the first user operation.
3. Promote the Safe to the singleton that was previously in storage.

The main difference with the previous flow is that we no longer have two
separate `setup` initializers that we `DELEGATECALL` to. Additionally,
we added checks that prevent double initialization as well as reentrency
issues in the execution function.

In addition, this also opens up a pretty clear path for supporting
multiple owners with the launchpad as the account has already undergone
"regular" Safe setup. This is relevant for #372.
nlordell added a commit that referenced this issue May 10, 2024
This PR does a rework of some of the implementation details of the
`SafeSignerLaunchpad` contract in light of some observations from the
previous PR #376.

Namely, this changes the initialization process to work in a slightly
different way:
1. Set the target singleton to a special slot when the entry point
executes the `initCode` for the account. Safe `setup` also happens at
this point, meaning that any `DELEGATECALL` to the Safe singleton should
work and be valid.
2. Signature verification checks that the account is an owner. This has
the side-effect that you can initialize an account with multiple custom
ECDSA owners and use any of them to sign the first user operation.
3. Promote the Safe to the singleton that was previously in storage.

The main difference with the previous flow is that we no longer have two
separate `setup` initializers that we `DELEGATECALL` to. Additionally,
we added checks that prevent double initialization as well as reentrency
issues in the execution function.

In addition, this also opens up a pretty clear path for supporting
multiple owners with the launchpad as the account has already undergone
"regular" Safe setup. This is relevant for #372.

Unit tests in order to reach 100% coverage will be introduced in a
follow up.
@nlordell
Copy link
Collaborator

This is blocked on product decision on what must be supported for SDK passkey support.

@nlordell nlordell added the blocked Issue or PR that is blocked by an external dependency label May 22, 2024
@nlordell nlordell removed the blocked Issue or PR that is blocked by an external dependency label Jun 5, 2024
@nlordell
Copy link
Collaborator

nlordell commented Jun 5, 2024

We found a general solution with the shared signer, which would work for arbitrary ownership structures, so this is once again unblocked.

nlordell added a commit that referenced this issue Jun 5, 2024
Fixes #372

This PR adds a new E2E test that demonstrates the use of a Safe being
deployed over 4337 with multiple passkey owners. This means that we can
have arbitrary ownership structures with passkeys and 4337 🎉.

There is an important caveat however, the tracing mechanism used by the
bundler is timing out when verifying multiple passkey signatures, so
this E2E test is only half complete. At least we verify that the
signature verification logic would work, and eventually when precompiles
become part of Geth, we will be able to switch the E2E test to use that
instead which should greatly speed up tracing.

I still think it is worth merging this PR as is, as it also checks that
multiple shared signer instances can be used in parallel, a new feature
added in #429.
nlordell added a commit that referenced this issue Jun 10, 2024
Fixes #372

This PR adds a new E2E test that demonstrates the use of a Safe being
deployed over 4337 with multiple passkey owners. This means that we can
have arbitrary ownership structures with passkeys and 4337 🎉.

There is an important caveat however, the tracing mechanism used by the
bundler is timing out when verifying multiple passkey signatures, so
this E2E test is only half complete. At least we verify that the
signature verification logic would work, and eventually when precompiles
become part of Geth, we will be able to switch the E2E test to use that
instead which should greatly speed up tracing.

I still think it is worth merging this PR as is, as it also checks that
multiple shared signer instances can be used in parallel, a new feature
added in #429.
nlordell added a commit that referenced this issue Jun 10, 2024
Fixes #372

This PR adds a new E2E test that demonstrates the use of a Safe being
deployed over 4337 with multiple passkey owners. This means that we can
have arbitrary ownership structures with passkeys and 4337 🎉.

There is an important caveat however, the tracing mechanism used by the
bundler is timing out when verifying multiple passkey signatures, so
this E2E test is only half complete. At least we verify that the
signature verification logic would work, and eventually when precompiles
become part of Geth, we will be able to switch the E2E test to use that
instead which should greatly speed up tracing.

I still think it is worth merging this PR as is, as it also checks that
multiple shared signer instances can be used in parallel, a new feature
added in #429.
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 a pull request may close this issue.

2 participants