Skip to content
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
29 changes: 29 additions & 0 deletions src/modules/ModuleEIP712.sol
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will need public view func to get the EIP712Domain fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed this in the PR body and in comments in the file - we can't safely expose that function from ERC-5267 because the domain separator changes by account, and the function does not take a parameter. We could do something non-standard by depending on msg.sender and reverting if msg.sender == address(0), but that is not generally compatible with tools for fetching via eip712Domain().

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, yeah, we will also need to install eip712Domain() as a exe function on the account.

With what we have, we also are not completely compliant with EIP-712.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With what we have, we also are not completely compliant with EIP-712.

How so? I think we're compliant with EIP-712, just not the extension EIP-5267.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Look at the definition of doamin-separator.

The EIP712Domain fields should be the order as above, skipping any absent fields. Future field additions must be in alphabetical order and come after the above fields. User-agents should accept fields in any order as specified by the EIPT712Domain type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, we are. Was not thinking the account converted to bytes32 would be the salt.

Actually, we might still be compliant with ERC5267.
What if we just return bytes32(0) for salt, but have fields to be 11111 to indicate salt is used and dynamic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a cool idea, but I wonder if doing so requires implementers to keep track of this specific behaviour-- which is functionally equivalent to having a new function.

I could be wrong but the tradeoff is either "use the old function, but be ready to handle a new special case" or "use a new function." Does that sound right?

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Aug 16, 2024

Choose a reason for hiding this comment

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

Right now, we dont have a funciton. One would need to read the code and do custom compute.

The tradeoff you (@Zer0dot ) mentioned is correct if we want to have a function. I also think it is valuable to add one.
which one is better? I am less sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have fields to be 11111 to indicate salt is used and dynamic?

I think the 5267 function eip712Domain()'s fields value only indicates that it is used in the domain calculation, not whether or not it is dynamic. So if we return 1s for the verifyingContract, chainId, and salt, then the caller would assume that the actual values we return in the view func are used to calculate the domain separator, not that they should be dynamically computed from elsewhere.

So, basically I don't think we can be fully compliant with 5267, because the caller will still always need module-specific context, at which point the caller can just use the known EIP712 domain rather than retrieving it via an eth_call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, 5267 compatibility is tricky if routing happens offchain.. Not sure if 5267 compatibility is necessary though, I think its more of a nice to have as the SDK is being used to generate signatures. Since the SDK should know which module it's creating a 1271 signature for, it could call eip712Domain or an equivalent on the module when preparing the signature

return keccak256(
abi.encode(
_DOMAIN_SEPARATOR_TYPEHASH, block.chainid, address(this), bytes32(uint256(uint160(account)))
)
);
}
}
32 changes: 32 additions & 0 deletions src/modules/ReplaySafeWrapper.sol
Original file line number Diff line number Diff line change
@@ -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));
}
}
14 changes: 9 additions & 5 deletions src/modules/validation/SingleSignerValidationModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions test/account/PerHookData.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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)});
Expand Down
12 changes: 10 additions & 2 deletions test/account/UpgradeableModularAccount.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -393,9 +393,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));

Expand Down
9 changes: 7 additions & 2 deletions test/module/SingleSignerValidationModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand Down