From b6471eed46f4773476090613d1d470c11201fab7 Mon Sep 17 00:00:00 2001 From: qd-qd Date: Mon, 15 Apr 2024 16:07:50 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A7=20Add=20creationFlowFuse=20for=20c?= =?UTF-8?q?reation=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add internal uint96 for tracking creation process - Implement logic to activate and deactivate fuse - Modify _validateCreationSignature to check and reset creationFlowFuse ensuring single use - Update unit tests for new creation flow logic --- src/v1/Account/SmartAccount.sol | 11 +++-- test/unit/v1/Account/initialize.t.sol | 43 ++++++++++++++----- test/unit/v1/Account/initialize.tree | 1 + .../Account/validateCreationSignature.t.sol | 41 ++++++++++++------ .../v1/Account/validateCreationSignature.tree | 3 +- test/unit/v1/AccountFactory/constructor.t.sol | 4 -- 6 files changed, 70 insertions(+), 33 deletions(-) diff --git a/src/v1/Account/SmartAccount.sol b/src/v1/Account/SmartAccount.sol index bd2086e..dacd2c6 100644 --- a/src/v1/Account/SmartAccount.sol +++ b/src/v1/Account/SmartAccount.sol @@ -28,6 +28,7 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou // ====================================== address internal factoryAddress; + uint96 internal creationFlowFuse; // ====================================== // =========== EVENTS/ERRORS ============ @@ -87,6 +88,9 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou // 2. set the first signer _addWebAuthnSigner(credIdHash, pubX, pubY, credId); + + // 3. activate the creation flow fuse + creationFlowFuse = 1; } // ====================================== @@ -149,18 +153,17 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou /// @param signature The signature field presents in the userOp. /// @param initCode The initCode field presents in the userOp. It has been used to create the account /// @return 0 if the signature is valid, 1 otherwise - // TODO: use transient storage for the expected signer/factory? function _validateCreationSignature( bytes calldata signature, bytes calldata initCode ) internal - view virtual returns (uint256) { - // 1. check that the nonce is 0x00. The value of the first key is checked here - if (getNonce() != 0) return Signature.State.FAILURE; + // 1. check that we are in the creation flow -- either return failure if not or deactivate the creation flow fuse + if (creationFlowFuse != 1) return Signature.State.FAILURE; + creationFlowFuse = 0; // 2. get the address of the factory and check it is the expected one address accountFactory = address(bytes20(initCode[:20])); diff --git a/test/unit/v1/Account/initialize.t.sol b/test/unit/v1/Account/initialize.t.sol index 06b8d0e..3902d8d 100644 --- a/test/unit/v1/Account/initialize.t.sol +++ b/test/unit/v1/Account/initialize.t.sol @@ -8,15 +8,15 @@ import { BaseTest } from "test/BaseTest/BaseTest.sol"; import { SignerVaultWebAuthnP256R1 } from "src/utils/SignerVaultWebAuthnP256R1.sol"; import "src/utils/Signature.sol" as Signature; -contract SmartAccount__Initiliaze is BaseTest { +contract AccountHarness__Initiliaze is BaseTest { // @DEV: constant used by the `Initializable` library bytes32 private constant INITIALIZABLE_STORAGE = 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00; - SmartAccount private accountImplementation; + AccountHarness private accountImplementation; AccountFactoryHarness private factory; function setUp() external { // 1. deploy an implementation of the account - accountImplementation = new SmartAccount(makeAddr("entrypoint"), makeAddr("verifier")); + accountImplementation = new AccountHarness(makeAddr("entrypoint"), makeAddr("verifier")); // 2. deploy an implementation of the factory and its instance factory = new AccountFactoryHarness(address(accountImplementation), makeAddr("factorySigner")); @@ -46,7 +46,7 @@ contract SmartAccount__Initiliaze is BaseTest { ); // 2. deploy the account and call the initialize function at the same time - SmartAccount account = factory.exposed_deployAccount( + AccountHarness account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -75,7 +75,7 @@ contract SmartAccount__Initiliaze is BaseTest { ); // 2. deploy the account and call the initialize function at the same time - SmartAccount account = factory.exposed_deployAccount( + AccountHarness account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -90,7 +90,7 @@ contract SmartAccount__Initiliaze is BaseTest { // it can be called using a proxy and set version to 1 // 1. deploy the account and call the initialize function at the same time - SmartAccount account = factory.exposed_deployAccount( + AccountHarness account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -109,7 +109,22 @@ contract SmartAccount__Initiliaze is BaseTest { ); } - // Duplicate of the event in the SmartAccount.sol file + function test_ItSetsTheCreationFuse() external setUpCreateFixture { + // it stores the signer + + // 1. deploy the account and call the initialize function at the same time + AccountHarness account = factory.exposed_deployAccount( + keccak256(createFixtures.signer.credId), + createFixtures.signer.pubX, + createFixtures.signer.pubY, + createFixtures.signer.credId + ); + + // 2. check the creation fuse has been set + assertEq(account.exposed_creationFlowFuse(), 1); + } + + // Duplicate of the event in the AccountHarness.sol file event SignerAdded( bytes1 indexed signatureType, bytes credId, bytes32 indexed credIdHash, uint256 pubKeyX, uint256 pubKeyY ); @@ -127,7 +142,7 @@ contract SmartAccount__Initiliaze is BaseTest { emit SignerAdded(Signature.Type.WEBAUTHN_P256R1, credId, credIdHash, pubX, pubY); // 2. deploy the account and call the initialize function at the same time - SmartAccount account = factory.exposed_deployAccount(credIdHash, pubX, pubY, credId); + AccountHarness account = factory.exposed_deployAccount(credIdHash, pubX, pubY, credId); // 3. get the starting slot of the signer bytes32 startingSlot = SignerVaultWebAuthnP256R1.getSignerStartingSlot(credIdHash); @@ -139,6 +154,14 @@ contract SmartAccount__Initiliaze is BaseTest { } } +contract AccountHarness is SmartAccount { + constructor(address entrypoint, address verifier) SmartAccount(entrypoint, verifier) { } + + function exposed_creationFlowFuse() external view returns (uint256) { + return creationFlowFuse; + } +} + contract AccountFactoryHarness is AccountFactory { constructor(address accountImplementation, address operator) AccountFactory(accountImplementation, operator) { } @@ -149,8 +172,8 @@ contract AccountFactoryHarness is AccountFactory { bytes calldata credId ) external - returns (SmartAccount account) + returns (AccountHarness account) { - return _deployAccount(credIdHash, pubX, pubY, credId); + return AccountHarness(payable(address(_deployAccount(credIdHash, pubX, pubY, credId)))); } } diff --git a/test/unit/v1/Account/initialize.tree b/test/unit/v1/Account/initialize.tree index 3f0cf45..386ea1a 100644 --- a/test/unit/v1/Account/initialize.tree +++ b/test/unit/v1/Account/initialize.tree @@ -3,4 +3,5 @@ SmartAccount__Initiliaze ├── it can be called using a proxy and set version to 1 ├── it store the initiator ├── it store the first signer +├── it sets the creation fuse └── it can not be called twice diff --git a/test/unit/v1/Account/validateCreationSignature.t.sol b/test/unit/v1/Account/validateCreationSignature.t.sol index 5617c30..cb89781 100644 --- a/test/unit/v1/Account/validateCreationSignature.t.sol +++ b/test/unit/v1/Account/validateCreationSignature.t.sol @@ -51,19 +51,15 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { ); } - function test_FailsIfTheNonceIsNot0(uint256 randomNonce) external { - // it fails if the nonce is not 0 + function test_FailsIfCalledTwice() external { + // it fails if called twice - // bound the nonce to a invalid range - randomNonce = bound(randomNonce, 1, type(uint256).max); - - // get valid initcode and signature + // 1. get valid initcode and signature (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); - // mock the call to the entrypoint to return the random nonce - vm.mockCall(address(entrypoint), abi.encodeWithSelector(MockEntryPoint.getNonce.selector), abi.encode(1)); - - // assert that the signature validation fails if the nonce is not equal to zero + // 2. check the signature validation is successful + assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.SUCCESS); + // 3. ensure we cannot calling it twice assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); } @@ -86,7 +82,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { account.exposed_validateCreationSignature(signature, invalidInitCode); } - function test_FailsIfTheUseropFactoryIsNotCorrect(address incorrectFactory) external view { + function test_FailsIfTheUseropFactoryIsNotCorrect(address incorrectFactory) external { // it fails if the userop factory is not correct vm.assume(incorrectFactory != address(factory)); @@ -139,7 +135,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { assertEq(account.exposed_validateCreationSignature(invalidSignature, initCode), Signature.State.FAILURE); } - function test_FailsIfSignatureTypeMissing() external view { + function test_FailsIfSignatureTypeMissing() external { // it fails if the passed signature is not correct // 1. get valid initcode and signature @@ -208,7 +204,21 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); } - function test_SucceedIfTheSignatureRecoveryIsCorrect() external view { + function test_SetsTheCreationFuseToZero() external { + // it succeed if the signature recovery is correct + + // 1. check that the creation flow fuse is initially set to 1 + assertEq(account.exposed_creationFlowFuse(), 1); + + // 2. get valid initcode and signature + (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); + + // 3. make sure the creation flow fuse is set to 0 after calling the function + account.exposed_validateCreationSignature(signature, initCode); + assertEq(account.exposed_creationFlowFuse(), 0); + } + + function test_SucceedIfTheSignatureRecoveryIsCorrect() external { // it succeed if the signature recovery is correct // 1. get valid initcode and signature @@ -227,11 +237,14 @@ contract SmartAccountHarness is SmartAccount { bytes calldata initCode ) external - view returns (uint256) { return _validateCreationSignature(signature, initCode); } + + function exposed_creationFlowFuse() external view returns (uint256) { + return creationFlowFuse; + } } contract AccountFactoryHarness is AccountFactory { diff --git a/test/unit/v1/Account/validateCreationSignature.tree b/test/unit/v1/Account/validateCreationSignature.tree index e1f7d2d..e914739 100644 --- a/test/unit/v1/Account/validateCreationSignature.tree +++ b/test/unit/v1/Account/validateCreationSignature.tree @@ -1,5 +1,5 @@ SmartAccount__ValidateCreationSignature -├── it fails if the nonce is not 0 +├── it fails if called twice ├── it reverts if the initCode is not correctly constructed ├── it fails if the userop factory is not correct ├── it fails if the admin of the factory is not correct @@ -8,4 +8,5 @@ SmartAccount__ValidateCreationSignature ├── it fails if the credId does not match the credId stored ├── it fails if the pubKeyX does not match the pubKeyX stored ├── it fails if the pubKeyY does not match the pubKeyY stored +├── it sets the creation fuse to zero └── it succeed if the signature recovery is correct diff --git a/test/unit/v1/AccountFactory/constructor.t.sol b/test/unit/v1/AccountFactory/constructor.t.sol index 332474b..7826855 100644 --- a/test/unit/v1/AccountFactory/constructor.t.sol +++ b/test/unit/v1/AccountFactory/constructor.t.sol @@ -1,14 +1,10 @@ // SPDX-License-Identifier: APACHE-2.0 pragma solidity >=0.8.20; -import { Initializable } from "@openzeppelin/proxy/utils/Initializable.sol"; import { AccountFactory } from "src/v1/AccountFactory.sol"; import { BaseTest } from "test/BaseTest/BaseTest.sol"; import { Metadata } from "src/v1/Metadata.sol"; -// @DEV: constant used by the `Initializable` library -bytes32 constant INITIALIZABLE_STORAGE = 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00; - contract AccountFactory__Constructor is BaseTest { function test_RevertIfAccountImplementationIs0() external { // it revert if account implementation is 0