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

Issue and potential solutions for EIP712 support #3643

Open
peekpi opened this issue Apr 8, 2021 · 5 comments
Open

Issue and potential solutions for EIP712 support #3643

peekpi opened this issue Apr 8, 2021 · 5 comments
Assignees
Labels
design Design and architectural plans/issues

Comments

@peekpi
Copy link
Contributor

peekpi commented Apr 8, 2021

Summary

When calculating a EIP712 transaction hash, the uint256 chainID may be used. This field is optional in EIP712, but it is used in most EIP712 dApps.
The problem is that harmony has two chainID, one is the original chainID and the other is added later for Ethereum format transactions.
Thie may cause EIP712 transactions to fail because the EIP712 contract uses the original chainID, but Metamask uses the Ethereum chainID.
For example, a typical code for getting a chainID in an EIP712 contract would look like this:

function getChainId() internal pure returns (uint) {
    uint256 chainId;
    assembly { chainId := chainid() }
    return chainId;
}

In mainnet it will return 1, then the contract uses the result to verify EIP712 transactions. But when Metamask generates an EIP712 transaction, it uses the Ethereum ChainID which is 1666600000 for the mainnet. Then the transaction commits to the contract and verifies fails.

Proposal

To avoid this issue, you can modify either the contract code or the front-end code.
For the contract, you can change it like follows:

function getChainId() public view returns (uint256) {
    uint256 chainId;
    assembly { chainId := chainid() }
    uint256 eth_id = chainId;
    if (chainId == 1) // mainnet
        eth_id = 1666600000;
    else if (chainId == 2) // testnet
        eth_id = 1666700000;
    return eth_id;
}

For the front-end, you need to change the Ethereum chainID to Harmony chainID:

EIP712 requires the wallet refuse to sign if the chainID in the typedData not match the currently active chain. So, change frontend doesn't work.

function toHarmonyID(chainID) {
    if(chainID == 1666600000) return 1;
    if(chainID == 1666700000) return 2;
}
@peekpi peekpi added the design Design and architectural plans/issues label Apr 8, 2021
@gupadhyaya
Copy link
Contributor

gupadhyaya commented Apr 8, 2021

@peekpi what is the front-end you are referring to?

changing the contract code as mention will result in compatibility to only metamask. harmony one wallet will not work.

@rlan35
Copy link
Contributor

rlan35 commented Apr 8, 2021

Another solution is to add hard fork in the chain to return eth-compatible chainID (1666600000...) for eth-compatible transactions.

@rlan35 rlan35 changed the title Issue for EIP712 dApps Issue and potential solutions for EIP712 support Apr 8, 2021
@gupadhyaya
Copy link
Contributor

Another solution is to add hard fork in the chain to return eth-compatible chainID (1666600000...) for eth-compatible transactions.

this is not possible right? currently the chainid() in assembly { chainId := chainid() } reads the network chainconfig parameter, which is not parameterized for transaction types.

@peekpi
Copy link
Contributor Author

peekpi commented Apr 9, 2021

@peekpi what is the front-end you are referring to?

changing the contract code as mention will result in compatibility to only metamask. harmony one wallet will not work.

you are right. actually, EIP712 requires that the wallet refuse to sign if the chainID in the typedData does not match the chainID in the wallet. so, change the frontend is not feasible.

@naiemk
Copy link

naiemk commented Dec 23, 2021

There are a few issues with the proposed change. Ideally the block.chainid should be fully compatible with EVM specs to maintain full ethereum compatibility.

For smart contracts that utilize harmony specific chain id, a better solution could be to introduce a new field, such as harmonyChainId. Such contracts are explicitly interested in harmony-specific logic so the concerns will be better separated.

  1. Breaks full EVM compatibilty
  2. Multi-chain Dapps cannot get the same contract address on Harmony (with CREATE2)
  3. Security concerns. Many dApps duplicate across chains. So there is a chance that dapps using EIP1772 deploy their contracts, and experience signature replay attack.
  4. Audited and widely used Openzepplin contracts won't work on harmony and dapps may be force to use unaudited code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design and architectural plans/issues
Projects
None yet
Development

No branches or pull requests

4 participants