Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ERC-6492 #153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

derekchiang
Copy link

Adding support for ERC-6492 using this library.

@obstropolos obstropolos requested a review from w4ll3 March 19, 2023 00:48
@turmeric-blend
Copy link

Hi will this be merged soon? 😇

@wyc
Copy link
Contributor

wyc commented May 6, 2023

Thank you so much for this proposed PR and checking in on it. Apologies on the delay in response.

This PR makes significant changes to the security model, namely relying the following library to perform signature validation instead of ethers, which seems to be a wrapper around ethers which would require another security evaluation:

https://github.com/AmbireTech/signature-validator

Though the library does mention that a formal audit is pending, one is not available yet, and though it suggests that "~80 lines of code in index.js" are readily reviewed easily, it is clear that it creates a dependency on the smart contract expressed here for evaluating the validity across three different signing mechanisms.

There was a formal security audit of this library (siwe), and this change is likely to warrant another audit requirement. So at this point, it doesn't seem like a good idea to move forward on these changes because:

  • It switches the critical path of the security model to an unaudited smart contract without a track record of use nor production testing.
  • The spec doesn't mention EIP-6492 and this specific repository aims to follow the spec without surprises to implementers.

However, I think there are many ways forward to support EIP-6492 along with SIWE, such as evaluating a SIWE message signature against EIP-6492, or even accepting an EOA signature with additional authorizations. You can see how Gnosis Safe support was implemented in SSX as an extension here, on top of SIWE. In general this approach may be better in the short term, but if you feel strongly about inclusion of EIP-6492 then the best path may be to consider a making a case to add it to EIP-4361 to add that as an option alongside EIP-1271, and if those changes are accepted then we could update this repository to follow the spec as not to surprise implementors.

Hope this was helpful, and thanks again for your interest and patience in awaiting a response. I will leave this open for discussion, but close this in 2 weeks if there is no response. Looking forward to identifying ways to collaborate and move SIWE forward while maintaining security and stability for people who use it.

@dokocat
Copy link

dokocat commented May 31, 2023

HI what can the community do to help get this merged?

@ryanshahine
Copy link

@wyc, @derekchiang Any progress on this?

@roy-sandoval
Copy link

Any updates on this?

@jd1900
Copy link

jd1900 commented Jan 13, 2024

Interested on this getting merged.

@Ivshti
Copy link

Ivshti commented Jan 13, 2024

hey @wyc @derekchiang, since this was initially proposed, ERC-6492 has been made final and stable, and it has undergone multiple reviews. There's no formal audit yet, but it is something we're working on.

A few clarifications:

  • 6492 is not a replacement for 1271, it's a superset of it and layer on top of it. It depends on 1271
  • 6492 can broadly be divided into two components
    • the wrap specification: in case a contract is not deployed, the standard defines a wrap specification which wraps the signature in a format that starts with recognizable magic bytes and packs deployment data in it, such as to allow verification
    • the helper contract which allows verification of 6492 (wrapped 1271 signatures), 1271 (pure unwrapped 1271 signatures) and ecrecover signatures - this is meant to make implementation easier but it's not mandatory to use this contract

The helper contract has another "clever" feature, which is that you can use it with eth_call to verify signatures off-chain.

If you already implement 1271 and EOA signatures, adding the support for the wrapping format could be easy. But you'll still need a helper contract to simulate deploying the account for the purposes of the verification, so you might as well rewrite or just use the contract provided with 6492 which also has branches for 1271 and EOA.

A very easy way to minimize both risk and effort on your end is to simply detect the magic bytes and only use the originally provided contract (in 6492) only if the magic bytes are detected as a prefix to the signature, otherwise use your usual branches for pure 1271 and EOA. Once again, given the simplicity of the contract that shouldn't be needed and you can actually drop a lot of complexity from your codebase by going "full 6492".

@estarossa0
Copy link

Any updates on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants