safe-global / safe-modules

A collection of modules that can be used with the Safe contract
GNU Lesser General Public License v3.0
140 stars 73 forks source link

Add User Story for Computing Safe Address #463

Closed nlordell closed 2 months ago

nlordell commented 2 months ago

This PR partially addresses the comment https://github.com/safe-global/safe-modules/pull/456#discussion_r1666935429 (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.

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9890279257

Details


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

💛 - Coveralls