From d01c20ca9fc0cb020580cf638156d9e387db07d2 Mon Sep 17 00:00:00 2001 From: Nicholas Rodrigues Lordello Date: Mon, 3 Jun 2024 09:52:05 +0200 Subject: [PATCH] Shared Signer Mapping This PR changes the behaviour of the shared signer to store its configuration as if it were behind a mapping where the key is the shared signer address. This allows multiple shared signer instances to be configured for a single account (in case you ever need to deploy a 2/2 Safe owned by passkeys with 4337). I created an "accessor" contract to prove that the storage location is indeed the equivalent to a `mapping(address self => Signer)`. --- .../experimental/SafeWebAuthnSharedSigner.sol | 37 +++++++---- .../contracts/test/TestDependencies.sol | 2 +- .../test/TestSharedWebAuthnSignerAccessor.sol | 19 ++++++ .../SafeWebAuthnSharedSigner.spec.ts | 66 +++++++++++++++---- 4 files changed, 98 insertions(+), 26 deletions(-) create mode 100644 modules/passkey/contracts/test/TestSharedWebAuthnSignerAccessor.sol diff --git a/modules/passkey/contracts/4337/experimental/SafeWebAuthnSharedSigner.sol b/modules/passkey/contracts/4337/experimental/SafeWebAuthnSharedSigner.sol index db9a90302..d4fb68c89 100644 --- a/modules/passkey/contracts/4337/experimental/SafeWebAuthnSharedSigner.sol +++ b/modules/passkey/contracts/4337/experimental/SafeWebAuthnSharedSigner.sol @@ -14,7 +14,7 @@ contract SafeWebAuthnSharedSigner is SignatureValidator { /** * @notice Data associated with a WebAuthn signer. It represents the X and Y coordinates of the * signer's public key as well as the P256 verifiers to use. This is stored in account storage - * starting at the storage slot {_SIGNER_SLOT}. + * starting at the storage slot {SIGNER_SLOT}. */ struct Signer { uint256 x; @@ -23,13 +23,14 @@ contract SafeWebAuthnSharedSigner is SignatureValidator { } /** - * @notice The starting storage slot containing the signer data. - * @custom:computed-as keccak256("SafeWebAuthnSharedSigner.signer") - 3 - * @dev This value is intentionally computed to be a hash -3 as a precaution to avoid any + * @notice The storage slot of the mapping from shared WebAuthn signer address to signer data. + * @custom:computed-as keccak256("SafeWebAuthnSharedSigner.signer") - 1 + * @dev This value is intentionally computed to be a hash -1 as a precaution to avoid any * potential issues from unintended hash collisions, and have enough space for all the signer - * fields. + * fields. Also, this is the slot of a `mapping(address self => Signer)` to ensure that multiple + * {SafeWebAuthnSharedSigner} instances can coexist with the same account. */ - uint256 private constant _SIGNER_SLOT = 0x2e0aed53485dc2290ceb5ce14725558ad3e3a09d38c69042410ad15c2b4ea4e6; + uint256 private constant _SIGNER_MAPPING_SLOT = 0x2e0aed53485dc2290ceb5ce14725558ad3e3a09d38c69042410ad15c2b4ea4e8; /** * @notice An error indicating a `CALL` to a function that should only be `DELEGATECALL`-ed. @@ -37,16 +38,23 @@ contract SafeWebAuthnSharedSigner is SignatureValidator { error NotDelegateCalled(); /** - * @dev Address of the launchpad contract itself. it is used for determining whether or not the - * contract is being `DELEGATECALL`-ed when setting signer data. + * @notice Address of the shared signer contract itself. + * @dev This is used for determining whether or not the contract is being `DELEGATECALL`-ed when + * setting signer data. */ address private immutable _SELF; + /** + * @notice The starting storage slot on the account containing the signer data. + */ + uint256 public immutable SIGNER_SLOT; + /** * @notice Create a new shared WebAuthn signer instance. */ constructor() { _SELF = address(this); + SIGNER_SLOT = uint256(keccak256(abi.encode(address(this), _SIGNER_MAPPING_SLOT))); } /** @@ -66,7 +74,7 @@ contract SafeWebAuthnSharedSigner is SignatureValidator { * @param account The account to request signer data for. */ function getConfiguration(address account) public view returns (Signer memory signer) { - bytes memory getStorageAtData = abi.encodeCall(ISafe(account).getStorageAt, (_SIGNER_SLOT, 3)); + bytes memory getStorageAtData = abi.encodeCall(ISafe(account).getStorageAt, (SIGNER_SLOT, 3)); // Call the {StorageAccessible.getStorageAt} with assembly. This allows us to return a // zeroed out signer configuration instead of reverting for `account`s that are not Safes. @@ -114,16 +122,17 @@ contract SafeWebAuthnSharedSigner is SignatureValidator { * @param signer The new signer data to set for the calling account. */ function configure(Signer memory signer) external onlyDelegateCall { - Signer storage signerSlot; + uint256 signerSlot = SIGNER_SLOT; + Signer storage signerStorage; // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { - signerSlot.slot := _SIGNER_SLOT + signerStorage.slot := signerSlot } - signerSlot.x = signer.x; - signerSlot.y = signer.y; - signerSlot.verifiers = signer.verifiers; + signerStorage.x = signer.x; + signerStorage.y = signer.y; + signerStorage.verifiers = signer.verifiers; } /** diff --git a/modules/passkey/contracts/test/TestDependencies.sol b/modules/passkey/contracts/test/TestDependencies.sol index e9c81a4ac..fcfd8ed63 100644 --- a/modules/passkey/contracts/test/TestDependencies.sol +++ b/modules/passkey/contracts/test/TestDependencies.sol @@ -5,5 +5,5 @@ pragma solidity >=0.8.0; import "@account-abstraction/contracts/interfaces/IEntryPoint.sol"; import "@account-abstraction/contracts/samples/VerifyingPaymaster.sol"; import "@safe-global/mock-contract/contracts/MockContract.sol"; -import "@safe-global/safe-contracts/contracts/proxies/SafeProxyFactory.sol"; import "@safe-global/safe-contracts/contracts/libraries/MultiSend.sol"; +import "@safe-global/safe-contracts/contracts/proxies/SafeProxyFactory.sol"; diff --git a/modules/passkey/contracts/test/TestSharedWebAuthnSignerAccessor.sol b/modules/passkey/contracts/test/TestSharedWebAuthnSignerAccessor.sol new file mode 100644 index 000000000..1ba8d974b --- /dev/null +++ b/modules/passkey/contracts/test/TestSharedWebAuthnSignerAccessor.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity ^0.8.0; + +import {SafeWebAuthnSharedSigner} from "../4337/experimental/SafeWebAuthnSharedSigner.sol"; + +contract TestSharedWebAuthnSignerAccessor { + function getSignerConfiguration(address sharedSigner) external view returns (SafeWebAuthnSharedSigner.Signer memory signer) { + signer = _getSigners()[sharedSigner]; + } + + function _getSigners() private pure returns (mapping(address => SafeWebAuthnSharedSigner.Signer) storage signers) { + uint256 signersSlot = uint256(keccak256("SafeWebAuthnSharedSigner.signer")) - 1; + + // solhint-disable-next-line no-inline-assembly + assembly ("memory-safe") { + signers.slot := signersSlot + } + } +} diff --git a/modules/passkey/test/4337/experimental/SafeWebAuthnSharedSigner.spec.ts b/modules/passkey/test/4337/experimental/SafeWebAuthnSharedSigner.spec.ts index 96c1bbc2b..ac1300945 100644 --- a/modules/passkey/test/4337/experimental/SafeWebAuthnSharedSigner.spec.ts +++ b/modules/passkey/test/4337/experimental/SafeWebAuthnSharedSigner.spec.ts @@ -5,32 +5,41 @@ import * as ERC1271 from '../../utils/erc1271' import { DUMMY_AUTHENTICATOR_DATA, base64UrlEncode, getSignatureBytes } from '../../../src/utils/webauthn' import { encodeWebAuthnSigningMessage } from '../../utils/webauthnShim' -const SIGNER_STORAGE_SLOT = ethers.toBeHex(BigInt(ethers.id('SafeWebAuthnSharedSigner.signer')) - 3n, 32) +const SIGNER_MAPPING_SLOT = BigInt(ethers.id('SafeWebAuthnSharedSigner.signer')) - 1n describe('SafeWebAuthnSharedSigner', () => { const setupTests = deployments.createFixture(async () => { - const { SafeL2, SafeProxyFactory, SafeWebAuthnSharedSigner } = await deployments.run() + const { SafeL2, SafeProxyFactory, CompatibilityFallbackHandler, SafeWebAuthnSharedSigner } = await deployments.run() const safeSingleton = await ethers.getContractAt('ISafe', SafeL2.address) + const fallbackHandler = await ethers.getContractAt(CompatibilityFallbackHandler.abi, CompatibilityFallbackHandler.address) const proxyFactory = await ethers.getContractAt('SafeProxyFactory', SafeProxyFactory.address) const sharedSigner = await ethers.getContractAt('SafeWebAuthnSharedSigner', SafeWebAuthnSharedSigner.address) + const signerSlot = ethers.solidityPackedKeccak256(['uint256', 'uint256'], [sharedSigner.target, SIGNER_MAPPING_SLOT]) + const MockContract = await ethers.getContractFactory('MockContract') const mockAccount = await MockContract.deploy() const mockVerifier = await MockContract.deploy() + const TestSharedWebAuthnSignerAccessor = await ethers.getContractFactory('TestSharedWebAuthnSignerAccessor') + const sharedSignerAccessor = await TestSharedWebAuthnSignerAccessor.deploy() + return { safeSingleton, + fallbackHandler, proxyFactory, sharedSigner, + signerSlot, mockAccount, mockVerifier, + sharedSignerAccessor, } }) describe('getConfiguration', function () { it('Should return a configuration for an account', async () => { - const { safeSingleton, sharedSigner, mockAccount, mockVerifier } = await setupTests() + const { safeSingleton, sharedSigner, signerSlot, mockAccount, mockVerifier } = await setupTests() const publicKey = { x: BigInt(ethers.id('publicKey.x')), @@ -38,7 +47,7 @@ describe('SafeWebAuthnSharedSigner', () => { } await mockAccount.givenCalldataReturn( - safeSingleton.interface.encodeFunctionData('getStorageAt', [SIGNER_STORAGE_SLOT, 3]), + safeSingleton.interface.encodeFunctionData('getStorageAt', [signerSlot, 3]), ethers.AbiCoder.defaultAbiCoder().encode( ['bytes'], [ethers.AbiCoder.defaultAbiCoder().encode(['uint256', 'uint256', 'uint176'], [publicKey.x, publicKey.y, mockVerifier.target])], @@ -94,11 +103,47 @@ describe('SafeWebAuthnSharedSigner', () => { expect(await sharedSigner.getConfiguration(mockAccount)).to.deep.equal([0n, 0n, 0n]) } }) + + it('Should store the signer configuration in a mapping with the shared signer address as a key', async () => { + const { safeSingleton, fallbackHandler, proxyFactory, sharedSigner, mockVerifier, sharedSignerAccessor } = await setupTests() + + const config = { + x: ethers.id('publicKey.x'), + y: ethers.id('publicKey.y'), + verifiers: ethers.toBeHex(await mockVerifier.getAddress(), 32), + } + + const initializer = safeSingleton.interface.encodeFunctionData('setup', [ + [sharedSigner.target], + 1, + sharedSigner.target, + sharedSigner.interface.encodeFunctionData('configure', [config]), + fallbackHandler.target, + ethers.ZeroAddress, + 0, + ethers.ZeroAddress, + ]) + + const safeProxy = fallbackHandler.attach( + await proxyFactory.createProxyWithNonce.staticCall(safeSingleton, initializer, 0), + ) as typeof fallbackHandler + await proxyFactory.createProxyWithNonce(safeSingleton, initializer, 0) + + expect( + ethers.AbiCoder.defaultAbiCoder().decode( + ['uint256', 'uint256', 'uint176'], + await safeProxy.simulate.staticCall( + sharedSignerAccessor.target, + sharedSignerAccessor.interface.encodeFunctionData('getSignerConfiguration', [sharedSigner.target]), + ), + ), + ).to.deep.equal([config.x, config.y, BigInt(config.verifiers)]) + }) }) describe('configure', function () { it('Should configure an account', async () => { - const { safeSingleton, proxyFactory, sharedSigner, mockVerifier } = await setupTests() + const { safeSingleton, proxyFactory, sharedSigner, signerSlot, mockVerifier } = await setupTests() const config = { x: ethers.id('publicKey.x'), @@ -120,10 +165,9 @@ describe('SafeWebAuthnSharedSigner', () => { const account = await proxyFactory.createProxyWithNonce.staticCall(safeSingleton, initializer, 0) await proxyFactory.createProxyWithNonce(safeSingleton, initializer, 0) - expect(await ethers.provider.getStorage(account, SIGNER_STORAGE_SLOT)).to.equal(config.x) - expect(await ethers.provider.getStorage(account, BigInt(SIGNER_STORAGE_SLOT) + 1n)).to.equal(config.y) - expect(await ethers.provider.getStorage(account, BigInt(SIGNER_STORAGE_SLOT) + 2n)).to.equal(config.verifiers) - expect(await ethers.provider.getStorage(account, ethers.id('SafeWebAuthnSharedSigner.signer'))).to.equal(ethers.ZeroHash) + expect(await ethers.provider.getStorage(account, signerSlot)).to.equal(config.x) + expect(await ethers.provider.getStorage(account, BigInt(signerSlot) + 1n)).to.equal(config.y) + expect(await ethers.provider.getStorage(account, BigInt(signerSlot) + 2n)).to.equal(config.verifiers) }) it('Should revert if not DELEGATECALL-ed', async () => { @@ -135,7 +179,7 @@ describe('SafeWebAuthnSharedSigner', () => { describe('isValidSignature', function () { it('Should return a magic value for an account', async () => { - const { safeSingleton, sharedSigner, mockAccount, mockVerifier } = await setupTests() + const { safeSingleton, sharedSigner, signerSlot, mockAccount, mockVerifier } = await setupTests() const data = ethers.toUtf8Bytes('some data to sign') const dataHash = ethers.keccak256(data) @@ -159,7 +203,7 @@ describe('SafeWebAuthnSharedSigner', () => { }) await mockAccount.givenCalldataReturn( - safeSingleton.interface.encodeFunctionData('getStorageAt', [SIGNER_STORAGE_SLOT, 3]), + safeSingleton.interface.encodeFunctionData('getStorageAt', [signerSlot, 3]), ethers.AbiCoder.defaultAbiCoder().encode( ['bytes'], [ethers.AbiCoder.defaultAbiCoder().encode(['uint256', 'uint256', 'uint176'], [x, y, mockVerifier.target])],