Audit fixes 1-4 and BLSWalletWrapper changes #350

jzaki · 2022-10-04T21:23:17Z

What is this PR doing?

Fixes audit points 1-4, and the latest commit has updates to BLSWalletWrapper to handle changing addresses referred to by a key.

How can these changes be manually tested?

yarn hardhat test inside contracts

Does this PR resolve or contribute to any issues?

Related to PR #340
closes #287

Checklist

I have manually tested these changes
Post a link to the PR in the group chat

Guidelines

If your PR is not ready, mark it as a draft
The resolve conversation button is for reviewers, not authors
- (But add a 'done' comment or similar)

… (01)

voltrevo · 2022-10-05T03:56:29Z

contracts/clients/src/BlsWalletWrapper.ts

  // eslint-disable-next-line camelcase
  VerificationGateway__factory,
-} from "../typechain";
+} from "../../typechain";


This is reaching out to contracts/typechain which is incorrect. contracts/clients needs to be self-contained so that it can be published on npm. contracts/clients/typechain in the repo is a symlink to contracts/typechain. (Replacing the symlink dir with the actual content occurs during build.)

Addressed ~~in #351~~

voltrevo · 2022-10-05T04:05:53Z

contracts/clients/src/BlsWalletWrapper.ts

-    const walletContract = BLSWallet__factory.connect(
-      contractAddress,
-      provider,
+    await blsWalletWrapper.syncWallet(verificationGateway);


I think the syncWallet here is redundant since the latest address information is gathered above during await BlsWalletWrapper.BLSWallet(privateKey, verificationGateway)?

This is currently only being used in the should recover before bls key update contracts test when recovering to a different key, so this still may be needed during the recovery workflow to keep non-async properties of BlsWalletWrapper up to date with their on-chain values.

Confirmed, removing that syncWallet call fails the test Error: VM Exception while processing transaction: reverted with reason string 'VG: Sig not verified'

voltrevo · 2022-10-05T04:08:39Z

contracts/clients/src/signer/verify.ts

@@ -25,6 +25,8 @@ export default (domain: Uint8Array, chainId: number) =>
        BigNumber.from(n2).toHexString(),
        BigNumber.from(n3).toHexString(),
      ]),
-      bundle.operations.map(encodeMessageForSigning(chainId)),
+      bundle.operations.map((op, i) =>


Nitpick: Unused variable i

voltrevo · 2022-10-05T04:18:59Z

contracts/contracts/VerificationGateway.sol

        for (uint256 i=0; i<32; i++) {
+            if (i<4) {
+                selectorId |= bytes4(encodedFunction[i]) >> i*8;
+            }


Are you sure this is the correct shift direction? Usually you'd shift left in this situation (to move a byte into the more significant location, since <<, >> follows big endian convention, even on modern architectures that are usually little endian).

I can see how it might be inverted for bytes4... curious.

It might be better to use uint32 for the arithmetic and then cast to bytes4 after?

voltrevo · 2022-10-05T04:21:37Z

contracts/shared/helpers/Fixture.ts

-  bundleFrom(
+  async bundleFrom(
    wallet: BlsWalletWrapper,
    contract: Contract,
    method: string,
    params: any[],
    nonce: BigNumberish,
    ethValue: BigNumberish = 0,
-  ): Bundle {
+  ): Promise<Bundle> {


Unnecessary async conversion here.

voltrevo · 2022-10-05T04:22:04Z

contracts/shared/helpers/Fixture.ts

@@ -187,7 +189,14 @@ export default class Fixture {
  ) {
    await (
      await this.verificationGateway.processBundle(
-        this.bundleFrom(wallet, contract, method, params, nonce, ethValue),
+        await this.bundleFrom(


After fixing bundleFrom, you shouldn't need await here.

voltrevo · 2022-10-05T04:22:12Z

contracts/shared/helpers/Fixture.ts

@@ -201,7 +210,7 @@ export default class Fixture {
    ethValue: BigNumberish = 0,
  ) {
    return await this.verificationGateway.callStatic.processBundle(
-      this.bundleFrom(wallet, contract, method, params, nonce, ethValue),
+      await this.bundleFrom(wallet, contract, method, params, nonce, ethValue),


voltrevo

A few things, mostly minor.

voltrevo · 2022-10-05T04:39:02Z

How can these changes be manually tested?

yarn hardhat test inside contracts

I don't see any new tests covering the critical changes that have been made:

A bundle signed for a different address cannot be replayed onto a wallet with the same bls key
VG disallows calling transferOwnership and renounceOwnership

Not that we're strict about always automating these things, but they should be manually tested. Have you manually tested them? (In particular, my question about the shift direction could be resolved by a manual test.)

jacque006 · 2022-10-05T21:34:10Z

Two ways I can see us finishing this up:

Merge as is into contract-updates, open a PR from that to main and address remaining comment(s) and client integrations in contract-updates branch/PR.
Change the target of this PR to main and address remaining comment(s) & client integrations here.

I am fine with either.

Update aggregator BundleService to work with new signature payload which includes wallet address.

jacque006 · 2022-10-05T23:26:00Z

contracts/clients/package.json

@@ -1,6 +1,6 @@
 {
  "name": "bls-wallet-clients",
-  "version": "0.7.3",
+  "version": "0.8.0",


@voltrevo Given contract changes, especially signed payload format w/ wallet address I bumped the minor to indicate a breaking change.

jacque006 · 2022-10-05T23:36:35Z

Remaining Tasks

@jzaki additional updates to contracts & tests. TODO Was this done?

Manual Testing

~~Regression test extension, which should also test aggregator.~~ Will do in PR to main

Automated testing (contracts)

Add test case(s) for A bundle signed for a different address cannot be replayed onto a wallet with the same bls key
Add test case(s) for VG disallows calling transferOwnership & renounceOwnership
Add test case for VG: first param to proxy admin is not calling wallet

~~Post Merge~~ Will do in PR to main

Tag new repo release (assuming all audit issues are addressed)
Publish and pin in repo bls-wallet-clients@0.8.0
Deploy contract updates to Goerli Arbitrum
Update other projects/repos?

blakecduncan · 2022-10-21T11:32:29Z

Two ways I can see us finishing this up:

Merge as is into contract-updates, open a PR from that to main and address remaining comment(s) and client integrations in contract-updates branch/PR.

Change the target of this PR to main and address remaining comment(s) & client integrations here.

I am fine with either.

@jacque006 Was there a decision made on how to move forward with this PR? Asking because I'll need the BLSWalletWrapper changes in order to get wallet recovery working end to end. Also, if there's anywhere I can help out with this let me know. I'm happy to jump in anywhere if I can.

jacque006 · 2022-10-24T19:59:24Z

@blakecduncan Yes, it was decided to merge into contract-updates, and from there then merge into main after more through regression testing/other changes.

If you need the wrapper changes now, you can use bls-wallet-clients@0.8.0-2d28b7b

Would love your help testing the extension when we move to testing before main

…udit

jacque006 · 2022-10-24T22:28:27Z

contracts/test/recovery-test.ts

@@ -248,4 +285,52 @@ describe("Recovery", async function () {
        .recoverWallet(addressSignature, hashAttacker, salt, wallet1Key),
    ).to.be.rejectedWith("VG: Signature not verified for wallet address");
  });
+
+  it("should NOT allow a bundle to be executed on a wallet with the same BLS pubkey but different address (replay attack)", async function () {


I believe this covers A bundle signed for a different address cannot be replayed onto a wallet with the same bls key test case that was needed, but it is difficult to tell as this key switch via recovery will de-register the first wallet with that pubkey.

Add getOperationResults to bls-wallet-clients to allow consumers to more easily get at operation errors. Change existing test case to use getOperationResults. Add message to require used to prevent ownership changes to proxy admin.

jacque006 · 2022-10-27T03:21:37Z

#366 will be pulled in to replace latest commit after merge to main

jacque006 · 2022-10-27T17:19:37Z

After discussion with @jzaki , decided to merge early to contract-updates. Will continue testing additions & client integration there.

jzaki added 5 commits October 4, 2022 22:14

Sign key hash with chainid and UserOp - protect from rogue key attack…

a485d84

… (01)

Increment nonce before operation - protect from relpay reentrancy (02)

e2ee017

Restrict Owner functions from ProxyAdmin call (03)

192db97

Sign wallet address in msg (04) - WIP

4147726

Sign wallet address in msg (04) tests

f0a3b56

jzaki requested a review from jacque006 October 4, 2022 21:23

github-actions bot added clients contracts Smart contract related labels Oct 4, 2022

jacque006 added 5 commits October 4, 2022 16:06

WIP Fix client tests, run agg tests on contract changes

998ee6b

WIP Remove main branch requirement to run CI steps

5a33e43

WIP Add back in main restriction for push workflow trigger

def1eaf

WIP Add build test to client CI process

17052cd

WIP Fix typechain import path

1481c69

voltrevo reviewed Oct 5, 2022

View reviewed changes

voltrevo suggested changes Oct 5, 2022

View reviewed changes

jzaki added 2 commits October 5, 2022 15:43

Minor fixes from review

f2a490a

Remove gateway param to return compatible to server tests

163af9d

jzaki changed the base branch from main to contract-updates October 5, 2022 16:28

github-actions bot added the automation CI/CD related label Oct 5, 2022

jacque006 added 3 commits October 5, 2022 16:02

Remove unneeded arg, expand on any cast.

c3e79a7

Add wallet AddressFromPublicKey function

92b9b13

Bump bls-wallet-clients to v0.8.0

2d28b7b

Bump bls-wallet-clients version to experimental v0.8.0

c2bf4bb

Update aggregator BundleService to work with new signature payload which includes wallet address.

github-actions bot added aggregator Aggregator backend related aggregator-proxy Aggregator proxy related extension Browser extension related labels Oct 5, 2022

jacque006 reviewed Oct 5, 2022

View reviewed changes

Fix import string & contract lint

647c1bb

jacque006 mentioned this pull request Oct 24, 2022

Fix issue getting existing wallet addresses #340

Closed

2 tasks

Merge branch 'main' of github.com:web3well/bls-wallet into contract-a…

5b7231b

…udit

github-actions bot added the documentation Improvements or additions to documentation label Oct 24, 2022

Add replay attack test after recovery

4bc4701

jacque006 reviewed Oct 24, 2022

View reviewed changes

Add getOperationResults to cleints

6379006

Add getOperationResults to bls-wallet-clients to allow consumers to more easily get at operation errors. Change existing test case to use getOperationResults. Add message to require used to prevent ownership changes to proxy admin.

jacque006 mentioned this pull request Oct 26, 2022

Debug BLS transactions #362

Closed

jacque006 merged commit 93c7935 into contract-updates Oct 27, 2022

jacque006 deleted the contract-audit branch October 27, 2022 15:54

jacque006 mentioned this pull request Oct 27, 2022

Contract Updates & Client Integration #367

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit fixes 1-4 and BLSWalletWrapper changes #350

Audit fixes 1-4 and BLSWalletWrapper changes #350

jzaki commented Oct 4, 2022 •

edited by kautukkundan

Loading

voltrevo Oct 5, 2022 •

edited

Loading

jacque006 Oct 5, 2022 •

edited

Loading

voltrevo Oct 5, 2022

jacque006 Oct 5, 2022

jacque006 Oct 5, 2022

voltrevo Oct 5, 2022

jacque006 Oct 5, 2022 •

edited

Loading

voltrevo Oct 5, 2022 •

edited

Loading

voltrevo Oct 5, 2022

jacque006 Oct 5, 2022

voltrevo Oct 5, 2022

jacque006 Oct 5, 2022

voltrevo Oct 5, 2022

jacque006 Oct 5, 2022

voltrevo left a comment

voltrevo commented Oct 5, 2022

How can these changes be manually tested?

jacque006 commented Oct 5, 2022

jacque006 Oct 5, 2022 •

edited

Loading

jacque006 commented Oct 5, 2022 •

edited

Loading

blakecduncan commented Oct 21, 2022

jacque006 commented Oct 24, 2022

jacque006 Oct 24, 2022

jacque006 commented Oct 27, 2022

jacque006 commented Oct 27, 2022

Audit fixes 1-4 and BLSWalletWrapper changes #350

Audit fixes 1-4 and BLSWalletWrapper changes #350

Conversation

jzaki commented Oct 4, 2022 • edited by kautukkundan Loading

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

voltrevo Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

jacque006 Oct 5, 2022 • edited Loading

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

jacque006 Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

voltrevo Oct 5, 2022 • edited Loading

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

voltrevo left a comment

Choose a reason for hiding this comment

voltrevo commented Oct 5, 2022

How can these changes be manually tested?

jacque006 commented Oct 5, 2022

jacque006 Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

jacque006 commented Oct 5, 2022 • edited Loading

Remaining Tasks

Manual Testing

Automated testing (contracts)

blakecduncan commented Oct 21, 2022

jacque006 commented Oct 24, 2022

Choose a reason for hiding this comment

jacque006 commented Oct 27, 2022

jacque006 commented Oct 27, 2022

jzaki commented Oct 4, 2022 •

edited by kautukkundan

Loading

voltrevo Oct 5, 2022 •

edited

Loading

jacque006 Oct 5, 2022 •

edited

Loading

jacque006 Oct 5, 2022 •

edited

Loading

voltrevo Oct 5, 2022 •

edited

Loading

jacque006 Oct 5, 2022 •

edited

Loading

jacque006 commented Oct 5, 2022 •

edited

Loading