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

Add User Story for Computing Safe Address #463

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented Jul 10, 2024

This PR partially addresses the comment #456 (comment) (cc @mmv08).

In doing some exploration around event indexing, I found that we don't really have a good developer story for it. There are two separate passkey flows, so I will address them separately.

Proxy + Factory Signer

The flow specific to using the SafeWebAuthnSignerFactory contract to deploy a credential-specific WebAuthn credential owner requires a service for indexing Safe owners in order to properly search Safes that are owned by a specific credential. While the WebAuthn credential signer address can be deterministically computed (see signerFactory.getSigner(...) function), it does not allow for us to search for all Safes that use this signer. This is because there are multiple events that affect the owners list for a Safe (Setup, Owner*), and specifically owners added during setup do not emit any event indexed by owner address, meaning you would have to check all Setup events for all Safes in order to determine which Safes use the specific WebAuthn credential signer.

There is, however, support for this in the transaction service API. So, right now, it should be possible to find all Safes owned by a WebAuthn credential by implementing something like:

const signerAddress = await signerFactory.getSigner(x, y, verifiers);
const safes = await safeTransactionServiceClient.getOwnerSafes(signerAddress);

However, this requires an API service to do this, and cannot be done alone with events.

Shared Signer

The flow specific to using the SafeWebAuthnSharedSigner contract suffers from the same issues as the other flow, in that finding Safes by owner is not really possible with events. Compounding on top of this, the transaction service API for listing Safes owned by addresses does not work for the shared signer at all, because it is a single address for all credentials.

In the aforementioned PR, there was a proposal to add an indexed field:

diff --git a/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol b/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol
index e065cb8..a924c6f 100644
--- a/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol
+++ b/modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol
@@ -50,11 +50,12 @@ contract SafeWebAuthnSharedSigner is SignatureValidator {
      * is done as a `DELEGATECALL`, the contract emitting the event is the configured account. This
      * is also why the event name is prefixed with `SafeWebAuthnSharedSigner`, in order to avoid
      * event `topic0` collisions with other contracts (seeing as "configured" is a common term).
+     * @param publicKeyHash The Keccak-256 hash of the public key coordinates.
      * @param x The x-coordinate of the public key.
      * @param y The y-coordinate of the public key.
      * @param verifiers The P-256 verifiers to use.
      */
-    event SafeWebAuthnSharedSignerConfigured(uint256 x, uint256 y, P256.Verifiers verifiers);
+    event SafeWebAuthnSharedSignerConfigured(bytes32 indexed publicKeyHash, uint256 x, uint256 y, P256.Verifiers verifiers);
 
     /**
      * @notice An error indicating a `CALL` to a function that should only be `DELEGATECALL`-ed.
@@ -146,7 +147,8 @@ contract SafeWebAuthnSharedSigner is SignatureValidator {
         signerStorage.y = signer.y;
         signerStorage.verifiers = signer.verifiers;
 
-        emit SafeWebAuthnSharedSignerConfigured(signer.x, signer.y, signer.verifiers);
+        bytes32 publicKeyHash = keccak256(abi.encode(signer.x, signer.y));
+        emit SafeWebAuthnSharedSignerConfigured(publicKeyHash, signer.x, signer.y, signer.verifiers);
     }
 
     /**

This would allow for clients to search for Configured events for a particular Safe. Just the event is not enough, as the configuration may have changed, the shared signer may have been removed as an owner, the Safe may have never added the shared signer as an owner (and just called the configure method for fun), or the shared signer may have been called by another contract that is not a Safe.

const publicKeyHash = ethers.solidityPackedKeccak256(["uint256", "uint256"], [x, y]);
const configuredSafes = await provider
  .getLogs({
    topics: sharedSigner.interface
      .sharedSigner.interface.encodeFilterTopics('SafeWebAuthnSharedSignerConfigured', [publicKeyHash]),
    fromBlock: 0,
  })
for (const { address: possibleSafe } of configuredSafes) {
  const signerConfig = await sharedSigner.getConfiguration(possibleSafe);
  const isOwner = await safe.attach(possibleSafe).isOwner(sharedSigner);

  if (signerConfig.x == x && signerConfig.y == y && isOnwer) {
    console.log(`${possibleSafe} is owned by WebAuthn credential (${x}, ${y})`);
  }
}

This is also not a great developer story. I would recommend adding similar support to finding Safes by WebAuthn credential to the Safe transaction service as exists for finding Safes by owner. I also don't think that adding an indexed publicKeyHash is necessarily valuable, but it does make searching for Safes by WebAuthn credential possible, so we add it to the contracts in this PR.

Conclusion

It isn't really possible to, just from indexed events, search for all Safes that are owned by a specific WebAuthn credential owner. If we do end up needing a feature for searching for Safes by WebAuthn credential public key (similar to searching for Safes by owner address), we would likely need to add support to the Safe transaction service for this.

For now, I created a user story to show how a "serverless" Dapp can use the Safe + Passkeys to "find" the Safe for the current credential. The idea is simply to compute the deterministic Safe address based on the public key credential.

@nlordell nlordell requested a review from a team as a code owner July 10, 2024 06:47
@nlordell nlordell requested review from akshay-ap, mmv08 and remedcu and removed request for a team July 10, 2024 06:47
@@ -161,7 +161,7 @@ describe('WebAuthn Library', () => {
// a large enough client data and exact gas limits to make this happen is a bit annoying, so
// lets hope for no gas schedule changes :fingers_crossed:.
const longClientDataFields = `"long":"${'a'.repeat(100000)}"`
await expect(webAuthnLib.encodeSigningMessage(ethers.ZeroHash, '0x', longClientDataFields, { gasLimit: 1701001 })).to.be.reverted
await expect(webAuthnLib.encodeSigningMessage(ethers.ZeroHash, '0x', longClientDataFields, { gasLimit: 1699001 })).to.be.reverted
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated fix, caused by gas changes.

@nlordell nlordell force-pushed the chore/userstory-for-safe branch from c3159a0 to d12d35a Compare July 10, 2024 07:04
@coveralls
Copy link

coveralls commented Jul 10, 2024

Pull Request Test Coverage Report for Build 9890279257

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 9869500757: 0.0%
Covered Lines: 92
Relevant Lines: 92

💛 - Coveralls

Copy link
Member

@remedcu remedcu left a comment

Choose a reason for hiding this comment

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

Small nit, rest LGTM.

0,
ethers.ZeroAddress,
])
const saltNonce = 42n
Copy link
Member

Choose a reason for hiding this comment

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

We have the answer to everything! 😅

@nlordell nlordell merged commit c3f6d36 into main Jul 11, 2024
10 checks passed
@nlordell nlordell deleted the chore/userstory-for-safe branch July 11, 2024 11:23
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants