From 0aad6b05d4872358a84def3b93a192ad4ae7d422 Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 15:30:51 +0200 Subject: [PATCH 1/8] Add support for EIP-712 --- script/DeployAccount.s.sol | 4 +- src/Account.sol | 76 ++++++++++++++++++++++---------------- test/Account.t.sol | 17 ++++++--- test/AccountERC1155.t.sol | 7 +++- test/AccountERC20.t.sol | 7 +++- test/AccountERC4337.t.sol | 64 ++++++++++++++++++++++++-------- test/AccountERC721.t.sol | 7 +++- test/AccountETH.t.sol | 7 +++- test/mocks/MockAccount.sol | 9 +++-- 9 files changed, 138 insertions(+), 60 deletions(-) diff --git a/script/DeployAccount.s.sol b/script/DeployAccount.s.sol index 3f455ee..a968a1d 100644 --- a/script/DeployAccount.s.sol +++ b/script/DeployAccount.s.sol @@ -15,7 +15,9 @@ contract DeployAccount is Script { salt: 0x6551655165516551655165516551655165516551655165516551655165516551 }( 0xB0219b60f0535FB3B62eeEC51EC4C765d138Ac0A, // guardian - 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 // entry point + 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, // entry point + "ERC6551-Account", + "1" ); new AccountProxy{ diff --git a/src/Account.sol b/src/Account.sol index 72fe28f..ff97940 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -12,8 +12,9 @@ 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"; +import {BaseAccount as BaseERC4337Account, IEntryPoint, UserOperation, calldataKeccak} from "account-abstraction/core/BaseAccount.sol"; import "./interfaces/IAccountGuardian.sol"; @@ -34,7 +35,8 @@ contract Account is IERC721Receiver, IERC1155Receiver, UUPSUpgradeable, - BaseERC4337Account + BaseERC4337Account, + EIP712 { using ECDSA for bytes32; @@ -81,7 +83,12 @@ contract Account is _; } - constructor(address _guardian, address entryPoint_) { + constructor( + address _guardian, + address entryPoint_, + string memory _name, + string memory _version + ) EIP712(_name, _version) { if (_guardian == address(0) || entryPoint_ == address(0)) revert InvalidInput(); @@ -171,11 +178,10 @@ contract Account is /// @dev EIP-1271 signature validation. By default, only the owner of the account is permissioned to sign. /// This function can be overriden. - function isValidSignature(bytes32 hash, bytes memory signature) - external - view - returns (bytes4 magicValue) - { + function isValidSignature( + bytes32 hash, + bytes memory signature + ) external view returns (bytes4 magicValue) { _handleOverrideStatic(); bool isValid = SignatureChecker.isValidSignatureNow( @@ -196,11 +202,7 @@ contract Account is function token() external view - returns ( - uint256 chainId, - address tokenContract, - uint256 tokenId - ) + returns (uint256 chainId, address tokenContract, uint256 tokenId) { return ERC6551AccountLib.token(); } @@ -264,12 +266,9 @@ contract Account is /// @dev Returns true if a given interfaceId is supported by this account. This method can be /// extended by an override. - function supportsInterface(bytes4 interfaceId) - public - view - override - returns (bool) - { + function supportsInterface( + bytes4 interfaceId + ) public view override returns (bool) { bool defaultSupport = interfaceId == type(IERC165).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || interfaceId == type(IERC6551Account).interfaceId; @@ -335,12 +334,9 @@ contract Account is /// @dev Contract upgrades can only be performed by the owner and the new implementation must /// be trusted - function _authorizeUpgrade(address newImplementation) - internal - view - override - onlyOwner - { + function _authorizeUpgrade( + address newImplementation + ) internal view override onlyOwner { bool isTrusted = IAccountGuardian(guardian).isTrustedImplementation( newImplementation ); @@ -352,8 +348,27 @@ contract Account is UserOperation calldata userOp, bytes32 userOpHash ) internal view override returns (uint256 validationData) { + // _hashTypedDataV4(userOp) + 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)" + ), + userOp.sender, + userOp.nonce, + userOp.initCode, + userOp.callData, + userOp.callGasLimit, + userOp.verificationGasLimit, + userOp.preVerificationGas, + userOp.maxFeePerGas, + userOp.maxPriorityFeePerGas, + userOp.paymasterAndData + ) + ); + bool isValid = this.isValidSignature( - userOpHash.toEthSignedMessageHash(), + _hashTypedDataV4(hashStruct), userOp.signature ) == IERC1271.isValidSignature.selector; @@ -393,11 +408,10 @@ contract Account is } /// @dev Executes a low-level static call - function _callStatic(address to, bytes calldata data) - internal - view - returns (bytes memory result) - { + function _callStatic( + address to, + bytes calldata data + ) internal view returns (bytes memory result) { bool success; (success, result) = to.staticcall(data); diff --git a/test/Account.t.sol b/test/Account.t.sol index 3e30245..9079b10 100644 --- a/test/Account.t.sol +++ b/test/Account.t.sol @@ -32,7 +32,12 @@ 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(); @@ -142,9 +147,9 @@ contract AccountTest is Test { assertEq(returnValue1, IERC1271.isValidSignature.selector); } - function testMessageVerificationForUnauthorizedUser(uint256 tokenId) - public - { + function testMessageVerificationForUnauthorizedUser( + uint256 tokenId + ) public { address user1 = vm.addr(1); tokenCollection.mint(user1, tokenId); @@ -543,7 +548,9 @@ contract AccountTest is Test { MockAccount upgradedImplementation = new MockAccount( address(guardian), - address(entryPoint) + address(entryPoint), + "ERC6551-Account", + "1" ); vm.expectRevert(UntrustedImplementation.selector); diff --git a/test/AccountERC1155.t.sol b/test/AccountERC1155.t.sol index d11f7de..5355dd8 100644 --- a/test/AccountERC1155.t.sol +++ b/test/AccountERC1155.t.sol @@ -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(); diff --git a/test/AccountERC20.t.sol b/test/AccountERC20.t.sol index 297a18c..bdd19b8 100644 --- a/test/AccountERC20.t.sol +++ b/test/AccountERC20.t.sol @@ -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(); diff --git a/test/AccountERC4337.t.sol b/test/AccountERC4337.t.sol index 96b0877..157e828 100644 --- a/test/AccountERC4337.t.sol +++ b/test/AccountERC4337.t.sol @@ -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(); @@ -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; @@ -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; @@ -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); @@ -276,4 +273,39 @@ contract AccountERC4337Test is Test { assertEq(accountAddress.balance, 1 ether); } + + function _buildOpHash( + 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); + } } diff --git a/test/AccountERC721.t.sol b/test/AccountERC721.t.sol index e461aee..b64e85b 100644 --- a/test/AccountERC721.t.sol +++ b/test/AccountERC721.t.sol @@ -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(); diff --git a/test/AccountETH.t.sol b/test/AccountETH.t.sol index 86610a7..b3316a3 100644 --- a/test/AccountETH.t.sol +++ b/test/AccountETH.t.sol @@ -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(); diff --git a/test/mocks/MockAccount.sol b/test/mocks/MockAccount.sol index 5423bc1..57b6461 100644 --- a/test/mocks/MockAccount.sol +++ b/test/mocks/MockAccount.sol @@ -4,9 +4,12 @@ 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) { return 12345; From e03703591307e7a46c004f923eda75e14dfd2308 Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 20:10:47 +0200 Subject: [PATCH 2/8] format --- src/Account.sol | 69 ++++++++++++++++++++------------------ test/mocks/MockAccount.sol | 9 ++--- 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/src/Account.sol b/src/Account.sol index ff97940..515872f 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -14,7 +14,7 @@ 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, calldataKeccak} from "account-abstraction/core/BaseAccount.sol"; +import {BaseAccount as BaseERC4337Account, IEntryPoint, UserOperation} from "account-abstraction/core/BaseAccount.sol"; import "./interfaces/IAccountGuardian.sol"; @@ -55,11 +55,7 @@ contract Account is /// @dev mapping from owner => caller => has permissions mapping(address => mapping(address => bool)) public permissions; - event OverrideUpdated( - address owner, - bytes4 selector, - address implementation - ); + event OverrideUpdated(address owner, bytes4 selector, address implementation); event PermissionUpdated(address owner, address caller, bool hasPermission); @@ -83,14 +79,12 @@ contract Account is _; } - constructor( - address _guardian, - address entryPoint_, - string memory _name, - string memory _version - ) EIP712(_name, _version) { - if (_guardian == address(0) || entryPoint_ == address(0)) + constructor(address _guardian, address entryPoint_, string memory _name, string memory _version) + EIP712(_name, _version) + { + if (_guardian == address(0) || entryPoint_ == address(0)) { revert InvalidInput(); + } _entryPoint = entryPoint_; guardian = _guardian; @@ -178,10 +172,11 @@ contract Account is /// @dev EIP-1271 signature validation. By default, only the owner of the account is permissioned to sign. /// This function can be overriden. - function isValidSignature( - bytes32 hash, - bytes memory signature - ) external view returns (bytes4 magicValue) { + function isValidSignature(bytes32 hash, bytes memory signature) + external + view + returns (bytes4 magicValue) + { _handleOverrideStatic(); bool isValid = SignatureChecker.isValidSignatureNow( @@ -202,7 +197,11 @@ contract Account is function token() external view - returns (uint256 chainId, address tokenContract, uint256 tokenId) + returns ( + uint256 chainId, + address tokenContract, + uint256 tokenId + ) { return ERC6551AccountLib.token(); } @@ -266,9 +265,12 @@ contract Account is /// @dev Returns true if a given interfaceId is supported by this account. This method can be /// extended by an override. - function supportsInterface( - bytes4 interfaceId - ) public view override returns (bool) { + function supportsInterface(bytes4 interfaceId) + public + view + override + returns (bool) + { bool defaultSupport = interfaceId == type(IERC165).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || interfaceId == type(IERC6551Account).interfaceId; @@ -334,9 +336,12 @@ contract Account is /// @dev Contract upgrades can only be performed by the owner and the new implementation must /// be trusted - function _authorizeUpgrade( - address newImplementation - ) internal view override onlyOwner { + function _authorizeUpgrade(address newImplementation) + internal + view + override + onlyOwner + { bool isTrusted = IAccountGuardian(guardian).isTrustedImplementation( newImplementation ); @@ -348,7 +353,6 @@ contract Account is UserOperation calldata userOp, bytes32 userOpHash ) internal view override returns (uint256 validationData) { - // _hashTypedDataV4(userOp) bytes32 hashStruct = keccak256( abi.encode( keccak256( @@ -367,10 +371,8 @@ contract Account is ) ); - bool isValid = this.isValidSignature( - _hashTypedDataV4(hashStruct), - userOp.signature - ) == IERC1271.isValidSignature.selector; + bool isValid = + this.isValidSignature(_hashTypedDataV4(hashStruct), userOp.signature) == IERC1271.isValidSignature.selector; if (isValid) { return 0; @@ -408,10 +410,11 @@ contract Account is } /// @dev Executes a low-level static call - function _callStatic( - address to, - bytes calldata data - ) internal view returns (bytes memory result) { + function _callStatic(address to, bytes calldata data) + internal + view + returns (bytes memory result) + { bool success; (success, result) = to.staticcall(data); diff --git a/test/mocks/MockAccount.sol b/test/mocks/MockAccount.sol index 57b6461..b409a8c 100644 --- a/test/mocks/MockAccount.sol +++ b/test/mocks/MockAccount.sol @@ -4,12 +4,9 @@ pragma solidity ^0.8.13; import "../../src/Account.sol"; contract MockAccount is Account { - constructor( - address _guardian, - address entryPoint_, - string memory _name, - string memory _version - ) Account(_guardian, entryPoint_, _name, _version) {} + constructor(address _guardian, address entryPoint_, string memory _name, string memory _version) + Account(_guardian, entryPoint_, _name, _version) + {} function customFunction() external pure returns (uint256) { return 12345; From 1c3bfe64e159a3e9ebee7ddff11884e516cc9fda Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 20:15:21 +0200 Subject: [PATCH 3/8] format --- src/Account.sol | 6 +- test/Account.t.sol | 140 +++++++++------------------------------------ 2 files changed, 31 insertions(+), 115 deletions(-) diff --git a/src/Account.sol b/src/Account.sol index 515872f..436446f 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -55,7 +55,11 @@ contract Account is /// @dev mapping from owner => caller => has permissions mapping(address => mapping(address => bool)) public permissions; - event OverrideUpdated(address owner, bytes4 selector, address implementation); + event OverrideUpdated( + address owner, + bytes4 selector, + address implementation + ); event PermissionUpdated(address owner, address caller, bool hasPermission); diff --git a/test/Account.t.sol b/test/Account.t.sol index 9079b10..f37563b 100644 --- a/test/Account.t.sol +++ b/test/Account.t.sol @@ -147,21 +147,16 @@ contract AccountTest is Test { assertEq(returnValue1, IERC1271.isValidSignature.selector); } - function testMessageVerificationForUnauthorizedUser( - uint256 tokenId - ) public { + function testMessageVerificationForUnauthorizedUser(uint256 tokenId) + public + { address user1 = vm.addr(1); tokenCollection.mint(user1, tokenId); assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); Account account = Account(payable(accountAddress)); @@ -183,12 +178,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -215,9 +205,7 @@ contract AccountTest is Test { // fallback calls should revert if account is locked vm.prank(user1); vm.expectRevert(AccountLocked.selector); - (bool success, bytes memory result) = accountAddress.call( - abi.encodeWithSignature("customFunction()") - ); + (bool success, bytes memory result) = accountAddress.call(abi.encodeWithSignature("customFunction()")); // silence unused variable compiler warnings success; @@ -258,10 +246,7 @@ contract AccountTest is Test { bytes32 hashAfterUnlock = keccak256("This is a signed message"); (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(1, hashAfterUnlock); bytes memory signature2 = abi.encodePacked(r2, s2, v2); - bytes4 returnValue1 = account.isValidSignature( - hashAfterUnlock, - signature2 - ); + bytes4 returnValue1 = account.isValidSignature(hashAfterUnlock, signature2); assertEq(returnValue1, IERC1271.isValidSignature.selector); } @@ -272,12 +257,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -287,9 +267,7 @@ contract AccountTest is Test { MockExecutor mockExecutor = new MockExecutor(); // calls succeed with noop if override is undefined - (bool success, bytes memory result) = accountAddress.call( - abi.encodeWithSignature("customFunction()") - ); + (bool success, bytes memory result) = accountAddress.call(abi.encodeWithSignature("customFunction()")); assertEq(success, true); assertEq(result, ""); @@ -318,48 +296,30 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); Account account = Account(payable(accountAddress)); - assertEq( - account.supportsInterface(type(IERC1155Receiver).interfaceId), - true - ); + assertEq(account.supportsInterface(type(IERC1155Receiver).interfaceId), true); assertEq(account.supportsInterface(0x12345678), false); MockExecutor mockExecutor = new MockExecutor(); // set overrides on account bytes4[] memory selectors = new bytes4[](1); - selectors[0] = bytes4( - abi.encodeWithSignature("supportsInterface(bytes4)") - ); + selectors[0] = bytes4(abi.encodeWithSignature("supportsInterface(bytes4)")); address[] memory implementations = new address[](1); implementations[0] = address(mockExecutor); vm.prank(user1); account.setOverrides(selectors, implementations); // override handles extra interface support - assertEq( - Account(payable(accountAddress)).supportsInterface(0x12345678), - true - ); + assertEq(Account(payable(accountAddress)).supportsInterface(0x12345678), true); // cannot override default interfaces - assertEq( - Account(payable(accountAddress)).supportsInterface( - type(IERC1155Receiver).interfaceId - ), - true - ); + assertEq(Account(payable(accountAddress)).supportsInterface(type(IERC1155Receiver).interfaceId), true); } /**/ @@ -371,12 +331,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -411,12 +366,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - chainId, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), chainId, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -442,21 +392,12 @@ contract AccountTest is Test { assertEq(user1.balance, 0.1 ether); address nativeAccountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.prank(crossChainExecutor); vm.expectRevert(NotAuthorized.selector); - Account(payable(nativeAccountAddress)).executeCall( - user1, - 0.1 ether, - "" - ); + Account(payable(nativeAccountAddress)).executeCall(user1, 0.1 ether, ""); assertEq(user1.balance, 0.1 ether); } @@ -468,12 +409,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -484,11 +420,7 @@ contract AccountTest is Test { vm.prank(user1); vm.expectRevert(MockReverter.MockError.selector); - account.executeCall( - payable(address(mockReverter)), - 0, - abi.encodeWithSignature("fail()") - ); + account.executeCall(payable(address(mockReverter)), 0, abi.encodeWithSignature("fail()")); } function testAccountOwnerIsNullIfContextNotSet() public { @@ -505,26 +437,15 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); Account account = Account(payable(accountAddress)); - assertEq( - account.supportsInterface(type(IERC6551Account).interfaceId), - true - ); - assertEq( - account.supportsInterface(type(IERC1155Receiver).interfaceId), - true - ); + assertEq(account.supportsInterface(type(IERC6551Account).interfaceId), true); + assertEq(account.supportsInterface(type(IERC1155Receiver).interfaceId), true); assertEq(account.supportsInterface(type(IERC165).interfaceId), true); } @@ -536,12 +457,7 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), - block.chainid, - address(tokenCollection), - tokenId, - 0, - abi.encodeWithSignature("initialize()") + address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") ); Account account = Account(payable(accountAddress)); @@ -557,15 +473,11 @@ contract AccountTest is Test { vm.prank(user1); account.upgradeTo(address(upgradedImplementation)); - guardian.setTrustedImplementation( - address(upgradedImplementation), - true - ); + guardian.setTrustedImplementation(address(upgradedImplementation), true); vm.prank(user1); account.upgradeTo(address(upgradedImplementation)); - uint256 returnValue = MockAccount(payable(accountAddress)) - .customFunction(); + uint256 returnValue = MockAccount(payable(accountAddress)).customFunction(); assertEq(returnValue, 12345); } From 5718d93b89b0243b2c0f3249af5db5fe983817b7 Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 20:22:49 +0200 Subject: [PATCH 4/8] format tests --- test/Account.t.sol | 143 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 113 insertions(+), 30 deletions(-) diff --git a/test/Account.t.sol b/test/Account.t.sol index f37563b..71e3981 100644 --- a/test/Account.t.sol +++ b/test/Account.t.sol @@ -32,12 +32,7 @@ contract AccountTest is Test { function setUp() public { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint), "ERC6551-Account", "1"); proxy = new AccountProxy(address(implementation)); registry = new ERC6551Registry(); @@ -156,7 +151,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); Account account = Account(payable(accountAddress)); @@ -178,7 +178,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -205,7 +210,9 @@ contract AccountTest is Test { // fallback calls should revert if account is locked vm.prank(user1); vm.expectRevert(AccountLocked.selector); - (bool success, bytes memory result) = accountAddress.call(abi.encodeWithSignature("customFunction()")); + (bool success, bytes memory result) = accountAddress.call( + abi.encodeWithSignature("customFunction()") + ); // silence unused variable compiler warnings success; @@ -246,7 +253,10 @@ contract AccountTest is Test { bytes32 hashAfterUnlock = keccak256("This is a signed message"); (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(1, hashAfterUnlock); bytes memory signature2 = abi.encodePacked(r2, s2, v2); - bytes4 returnValue1 = account.isValidSignature(hashAfterUnlock, signature2); + bytes4 returnValue1 = account.isValidSignature( + hashAfterUnlock, + signature2 + ); assertEq(returnValue1, IERC1271.isValidSignature.selector); } @@ -257,7 +267,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -267,7 +282,9 @@ contract AccountTest is Test { MockExecutor mockExecutor = new MockExecutor(); // calls succeed with noop if override is undefined - (bool success, bytes memory result) = accountAddress.call(abi.encodeWithSignature("customFunction()")); + (bool success, bytes memory result) = accountAddress.call( + abi.encodeWithSignature("customFunction()") + ); assertEq(success, true); assertEq(result, ""); @@ -296,30 +313,48 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); Account account = Account(payable(accountAddress)); - assertEq(account.supportsInterface(type(IERC1155Receiver).interfaceId), true); + assertEq( + account.supportsInterface(type(IERC1155Receiver).interfaceId), + true + ); assertEq(account.supportsInterface(0x12345678), false); MockExecutor mockExecutor = new MockExecutor(); // set overrides on account bytes4[] memory selectors = new bytes4[](1); - selectors[0] = bytes4(abi.encodeWithSignature("supportsInterface(bytes4)")); + selectors[0] = bytes4( + abi.encodeWithSignature("supportsInterface(bytes4)") + ); address[] memory implementations = new address[](1); implementations[0] = address(mockExecutor); vm.prank(user1); account.setOverrides(selectors, implementations); // override handles extra interface support - assertEq(Account(payable(accountAddress)).supportsInterface(0x12345678), true); + assertEq( + Account(payable(accountAddress)).supportsInterface(0x12345678), + true + ); // cannot override default interfaces - assertEq(Account(payable(accountAddress)).supportsInterface(type(IERC1155Receiver).interfaceId), true); + assertEq( + Account(payable(accountAddress)).supportsInterface( + type(IERC1155Receiver).interfaceId + ), + true + ); } /**/ @@ -331,7 +366,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -366,7 +406,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), chainId, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + chainId, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -392,12 +437,21 @@ contract AccountTest is Test { assertEq(user1.balance, 0.1 ether); address nativeAccountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.prank(crossChainExecutor); vm.expectRevert(NotAuthorized.selector); - Account(payable(nativeAccountAddress)).executeCall(user1, 0.1 ether, ""); + Account(payable(nativeAccountAddress)).executeCall( + user1, + 0.1 ether, + "" + ); assertEq(user1.balance, 0.1 ether); } @@ -409,7 +463,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); @@ -420,7 +479,11 @@ contract AccountTest is Test { vm.prank(user1); vm.expectRevert(MockReverter.MockError.selector); - account.executeCall(payable(address(mockReverter)), 0, abi.encodeWithSignature("fail()")); + account.executeCall( + payable(address(mockReverter)), + 0, + abi.encodeWithSignature("fail()") + ); } function testAccountOwnerIsNullIfContextNotSet() public { @@ -437,15 +500,26 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); vm.deal(accountAddress, 1 ether); Account account = Account(payable(accountAddress)); - assertEq(account.supportsInterface(type(IERC6551Account).interfaceId), true); - assertEq(account.supportsInterface(type(IERC1155Receiver).interfaceId), true); + assertEq( + account.supportsInterface(type(IERC6551Account).interfaceId), + true + ); + assertEq( + account.supportsInterface(type(IERC1155Receiver).interfaceId), + true + ); assertEq(account.supportsInterface(type(IERC165).interfaceId), true); } @@ -457,7 +531,12 @@ contract AccountTest is Test { assertEq(tokenCollection.ownerOf(tokenId), user1); address accountAddress = registry.createAccount( - address(proxy), block.chainid, address(tokenCollection), tokenId, 0, abi.encodeWithSignature("initialize()") + address(proxy), + block.chainid, + address(tokenCollection), + tokenId, + 0, + abi.encodeWithSignature("initialize()") ); Account account = Account(payable(accountAddress)); @@ -473,12 +552,16 @@ contract AccountTest is Test { vm.prank(user1); account.upgradeTo(address(upgradedImplementation)); - guardian.setTrustedImplementation(address(upgradedImplementation), true); + guardian.setTrustedImplementation( + address(upgradedImplementation), + true + ); vm.prank(user1); account.upgradeTo(address(upgradedImplementation)); - uint256 returnValue = MockAccount(payable(accountAddress)).customFunction(); + uint256 returnValue = MockAccount(payable(accountAddress)) + .customFunction(); assertEq(returnValue, 12345); } -} +} \ No newline at end of file From 3ef7af2e2af95e2047da0c46a0435e80f03c0dee Mon Sep 17 00:00:00 2001 From: Yannick Chi <0xyanc@gmail.com> Date: Fri, 19 May 2023 21:03:03 +0200 Subject: [PATCH 5/8] Update src/Account.sol Co-authored-by: Jayden Windle --- src/Account.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Account.sol b/src/Account.sol index 436446f..40d635b 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -360,7 +360,7 @@ contract Account is 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)" + "UserOperation(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData)" ), userOp.sender, userOp.nonce, From fb86fab0c60596634489ae4b7204ee5aabe5bdf7 Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 21:19:24 +0200 Subject: [PATCH 6/8] Change EIP-712 name and version fields as constants --- script/DeployAccount.s.sol | 4 +--- src/Account.sol | 10 ++++++++-- test/Account.t.sol | 2 +- test/AccountERC1155.t.sol | 7 +------ test/AccountERC20.t.sol | 7 +------ test/AccountERC4337.t.sol | 7 +------ test/AccountERC721.t.sol | 7 +------ test/AccountETH.t.sol | 7 +------ test/mocks/MockAccount.sol | 2 +- 9 files changed, 16 insertions(+), 37 deletions(-) diff --git a/script/DeployAccount.s.sol b/script/DeployAccount.s.sol index a968a1d..3f455ee 100644 --- a/script/DeployAccount.s.sol +++ b/script/DeployAccount.s.sol @@ -15,9 +15,7 @@ contract DeployAccount is Script { salt: 0x6551655165516551655165516551655165516551655165516551655165516551 }( 0xB0219b60f0535FB3B62eeEC51EC4C765d138Ac0A, // guardian - 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789, // entry point - "ERC6551-Account", - "1" + 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 // entry point ); new AccountProxy{ diff --git a/src/Account.sol b/src/Account.sol index 436446f..6848041 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -40,6 +40,12 @@ contract Account is { using ECDSA for bytes32; + /// @dev EIP-712 name + string public constant NAME = "ERC6551-Account"; + + /// @dev EIP-712 version + string public constant VERSION = "1"; + /// @dev ERC-4337 entry point address address public immutable _entryPoint; @@ -83,8 +89,8 @@ contract Account is _; } - constructor(address _guardian, address entryPoint_, string memory _name, string memory _version) - EIP712(_name, _version) + constructor(address _guardian, address entryPoint_) + EIP712(NAME, VERSION) { if (_guardian == address(0) || entryPoint_ == address(0)) { revert InvalidInput(); diff --git a/test/Account.t.sol b/test/Account.t.sol index 71e3981..598ca50 100644 --- a/test/Account.t.sol +++ b/test/Account.t.sol @@ -32,7 +32,7 @@ contract AccountTest is Test { function setUp() public { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account(address(guardian), address(entryPoint), "ERC6551-Account", "1"); + implementation = new Account(address(guardian), address(entryPoint)); proxy = new AccountProxy(address(implementation)); registry = new ERC6551Registry(); diff --git a/test/AccountERC1155.t.sol b/test/AccountERC1155.t.sol index 5355dd8..d11f7de 100644 --- a/test/AccountERC1155.t.sol +++ b/test/AccountERC1155.t.sol @@ -33,12 +33,7 @@ contract AccountERC1155Test is Test { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint)); registry = new ERC6551Registry(); tokenCollection = new MockERC721(); diff --git a/test/AccountERC20.t.sol b/test/AccountERC20.t.sol index bdd19b8..297a18c 100644 --- a/test/AccountERC20.t.sol +++ b/test/AccountERC20.t.sol @@ -32,12 +32,7 @@ contract AccountERC20Test is Test { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint)); registry = new ERC6551Registry(); tokenCollection = new MockERC721(); diff --git a/test/AccountERC4337.t.sol b/test/AccountERC4337.t.sol index 157e828..d1278b4 100644 --- a/test/AccountERC4337.t.sol +++ b/test/AccountERC4337.t.sol @@ -30,12 +30,7 @@ contract AccountERC4337Test is Test { function setUp() public { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint)); registry = new ERC6551Registry(); diff --git a/test/AccountERC721.t.sol b/test/AccountERC721.t.sol index b64e85b..e461aee 100644 --- a/test/AccountERC721.t.sol +++ b/test/AccountERC721.t.sol @@ -32,12 +32,7 @@ contract AccountERC721Test is Test { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint)); registry = new ERC6551Registry(); tokenCollection = new MockERC721(); diff --git a/test/AccountETH.t.sol b/test/AccountETH.t.sol index b3316a3..86610a7 100644 --- a/test/AccountETH.t.sol +++ b/test/AccountETH.t.sol @@ -27,12 +27,7 @@ contract AccountETHTest is Test { function setUp() public { entryPoint = new EntryPoint(); guardian = new AccountGuardian(); - implementation = new Account( - address(guardian), - address(entryPoint), - "ERC6551-Account", - "1" - ); + implementation = new Account(address(guardian), address(entryPoint)); registry = new ERC6551Registry(); tokenCollection = new MockERC721(); diff --git a/test/mocks/MockAccount.sol b/test/mocks/MockAccount.sol index b409a8c..0b019f4 100644 --- a/test/mocks/MockAccount.sol +++ b/test/mocks/MockAccount.sol @@ -5,7 +5,7 @@ import "../../src/Account.sol"; contract MockAccount is Account { constructor(address _guardian, address entryPoint_, string memory _name, string memory _version) - Account(_guardian, entryPoint_, _name, _version) + Account(_guardian, entryPoint_) {} function customFunction() external pure returns (uint256) { From 7d856f73967dc2eab4edd592c5a78e1938e61f05 Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 22:09:04 +0200 Subject: [PATCH 7/8] Add userOpHash in the typed data fields --- src/Account.sol | 35 +++++++++++++++++++++-------------- test/AccountERC4337.t.sol | 29 +++++++++++++++++------------ 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/Account.sol b/src/Account.sol index cf7f853..d33bc05 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; - +import "forge-std/console2.sol"; import "erc6551/interfaces/IERC6551Account.sol"; import "erc6551/lib/ERC6551AccountLib.sol"; @@ -46,6 +46,9 @@ contract Account is /// @dev EIP-712 version string public constant VERSION = "1"; + /// @dev EIP-712 Type for UserOperation + bytes32 public immutable userOperationType; + /// @dev ERC-4337 entry point address address public immutable _entryPoint; @@ -98,6 +101,9 @@ contract Account is _entryPoint = entryPoint_; guardian = _guardian; + userOperationType = keccak256( + "UserOperation(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,bytes32 userOpHash)" + ); } /// @dev allows eth transfers by default, but allows account owner to override @@ -363,23 +369,24 @@ contract Account is UserOperation calldata userOp, bytes32 userOpHash ) internal view override returns (uint256 validationData) { + UserOperation memory userOpMemory = userOp; bytes32 hashStruct = keccak256( abi.encode( - keccak256( - "UserOperation(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 + userOperationType, + userOpMemory.sender, + userOpMemory.nonce, + userOpMemory.initCode, + userOpMemory.callData, + userOpMemory.callGasLimit, + userOpMemory.verificationGasLimit, + userOpMemory.preVerificationGas, + userOpMemory.maxFeePerGas, + userOpMemory.maxPriorityFeePerGas, + userOpMemory.paymasterAndData, + userOpHash ) ); + console2.logBytes32(hashStruct); bool isValid = this.isValidSignature(_hashTypedDataV4(hashStruct), userOp.signature) == IERC1271.isValidSignature.selector; diff --git a/test/AccountERC4337.t.sol b/test/AccountERC4337.t.sol index d1278b4..a49ab34 100644 --- a/test/AccountERC4337.t.sol +++ b/test/AccountERC4337.t.sol @@ -136,9 +136,9 @@ contract AccountERC4337Test is Test { paymasterAndData: "", signature: "" }); - bytes32 opHash = _buildOpHash(op, accountAddress); - - (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash); + bytes32 opHash = entryPoint.getUserOpHash(op); + bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); bytes memory signature = abi.encodePacked(r, s, v); op.signature = signature; @@ -194,8 +194,9 @@ contract AccountERC4337Test is Test { signature: "" }); - bytes32 opHash = _buildOpHash(op, accountAddress); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash); + bytes32 opHash = entryPoint.getUserOpHash(op); + bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); bytes memory signature = abi.encodePacked(r, s, v); op.signature = signature; @@ -251,8 +252,9 @@ contract AccountERC4337Test is Test { signature: "" }); - bytes32 opHash = _buildOpHash(op, accountAddress); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, opHash); + bytes32 opHash = entryPoint.getUserOpHash(op); + bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); // invalidate signature bytes memory signature = abi.encodePacked(r, s, v + 1); @@ -271,8 +273,9 @@ contract AccountERC4337Test is Test { function _buildOpHash( UserOperation memory op, - address accountAddress - ) private view returns (bytes32 opHash) { + address accountAddress, + bytes32 opHash + ) private view returns (bytes32 op712Hash) { bytes32 domainSeparator = keccak256( abi.encode( keccak256( @@ -287,7 +290,7 @@ contract AccountERC4337Test is Test { 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)" + "UserOperation(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,bytes32 userOpHash)" ), op.sender, op.nonce, @@ -298,9 +301,11 @@ contract AccountERC4337Test is Test { op.preVerificationGas, op.maxFeePerGas, op.maxPriorityFeePerGas, - op.paymasterAndData + op.paymasterAndData, + opHash ) ); - opHash = ECDSA.toTypedDataHash(domainSeparator, hashStruct); + console2.logBytes32(hashStruct); + op712Hash = ECDSA.toTypedDataHash(domainSeparator, hashStruct); } } From 7de324f27747842b58e6052434bcd83ce333936b Mon Sep 17 00:00:00 2001 From: 0xyanc Date: Fri, 19 May 2023 22:19:42 +0200 Subject: [PATCH 8/8] Expose function to build UserOp hash compliant with EIP-712 --- src/Account.sol | 39 +++++++++++++++++++++++++++++++-- test/AccountERC4337.t.sol | 46 +++------------------------------------ 2 files changed, 40 insertions(+), 45 deletions(-) diff --git a/src/Account.sol b/src/Account.sol index d33bc05..12259d8 100644 --- a/src/Account.sol +++ b/src/Account.sol @@ -1,6 +1,5 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13; -import "forge-std/console2.sol"; import "erc6551/interfaces/IERC6551Account.sol"; import "erc6551/lib/ERC6551AccountLib.sol"; @@ -350,6 +349,43 @@ contract Account is return this.onERC1155BatchReceived.selector; } + /// @dev + function buildUserOp712Hash( + UserOperation memory op, + bytes32 opHash + ) public view returns (bytes32 op712Hash) { + bytes32 domainSeparator = keccak256( + abi.encode( + keccak256( + "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" + ), + keccak256(bytes(NAME)), + keccak256(bytes(VERSION)), + block.chainid, + op.sender + ) + ); + bytes32 hashStruct = keccak256( + abi.encode( + keccak256( + "UserOperation(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,bytes32 userOpHash)" + ), + op.sender, + op.nonce, + op.initCode, + op.callData, + op.callGasLimit, + op.verificationGasLimit, + op.preVerificationGas, + op.maxFeePerGas, + op.maxPriorityFeePerGas, + op.paymasterAndData, + opHash + ) + ); + op712Hash = ECDSA.toTypedDataHash(domainSeparator, hashStruct); + } + /// @dev Contract upgrades can only be performed by the owner and the new implementation must /// be trusted function _authorizeUpgrade(address newImplementation) @@ -386,7 +422,6 @@ contract Account is userOpHash ) ); - console2.logBytes32(hashStruct); bool isValid = this.isValidSignature(_hashTypedDataV4(hashStruct), userOp.signature) == IERC1271.isValidSignature.selector; diff --git a/test/AccountERC4337.t.sol b/test/AccountERC4337.t.sol index a49ab34..b9547d5 100644 --- a/test/AccountERC4337.t.sol +++ b/test/AccountERC4337.t.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.13; import "forge-std/Test.sol"; - import "openzeppelin-contracts/utils/cryptography/ECDSA.sol"; import "openzeppelin-contracts/token/ERC20/ERC20.sol"; import "openzeppelin-contracts/proxy/Clones.sol"; @@ -137,7 +136,7 @@ contract AccountERC4337Test is Test { signature: "" }); bytes32 opHash = entryPoint.getUserOpHash(op); - bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + bytes32 op712Hash = implementation.buildUserOp712Hash(op, opHash); (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); bytes memory signature = abi.encodePacked(r, s, v); @@ -193,9 +192,8 @@ contract AccountERC4337Test is Test { paymasterAndData: "", signature: "" }); - bytes32 opHash = entryPoint.getUserOpHash(op); - bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + bytes32 op712Hash = implementation.buildUserOp712Hash(op, opHash); (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); bytes memory signature = abi.encodePacked(r, s, v); @@ -253,7 +251,7 @@ contract AccountERC4337Test is Test { }); bytes32 opHash = entryPoint.getUserOpHash(op); - bytes32 op712Hash = _buildOpHash(op, accountAddress, opHash); + bytes32 op712Hash = implementation.buildUserOp712Hash(op, opHash); (uint8 v, bytes32 r, bytes32 s) = vm.sign(1, op712Hash); // invalidate signature @@ -270,42 +268,4 @@ contract AccountERC4337Test is Test { assertEq(accountAddress.balance, 1 ether); } - - function _buildOpHash( - UserOperation memory op, - address accountAddress, - bytes32 opHash - ) private view returns (bytes32 op712Hash) { - 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( - "UserOperation(address sender,uint256 nonce,bytes initCode,bytes callData,uint256 callGasLimit,uint256 verificationGasLimit,uint256 preVerificationGas,uint256 maxFeePerGas,uint256 maxPriorityFeePerGas,bytes paymasterAndData,bytes32 userOpHash)" - ), - op.sender, - op.nonce, - op.initCode, - op.callData, - op.callGasLimit, - op.verificationGasLimit, - op.preVerificationGas, - op.maxFeePerGas, - op.maxPriorityFeePerGas, - op.paymasterAndData, - opHash - ) - ); - console2.logBytes32(hashStruct); - op712Hash = ECDSA.toTypedDataHash(domainSeparator, hashStruct); - } }