Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Includes CallData in Deployment Signature #95

Merged
merged 2 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions script/AccountFactory/02_FactoryCreateAndInitAccount.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,35 @@ import { BaseScript } from "../Base.s.sol";
/// @notice Create and init an account using an already deployed factory
/// @dev The signature must be signed by the admin of the factory
contract FactoryCreateAndInitAccount is BaseScript {
/// @notice Deploy an account and init it
/// @return accountAddress The address of the deployed account
function run() public broadcast returns (address accountAddress) {
function run() public returns (address accountAddress) {
address factoryAddress = vm.envAddress("FACTORY");
AccountFactory factory = AccountFactory(factoryAddress);

// arguments to pass to the `createAndInit` function
bytes memory authData = vm.envBytes("AUTH_DATA");
bytes memory signature = vm.envBytes("SIGNATURE");
bytes32 callDataHash = vm.envBytes32("CALLDATA_HASH");

return run(factoryAddress, authData, signature, callDataHash);
}

/// @notice Deploy an account and init it
/// @return accountAddress The address of the deployed account
function run(
address factoryAddress,
bytes memory authData,
bytes memory signature,
bytes32 callDataHash
)
internal
broadcast
returns (address accountAddress)
{
AccountFactory factory = AccountFactory(factoryAddress);

// check the account is not already deployed
accountAddress = factory.getAddress(authData);
require(accountAddress.code.length == 0, "Account already exists");

// deploy and init the account
address deployedAddress = factory.createAndInitAccount(authData, signature);
address deployedAddress = factory.createAndInitAccount(authData, signature, callDataHash);

// ensure the account has been deployed at the correct address
require(deployedAddress == accountAddress, "Invalid account address");
Expand Down
17 changes: 11 additions & 6 deletions src/v1/Account/SmartAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { Initializable } from "@openzeppelin/proxy/utils/Initializable.sol";
import { UUPSUpgradeable } from "@openzeppelin/proxy/utils/UUPSUpgradeable.sol";
import { IWebAuthn256r1 } from "@webauthn/IWebAuthn256r1.sol";
import { UV_FLAG_MASK } from "@webauthn/utils.sol";

Check warning on line 10 in src/v1/Account/SmartAccount.sol

View workflow job for this annotation

GitHub Actions / lint

imported name UV_FLAG_MASK is not used

Check warning on line 10 in src/v1/Account/SmartAccount.sol

View workflow job for this annotation

GitHub Actions / lint

imported name UV_FLAG_MASK is not used
import { SignerVaultWebAuthnP256R1 } from "src/utils/SignerVaultWebAuthnP256R1.sol";
import { AccountFactory } from "src/v1/AccountFactory.sol";
import "src/utils/Signature.sol" as Signature;
Expand Down Expand Up @@ -150,11 +150,13 @@
/// deployment. The use of the account factory is gated by this signature.
/// @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
/// @param callData The callData field presents in the userOp.
/// @return 0 if the signature is valid, 1 otherwise
function _validateCreationSignature(
uint256 nonce,
bytes calldata signature,
bytes calldata initCode
bytes calldata initCode,
bytes calldata callData,
bytes calldata signature
)
internal
virtual
Expand All @@ -169,22 +171,25 @@
if (accountFactory != storedFactoryAddress) return Signature.State.FAILURE;

// 3. decode the rest of the initCode (skip the first 4 bytes -- function selector)
(bytes memory authenticatorData,) = abi.decode(initCode[24:], (bytes, bytes));
(bytes memory authenticatorData,,) = abi.decode(initCode[24:], (bytes, bytes, bytes32));

// 4. extract the signer from the authenticatorData
// TODO: once tested, rework this shit by using a more efficient way
(, bytes32 credIdHash, uint256 pubX, uint256 pubY) =
SmartAccount(payable(address(this))).extractSignerFromAuthData(authenticatorData);

// 5. recreate the message and try to recover the signer
bytes memory message = abi.encode(Signature.Type.CREATION, authenticatorData, address(this), block.chainid);
bytes memory message =
abi.encode(Signature.Type.CREATION, authenticatorData, address(this), block.chainid, keccak256(callData));

// 6. fetch the expected signer from the factory contract
address expectedSigner = AccountFactory(storedFactoryAddress).owner();

// 7. Check the signature is valid and revert if it is not
// NOTE: The signature prefix, added manually to identify the signature, is removed before the recovery process
if (Signature.recover(expectedSigner, message, signature[1:]) == false) return Signature.State.FAILURE;
if (Signature.recover(expectedSigner, message, signature[1:]) == false) {
return Signature.State.FAILURE;
}

// 8. Ensure the signer is allowed. This is the signer added by the factory during the deployment process.
// solhint-disable-next-line var-name-mixedcase
Expand Down Expand Up @@ -249,7 +254,7 @@

// 1.b check the signature is a "creation" signature (length is checked by the signature library)
if (signatureType == Signature.Type.CREATION) {
return _validateCreationSignature(userOp.nonce, userOp.signature, userOp.initCode);
return _validateCreationSignature(userOp.nonce, userOp.initCode, userOp.callData, userOp.signature);
}

return Signature.State.FAILURE;
Expand Down Expand Up @@ -372,7 +377,7 @@
}

/// @notice authorize account upgrade to a new implementation if the caller is the account itself
function _authorizeUpgrade(address) internal virtual override onlySelf { }

Check warning on line 380 in src/v1/Account/SmartAccount.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks

Check warning on line 380 in src/v1/Account/SmartAccount.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks

// ======================================
// === EXTERNAL ENTRYPOINT FUNCTIONS ====
Expand Down
15 changes: 10 additions & 5 deletions src/v1/AccountFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract AccountFactory {

event AccountCreated(address account, bytes authenticatorData);

error InvalidSignature(address accountAddress, bytes authenticatorData, bytes signature);
error InvalidSignature(address accountAddress, bytes32 callDataHash, bytes authenticatorData, bytes signature);
error InvalidAccountImplementation();
error InvalidSigner();

Expand Down Expand Up @@ -62,12 +62,14 @@ contract AccountFactory {

/// @notice This function checks if the signature is signed by the operator
/// @param accountAddress The address of the account that would be deployed
/// @param callDataHash The hash of the calldata of the transaction that would be sent to the account
/// @param authenticatorData The authenticatorData field of the WebAuthn response when creating a signer
/// @param signature Signature made off-chain by made the operator of the factory. It gates the use of the factory.
/// @return True if the signature is legit, false otherwise
/// @dev Incorrect signatures are expected to lead to a revert by the library used
function _isSignatureLegit(
address accountAddress,
bytes32 callDataHash,
bytes calldata authenticatorData,
bytes calldata signature
)
Expand All @@ -77,7 +79,9 @@ contract AccountFactory {
returns (bool)
{
// 1. Recreate the message signed by the operator
bytes memory message = abi.encode(Signature.Type.CREATION, authenticatorData, accountAddress, block.chainid);
// NOTE: msg.sender is assumed to be the expected entrypoint
bytes memory message =
abi.encode(Signature.Type.CREATION, authenticatorData, accountAddress, block.chainid, callDataHash);

// 2. Try to recover the address and return if the signature is legit
return Signature.recover(operator, message, signature[1:]);
Expand Down Expand Up @@ -168,7 +172,8 @@ contract AccountFactory {
/// @return The address of the existing account (either deployed by this function or not)
function createAndInitAccount(
bytes calldata authenticatorData,
bytes calldata signature
bytes calldata signature,
bytes32 callDataHash
)
external
virtual
Expand All @@ -185,8 +190,8 @@ contract AccountFactory {
if (accountAddress.code.length > 0) return accountAddress;

// 4. check if the signature is valid
if (_isSignatureLegit(accountAddress, authenticatorData, signature) == false) {
revert InvalidSignature(accountAddress, authenticatorData, signature);
if (_isSignatureLegit(accountAddress, callDataHash, authenticatorData, signature) == false) {
revert InvalidSignature(accountAddress, callDataHash, authenticatorData, signature);
}

// 5. deploy the proxy for the user. During the deployment, the initialize function in the implementation
Expand Down
29 changes: 26 additions & 3 deletions test/BaseTest/BaseTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,41 @@ contract BaseTest is Test, BaseTestUtils, BaseTestCreateFixtures {
/// This utility function is used to craft a signature for the deployment of an account.
/// This is for testing purposes only.
/// @param authenticatorData The authenticator data returned by the authenticator on the signer creation
/// @param account The address of the account that will be deployed
/// @param accountAddress The address of the account that will be deployed
/// @return signature The signature to be used for the deployment
function craftDeploymentSignature(
bytes memory authenticatorData,
address account
address accountAddress
)
internal
view
returns (bytes memory signature)
{
return craftDeploymentSignature(authenticatorData, accountAddress, createFixtures.transaction.calldataHash);
}

/// @notice Utility function to craft a deployment signature
/// @dev In production, the deployment of an account using our factory is gated by an approval from us.
/// The factory will check if smoo.th approved the deployment by verifying a signature
/// we create using an approved signer (the operator and also the owner of the factory).
/// This utility function is used to craft a signature for the deployment of an account.
/// This is for testing purposes only.
/// @param authenticatorData The authenticator data returned by the authenticator on the signer creation
/// @param accountAddress The address of the account that will be deployed
/// @param calldataHash The hash of the calldata that will be executed after the deployment
/// @return signature The signature to be used for the deployment
function craftDeploymentSignature(
bytes memory authenticatorData,
address accountAddress,
bytes32 calldataHash
)
internal
view
returns (bytes memory signature)
{
// recreate the message to sign
bytes memory message = abi.encode(Signature.Type.CREATION, authenticatorData, account, block.chainid);
bytes memory message =
abi.encode(Signature.Type.CREATION, authenticatorData, accountAddress, block.chainid, calldataHash);

// hash the message with the EIP-191 prefix
bytes32 hash = MessageHashUtils.toEthSignedMessageHash(message);
Expand Down
18 changes: 17 additions & 1 deletion test/BaseTest/BaseTestCreateFixtures.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,18 @@ struct FixturesResponse {
bytes authData;
}

struct FixturesTransaction {
bytes callData;
bytes32 calldataHash;
}

struct CreateFixtures {
uint256 id;
FixturesUser user;
FixturesSignature signature;
FixturesSigner signer;
FixturesResponse response;
FixturesTransaction transaction;
}

/// @title BaseTestCreateFixtures
Expand Down Expand Up @@ -90,6 +96,7 @@ contract BaseTestCreateFixtures is Test {
FixturesSignature memory signature;
FixturesSigner memory signer;
FixturesResponse memory response;
FixturesTransaction memory transaction;

// load and store the user data
{
Expand Down Expand Up @@ -137,14 +144,23 @@ contract BaseTestCreateFixtures is Test {
});
}

// craft the dummy alldata passed during the deployment
{
bytes memory callData =
abi.encodeWithSignature("transfer(address,address,uint256)", address(234), address(500), 1);
bytes32 calldataHash = keccak256(callData);
transaction = FixturesTransaction({ callData: callData, calldataHash: calldataHash });
}

// create the fixture and store it in the storage
// forgefmt: disable-next-item
createFixtures = CreateFixtures({
id: id,
user: user,
signature: signature,
signer: signer,
response: response
response: response,
transaction: transaction
});
}
}
6 changes: 5 additions & 1 deletion test/unit/v1/Account/SmartAccountERC1271/EIP191.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { WebAuthn256r1Wrapper } from "script/WebAuthn256r1/WebAuthn256r1Wrapper.sol";
import { SmartAccount } from "src/v1/Account/SmartAccount.sol";
import { AccountFactory } from "src/v1/AccountFactory.sol";
import { EIP1271_VALIDATION_SUCCESS, EIP1271_VALIDATION_FAILURE } from "src/v1/Account/SmartAccountEIP1271.sol";

Check warning on line 8 in test/unit/v1/Account/SmartAccountERC1271/EIP191.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_SUCCESS is not used

Check warning on line 8 in test/unit/v1/Account/SmartAccountERC1271/EIP191.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_FAILURE is not used

Check warning on line 8 in test/unit/v1/Account/SmartAccountERC1271/EIP191.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_SUCCESS is not used

Check warning on line 8 in test/unit/v1/Account/SmartAccountERC1271/EIP191.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_FAILURE is not used
import { BaseTest } from "test/BaseTest/BaseTest.sol";
import { SignerVaultWebAuthnP256R1 } from "src/utils/SignerVaultWebAuthnP256R1.sol";

Expand Down Expand Up @@ -69,7 +69,11 @@

// 8. deploy the proxy that targets the implementation and set the first signer using the creationAuthData
bytes memory signature = craftDeploymentSignature(data.creationAuthData, accountFutureAddress);
account = SmartAccount(payable(factory.createAndInitAccount(data.creationAuthData, signature)));
account = SmartAccount(
payable(
factory.createAndInitAccount(data.creationAuthData, signature, createFixtures.transaction.calldataHash)
)
);
}

function test_CanValidateEIP191Signature() external {
Expand Down
6 changes: 5 additions & 1 deletion test/unit/v1/Account/SmartAccountERC1271/EIP712.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { WebAuthn256r1Wrapper } from "script/WebAuthn256r1/WebAuthn256r1Wrapper.sol";
import { SmartAccount } from "src/v1/Account/SmartAccount.sol";
import { AccountFactory } from "src/v1/AccountFactory.sol";
import { EIP1271_VALIDATION_SUCCESS, EIP1271_VALIDATION_FAILURE } from "src/v1/Account/SmartAccountEIP1271.sol";

Check warning on line 7 in test/unit/v1/Account/SmartAccountERC1271/EIP712.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_SUCCESS is not used

Check warning on line 7 in test/unit/v1/Account/SmartAccountERC1271/EIP712.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_FAILURE is not used

Check warning on line 7 in test/unit/v1/Account/SmartAccountERC1271/EIP712.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_SUCCESS is not used

Check warning on line 7 in test/unit/v1/Account/SmartAccountERC1271/EIP712.t.sol

View workflow job for this annotation

GitHub Actions / lint

imported name EIP1271_VALIDATION_FAILURE is not used
import { BaseTest } from "test/BaseTest/BaseTest.sol";
import { SignerVaultWebAuthnP256R1 } from "src/utils/SignerVaultWebAuthnP256R1.sol";

Expand Down Expand Up @@ -71,7 +71,11 @@

// 8. deploy the proxy that targets the implementation and set the first signer using the creationAuthData
bytes memory signature = craftDeploymentSignature(data.creationAuthData, accountFutureAddress);
account = SmartAccount(payable(factory.createAndInitAccount(data.creationAuthData, signature)));
account = SmartAccount(
payable(
factory.createAndInitAccount(data.creationAuthData, signature, createFixtures.transaction.calldataHash)
)
);
}

function _getDomainSeparator() internal pure returns (bytes32) {
Expand Down
8 changes: 7 additions & 1 deletion test/unit/v1/Account/addWebAuthnP256R1Signer.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ contract SmartAccount__AddWebAuthnP256R1Signer is BaseTest {

// 5. deploy the proxy that targets the implementation and set the first signer
bytes memory signature = craftDeploymentSignature(createFixtures.response.authData, accountFutureAddress);
account = SmartAccount(payable(factory.createAndInitAccount(createFixtures.response.authData, signature)));
account = SmartAccount(
payable(
factory.createAndInitAccount(
createFixtures.response.authData, signature, createFixtures.transaction.calldataHash
)
)
);
}

function test_RevertsIfTheSignerAlreadyExists() external {
Expand Down
8 changes: 7 additions & 1 deletion test/unit/v1/Account/getSigner.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ contract SmartAccount__GetSigner is BaseTest {
bytes memory signature = craftDeploymentSignature(createFixtures.response.authData, accountFutureAddress);

// 3. deploy the proxy that targets the implementation and set the first signer
return SmartAccount(payable(factory.createAndInitAccount(createFixtures.response.authData, signature)));
return SmartAccount(
payable(
factory.createAndInitAccount(
createFixtures.response.authData, signature, createFixtures.transaction.calldataHash
)
)
);
}

function test_ReturnsTheStoredSignerWhenPassingCredId() external {
Expand Down
8 changes: 7 additions & 1 deletion test/unit/v1/Account/removeWebAuthnP256R1Signer.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,13 @@ contract SmartAccount__RemoveWebAuthnP256R1Signer is BaseTest {

// 5. deploy the proxy that targets the implementation and set the first signer
bytes memory signature = craftDeploymentSignature(createFixtures.response.authData, accountFutureAddress);
account = SmartAccount(payable(factory.createAndInitAccount(createFixtures.response.authData, signature)));
account = SmartAccount(
payable(
factory.createAndInitAccount(
createFixtures.response.authData, signature, createFixtures.transaction.calldataHash
)
)
);
}

function test_CanOnlyBeCalledByItself(address caller) external {
Expand Down
8 changes: 7 additions & 1 deletion test/unit/v1/Account/upgradeable.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@
bytes memory signature = craftDeploymentSignature(createFixtures.response.authData, accountFutureAddress);

// 6. deploy an account instance and set the first signer
account = SmartAccount(payable(factory.createAndInitAccount(createFixtures.response.authData, signature)));
account = SmartAccount(
payable(
factory.createAndInitAccount(
createFixtures.response.authData, signature, createFixtures.transaction.calldataHash
)
)
);
}

function test_CanBeUpgradedWithoutData() external {
Expand Down Expand Up @@ -276,7 +282,7 @@
constructor(address _entrypoint, address _verifier) SmartAccount(_entrypoint, _verifier) { }

// expected to work as expected as the version is higher than the curret one
function correctInitialize(address) external reinitializer(2) { }

Check warning on line 285 in test/unit/v1/Account/upgradeable.t.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks

Check warning on line 285 in test/unit/v1/Account/upgradeable.t.sol

View workflow job for this annotation

GitHub Actions / lint

Code contains empty blocks

// expected to revert as the version is equal to the current one
function incorrectInitialize() external reinitializer(1) { }
Expand Down
Loading
Loading