Refactor/email auth simplification #104

zkfriendly · 2025-01-14T12:00:45Z

Description

This PR introduces a new simplified EmailAuthSigner contract that provides a signature-like authentication mechanism using emails. Unlike the existing EmailAuth contract which handles nullifiers internally, this new contract is designed to be used like a signature verification mechanism where the calling contract manages its own replay protection.

Key changes:

Added new EmailAuthSigner.sol contract
Created IEmailAuth.sol interface to standardize common functionality
Moved EmailAuthMsg struct to the interface
Updated existing contracts to implement the interface
Added comprehensive tests for the new signer contract

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Added new test files and cases:

EmailAuthSigner.t.sol with comprehensive test coverage including:
- Basic initialization and configuration
- DKIM registry updates
- Verifier updates
- Email authentication
- Error cases and invalid inputs
- Upgrade functionality
- Template ID validation

All existing tests have been updated to work with the interface changes and continue to pass.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…and tests to use the interface

…and replay protection

openzeppelin-code · 2025-01-14T12:03:57Z

Refactor/email auth simplification

Generated at commit: 5ee69daf5667acf8f11865eca28acc141d1b7296

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 6 28 36
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

…ure validation

… update signature validation logic

…te EmailAuthSigner import

SoraSuegami · 2025-01-28T03:56:26Z

@zkfriendly Thank you for your PR!
Could you minimize duplicated codes in EmailAuthSigner.sol?
I think the EmailAuthSigner can inherit the EmailAuth implementation.
Please feel free to add more functions to EmailAuth if you want/need.
However, it is ideal if all of the existing functions/structs in EmailAuth remain same as the current ones.

zkfriendly · 2025-01-28T07:58:27Z

Thank you for the review.

I think the EmailAuthSigner can inherit the EmailAuth implementation

The main goal of this PR is to provide a simpler primitive that can be used everywhere. For context, there is already an implementation here that doesn’t modify the EmailAuth library; instead, it uses it internally while exposing a much simpler interface. However, after reviewing it, @JohnGuilding suggested that we should aim to simplify the underlying EmailAuth library as well.

Please feel free to add more functions to EmailAuth if you want/need.

Given our goal and the fact that an implementation with a simpler interface already exists, it seems that if cloning and modifying EmailAuth is not an option, we can simply discard this PR and adopt the other implementation that builds upon the existing EmailAuth. cc @JohnGuilding

it is ideal if all of the existing functions/structs in EmailAuth remain same as the current ones.

I agree. Even with the changes in this PR, the functions and structs in EmailAuth remain unchanged; code organization is a bit different to prevent duplicate code, but everything stays the same.

JohnGuilding

Cheers @zkfriendly left some comments.

Let's jump on a call tomorrow if we need to discuss the 1271 signature comments some more

JohnGuilding · 2025-01-28T10:34:21Z

package.json

+  },
+  "packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"


why was this needed?

This is not strictly needed and can be removed safely. That said, benefits are mainly consistency across environments and corepack compatibility

JohnGuilding · 2025-01-28T10:40:49Z

packages/contracts/src/EmailAuthSigner.sol

+/// @dev Unlike EmailAuth.sol which handles nullifiers internally, this contract is designed
+/// to be used like a signature verification mechanism where the calling contract manages
+/// its own replay protection.
+contract EmailAuthSigner is


Thoughts an EmailSigner?

JohnGuilding · 2025-01-28T10:42:34Z

packages/contracts/src/EmailAuthSigner.sol

+    /// The templateId of the sign hash command.
+    uint256 public templateId;
+
+    constructor() {}


We should add _disableInitializers() here https://docs.openzeppelin.com/contracts/5.x/api/proxy#Initializable-_disableInitializers--

Since it's a security feature, I think it's worth adding to EmailAuth constructor too

JohnGuilding · 2025-01-28T10:44:40Z

packages/contracts/src/EmailAuthSigner.sol

+        templateId = _templateId;
+        require(
+            _dkimRegistryAddr != address(0),
+            "invalid dkim registry address"


As discussed in breakout call, lets move to use custom errors if you're in agreement

JohnGuilding · 2025-01-28T10:47:01Z

packages/contracts/src/ERC1271SignatureValidator.sol

+
+    // Magic value returned by older versions of EIP1271 when validating data and signature
+    // bytes4(keccak256("isValidSignature(bytes,bytes)")). Used by Gnosis Safe and others.
+    bytes4 internal constant EIP1271_MAGIC_VALUE_DATA = 0x20c13b0b;


Nit: thoughts on this naming convention to make it explicit https://github.com/safe-global/safe-modules/blob/main/modules/passkey/contracts/libraries/ERC1271.sol

nice yes, we should do that.

JohnGuilding · 2025-01-28T10:51:47Z

packages/contracts/src/ERC1271SignatureValidator.sol

+
+contract ERC1271SignatureValidator {
+    /// Mapping to store if a hash has been signed.
+    mapping(bytes32 => bool) public isHashSigned;


Where is this updated?

What do you think about verifying proof etc directly in isValidSignature? And state updates can be handled in higher level logic such as how contract signatures work on Safes https://github.com/safe-global/safe-smart-account/blob/1c8b24a0a438e8c2cd089a9d830d1688a47a28d5/contracts/Safe.sol#L140-L142

This way the EmailSigner can be used to verify "signatures" in a single step rather than approve + verify

Where is this updated?

this part is actually not implemented here yet. And I think your second point is a great idea for how to build this.

JohnGuilding · 2025-01-28T10:54:05Z

packages/contracts/src/EmailAuthSigner.sol

+    /// handling replay protection. The calling contract should implement its own mechanisms
+    /// to prevent replay attacks, similar to how nonces are used with ECDSA signatures.
+    /// @param emailAuthMsg The email auth message containing all necessary information for authentication.
+    function authEmail(EmailAuthMsg memory emailAuthMsg) public {


Related to above comment on 1271 validation - thinking that verifySignature is executing most of this logic

zkfriendly added 5 commits January 14, 2025 11:28

chore: explicitly define packag manager

352acc8

feat: simplified email signer authenticator

a2e4cd9

feat: implement IEmailAuth interface and refactor EmailAuth contract …

45dd9a6

…and tests to use the interface

feat: integrate templateId into EmailAuthSigner and update related tests

ec5df70

docs: enhance EmailAuthSigner documentation for signature-like usage …

8b41189

…and replay protection

zkfriendly requested a review from JohnGuilding January 14, 2025 12:00

zkfriendly requested a review from SoraSuegami January 14, 2025 12:05

zkfriendly added 4 commits January 27, 2025 15:03

feat: ISignatureValidator with support for _data, _signature

1859c24

feat: update ISignatureValidator to support both data and hash signat…

7cca7cb

…ure validation

feat: refactor EmailAuthSigner to inherit from SignatureValidator and…

51a99b2

… update signature validation logic

feat: rename SignatureValidator to ERC1271SignatureValidator and upda…

5ee69da

…te EmailAuthSigner import

JohnGuilding reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/email auth simplification #104

Refactor/email auth simplification #104

zkfriendly commented Jan 14, 2025

openzeppelin-code bot commented Jan 14, 2025 •

edited

Loading

SoraSuegami commented Jan 28, 2025

zkfriendly commented Jan 28, 2025

JohnGuilding left a comment

JohnGuilding Jan 28, 2025

zkfriendly Jan 28, 2025

JohnGuilding Jan 28, 2025

JohnGuilding Jan 28, 2025

JohnGuilding Jan 28, 2025

JohnGuilding Jan 28, 2025

JohnGuilding Jan 28, 2025

zkfriendly Jan 28, 2025

JohnGuilding Jan 28, 2025

zkfriendly Jan 28, 2025

JohnGuilding Jan 28, 2025

		},
		"packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"

Refactor/email auth simplification #104

Are you sure you want to change the base?

Refactor/email auth simplification #104

Conversation

zkfriendly commented Jan 14, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

openzeppelin-code bot commented Jan 14, 2025 • edited Loading

Refactor/email auth simplification

🚨 Report Summary

SoraSuegami commented Jan 28, 2025

zkfriendly commented Jan 28, 2025

JohnGuilding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openzeppelin-code bot commented Jan 14, 2025 •

edited

Loading