Skip to content

Add EIP-712 support for Account v2 #21

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: jw/account-v2
Choose a base branch
from
Open

Add EIP-712 support for Account v2 #21

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0aad6b0
Add support for EIP-712
0xyanc May 19, 2023
e037035
format
0xyanc May 19, 2023
1c3bfe6
format
0xyanc May 19, 2023
5718d93
format tests
0xyanc May 19, 2023
3ef7af2
Update src/Account.sol
0xyanc May 19, 2023
fb86fab
Change EIP-712 name and version fields as constants
0xyanc May 19, 2023
600be8f
Merge branch 'account-v2-eip712' of https://github.com/0xyanc/contrac…
0xyanc May 19, 2023
7d856f7
Add userOpHash in the typed data fields
0xyanc May 19, 2023
7de324f
Expose function to build UserOp hash compliant with EIP-712
0xyanc May 19, 2023
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
4 changes: 3 additions & 1 deletion script/DeployAccount.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ contract DeployAccount is Script {
salt: 0x6551655165516551655165516551655165516551655165516551655165516551
}(
0xB0219b60f0535FB3B62eeEC51EC4C765d138Ac0A, // guardian
0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 // entry point
0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, // entry point
"ERC6551-Account",
"1"
);

new AccountProxy{
Expand Down
35 changes: 28 additions & 7 deletions src/Account.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import "openzeppelin-contracts/token/ERC1155/IERC1155Receiver.sol";
import "openzeppelin-contracts/interfaces/IERC1271.sol";
import "openzeppelin-contracts/utils/cryptography/SignatureChecker.sol";
import "openzeppelin-contracts/proxy/utils/UUPSUpgradeable.sol";
import "openzeppelin-contracts/utils/cryptography/EIP712.sol";

import {BaseAccount as BaseERC4337Account, IEntryPoint, UserOperation} from "account-abstraction/core/BaseAccount.sol";

Expand All @@ -34,7 +35,8 @@ contract Account is
IERC721Receiver,
IERC1155Receiver,
UUPSUpgradeable,
BaseERC4337Account
BaseERC4337Account,
EIP712
{
using ECDSA for bytes32;

Expand Down Expand Up @@ -81,9 +83,12 @@ contract Account is
_;
}

constructor(address _guardian, address entryPoint_) {
if (_guardian == address(0) || entryPoint_ == address(0))
constructor(address _guardian, address entryPoint_, string memory _name, string memory _version)
EIP712(_name, _version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the name and version fields constant instead of passed as constructor arguments? Makes it easier to reason about when deploying :)

{
if (_guardian == address(0) || entryPoint_ == address(0)) {
revert InvalidInput();
}

_entryPoint = entryPoint_;
guardian = _guardian;
Expand Down Expand Up @@ -352,10 +357,26 @@ contract Account is
UserOperation calldata userOp,
bytes32 userOpHash
) internal view override returns (uint256 validationData) {
bool isValid = this.isValidSignature(
userOpHash.toEthSignedMessageHash(),
userOp.signature
) == IERC1271.isValidSignature.selector;
bytes32 hashStruct = keccak256(
abi.encode(
keccak256(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the hash of this typed data a public constant?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an immutable variable so the hash is not computed each time

"UserOp(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData)"
),
userOp.sender,
userOp.nonce,
userOp.initCode,
userOp.callData,
userOp.callGasLimit,
userOp.verificationGasLimit,
userOp.preVerificationGas,
userOp.maxFeePerGas,
userOp.maxPriorityFeePerGas,
userOp.paymasterAndData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include userOpHash in the typed data fields?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This userOpHash would be the hash returned by the EntryPoint?

Copy link
Contributor

@jaydenwindle jaydenwindle May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Also passed into the _validateSignature function as a parameter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to copy the UserOperation in memory as I get "Stack too deep" error otherwise

)
);

bool isValid =
this.isValidSignature(_hashTypedDataV4(hashStruct), userOp.signature) == IERC1271.isValidSignature.selector;

if (isValid) {
return 0;
Expand Down
8 changes: 5 additions & 3 deletions test/Account.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract AccountTest is Test {
function setUp() public {
entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(address(guardian), address(entryPoint), "ERC6551-Account", "1");
proxy = new AccountProxy(address(implementation));

registry = new ERC6551Registry();
Expand Down Expand Up @@ -543,7 +543,9 @@ contract AccountTest is Test {

MockAccount upgradedImplementation = new MockAccount(
address(guardian),
address(entryPoint)
address(entryPoint),
"ERC6551-Account",
"1"
);

vm.expectRevert(UntrustedImplementation.selector);
Expand All @@ -562,4 +564,4 @@ contract AccountTest is Test {

assertEq(returnValue, 12345);
}
}
}
7 changes: 6 additions & 1 deletion test/AccountERC1155.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,12 @@ contract AccountERC1155Test is Test {

entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(
address(guardian),
address(entryPoint),
"ERC6551-Account",
"1"
);
registry = new ERC6551Registry();

tokenCollection = new MockERC721();
Expand Down
7 changes: 6 additions & 1 deletion test/AccountERC20.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ contract AccountERC20Test is Test {

entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(
address(guardian),
address(entryPoint),
"ERC6551-Account",
"1"
);
registry = new ERC6551Registry();

tokenCollection = new MockERC721();
Expand Down
64 changes: 48 additions & 16 deletions test/AccountERC4337.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ contract AccountERC4337Test is Test {
function setUp() public {
entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(
address(guardian),
address(entryPoint),
"ERC6551-Account",
"1"
);

registry = new ERC6551Registry();

tokenCollection = new MockERC721();
Expand Down Expand Up @@ -135,12 +141,9 @@ contract AccountERC4337Test is Test {
paymasterAndData: "",
signature: ""
});
bytes32 opHash = _buildOpHash(op, accountAddress);

bytes32 opHash = entryPoint.getUserOpHash(op);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(
1,
opHash.toEthSignedMessageHash()
);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash);

bytes memory signature = abi.encodePacked(r, s, v);
op.signature = signature;
Expand Down Expand Up @@ -196,11 +199,8 @@ contract AccountERC4337Test is Test {
signature: ""
});

bytes32 opHash = entryPoint.getUserOpHash(op);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(
1,
opHash.toEthSignedMessageHash()
);
bytes32 opHash = _buildOpHash(op, accountAddress);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash);

bytes memory signature = abi.encodePacked(r, s, v);
op.signature = signature;
Expand Down Expand Up @@ -256,11 +256,8 @@ contract AccountERC4337Test is Test {
signature: ""
});

bytes32 opHash = entryPoint.getUserOpHash(op);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(
1,
opHash.toEthSignedMessageHash()
);
bytes32 opHash = _buildOpHash(op, accountAddress);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash);

// invalidate signature
bytes memory signature = abi.encodePacked(r, s, v + 1);
Expand All @@ -276,4 +273,39 @@ contract AccountERC4337Test is Test {

assertEq(accountAddress.balance, 1 ether);
}

function _buildOpHash(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expose this as a public getter on the account contract? So that it can be accessed easily by clients.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposed buildUserOp712Hash(UserOperation memory op, bytes32 opHash) in Account.sol

UserOperation memory op,
address accountAddress
) private view returns (bytes32 opHash) {
bytes32 domainSeparator = keccak256(
abi.encode(
keccak256(
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
),
keccak256(bytes("ERC6551-Account")),
keccak256(bytes("1")),
block.chainid,
address(accountAddress)
)
);
bytes32 hashStruct = keccak256(
abi.encode(
keccak256(
"UserOp(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData)"
),
op.sender,
op.nonce,
op.initCode,
op.callData,
op.callGasLimit,
op.verificationGasLimit,
op.preVerificationGas,
op.maxFeePerGas,
op.maxPriorityFeePerGas,
op.paymasterAndData
)
);
opHash = ECDSA.toTypedDataHash(domainSeparator, hashStruct);
}
}
7 changes: 6 additions & 1 deletion test/AccountERC721.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ contract AccountERC721Test is Test {

entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(
address(guardian),
address(entryPoint),
"ERC6551-Account",
"1"
);
registry = new ERC6551Registry();

tokenCollection = new MockERC721();
Expand Down
7 changes: 6 additions & 1 deletion test/AccountETH.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ contract AccountETHTest is Test {
function setUp() public {
entryPoint = new EntryPoint();
guardian = new AccountGuardian();
implementation = new Account(address(guardian), address(entryPoint));
implementation = new Account(
address(guardian),
address(entryPoint),
"ERC6551-Account",
"1"
);
registry = new ERC6551Registry();

tokenCollection = new MockERC721();
Expand Down
4 changes: 2 additions & 2 deletions test/mocks/MockAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ pragma solidity ^0.8.13;
import "../../src/Account.sol";

contract MockAccount is Account {
constructor(address _guardian, address entryPoint_)
Account(_guardian, entryPoint_)
constructor(address _guardian, address entryPoint_, string memory _name, string memory _version)
Account(_guardian, entryPoint_, _name, _version)
{}

function customFunction() external pure returns (uint256) {
Expand Down