Add canonical address registry contract + fixed circuit #437

Sladuca · 2023-09-05T22:47:44Z

Motivation

Fix canon addr frontrunning vulnerability due to fact that signed message didn't include eth address. Need EIP712 for general replay protection across chains.

Solution

Have circuit take msg directly as PI, then separately compute the msg digest in a replay/frontrunning-resistant manner
Add canon addr registry entry EIP712 hashing
Add CanonicalAddressRegistry contract, unit tests, and e2e tests
Add snap<>fe-sdk methods that allow interface to register addr in mapping

Proof

Unnecessary. Didn't change anything that would break this

PR Checklist

added tests
updated documentation
added changeset if necessary
tested in dev/testnet
tested site with snap (we haven't automated this yet)
re-built & tested circuits if any of them changed
updated contracts storage layout (if contracts were updated)

changeset-bot · 2023-09-05T22:47:47Z

🦋 Changeset detected

Latest commit: ceb405d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages

Name	Type
@nocturne-xyz/frontend-sdk	Minor
@nocturne-xyz/core	Minor
@nocturne-xyz/snap	Minor
@nocturne-xyz/contracts	Minor
@nocturne-xyz/e2e-tests	Minor
@nocturne-xyz/config	Minor
@nocturne-xyz/deploy	Minor
@nocturne-xyz/local-prover	Minor
@nocturne-xyz/circuits	Minor
@nocturne-xyz/idb-kv-store	Patch
@nocturne-xyz/persistent-log	Patch
@nocturne-xyz/bundler	Patch
@nocturne-xyz/deposit-screener	Patch
@nocturne-xyz/insertion-writer	Patch
@nocturne-xyz/subtree-updater	Patch
@nocturne-xyz/test-actor	Patch
@nocturne-xyz/offchain-utils	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Sladuca · 2023-09-05T22:47:55Z

Current dependencies on/for this PR:

main
- PR Add canonical address registry contract + fixed circuit #437 👈

This comment was auto-generated by Graphite.

apps/snap/src/index.ts

packages/frontend-sdk/src/sdk.ts

Sladuca · 2023-09-06T20:33:52Z

packages/contracts/contracts/adapters/WstethAdapter.sol

@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: MIT OR Apache-2.0
+// SPDX-License-Identifier: MIT OR Apache-2.0 OR Apache-2.0


Sladuca · 2023-09-06T20:48:58Z

packages/core/src/snapJsonRpc.ts

-import { CanonAddrSigCheckInputs } from "./proof";
+import { CanonAddress, NocturneSignature, SpendPk, ViewingKey } from "./crypto";
+
+export interface SignCanonAddrRegistryEntry {


nit: SignCanonAddrRegistryEntryMethod

Sladuca · 2023-09-06T20:49:32Z

packages/deploy/src/checks.ts

+  // Deposit manager proxy admin matches deployment proxy admin
+  const depositManagerProxyAdmin = await proxyAdmin(
+    provider,
+    deployment.depositManagerProxy.proxy
+  );
+  assertOrErr(
+    depositManagerProxyAdmin === deployment.proxyAdmin,
+    "deposit manager proxy admin incorrectly set"
+  );


was this not there previously?

Yeah I think it was just forgotten way back when

Sladuca · 2023-09-06T20:51:51Z

packages/e2e-tests/tests/CanonAddrRegistry.ts

+    try {
+      await register(aliceEoa, compressedCanonAddr, proof);
+      throw new Error("invalid proof, should have reverted");
+    } catch {}


nit: use expect().to.eventually.be.rejected

Sladuca · 2023-09-06T20:54:02Z

packages/e2e-tests/tests/CanonAddrRegistry.ts

+      await canonAddrRegistry
+        .connect(aliceEoa)
+        .setCanonAddr(compressedCanonAddr, packToSolidityProof(proof));
+      throw new Error("invalid digest, should have reverted");
+    } catch {}


ditto expect(...).to.eventually.be.rejected

Sladuca · 2023-09-06T20:55:31Z

packages/frontend-sdk/src/sdk.ts

+  "https://frontend-sdk-circuit-artifacts.s3.us-east-2.amazonaws.com/canonAddrSigCheck/canonAddrSigCheck.wasm";
+const CANON_ADDR_SIG_CHECK_ZKEY_PATH =
+  "https://frontend-sdk-circuit-artifacts.s3.us-east-2.amazonaws.com/canonAddrSigCheck/canonAddrSigCheck.zkey";


Oh right I need to upload these - will do right after I finish reviewing

Sladuca · 2023-09-06T20:56:12Z

packages/frontend-sdk/src/sdk.ts

@@ -285,6 +299,47 @@ export class NocturneSdk implements NocturneSdkApi {
    );
  }

+  // TODO: use this method in interface
+  async registerCanonicalAddress(): Promise<ethers.ContractTransaction> {


add to api.ts

clvv · 2023-09-06T21:33:27Z

packages/contracts/contracts/libs/OperationUtils.sol

@@ -1,12 +1,11 @@
-// SPDX-License-Identifier: MIT OR Apache-2.0
+// SPDX-License-Identifier: MIT OR Apache-2.0 OR Apache-2.0


Nit: extra Apache-2.0

clvv · 2023-09-06T21:34:20Z

packages/contracts/contracts/libs/Queue.sol

@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: MIT OR Apache-2.0
+// SPDX-License-Identifier: MIT OR Apache-2.0 OR Apache-2.0


Same as above. Also applies to couple of files below.

clvv · 2023-09-06T21:39:28Z

packages/contracts/contracts/libs/Pairing.sol

@@ -9,7 +9,7 @@
 //      added requiere error messages
 //
 //
-// SPDX-License-Identifier: GPL-3.0
+// SPDX-License-Identifier: MIT OR Apache-2.0


May need to keep this GPL..

But original source is in MIT! https://gist.github.com/chriseth/f9be9d9391efc5beb9704255a8e2989d

clvv · 2023-09-06T21:44:25Z

packages/contracts/contracts/CanonAddrRegistryEntryEIP712.sol

+        bytes32 domainSeparator = _domainSeparatorV4();
+        bytes32 structHash = _hashCanonAddrRegistryEntry(entry);
+
+        bytes32 digest = ECDSAUpgradeable.toTypedDataHash(


Are we using the latest library here? It seems this function has moved to MessageHashUtils in the main branch of openzepplin contracts.

It was the latest published version on npm. We use 4.9.2 but last month came out with 4.9.3. Main is probably not stable

clvv · 2023-09-06T21:46:36Z

packages/contracts/contracts/CanonAddrRegistryEntryEIP712.sol

+        );
+
+        // mod digest by 2^252 to fit compressed addr sign bit in 253rd PI bit
+        return uint256(digest) % MODULUS_252;


This is very minor: but we can also XOR with a mask here. Not sure if we want to keep that convention

If we want to be consistent, we should do uint256(digest) & (1 << 252 - 1)

clvv · 2023-09-06T21:48:30Z

packages/contracts/contracts/CanonAddrRegistryEntryEIP712.sol

+    /// @param entry Canon addr registry entry
+    function _computeDigest(
+        CanonAddrRegistryEntry memory entry
+    ) public view returns (uint256) {


Should we document here that this return a uint256 whose top 4 bits is guaranteed to be zero?

Yep added dev comment

…tion for luke

…fix fe-sdk and snap

…ut are not

…ised

Sladuca marked this pull request as ready for review September 5, 2023 22:58

Sladuca requested review from luketchang and panieldark as code owners September 5, 2023 22:58

Sladuca commented Sep 5, 2023

View reviewed changes

apps/snap/src/index.ts Outdated Show resolved Hide resolved

Sladuca commented Sep 5, 2023

View reviewed changes

packages/frontend-sdk/src/sdk.ts Outdated Show resolved Hide resolved

Sladuca commented Sep 6, 2023

View reviewed changes

clvv reviewed Sep 6, 2023

View reviewed changes

luketchang force-pushed the seb/canon-addr-sig-check-r2 branch from 2aada30 to f23d974 Compare September 6, 2023 23:26

Sladuca and others added 15 commits September 6, 2023 21:19

have circuit take msg directly to avoid replays, leave digest calcula…

d07d75d

…tion for luke

fix typo in comment

713938f

changeset

98fd21b

fix bitmath

c7efb1a

fix posec comment

3cfa9b8

add draft contracts

f2327f5

add script to gen test case to core

2b8f234

add eip712 test case for canon addr registry entry

49d5cff

fix nits

25ca12b

add canonical address registry contract

de795de

remove abicoder v2, tob rec

288e88d

add unit test with mock verifier for canon addr registry

988c918

make all license comments in sol files mit or apache 2

b93ca02

update deploy script and e2e tests to include canon addr registry

bfd5412

rename deposit request hash file in core to deposit request

1432780

luketchang added 16 commits September 6, 2023 21:19

prog: e2e test submits canon addr set tx success

33a0f57

finish success test case for registry

0a41ce2

add rest of e2e tests for registry

67f445f

save snap progress before fixing op digest calc to include canon addr

77fec1b

snap builds with method to sign registry entry

ccf878a

expose fe sdk method for proving and registering addr

98b7398

add compressedCanonAddr to digest, contract unit tests pass, need to …

6fd8ec8

…fix fe-sdk and snap

fix: fe sdk passes canon addr as part of canon addr entry

8823744

fix e2e tests and add canon addr to entry

6323eb9

cleanup

4440ac7

changesets

ff576d6

save progress, bad digest and msg.sender e2e test calls should fail b…

bf22ebf

…ut are not

fix: require verifyProof in addr registry, e2e tests use chai as prom…

10aa163

…ised

post rebase yarn i

17fd739

use mask instead of mod 2^252

b34df46

rebase

ceb405d

luketchang force-pushed the seb/canon-addr-sig-check-r2 branch from 703d2c1 to ceb405d Compare September 7, 2023 01:21

luketchang changed the title ~~CanonAddrSigCheck takes msg directly as PI~~ Add canonical address registry contract + fixed circuit Sep 7, 2023

luketchang approved these changes Sep 7, 2023

View reviewed changes

luketchang enabled auto-merge (squash) September 7, 2023 01:33

luketchang merged commit 77c4063 into main Sep 7, 2023
3 checks passed

luketchang deleted the seb/canon-addr-sig-check-r2 branch September 7, 2023 02:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add canonical address registry contract + fixed circuit #437

Add canonical address registry contract + fixed circuit #437

Sladuca commented Sep 5, 2023 •

edited by luketchang

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

Sladuca commented Sep 5, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

luketchang Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

Sladuca Sep 6, 2023

clvv Sep 6, 2023

clvv Sep 6, 2023

clvv Sep 6, 2023

clvv Sep 6, 2023

clvv Sep 6, 2023

luketchang Sep 6, 2023 •

edited

Loading

clvv Sep 6, 2023

Sladuca Sep 6, 2023 •

edited

Loading

clvv Sep 6, 2023

luketchang Sep 6, 2023

		@@ -1,4 +1,4 @@
		// SPDX-License-Identifier: MIT OR Apache-2.0
		// SPDX-License-Identifier: MIT OR Apache-2.0 OR Apache-2.0

		@@ -1,12 +1,11 @@
		// SPDX-License-Identifier: MIT OR Apache-2.0
		// SPDX-License-Identifier: MIT OR Apache-2.0 OR Apache-2.0

Add canonical address registry contract + fixed circuit #437

Add canonical address registry contract + fixed circuit #437

Conversation

Sladuca commented Sep 5, 2023 • edited by luketchang Loading

Motivation

Solution

Proof

PR Checklist

changeset-bot bot commented Sep 5, 2023 • edited Loading

🦋 Changeset detected

Sladuca commented Sep 5, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luketchang Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sladuca Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sladuca commented Sep 5, 2023 •

edited by luketchang

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

luketchang Sep 6, 2023 •

edited

Loading

Sladuca Sep 6, 2023 •

edited

Loading