From d55272ec6596662595f79b927103d08d4001bffc Mon Sep 17 00:00:00 2001 From: qd-qd Date: Thu, 18 Apr 2024 17:37:26 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor=20signature=20val?= =?UTF-8?q?idation=20and=20remove=20fuse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the creation flow fuse mechanism - Replace fuse checks with nonce checks in signature validation - Adjust initialization tests for SmartAccount --- src/v1/Account/SmartAccount.sol | 12 ++--- test/unit/v1/Account/initialize.t.sol | 43 ++++------------ test/unit/v1/Account/initialize.tree | 1 - test/unit/v1/Account/receive.t.sol | 1 - .../Account/validateCreationSignature.t.sol | 49 ++++++------------- .../v1/Account/validateCreationSignature.tree | 3 +- 6 files changed, 31 insertions(+), 78 deletions(-) diff --git a/src/v1/Account/SmartAccount.sol b/src/v1/Account/SmartAccount.sol index 7a7b1df..09444d4 100644 --- a/src/v1/Account/SmartAccount.sol +++ b/src/v1/Account/SmartAccount.sol @@ -28,7 +28,6 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou // ====================================== address internal factoryAddress; - uint96 internal creationFlowFuse; uint256[50] internal __gap; // ====================================== @@ -89,9 +88,6 @@ 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; } // ====================================== @@ -155,6 +151,7 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou /// @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 function _validateCreationSignature( + uint256 nonce, bytes calldata signature, bytes calldata initCode ) @@ -162,9 +159,8 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou virtual returns (uint256) { - // 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; + // 1. check that the nonce is equal to 0 + if (nonce != 0) return Signature.State.FAILURE; // 2. get the address of the factory and check it is the expected one address accountFactory = address(bytes20(initCode[:20])); @@ -252,7 +248,7 @@ contract SmartAccount is Initializable, UUPSUpgradeable, BaseAccount, SmartAccou // 1.b check the signature is a "creation" signature (length is checked by the signature library) if (signatureType == Signature.Type.CREATION) { - return _validateCreationSignature(userOp.signature, userOp.initCode); + return _validateCreationSignature(userOp.nonce, userOp.signature, userOp.initCode); } return Signature.State.FAILURE; diff --git a/test/unit/v1/Account/initialize.t.sol b/test/unit/v1/Account/initialize.t.sol index 3902d8d..cae67be 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 AccountHarness__Initiliaze is BaseTest { +contract SmartAccount__Initiliaze is BaseTest { // @DEV: constant used by the `Initializable` library bytes32 private constant INITIALIZABLE_STORAGE = 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00; - AccountHarness private accountImplementation; + SmartAccount private accountImplementation; AccountFactoryHarness private factory; function setUp() external { // 1. deploy an implementation of the account - accountImplementation = new AccountHarness(makeAddr("entrypoint"), makeAddr("verifier")); + accountImplementation = new SmartAccount(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 AccountHarness__Initiliaze is BaseTest { ); // 2. deploy the account and call the initialize function at the same time - AccountHarness account = factory.exposed_deployAccount( + SmartAccount account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -75,7 +75,7 @@ contract AccountHarness__Initiliaze is BaseTest { ); // 2. deploy the account and call the initialize function at the same time - AccountHarness account = factory.exposed_deployAccount( + SmartAccount account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -90,7 +90,7 @@ contract AccountHarness__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 - AccountHarness account = factory.exposed_deployAccount( + SmartAccount account = factory.exposed_deployAccount( keccak256(createFixtures.signer.credId), createFixtures.signer.pubX, createFixtures.signer.pubY, @@ -109,22 +109,7 @@ contract AccountHarness__Initiliaze is BaseTest { ); } - 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 + // Duplicate of the event in the SmartAccount.sol file event SignerAdded( bytes1 indexed signatureType, bytes credId, bytes32 indexed credIdHash, uint256 pubKeyX, uint256 pubKeyY ); @@ -142,7 +127,7 @@ contract AccountHarness__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 - AccountHarness account = factory.exposed_deployAccount(credIdHash, pubX, pubY, credId); + SmartAccount account = factory.exposed_deployAccount(credIdHash, pubX, pubY, credId); // 3. get the starting slot of the signer bytes32 startingSlot = SignerVaultWebAuthnP256R1.getSignerStartingSlot(credIdHash); @@ -154,14 +139,6 @@ contract AccountHarness__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) { } @@ -172,8 +149,8 @@ contract AccountFactoryHarness is AccountFactory { bytes calldata credId ) external - returns (AccountHarness account) + returns (SmartAccount account) { - return AccountHarness(payable(address(_deployAccount(credIdHash, pubX, pubY, credId)))); + return SmartAccount(payable(address(_deployAccount(credIdHash, pubX, pubY, credId)))); } } diff --git a/test/unit/v1/Account/initialize.tree b/test/unit/v1/Account/initialize.tree index 386ea1a..3f0cf45 100644 --- a/test/unit/v1/Account/initialize.tree +++ b/test/unit/v1/Account/initialize.tree @@ -3,5 +3,4 @@ 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/receive.t.sol b/test/unit/v1/Account/receive.t.sol index c95322d..e00a9c7 100644 --- a/test/unit/v1/Account/receive.t.sol +++ b/test/unit/v1/Account/receive.t.sol @@ -14,7 +14,6 @@ contract SmartAccount__Receive is BaseTest { function test_CanReceiveNativeTokens(address sender, uint256 amount) external { // it can receive native tokens - // sender = bound(100, type(address).max); amount = bound(amount, 1, type(uint256).max); // make the sender the caller for the next call and send him n native tokens diff --git a/test/unit/v1/Account/validateCreationSignature.t.sol b/test/unit/v1/Account/validateCreationSignature.t.sol index cb89781..cfb116b 100644 --- a/test/unit/v1/Account/validateCreationSignature.t.sol +++ b/test/unit/v1/Account/validateCreationSignature.t.sol @@ -51,16 +51,16 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { ); } - function test_FailsIfCalledTwice() external { + function test_FailsIfNonceNot0(uint256 fuzzedNonce) external { // it fails if called twice + fuzzedNonce = bound(fuzzedNonce, 1, type(uint256).max); + // 1. get valid initcode and signature (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); - // 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); + // 2. check the signature validation is failure -- nonce not equal to 0 + assertEq(account.exposed_validateCreationSignature(fuzzedNonce, signature, initCode), Signature.State.FAILURE); } function test_RevertsIfTheInitCodeIsNotCorrectlyConstructed() external { @@ -79,7 +79,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { // 3. check the signature validation is failure vm.expectRevert(); - account.exposed_validateCreationSignature(signature, invalidInitCode); + account.exposed_validateCreationSignature(0, signature, invalidInitCode); } function test_FailsIfTheUseropFactoryIsNotCorrect(address incorrectFactory) external { @@ -99,7 +99,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { ); // 3. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(signature, invalidInitCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, signature, invalidInitCode), Signature.State.FAILURE); } function test_FailsIfTheAdminOfTheFactoryIsNotCorrect(address incorrectSigner) external { @@ -117,7 +117,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { ); // 3. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, signature, initCode), Signature.State.FAILURE); } function test_FailsIfThePassedSignatureIsNotCorrect(string memory name) external { @@ -132,7 +132,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { (bytes memory initCode,) = _calculateInitCodeAndSignature(); // 3. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(invalidSignature, initCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, invalidSignature, initCode), Signature.State.FAILURE); } function test_FailsIfSignatureTypeMissing() external { @@ -145,7 +145,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { bytes memory invalidSignature = truncBytes(signature, 1, signature.length); // 3. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(invalidSignature, initCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, invalidSignature, initCode), Signature.State.FAILURE); } function test_FailsIfTheCredIdDoesNotMatchTheCredIdStored(bytes32 incorrectCredIdHash) external { @@ -163,7 +163,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); // 4. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, signature, initCode), Signature.State.FAILURE); } function test_FailsIfThePubKeyXDoesNotMatchThePubKeyXStored(uint256 incorrectPubKeyX) external { @@ -182,7 +182,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); // 4. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); + assertEq(account.exposed_validateCreationSignature(0, signature, initCode), Signature.State.FAILURE); } function test_FailsIfThePubKeyYDoesNotMatchThePubKeyYStored(uint256 incorrectPubKeyY) external { @@ -201,21 +201,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); // 4. check the signature validation is failure - assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.FAILURE); - } - - 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); + assertEq(account.exposed_validateCreationSignature(0, signature, initCode), Signature.State.FAILURE); } function test_SucceedIfTheSignatureRecoveryIsCorrect() external { @@ -225,7 +211,7 @@ contract SmartAccount__ValidateCreationSignature is BaseTest { (bytes memory initCode, bytes memory signature) = _calculateInitCodeAndSignature(); // 2. check the signature validation is successful - assertEq(account.exposed_validateCreationSignature(signature, initCode), Signature.State.SUCCESS); + assertEq(account.exposed_validateCreationSignature(0, signature, initCode), Signature.State.SUCCESS); } } @@ -233,17 +219,14 @@ contract SmartAccountHarness is SmartAccount { constructor(address entryPoint, address webAuthnVerifier) SmartAccount(entryPoint, webAuthnVerifier) { } function exposed_validateCreationSignature( + uint256 nonce, bytes calldata signature, bytes calldata initCode ) external returns (uint256) { - return _validateCreationSignature(signature, initCode); - } - - function exposed_creationFlowFuse() external view returns (uint256) { - return creationFlowFuse; + return _validateCreationSignature(nonce, signature, initCode); } } diff --git a/test/unit/v1/Account/validateCreationSignature.tree b/test/unit/v1/Account/validateCreationSignature.tree index e914739..913d5da 100644 --- a/test/unit/v1/Account/validateCreationSignature.tree +++ b/test/unit/v1/Account/validateCreationSignature.tree @@ -1,5 +1,5 @@ SmartAccount__ValidateCreationSignature -├── it fails if called twice +├── it fails if nonce not 0 ├── 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,5 +8,4 @@ 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