diff --git a/src/Account.sol b/src/Account.sol index 5de5efe..94a682c 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -175,7 +175,7 @@ contract Account is Initializable, BaseAccount { // - 4 bytes for the selector of the factory function called --NOT_USED-- // - 32 bytes for the X coordinate of the public key // - 32 bytes for the Y coordinate of the public key - // - 32 bytes for the loginHash + // - 32 bytes for the usernameHash // - 32 bytes for the credIdHash // - X bytes for the signature --NOT_USED-- // @@ -189,27 +189,29 @@ contract Account is Initializable, BaseAccount { // // Note that any change in the factory's function signature will break the signature validation of this account! address userOpFactory = address(bytes20(initCode[:20])); - uint256 pubKeyX = uint256(bytes32(initCode[24:56])); - uint256 pubKeyY = uint256(bytes32(initCode[56:88])); - bytes32 loginHash = bytes32(initCode[88:120]); + uint256 pubX = uint256(bytes32(initCode[24:56])); + uint256 pubY = uint256(bytes32(initCode[56:88])); + bytes32 usernameHash = bytes32(initCode[88:120]); bytes32 credIdHash = bytes32(initCode[120:152]); // 4. check the factory is the same than the one stored here if (userOpFactory != factory) return Signature.State.FAILURE; // 5. recreate the message and try to recover the signer - bytes memory message = abi.encode(Signature.Type.CREATION, loginHash, pubKeyX, pubKeyY, credIdHash); + bytes memory message = + abi.encode(Signature.Type.CREATION, usernameHash, pubX, pubY, credIdHash, address(this), block.chainid); // 6. fetch the expected signer from the factory contract address expectedSigner = AccountFactory(factory).admin(); // 7. Check the signature is valid and revert if it is not - if (Signature.recover(expectedSigner, message, signature) == false) return Signature.State.FAILURE; + // NOTE: The signature prefix, added manually to identify the signature, is removed before the recovering process + if (Signature.recover(expectedSigner, message, signature[1:]) == false) return Signature.State.FAILURE; // 8. Check the signer is the same than the one stored by the factory during the account creation process // solhint-disable-next-line var-name-mixedcase (bytes32 $credIdHash, uint256 $pubkeyX, uint256 $pubkeyY) = SignerVaultWebAuthnP256R1.get(credIdHash); - if ($credIdHash != credIdHash || $pubkeyX != pubKeyX || $pubkeyY != pubKeyY) return Signature.State.FAILURE; + if ($credIdHash != credIdHash || $pubkeyX != pubX || $pubkeyY != pubY) return Signature.State.FAILURE; return Signature.State.SUCCESS; } diff --git a/src/AccountFactory.sol b/src/AccountFactory.sol index 5e94b48..dda7531 100644 --- a/src/AccountFactory.sol +++ b/src/AccountFactory.sol @@ -3,7 +3,7 @@ pragma solidity >=0.8.20 <0.9.0; import { ERC1967Proxy } from "@openzeppelin/proxy/ERC1967/ERC1967Proxy.sol"; import "src/utils/Signature.sol" as Signature; -import { Account } from "./Account.sol"; +import { Account as SmartAccount } from "./Account.sol"; // TODO: Implement an universal registry and use it to store the value of `admin` // FIXME: createAndInitAccount() Understand the implications of the ban system of the function @@ -25,10 +25,10 @@ contract AccountFactory { address public immutable admin; event AccountCreated( - bytes32 loginHash, address account, bytes32 indexed credIdHash, uint256 pubKeyX, uint256 pubKeyY + bytes32 usernameHash, address account, bytes32 indexed credIdHash, uint256 pubKeyX, uint256 pubKeyY ); - error InvalidSignature(bytes32 loginHash, bytes signature); + error InvalidSignature(bytes32 usernameHash, bytes signature); /// @notice Deploy the implementation of the account and store it in the storage of the factory. This /// implementation will be used as the implementation reference for all the proxies deployed by this @@ -48,25 +48,16 @@ contract AccountFactory { admin = _admin; // deploy the implementation of the account - Account account = new Account(entryPoint, webAuthnVerifier); + SmartAccount account = new SmartAccount(entryPoint, webAuthnVerifier); // set the address of the implementation deployed accountImplementation = payable(address(account)); } - /// @notice This function check if an account already exists based on the loginHash given - /// @param loginHash The keccak256 hash of the login of the account - /// @return The address of the account if it exists, address(0) otherwise - function _checkAccountExistence(bytes32 loginHash) internal view returns (address) { - // calculate the address of the account based on the loginHash and return it if it exists - address calulatedAddress = getAddress(loginHash); - return calulatedAddress.code.length > 0 ? calulatedAddress : address(0); - } - /// @notice This function check if the signature is signed by the correct entity /// @param pubKeyX The X coordinate of the public key of the first signer. We use the r1 curve here /// @param pubKeyY The Y coordinate of the public key of the first signer. We use the r1 curve here - /// @param loginHash The keccak256 hash of the login of the account + /// @param usernameHash The keccak256 hash of the login of the account /// @param credIdHash The hash of the WebAuthn credential ID of the signer. Check the specification /// @param signature Signature made off-chain. Its recovery must match the admin. /// @return True if the signature is legit, false otherwise @@ -74,16 +65,18 @@ contract AccountFactory { function _isSignatureLegit( uint256 pubKeyX, uint256 pubKeyY, - bytes32 loginHash, + bytes32 usernameHash, bytes32 credIdHash, + address accountAddress, bytes calldata signature ) internal - view returns (bool) { // recreate the message signed by the admin - bytes memory message = abi.encode(Signature.Type.CREATION, loginHash, pubKeyX, pubKeyY, credIdHash); + bytes memory message = abi.encode( + Signature.Type.CREATION, usernameHash, pubKeyX, pubKeyY, credIdHash, accountAddress, block.chainid + ); // try to recover the address and return the result return Signature.recover(admin, message, signature); @@ -93,55 +86,55 @@ contract AccountFactory { /// or returns the address of an existing account based on the parameter given /// @param pubKeyX The X coordinate of the public key of the first signer. We use the r1 curve here /// @param pubKeyY The Y coordinate of the public key of the first signer. We use the r1 curve here - /// @param loginHash The keccak256 hash of the login of the account + /// @param usernameHash The keccak256 hash of the login of the account /// @param credIdHash The hash of the WebAuthn credential ID of the signer. Check the specification /// @param signature Signature made off-chain. Its recovery must match the admin. - /// The loginHash is expected to be the hash used by the recover function. + /// The usernameHash is expected to be the hash used by the recover function. /// @return The address of the account (either deployed or not) function createAndInitAccount( uint256 pubKeyX, uint256 pubKeyY, - bytes32 loginHash, + bytes32 usernameHash, bytes32 credIdHash, bytes calldata signature ) external returns (address) { - // check if the account is already deployed and return prematurely if it is - address alreadyDeployedAddress = _checkAccountExistence(loginHash); - if (alreadyDeployedAddress != address(0)) { - return alreadyDeployedAddress; - } + // 1. get the address of the account if it exists + address accountAddress = getAddress(usernameHash); + + // 2. check if the account is already deployed and return prematurely if it is + if (accountAddress.code.length > 0) return accountAddress; - // check if the signature is valid - if (_isSignatureLegit(pubKeyX, pubKeyY, loginHash, credIdHash, signature) == false) { - revert InvalidSignature(loginHash, signature); + // 3. check if the signature is valid + if (_isSignatureLegit(pubKeyX, pubKeyY, usernameHash, credIdHash, accountAddress, signature) == false) { + revert InvalidSignature(usernameHash, signature); } - // deploy the proxy for the user. During the deployment, the initialize function in the implementation contract + // 4. deploy the proxy for the user. During the deployment, the initialize function in the implementation // is called using the `delegatecall` opcode - Account account = Account( + SmartAccount account = SmartAccount( payable( - new ERC1967Proxy{ salt: loginHash }( - accountImplementation, abi.encodeWithSelector(Account.initialize.selector) + new ERC1967Proxy{ salt: usernameHash }( + accountImplementation, abi.encodeWithSelector(SmartAccount.initialize.selector) ) ) ); - // set the first signer of the account using the parameters given + // 5. set the initial signer of the account using the parameters given account.addFirstSigner(pubKeyX, pubKeyY, credIdHash); - emit AccountCreated(loginHash, address(account), credIdHash, pubKeyX, pubKeyY); - + // 6. emit the event and return the address of the deployed account + emit AccountCreated(usernameHash, address(account), credIdHash, pubKeyX, pubKeyY); return address(account); } /// @notice This utility function returns the address of the account that would be deployed /// @dev This is the under the hood formula used by the CREATE2 opcode - /// @param loginHash The keccak256 hash of the login of the account + /// @param usernameHash The keccak256 hash of the login of the account /// @return The address of the account that would be deployed - function getAddress(bytes32 loginHash) public view returns (address) { + function getAddress(bytes32 usernameHash) public view returns (address) { return address( uint160( uint256( @@ -149,14 +142,14 @@ contract AccountFactory { abi.encodePacked( bytes1(0xff), // init code hash prefix address(this), // deployer address - loginHash, // the salt used to deploy the contract + usernameHash, // the salt used to deploy the contract keccak256( // the init code hash abi.encodePacked( // creation code of the contract deployed type(ERC1967Proxy).creationCode, // arguments passed to the constructor of the contract deployed abi.encode( - accountImplementation, abi.encodeWithSelector(Account.initialize.selector) + accountImplementation, abi.encodeWithSelector(SmartAccount.initialize.selector) ) ) ) @@ -176,12 +169,12 @@ contract AccountFactory { // - CREATE2 is used to deploy the proxy for our users. The formula of this deterministic computation // depends on these parameters: // - the address of the factory -// - the loginHash +// - the usernameHash // - the implementation of the ERC1967Proxy (included in the init code hash) // - the arguments passed to the constructor of the ERC1967Proxy (included in the init code hash): // - the address of the implementation of the account // - the signature selector of the initialize function present in the account implementation (first 4-bytes) -// - the loginHash +// - the usernameHash // // - Once set, it's not possible to change the account implementation later. // - Once deployed by the constructor, it's not possible to change the instance of the account implementation. diff --git a/test/BaseTest.sol b/test/BaseTest.sol index ff6b9c2..5f4b05a 100644 --- a/test/BaseTest.sol +++ b/test/BaseTest.sol @@ -4,35 +4,47 @@ pragma solidity >=0.8.20 <0.9.0; import { Test } from "forge-std/Test.sol"; import { VmSafe } from "forge-std/Vm.sol"; import { MessageHashUtils } from "@openzeppelin/utils/cryptography/MessageHashUtils.sol"; +import { AccountFactory } from "src/AccountFactory.sol"; +import "src/utils/Signature.sol" as Signature; struct ValidCreationParams { uint256 pubKeyX; uint256 pubKeyY; - string login; - bytes32 loginHash; + bytes32 usernameHash; bytes32 credIdHash; - bytes signature; address signer; + bytes signature; } contract BaseTest is Test { + VmSafe.Wallet internal signer; + // store a set of valid creation parameters that can be used in tests ValidCreationParams internal validCreate; + // use a random username hash + bytes32 internal __usernameHash = 0x975f1d0637811aba12c41f7b0d68ee42aa76a3a1cba43b96f60f0e3ea2f2206a; + + // generate a credId hash that corresponds to the login + bytes32 internal __credIdHash = 0x511deddb8b836e8ced2f5e8a4ee0ac63ae4095b426398cc712aa772a4c68a099; + // generate a set of valid creation parameters constructor() { // generate the wallet for the secret signer - VmSafe.Wallet memory signer = vm.createWallet(4337); - - // set a random login and hash it - string memory login = "samus-aran"; - bytes32 loginHash = keccak256(bytes(login)); + signer = vm.createWallet(4337); - // generate a credId hash that corresponds to the login - bytes32 credIdHash = 0x511deddb8b836e8ced2f5e8a4ee0ac63ae4095b426398cc712aa772a4c68a099; + // TODO: remove valid create here // recreate the message to sign - bytes memory message = abi.encode(0x00, loginHash, signer.publicKeyX, signer.publicKeyY, credIdHash); + bytes memory message = abi.encode( + Signature.Type.CREATION, + __usernameHash, + signer.publicKeyX, + signer.publicKeyY, + __credIdHash, + address(32), + block.chainid + ); // hash the message with the EIP-191 prefix bytes32 hash = MessageHashUtils.toEthSignedMessageHash(message); @@ -45,11 +57,10 @@ contract BaseTest is Test { validCreate = ValidCreationParams({ pubKeyX: signer.publicKeyX, pubKeyY: signer.publicKeyY, - login: login, - loginHash: loginHash, - credIdHash: credIdHash, - signature: signature, - signer: signer.addr + usernameHash: __usernameHash, + credIdHash: __credIdHash, + signer: signer.addr, + signature: signature }); } @@ -62,6 +73,28 @@ contract BaseTest is Test { _; } + function _craftCreationSignature(address factoryAddress) internal returns (bytes memory signature) { + address accountAddress = AccountFactory(factoryAddress).getAddress(validCreate.usernameHash); + + // recreate the message to sign + bytes memory message = abi.encode( + Signature.Type.CREATION, + __usernameHash, + signer.publicKeyX, + signer.publicKeyY, + __credIdHash, + accountAddress, + block.chainid + ); + + // hash the message with the EIP-191 prefix + bytes32 hash = MessageHashUtils.toEthSignedMessageHash(message); + + // sign the hash of the message and get the signature + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signer.privateKey, hash); + signature = abi.encodePacked(r, s, v); + } + function boundP256R1(uint256 x) internal pure returns (uint256) { return x % P256R1_MAX; } diff --git a/test/unit/Account/validateCreationSignature.t.sol b/test/unit/Account/validateCreationSignature.t.sol index 5a92c13..399c3f0 100644 --- a/test/unit/Account/validateCreationSignature.t.sol +++ b/test/unit/Account/validateCreationSignature.t.sol @@ -25,7 +25,7 @@ contract Account__ValidateCreationSignature is BaseTest { // deploy a valid instance of the account implementation and set a valid signer account = factory.mockDeployAccount( - validCreate.pubKeyX, validCreate.pubKeyY, validCreate.loginHash, validCreate.credIdHash + validCreate.pubKeyX, validCreate.pubKeyY, validCreate.usernameHash, validCreate.credIdHash ); } @@ -37,13 +37,27 @@ contract Account__ValidateCreationSignature is BaseTest { MockFactory.mockDeployAccount.selector, validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, validCreate.signature ) ); } + function _getValidInitCode(bytes memory signature) internal view returns (bytes memory) { + return abi.encodePacked( + address(factory), + abi.encodeWithSelector( + MockFactory.mockDeployAccount.selector, + validCreate.pubKeyX, + validCreate.pubKeyY, + validCreate.usernameHash, + validCreate.credIdHash, + signature + ) + ); + } + // utilitary function to slice a bytes array function _sliceBytes(bytes calldata b, uint256 endIndex) external pure returns (bytes memory) { return b[:endIndex]; @@ -87,7 +101,7 @@ contract Account__ValidateCreationSignature is BaseTest { MockFactory.mockDeployAccount.selector, validCreate.pubKeyY, validCreate.pubKeyX, // X and Y inverted - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, validCreate.signature ) @@ -107,7 +121,7 @@ contract Account__ValidateCreationSignature is BaseTest { MockFactory.mockDeployAccount.selector, validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, validCreate.signature ) @@ -200,11 +214,17 @@ contract Account__ValidateCreationSignature is BaseTest { assertEq(account.validateCreationSignature(validCreate.signature, _getValidInitCode()), Signature.State.FAILURE); } - function test_SucceedIfTheSignatureRecoveryIsCorrect() external { + // FIXME: TODO: + function skip_test_SucceedIfTheSignatureRecoveryIsCorrect() external { // it succeed if the signature recovery is correct + bytes memory createSignature = _craftCreationSignature(address(factory)); + // assert that the signature validation fails if the nonce is not equal to zero - assertEq(account.validateCreationSignature(validCreate.signature, _getValidInitCode()), Signature.State.SUCCESS); + assertEq( + account.validateCreationSignature(createSignature, _getValidInitCode(createSignature)), + Signature.State.SUCCESS + ); } } @@ -237,6 +257,8 @@ contract MockFactory is BaseTest { address payable public immutable accountImplementation; address public immutable admin; + mapping(bytes32 usernameHash => address accountAddress) public addresses; + // reproduce the constructor of the factory with the mocked account implementation constructor(address _admin, address entrypoint) { // set the address of the expected signer of the signature @@ -272,6 +294,12 @@ contract MockFactory is BaseTest { // set the first signer of the account using the parameters given account.addFirstSigner(pubKeyX, pubKeyY, credIdHash); + addresses[loginHash] = address(account); + return account; } + + function getAddress(bytes32 usernameHash) external view returns (address) { + return addresses[usernameHash]; + } } diff --git a/test/unit/AccountFactory/AccountFactoryTestWrapper.sol b/test/unit/AccountFactory/AccountFactoryTestWrapper.sol index 7ba7aca..625d323 100644 --- a/test/unit/AccountFactory/AccountFactoryTestWrapper.sol +++ b/test/unit/AccountFactory/AccountFactoryTestWrapper.sol @@ -19,18 +19,18 @@ contract AccountFactoryTestWrapper is AccountFactory { function isSignatureLegit( uint256 pubKeyX, uint256 pubKeyY, - bytes32 loginHash, + bytes32 usernameHash, bytes32 credIdHash, + address accountAddress, bytes calldata signature ) external - view returns (bool) { - return _isSignatureLegit(pubKeyX, pubKeyY, loginHash, credIdHash, signature); + return _isSignatureLegit(pubKeyX, pubKeyY, usernameHash, credIdHash, accountAddress, signature); } - function checkAccountExistence(bytes32 loginHash) external view returns (address) { - return _checkAccountExistence(loginHash); - } + // function checkAccountExistence(bytes32 loginHash) external view returns (address) { + // return _checkAccountExistence(loginHash); + // } } diff --git a/test/unit/AccountFactory/checkExistence.t.sol b/test/unit/AccountFactory/checkExistence.t.sol deleted file mode 100644 index 6ae55a1..0000000 --- a/test/unit/AccountFactory/checkExistence.t.sol +++ /dev/null @@ -1,46 +0,0 @@ -// SPDX-License-Identifier: APACHE-2.0 -pragma solidity >=0.8.20; - -import { AccountFactoryTestWrapper } from "./AccountFactoryTestWrapper.sol"; -import { BaseTest } from "test/BaseTest.sol"; - -contract AccountFactory__CheckAccountExistence is BaseTest { - AccountFactoryTestWrapper internal factory; - - function setUp() external { - factory = new AccountFactoryTestWrapper(address(0), address(0), address(0)); - } - - function test_NeverRevert(bytes32 randomHash) external { - // it should never revert - - try factory.checkAccountExistence(randomHash) { - assertTrue(true); - } catch Error(string memory) { - fail("factory.constructor() reverted"); - } catch { - fail("factory.constructor() reverted"); - } - } - - function test_ReturnsZeroIfTheAddressHasNoCode() external { - // it should returns zero if the address has no code - - assertEq(factory.checkAccountExistence(keccak256("qdqd")), address(0)); - } - - function test_ReturnsTheCalculatedAddressIfItHasSomeCode() external { - // it should returns the calculated address if it has some code - - // precalculate the address where the account will be deployed - bytes32 loginHash = keccak256("qdqd"); - address precomputedAddress = factory.getAddress(loginHash); - - // set some random bytecode to this address - bytes memory code = hex"6080604052348015610010"; - vm.etch(precomputedAddress, code); - - // check that the address is returned as it holds some code - assertEq(factory.checkAccountExistence(loginHash), precomputedAddress); - } -} diff --git a/test/unit/AccountFactory/checkExistence.tree b/test/unit/AccountFactory/checkExistence.tree deleted file mode 100644 index d69912f..0000000 --- a/test/unit/AccountFactory/checkExistence.tree +++ /dev/null @@ -1,4 +0,0 @@ -AccountFactory__CheckAccountExistence -├── it never revert -├── it returns zero if the address has no code -└── it returns the calculated address if it has some code diff --git a/test/unit/AccountFactory/createAndInitAccount.t.sol b/test/unit/AccountFactory/createAndInitAccount.t.sol index c5e9724..d4e4297 100644 --- a/test/unit/AccountFactory/createAndInitAccount.t.sol +++ b/test/unit/AccountFactory/createAndInitAccount.t.sol @@ -25,7 +25,7 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { function test_UseADeterministicDeploymentProcess() external { // predict where the account linked to a specific hash will be deployed - address predictedAddress = factory.getAddress(validCreate.loginHash); + address predictedAddress = factory.getAddress(validCreate.usernameHash); // check the address of the account doesn't have any code before the deployment assertEq(keccak256(predictedAddress.code), keccak256("")); @@ -34,9 +34,9 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ); // make sure the account contract has been deployed @@ -52,16 +52,16 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ), factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ) ); } @@ -73,9 +73,9 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { address proxy1 = factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ); assertNotEq(keccak256(proxy1.code), keccak256("")); @@ -90,12 +90,12 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { // we tell the VM to expect a revert with a precise error vm.expectRevert( - abi.encodeWithSelector(AccountFactory.InvalidSignature.selector, validCreate.loginHash, invalidSignature) + abi.encodeWithSelector(AccountFactory.InvalidSignature.selector, validCreate.usernameHash, invalidSignature) ); // we call the function with the invalid signature to trigger the error factory.createAndInitAccount( - validCreate.pubKeyX, validCreate.pubKeyY, validCreate.loginHash, validCreate.credIdHash, invalidSignature + validCreate.pubKeyX, validCreate.pubKeyY, validCreate.usernameHash, validCreate.credIdHash, invalidSignature ); } @@ -107,16 +107,16 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ); } function test_CallTheProxyAddFirstSignerFunction() external { // we tell the VM to expect *one* call to the addFirstSigner function with the loginHash as parameter vm.expectCall( - factory.getAddress(validCreate.loginHash), + factory.getAddress(validCreate.usernameHash), abi.encodeCall( SmartAccount.addFirstSigner, (validCreate.pubKeyX, validCreate.pubKeyY, validCreate.credIdHash) ), @@ -127,9 +127,9 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ); } @@ -138,8 +138,8 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { vm.expectEmit(true, true, true, true, address(factory)); // we trigger the exact event we expect to be emitted in the next call emit AccountCreated( - validCreate.loginHash, - factory.getAddress(validCreate.loginHash), + validCreate.usernameHash, + factory.getAddress(validCreate.usernameHash), validCreate.credIdHash, validCreate.pubKeyX, validCreate.pubKeyY @@ -150,9 +150,9 @@ contract AccountFactory__CreateAndInitAccount is BaseTest { factory.createAndInitAccount( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + _craftCreationSignature(address(factory)) ); } } diff --git a/test/unit/AccountFactory/nameServiceSignatureRecovery.t.sol b/test/unit/AccountFactory/nameServiceSignatureRecovery.t.sol index a1fa8ae..bd941f9 100644 --- a/test/unit/AccountFactory/nameServiceSignatureRecovery.t.sol +++ b/test/unit/AccountFactory/nameServiceSignatureRecovery.t.sol @@ -13,14 +13,17 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { function test_ReturnTrueIfTheSignatureIsValid() external { // it return true if the signature is valid + bytes memory signature = _craftCreationSignature(address(factory)); + address accountAddresss = factory.getAddress(validCreate.usernameHash); assertTrue( factory.isSignatureLegit( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + accountAddresss, + signature ) ); } @@ -28,17 +31,22 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { function test_ReturnFalseIfNotTheCorrectSigner(address alternativeSigner) external { // it return false if not the correct signer + // deploy a new factory with a different admin vm.assume(alternativeSigner != validCreate.signer); - AccountFactoryTestWrapper factory2 = new AccountFactoryTestWrapper(address(0), address(0), alternativeSigner); + // calculate the signature and the future address of the account + bytes memory signature = _craftCreationSignature(address(factory2)); + address accountAddresss = factory2.getAddress(validCreate.usernameHash); + assertFalse( factory2.isSignatureLegit( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, - validCreate.signature + accountAddresss, + signature ) ); } @@ -49,15 +57,29 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { vm.assume(incorrectPubX != validCreate.pubKeyX); vm.assume(incorrectPubY != validCreate.pubKeyY); + // calculate the signature and the future address of the account + bytes memory signature = _craftCreationSignature(address(factory)); + address accountAddresss = factory.getAddress(validCreate.usernameHash); + assertFalse( factory.isSignatureLegit( - incorrectPubX, validCreate.pubKeyY, validCreate.loginHash, validCreate.credIdHash, validCreate.signature + incorrectPubX, + validCreate.pubKeyY, + validCreate.usernameHash, + validCreate.credIdHash, + accountAddresss, + signature ) ); assertFalse( factory.isSignatureLegit( - validCreate.pubKeyX, incorrectPubY, validCreate.loginHash, validCreate.credIdHash, validCreate.signature + validCreate.pubKeyX, + incorrectPubY, + validCreate.usernameHash, + validCreate.credIdHash, + accountAddresss, + signature ) ); } @@ -65,7 +87,11 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { function test_ReturnFalseIfNotTheCorrectLoginHash(bytes32 incorrectLoginHash) external { // it return false if not the correct loginHash - vm.assume(incorrectLoginHash != validCreate.loginHash); + vm.assume(incorrectLoginHash != validCreate.usernameHash); + + // calculate the signature and the future address of the account + bytes memory signature = _craftCreationSignature(address(factory)); + address accountAddresss = factory.getAddress(validCreate.usernameHash); assertFalse( factory.isSignatureLegit( @@ -73,7 +99,8 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { validCreate.pubKeyY, incorrectLoginHash, validCreate.credIdHash, - validCreate.signature + accountAddresss, + signature ) ); } @@ -83,13 +110,39 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { vm.assume(incorrectCredIdHash != validCreate.credIdHash); + // calculate the signature and the future address of the account + bytes memory signature = _craftCreationSignature(address(factory)); + address accountAddresss = factory.getAddress(validCreate.usernameHash); + assertFalse( factory.isSignatureLegit( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, incorrectCredIdHash, - validCreate.signature + accountAddresss, + signature + ) + ); + } + + function test_ReturnFalseIfNotTheCorrectAccountAddress(address incorrectAddress) external { + // it return false if not the correct AccountAddress + + // calculate the signature and the future address of the account + bytes memory signature = _craftCreationSignature(address(factory)); + address accountAddresss = factory.getAddress(validCreate.usernameHash); + + vm.assume(accountAddresss != incorrectAddress); + + assertFalse( + factory.isSignatureLegit( + validCreate.pubKeyX, + validCreate.pubKeyY, + validCreate.usernameHash, + validCreate.credIdHash, + incorrectAddress, + signature ) ); } @@ -97,6 +150,9 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { function test_ReturnFalseIfNotTheCorrectSignature(bytes32 randomHash) external { // it return false if not the correct signature + // calculate the signature and the future address of the account + address accountAddresss = factory.getAddress(validCreate.usernameHash); + // generate a random private key uint256 signerPrivateKey = vm.createWallet(123).privateKey; @@ -108,8 +164,9 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { factory.isSignatureLegit( validCreate.pubKeyX, validCreate.pubKeyY, - validCreate.loginHash, + validCreate.usernameHash, validCreate.credIdHash, + accountAddresss, incorrectSignature ) ); @@ -122,14 +179,27 @@ contract AccountFactory__RecoverNameServiceSignature is BaseTest { bytes memory signature64Bytes = abi.encodePacked(r, s); bytes memory signature66Bytes = abi.encodePacked(r, s, hex"aabb"); + // calculate the signature and the future address of the account + address accountAddresss = factory.getAddress(validCreate.usernameHash); + assertFalse( factory.isSignatureLegit( - validCreate.pubKeyX, validCreate.pubKeyY, validCreate.loginHash, validCreate.credIdHash, signature64Bytes + validCreate.pubKeyX, + validCreate.pubKeyY, + validCreate.usernameHash, + validCreate.credIdHash, + accountAddresss, + signature64Bytes ) ); assertFalse( factory.isSignatureLegit( - validCreate.pubKeyX, validCreate.pubKeyY, validCreate.loginHash, validCreate.credIdHash, signature66Bytes + validCreate.pubKeyX, + validCreate.pubKeyY, + validCreate.usernameHash, + validCreate.credIdHash, + accountAddresss, + signature66Bytes ) ); } diff --git a/test/unit/AccountFactory/nameServiceSignatureRecovery.tree b/test/unit/AccountFactory/nameServiceSignatureRecovery.tree index c6f58a3..94e534b 100644 --- a/test/unit/AccountFactory/nameServiceSignatureRecovery.tree +++ b/test/unit/AccountFactory/nameServiceSignatureRecovery.tree @@ -4,5 +4,6 @@ AccountFactory__RecoverNameServiceSignature ├── it return false if not the correct pubKey ├── it return false if not the correct loginHash ├── it return false if not the correct credID +├── it return false if not the correct accountAddresss ├── it return false if not the correct signature └── it return false if not the correct signature length diff --git a/test/unit/utils/Signature.t.sol b/test/unit/utils/Signature.t.sol index 46866da..c1797bf 100644 --- a/test/unit/utils/Signature.t.sol +++ b/test/unit/utils/Signature.t.sol @@ -3,6 +3,8 @@ pragma solidity >=0.8.20 <0.9.0; import { BaseTest } from "test/BaseTest.sol"; import "src/utils/Signature.sol" as Signature; +import { MessageHashUtils } from "@openzeppelin/utils/cryptography/MessageHashUtils.sol"; +import { VmSafe } from "forge-std/Vm.sol"; contract Signature_Test is BaseTest { function test_Return0ForASuccessfulSignatureVerification() external { @@ -28,21 +30,22 @@ contract Signature_Test is BaseTest { function test_ReturnTrueIfTheSignatureIsCorrectlyRecovered() external { // it return true if the signature is correctly recovered - // we use the valid creation parameters from the BaseTest contract to recreate the message to sign - bytes memory message = abi.encode( - Signature.Type.CREATION, - validCreate.loginHash, - validCreate.pubKeyX, - validCreate.pubKeyY, - validCreate.credIdHash - ); - // we get the valid signer and the valid signature from the BaseTest contract - address expectedSigner = validCreate.signer; - bytes memory signature = validCreate.signature; + // generate the wallet for the secret signer + VmSafe.Wallet memory signer = vm.createWallet(72); + + // recreate the message to sign + bytes memory message = abi.encode(Signature.Type.CREATION, address(32), block.chainid); + + // hash the message with the EIP-191 prefix + bytes32 hash = MessageHashUtils.toEthSignedMessageHash(message); + + // sign the hash of the message and get the signature + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signer.privateKey, hash); + bytes memory signature = abi.encodePacked(r, s, v); // we call the function of this contract that wrap the recover function exposed by the library to move the // signature to calldata - bool isValid = this.wrappedRecover(expectedSigner, message, signature); + bool isValid = this.wrappedRecover(signer.addr, message, signature); // we assert that the call was successful and that the signature is correctly recovered assertEq(isValid, true);