From bd33993b646e90bc3a48e959417b3f50153e9262 Mon Sep 17 00:00:00 2001 From: Adam Egyed <5456061+adamegyed@users.noreply.github.com> Date: Thu, 22 Aug 2024 14:15:11 -0400 Subject: [PATCH] feat(SingleSignerValidation): add replay safe hash and utility contracts for EIP-712 in modules [2/2] (#141) --- src/modules/ModuleEIP712.sol | 29 +++++++++++++++++ src/modules/ReplaySafeWrapper.sol | 32 +++++++++++++++++++ .../SingleSignerValidationModule.sol | 14 +++++--- test/account/PerHookData.t.sol | 6 ++-- test/account/UpgradeableModularAccount.t.sol | 12 +++++-- .../module/SingleSignerValidationModule.t.sol | 9 ++++-- 6 files changed, 89 insertions(+), 13 deletions(-) create mode 100644 src/modules/ModuleEIP712.sol create mode 100644 src/modules/ReplaySafeWrapper.sol diff --git a/src/modules/ModuleEIP712.sol b/src/modules/ModuleEIP712.sol new file mode 100644 index 00000000..84c323e1 --- /dev/null +++ b/src/modules/ModuleEIP712.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +// A base for modules that use EIP-712 structured data signing. +// +// Unlike other EIP712 libraries, this mixin uses the salt field to hold the account address. +// +// It omits the name and version from the EIP-712 domain, as modules are intended to be deployed as +// immutable singletons, thus a different versions and instances would have a different module address. +// +// Due to depending on the account address to calculate the domain separator, this abstract contract does not +// implement EIP-5267, as the domain retrieval function does not provide a parameter to use for the account address +// (internally the verifyingContract), and the computed `msg.sender` for an `eth_call` without an override is +// address(0). +abstract contract ModuleEIP712 { + // keccak256( + // "EIP712Domain(uint256 chainId,address verifyingContract,bytes32 salt)" + // ) + bytes32 private constant _DOMAIN_SEPARATOR_TYPEHASH = + 0x71062c282d40422f744945d587dbf4ecfd4f9cfad1d35d62c944373009d96162; + + function _domainSeparator(address account) internal view returns (bytes32) { + return keccak256( + abi.encode( + _DOMAIN_SEPARATOR_TYPEHASH, block.chainid, address(this), bytes32(uint256(uint160(account))) + ) + ); + } +} diff --git a/src/modules/ReplaySafeWrapper.sol b/src/modules/ReplaySafeWrapper.sol new file mode 100644 index 00000000..0ff9930b --- /dev/null +++ b/src/modules/ReplaySafeWrapper.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +import {ModuleEIP712} from "./ModuleEIP712.sol"; + +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; + +// A contract mixin for modules that wish to use EIP-712 to wrap the hashes sent to the EIP-1271 function +// `isValidSignature`. +// This makes signatures generated by owners of contract accounts non-replayable across multiple accounts owned by +// the same owner. +abstract contract ReplaySafeWrapper is ModuleEIP712 { + // keccak256("ReplaySafeHash(bytes32 hash)") + bytes32 private constant _REPLAY_SAFE_HASH_TYPEHASH = + 0x294a8735843d4afb4f017c76faf3b7731def145ed0025fc9b1d5ce30adf113ff; + + /// @notice Wraps a hash in an EIP-712 envelope to prevent cross-account replay attacks. + /// Uses the ModuleEIP712 domain separator, which includes the chainId, module address, and account address. + /// @param account The account that will validate the message hash. + /// @param hash The hash to wrap. + /// @return The the replay-safe hash, computed by wrapping the input hash in an EIP-712 struct. + function replaySafeHash(address account, bytes32 hash) public view virtual returns (bytes32) { + return MessageHashUtils.toTypedDataHash({ + domainSeparator: _domainSeparator(account), + structHash: _hashStruct(hash) + }); + } + + function _hashStruct(bytes32 hash) internal view virtual returns (bytes32) { + return keccak256(abi.encode(_REPLAY_SAFE_HASH_TYPEHASH, hash)); + } +} diff --git a/src/modules/validation/SingleSignerValidationModule.sol b/src/modules/validation/SingleSignerValidationModule.sol index c869af22..c9d2b938 100644 --- a/src/modules/validation/SingleSignerValidationModule.sol +++ b/src/modules/validation/SingleSignerValidationModule.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.25; import {IModule, ModuleMetadata} from "../../interfaces/IModule.sol"; import {IValidationModule} from "../../interfaces/IValidationModule.sol"; import {BaseModule} from "../BaseModule.sol"; + +import {ReplaySafeWrapper} from "../ReplaySafeWrapper.sol"; import {ISingleSignerValidationModule} from "./ISingleSignerValidationModule.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; @@ -17,12 +19,11 @@ import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/Signa /// the account. Account states are to be retrieved from this global singleton directly. /// /// - This validation supports ERC-1271. The signature is valid if it is signed by the owner's private key -/// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the -/// owner (if the owner is a contract). +/// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the owner (if the owner is a contract). /// /// - This validation supports composition that other validation can relay on entities in this validation /// to validate partially or fully. -contract SingleSignerValidationModule is ISingleSignerValidationModule, BaseModule { +contract SingleSignerValidationModule is ISingleSignerValidationModule, ReplaySafeWrapper, BaseModule { using MessageHashUtils for bytes32; string internal constant _NAME = "SingleSigner Validation"; @@ -93,14 +94,17 @@ contract SingleSignerValidationModule is ISingleSignerValidationModule, BaseModu /// @inheritdoc IValidationModule /// @dev The signature is valid if it is signed by the owner's private key /// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the - /// owner (if the owner is a contract). Note that the signature is wrapped in an EIP-191 message + /// owner (if the owner is a contract). + /// Note that the digest is wrapped in an EIP-712 struct to prevent cross-account replay attacks. The + /// replay-safe hash may be retrieved by calling the public function `replaySafeHash`. function validateSignature(address account, uint32 entityId, address, bytes32 digest, bytes calldata signature) external view override returns (bytes4) { - if (SignatureChecker.isValidSignatureNow(signers[entityId][account], digest, signature)) { + bytes32 _replaySafeHash = replaySafeHash(account, digest); + if (SignatureChecker.isValidSignatureNow(signers[entityId][account], _replaySafeHash, signature)) { return _1271_MAGIC_VALUE; } return _1271_INVALID; diff --git a/test/account/PerHookData.t.sol b/test/account/PerHookData.t.sol index 505fd00f..f9657ca6 100644 --- a/test/account/PerHookData.t.sol +++ b/test/account/PerHookData.t.sol @@ -23,9 +23,6 @@ contract PerHookDataTest is CustomValidationTestBase { Counter internal _counter; function setUp() public { - _signerValidation = - ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID); - _counter = new Counter(); _accessControlHookModule = new MockAccessControlHookModule(); @@ -360,7 +357,8 @@ contract PerHookDataTest is CustomValidationTestBase { bytes32 messageHash = keccak256(abi.encodePacked(message)); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, messageHash); + bytes32 replaySafeHash = singleSignerValidationModule.replaySafeHash(address(account1), messageHash); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, replaySafeHash); PreValidationHookData[] memory preValidationHookData = new PreValidationHookData[](1); preValidationHookData[0] = PreValidationHookData({index: 0, validationData: abi.encodePacked(message)}); diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index b5671145..cfc99497 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -392,9 +392,17 @@ contract UpgradeableModularAccountTest is AccountTestBase { function test_isValidSignature() public { bytes32 message = keccak256("hello world"); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, message); + uint8 v; + bytes32 r; + bytes32 s; - // singleSignerValidationModule.ownerOf(address(account1)); + if (vm.envOr("SMA_TEST", false)) { + // todo: implement replay-safe hashing for SMA + (v, r, s) = vm.sign(owner1Key, message); + } else { + bytes32 replaySafeHash = singleSignerValidationModule.replaySafeHash(address(account1), message); + (v, r, s) = vm.sign(owner1Key, replaySafeHash); + } bytes memory signature = _encode1271Signature(_signerValidation, abi.encodePacked(r, s, v)); diff --git a/test/module/SingleSignerValidationModule.t.sol b/test/module/SingleSignerValidationModule.t.sol index 5235ad87..07d08246 100644 --- a/test/module/SingleSignerValidationModule.t.sol +++ b/test/module/SingleSignerValidationModule.t.sol @@ -113,8 +113,10 @@ contract SingleSignerValidationModuleTest is AccountTestBase { address accountAddr = address(account); + bytes32 replaySafeHash = singleSignerValidationModule.replaySafeHash(accountAddr, digest); + vm.startPrank(accountAddr); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, replaySafeHash); // sig check should fail assertEq( @@ -141,7 +143,10 @@ contract SingleSignerValidationModuleTest is AccountTestBase { address accountAddr = address(account); vm.startPrank(accountAddr); singleSignerValidationModule.transferSigner(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(contractOwner)); - bytes memory signature = contractOwner.sign(digest); + + bytes32 replaySafeHash = singleSignerValidationModule.replaySafeHash(accountAddr, digest); + + bytes memory signature = contractOwner.sign(replaySafeHash); assertEq( singleSignerValidationModule.validateSignature( accountAddr, TEST_DEFAULT_VALIDATION_ENTITY_ID, address(this), digest, signature