Fix/eip712

[retocrooman]

EIP712のdomain separatorの修正

現状の問題点

ThirdWebを用いてJPYCv2のpermit機能を使用しようとしたところ失敗した。原因を究明したところJPYCv2のdomain separatorに問題があることが分かった。

async function getDomainSeperator(signer: Signer, contractAddress: string) {
  const contract = new Contract(contractAddress, DOMAIN_SEPARATOR_ABI, signer);
  try {
    return await contract.DOMAIN_SEPARATOR();
  } catch (err) {
    return await contract.getDomainSeperator();
  }
}

https://github.com/thirdweb-dev/typescript-sdk/blob/210ec0a609ad57d5e06bde22b349bba88793a40e/src/common/permit.ts#L69-L76
ThirdWebでは上記のような方法でdomain separatorを取得しているがJPYCにはDOMAIN_SEPARATORやgetDomainSeperatorという名前の関数は実装されていない(USDCはDOMAIN_SEPARATOR()を実装している)。なのでランタイムエラーになり失敗していると考えられる。ThirdWebの実装をよく見てみると

const contractDomainSeparator = await getDomainSeperator(
    signer,
    domain.verifyingContract,
  );

  const polygonDomain: EIP712Domain = {
    name: domain.name,
    version: domain.version,
    verifyingContract: domain.verifyingContract,
    salt: ethers.utils.hexZeroPad(
      BigNumber.from(domain.chainId).toHexString(),
      32,
    ),
  };

  if (
    ethers.utils._TypedDataEncoder.hashDomain(polygonDomain) ===
    contractDomainSeparator
  ) {
    return polygonDomain;
  }

  return domain;

https://github.com/thirdweb-dev/typescript-sdk/blob/210ec0a609ad57d5e06bde22b349bba88793a40e/src/common/permit.ts#L181-L186
このようにdomain separatorを計算しているのだが、実際に一致しているかどうか調べているのはpolygonだけという妙な実装になっている。またversionも"1"で固定されている点も留意すべきである（USDCのversionは"2"だがUSDCには対応していない？）。

  const domain = await getChainDomainSeperator(signer, {
    name: await getTokenName(signer, currencyAddress),
    version: "1",
    chainId: await signer.getChainId(),
    verifyingContract: currencyAddress,
  });

https://github.com/thirdweb-dev/typescript-sdk/blob/210ec0a609ad57d5e06bde22b349bba88793a40e/src/common/permit.ts#L181-L186
*ThirdWebではなぜかseparatorではなくseperatorを使っている。

解決方法

USDCに倣いDOMAIN_SEPARATORという名前の関数を実装する。現状同機能の_domainSeparatorV4()をinternalにし、DOMAIN_SEPARATOR()でラップする。また、USDCで実装しているversion()も念のため追加する。

テスト

ローカルネットでテストをした。v1のテストと同じものをやったが、新たに追加した機能であるDOMAIN_SEPARATOR()とversion()のテストを追加し、_domainSeparatorV4()のテストを削除した。ストレージを一つずつ確認するテストもv1と同じものをクリアした。

変更箇所

v1_1のディレクトリを新しく作成し、そこに変更したEP712Domainとパスだけ変更したEIP2612, EIP3009とFiatTokenV1を基にしたFiatTokenV1_1を置いた。それに伴い、新しくテストディレクトリv1_1とテストファイルを作成した。また、README.mdを必要な箇所更新し、その他も適宜修正した。

やっていないこと

• ThirdWebでUSDCでガスレスができるかどうかのテスト。
• goerliでのテスト。
• ThirdWebでガスレスができるかどうかのテスト。
• v1からv1_1にアップグレードするスクリプトの用意。
• アップグレードさせるコントラクトの用意（今回はマイナーチェンジなので不要では）。

[retocrooman]

uniswap/interfaceではこのリストのみ対応しているっぽいです。

// todo: read this information from extensions on token lists or elsewhere (permit registry?)
const PERMITTABLE_TOKENS: {
  [chainId: number]: {
    [checksummedTokenAddress: string]: PermitInfo
  }
} = {
  1: {
    [USDC_MAINNET.address]: { type: PermitType.AMOUNT, name: 'USD Coin', version: '2' },
    [DAI.address]: { type: PermitType.ALLOWED, name: 'Dai Stablecoin', version: '1' },
    [UNI[1].address]: { type: PermitType.AMOUNT, name: 'Uniswap' },
  },
  4: {
    '0xc7AD46e0b8a400Bb3C915120d284AafbA8fc4735': { type: PermitType.ALLOWED, name: 'Dai Stablecoin', version: '1' },
    [UNI[4].address]: { type: PermitType.AMOUNT, name: 'Uniswap' },
  },
  3: {
    [UNI[3].address]: { type: PermitType.AMOUNT, name: 'Uniswap' },
    '0x07865c6E87B9F70255377e024ace6630C1Eaa37F': { type: PermitType.AMOUNT, name: 'USD Coin', version: '2' },
  },
  5: {
    [UNI[5].address]: { type: PermitType.AMOUNT, name: 'Uniswap' },
  },
  42: {
    [UNI[42].address]: { type: PermitType.AMOUNT, name: 'Uniswap' },
  },
}

https://github.com/Uniswap/interface/blob/247fbfdf01cf1c344cb09c8a59d70671ae9ece27/src/hooks/useERC20Permit.ts#L30-L55

[yuki-js]

修正を拒否します。
現状のコントラクトはEIP-712に準拠しています。そもそも、現在のEIP-712仕様と、以前のEIP-712仕様ではdomainSeparatorの計算ルールが異なっており、その結果、異なるトークン間でdomainSeparatorの計算ルールが異なっており、それぞれのトークンごとにdomainSeparatorを計算するようになっています。

例: Polygon PoSトークンコントラクトはフィールドの順序が異なります。

この問題を解決するために、EIP-5267が提起されました。現在Last Callで、2023-01-23までにFinalになる見込みです。もし修正をするなら、EIP-5267のための修正にするべきです。

個人的には、EIP-5267がすぐに浸透するとも思いません。なので、ThirdWebにJPYCを追加してもらうようにお願いしたほうが早そうです。

ref: https://ethereum-magicians.org/t/eip-5267-retrieval-of-eip-712-domain/9951/7

[retocrooman]

ありがとうございます。
現状そういう対応になっているんですね。domainSeparatorの計算ルールは違うというものの、EIP2612に必要なDOMAIN_SEPARATORの関数がなく、EIP2612の仕様に合わせる必要はあるのではないでしょうか？
あと、EIP-5267の理解が正しいか分かりませんが、下位互換性があり、domainSeparatorの計算方法を明示するEIP712の拡張規格という認識でDOMAIN_SEPARATORの値が変わるわけではないですよね？

[yuki-js]

EIP2612の仕様に合わせる必要はあるのではないでしょうか？

EIP-2612の話でしたか。てっきりEIP-712とEIP-3009に関係する話だと勘違いしてました。
たしかにEIP-2612には準拠していないと言えますね…
ただし、domainSeparatorの計算は署名者自身が行うことが理想ではあるので、ThirdWebやUniswap Interfaceなど特定少数のソフトウェアで動作すればいいのなら、それらを直せば正直無理に修正する必要もないのかなと思います。
ところでcode4renaで指摘されなかったんですか？

EIP-5267の理解が正しいか分かりませんが、下位互換性があり、domainSeparatorの計算方法を明示するEIP712の拡張規格という認識でDOMAIN_SEPARATORの値が変わるわけではないですよね？

はい、下位互換性はあり、DOMAIN_SEPARATORの計算方法を変更する必要はありません。ただし、versionを変更し、未使用の電子署名を無効化する必要はあるかもしれません。

[yuki-js]

[推奨]EIP-2612に準拠するという意味ならここではなく、EIP2612.solに置くべきです。

[retocrooman]

EIP-2612の話でしたか。てっきりEIP-712とEIP-3009に関係する話だと勘違いしてました。

EIP712にはDOMAIN_SEPARATOR関数についての記述がないなと思っていましたが、EIP-5267を読んでEIP2612で指定されていたことに気づきました。

ただし、domainSeparatorの計算は署名者自身が行うことが理想ではあるので、ThirdWebやUniswap Interfaceなど特定少数のソフトウェアで動作すればいいのなら、それらを直せば正直無理に修正する必要もないのかなと思います。

自分はThirdWebとUniswapしか調べていませんが、個別対応が普通であれば同感ですね。

ところでcode4renaで指摘されなかったんですか？

code4renaでハードフォークする際にDOMAIN_SEPARATORが変わることを指摘されて、変更を加えた際に規格に沿わなくなってしまいました。修正内容はcode4renaに見てもらっていませんでした…

はい、下位互換性はあり、DOMAIN_SEPARATORの計算方法を変更する必要はありません。ただし、versionを変更し、未使用の電子署名を無効化する必要はあるかもしれません。

承知しました。ありがとうございます。

[推奨]EIP-2612に準拠するという意味ならここではなく、EIP2612.solに置くべきです。

DOMAIN_SEPARATORはEIP-3009でも使いますし、USDC同様別ファイルでもいいと思いますが問題ありますでしょうか？

[yuki-js]

DOMAIN_SEPARATORが変わることを指摘

EVM命令 CHAINID の返値が変化したときの話ね、見ました。その時に修正をミスったってことですかね。うーん…

USDC同様別ファイルでもいいと思いますが

1. Compliant contracts must implement 3 new functions in addition to EIP-20: と言っている
2. 英語話者の間において 、 一般的には mustは「しなければならない」という意味であると解される

• なぜこんな書き方をしているかというと、EIP-2612には、mustの解釈に関する客観的な指示がないからである。
• 通常は、RFC 2119とRFC 8174に従う旨を書き、大文字にする。
• 例: https://github.com/AokiApp/EIPs/blob/8095428c1c8e3dbc6fd91d156737c278f605a053/EIPS/eip-6268.md?plain=1#L24

3. なので、DOMAIN_SEPARATOR と EIP-2612 は不可分である
4. また、 DOMAIN_SEPARATOR は EIP-2612以外で使われるとは限らない。

• domainSeparator と DOMAIN_SEPARATOR は別物であり、前者はルールの呼び名であり、後者は関数(定数)のシグネチャである
• 内部で利用するための参照経路として、すでに _domainSeparatorV4 が存在している

なので、同じファイルにあるほうが、コンパイル結果は同じでも、可読性が良く、影響範囲が少ない。

[retocrooman]

EVM命令 CHAINID の返値が変化したときの話ね、見ました。その時に修正をミスったってことですかね。うーん…

おっしゃる通りです…

...なので、同じファイルにあるほうが、コンパイル結果は同じでも、可読性が良く、影響範囲が少ない。

おお、丁寧にありがとうございます！しっくりきました。OpenZeppelinでもそうなっていますね！