Skip to content

Commit

Permalink
Add User Story for Computing Safe Address (#463)
Browse files Browse the repository at this point in the history
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](https://docs.safe.global/core-api/transaction-service-reference#Owners).
So, right now, it should be possible to find all Safes owned by a
WebAuthn credential by implementing something like:

```ts
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
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.

```ts
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.

---------

Co-authored-by: Shebin John <[email protected]>
  • Loading branch information
nlordell and remedcu authored Jul 11, 2024
1 parent 37def80 commit c3f6d36
Show file tree
Hide file tree
Showing 4 changed files with 179 additions and 4 deletions.
6 changes: 4 additions & 2 deletions modules/passkey/contracts/4337/SafeWebAuthnSharedSigner.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion modules/passkey/test/4337/SafeWebAuthnSharedSigner.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ describe('SafeWebAuthnSharedSigner', () => {
y: ethers.id('publicKey.y'),
verifiers: ethers.toBeHex(await mockVerifier.getAddress(), 32),
}
const publicKeyHash = ethers.solidityPackedKeccak256(['uint256', 'uint256'], [config.x, config.y])

const initializer = safeSingleton.interface.encodeFunctionData('setup', [
[sharedSigner.target],
Expand All @@ -193,7 +194,7 @@ describe('SafeWebAuthnSharedSigner', () => {
const account = await proxyFactory.createProxyWithNonce.staticCall(safeSingleton, initializer, 0)
await expect(proxyFactory.createProxyWithNonce(safeSingleton, initializer, 0))
.to.emit(sharedSigner.attach(account), 'SafeWebAuthnSharedSignerConfigured')
.withArgs(config.x, config.y, config.verifiers)
.withArgs(publicKeyHash, config.x, config.y, config.verifiers)
})

it('Should revert if not DELEGATECALL-ed', async () => {
Expand Down
2 changes: 1 addition & 1 deletion modules/passkey/test/libraries/WebAuthn.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
})

Expand Down
172 changes: 172 additions & 0 deletions modules/passkey/test/userstories/SafeAddressForPasskey.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
import { expect } from 'chai'
import { deployments, ethers } from 'hardhat'

import { WebAuthnCredentials } from '../../test/utils/webauthnShim'
import { decodePublicKey } from '../../src/utils/webauthn'

/**
* User story: Find Safe for Passkey
* This user story demonstrates how to compute the address of a Safe deterministically for a given
* WebAuthn credential. Note that searching for Safes by owner is not really practical without a
* service (as building Safe owners from Ethereum logs is non-trivial). Instead we show that, given
* a Dapp-specific initial Safe setup with a passkey owner, it is possible to find the Safe address
* corresponding to the passkey.
*/
describe('Safe Address for Passkey [@userstory]', () => {
const setupTests = deployments.createFixture(async ({ deployments }) => {
const { SafeProxyFactory, SafeL2, FCLP256Verifier, SafeWebAuthnSignerFactory, SafeWebAuthnSharedSigner } = await deployments.run()

const safeProxyFactory = await ethers.getContractAt(SafeProxyFactory.abi, SafeProxyFactory.address)
const safeSingleton = await ethers.getContractAt(SafeL2.abi, SafeL2.address)
const signerFactory = await ethers.getContractAt('SafeWebAuthnSignerFactory', SafeWebAuthnSignerFactory.address)
const sharedSigner = await ethers.getContractAt('SafeWebAuthnSharedSigner', SafeWebAuthnSharedSigner.address)
const verifier = await ethers.getContractAt('IP256Verifier', FCLP256Verifier.address)

const navigator = {
credentials: new WebAuthnCredentials(),
}

const credential = navigator.credentials.create({
publicKey: {
rp: {
name: 'Safe',
id: 'safe.global',
},
user: {
id: ethers.getBytes(ethers.id('chucknorris')),
name: 'chucknorris',
displayName: 'Chuck Norris',
},
challenge: ethers.toBeArray(Date.now()),
pubKeyCredParams: [{ type: 'public-key', alg: -7 }],
},
})

const signerConfig = {
...decodePublicKey(credential.response),
verifiers: ethers.solidityPacked(['uint16', 'address'], [0, await verifier.getAddress()]),
}

const deploySafe = async ({ initializer, saltNonce }: { initializer: string; saltNonce: bigint }) => {
const safeAddress = await safeProxyFactory.createProxyWithNonce.staticCall(safeSingleton, initializer, saltNonce)
await safeProxyFactory.createProxyWithNonce(safeSingleton, initializer, saltNonce)
return await ethers.getContractAt(SafeL2.abi, safeAddress)
}

return {
safeSingleton,
safeProxyFactory,
signerFactory,
sharedSigner,
signerConfig,
deploySafe,
}
})

it('should compute the Safe address owned by a WebAuthn proxy signer', async () => {
const { safeSingleton, safeProxyFactory, signerFactory, signerConfig, deploySafe } = await setupTests()

await signerFactory.getSigner(signerConfig.x, signerConfig.y, signerConfig.verifiers)
const signer = await ethers.getContractAt(
'SafeWebAuthnSignerSingleton',
await signerFactory.getSigner(signerConfig.x, signerConfig.y, signerConfig.verifiers),
)

const initializer = safeSingleton.interface.encodeFunctionData('setup', [
[await signer.getAddress()],
1,
ethers.ZeroAddress,
'0x',
ethers.ZeroAddress,
ethers.ZeroAddress,
0,
ethers.ZeroAddress,
])
const saltNonce = 42n

const safe = await deploySafe({ initializer, saltNonce })
const deterministicSafeAddress = ethers.getCreate2Address(
await safeProxyFactory.getAddress(),
ethers.solidityPackedKeccak256(['bytes32', 'uint256'], [ethers.keccak256(initializer), saltNonce]),
ethers.solidityPackedKeccak256(
['bytes', 'bytes'],
[
await safeProxyFactory.proxyCreationCode(),
ethers.AbiCoder.defaultAbiCoder().encode(['address'], [await safeSingleton.getAddress()]),
],
),
)

expect(deterministicSafeAddress).to.equal(await safe.getAddress())
})

it('should compute the Safe address owned by a WebAuthn shared signer', async () => {
const { safeSingleton, safeProxyFactory, sharedSigner, signerConfig, deploySafe } = await setupTests()

const initializer = safeSingleton.interface.encodeFunctionData('setup', [
[await sharedSigner.getAddress()],
1,
await sharedSigner.getAddress(),
sharedSigner.interface.encodeFunctionData('configure', [signerConfig]),
ethers.ZeroAddress,
ethers.ZeroAddress,
0,
ethers.ZeroAddress,
])
const saltNonce = 42n

const safe = await deploySafe({ initializer, saltNonce })
const deterministicSafeAddress = ethers.getCreate2Address(
await safeProxyFactory.getAddress(),
ethers.solidityPackedKeccak256(['bytes32', 'uint256'], [ethers.keccak256(initializer), saltNonce]),
ethers.solidityPackedKeccak256(
['bytes', 'bytes'],
[
await safeProxyFactory.proxyCreationCode(),
ethers.AbiCoder.defaultAbiCoder().encode(['address'], [await safeSingleton.getAddress()]),
],
),
)

expect(deterministicSafeAddress).to.equal(await safe.getAddress())
})

it('should search for Safes owned by a WebAuthn shared signer', async () => {
const { safeSingleton, sharedSigner, signerConfig, deploySafe } = await setupTests()

const safe = await deploySafe({
initializer: safeSingleton.interface.encodeFunctionData('setup', [
[await sharedSigner.getAddress()],
1,
await sharedSigner.getAddress(),
sharedSigner.interface.encodeFunctionData('configure', [signerConfig]),
ethers.ZeroAddress,
ethers.ZeroAddress,
0,
ethers.ZeroAddress,
]),
saltNonce: 0n,
})

let foundSafeAddress = null

const publicKeyHash = ethers.solidityPackedKeccak256(['uint256', 'uint256'], [signerConfig.x, signerConfig.y])
const configuredSafes = await ethers.provider.getLogs({
topics: sharedSigner.interface.encodeFilterTopics('SafeWebAuthnSharedSignerConfigured', [publicKeyHash]),
fromBlock: 0,
})
for (const { address: possibleSafeAddress } of configuredSafes) {
const possibleSafe = safeSingleton.attach(possibleSafeAddress) as typeof safeSingleton

const { x, y } = await sharedSigner.getConfiguration(possibleSafe)
const isOwner = await possibleSafe.isOwner(sharedSigner)

if (signerConfig.x === x && signerConfig.y === y && isOwner) {
foundSafeAddress = possibleSafe
break
}
}

expect(foundSafeAddress).to.equal(await safe.getAddress())
})
})

0 comments on commit c3f6d36

Please sign in to comment.