feat(SingleSignerValidation): add replay safe hash and utility contracts for EIP-712 in modules [2/2]

[adamegyed]

Motivation

The current SingleSignerValidation module uses the raw owner signature to implement 1271 for the account. This is unfortunately vulnerable to replay across multiple accounts owned by the same owner (which can be an EOA key or another contract account).

It would also be beneficial to have reusable building blocks for the workflow of creating module-specific EIP-712 struct hashes, and replay-safe wrappers for 1271 signature validation.

Solution

Create two new abstract contracts (mixins):

• ModuleEIP712 provides a way to calculate an EIP-712 domain separator for modules, that incorporates both the module address and the account address. This does not define any EIP-712 structs on its own.
• ModuleEIP1271, that uses the ModuleEIP712 style domain separator and an EIP-712 struct ReplaySafeHash(bytes32 hash) to create replay-safe 1271 signatures for modules.

Use the ModuleEIP1271 mixin for SingleSignerValidation, and update tests to expect the replay-safe hash.

Rationale

We are unfortunately unable to use the EIP712 mixin contracts from libraries like OpenZeppelin and solady because they don't provide a way for us to add the account address to the domain separator. This is a problem that is unique to modular accounts, specifically when the modules are not called via delegatecall, because the result of address(this) in the module will be the module's address, not the accounts.

Furthermore, we can make other safe assumptions about how EIP-712 structs will be used in modules. We can safely omit the name and version fields, because of the design assumption that modules are immutable singletons, therefore any version upgrades will result in a new module address. The name field can be omitted from the domain separator because messages of different struct types can have different struct names, and the non-upgradability of modules prevents these struct declarations from changing.

The account address is included in the salt portion simply because there is no other location to store it.

Additionally, each of these features is implemented as a separate abstract contract because there may be other modules that wish to do module-specific EIP-712 hashing, or using the replay-safe 1271 wrapper, than just the sample SingleSignerValidation. The module-specific EIP-712 hashing may also be useful for things other than 1271 wrapping, for example, a signature-base account recovery module could use this.

To discuss

As noted in a comment, this implementation chooses not to expose and eip712Domain() function because it depends on the account address to calculate the domain. We could have opted to use msg.sender, as would be the case when the module is used, however this would be a new assumption that isn't communicated to other users, libraries, or tools that depend on ERC-5267.

One possible solution is to have the eip712Domain() function revert if msg.sender == address(0), indicating to users that they must pay attention to the parameters sent to eth_call, but this is again non-standard behavior.

[fangting-alchemy]

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

[adamegyed]

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().

[fangting-alchemy]

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.

[adamegyed]

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.

[fangting-alchemy]

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.

[fangting-alchemy]

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?

[Zer0dot]

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?

[fangting-alchemy]

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.

[adamegyed]

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.

[howydev]

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

[adamegyed]

Note: Since rebasing after #123, the tests that wrap using replaySafeHash started to fail because the SemiModularAccount doesn't have an equivalent replay-safe hash wrapper. That would require a different EIP-712 struct because there is no module address, so for now I left that as a TODO in the tests within this pr.

[howydev]

LGTM

[howydev]

small QQ: is it possible to combine the ModuleEIP712 and ReplaySafeHash files? Are there cases which we'll use 1 without the other?

[adamegyed]

Are there cases which we'll use 1 without the other?

I think so, yeah. A recovery module would probably want to use the Module-style EIP712 domain separator calculation, but with a different EIP-712 struct to hash and sign. I thought it'd be easier to separate now, than to try to refactor later.