Merge pull request #11 from dapperlabs/bug-multi-sig

Fix multi-sig bug

Author: pazams

index.js

@@ -15,26 +15,33 @@ module.exports = class DappAuth {
       ethUtil.toBuffer(challenge),
     );
 
-    // Get the address of whoever signed this message
-    const { v, r, s } = ethUtil.fromRpcSig(signature);
-    const recoveredKey = ethUtil.ecrecover(challengeHash, v, r, s);
-    const recoveredAddress = ethUtil.publicToAddress(recoveredKey);
+    let isAuthorizedDirectKey;
 
     // try direct-keyed wallet
-    if (
-      address.toLowerCase() ===
-      ethUtil.bufferToHex(recoveredAddress).toLowerCase()
-    ) {
-      return true;
+    try {
+      // Get the address of whoever signed this message
+      const { v, r, s } = ethUtil.fromRpcSig(signature);
+      const recoveredKey = ethUtil.ecrecover(challengeHash, v, r, s);
+      const recoveredAddress = ethUtil.publicToAddress(recoveredKey);
+
+      if (
+        address.toLowerCase() ===
+        ethUtil.bufferToHex(recoveredAddress).toLowerCase()
+      ) {
+        isAuthorizedDirectKey = true;
+      }
+    } catch (e) {
+      // if either direct-keyed auth flow threw an error, or it did not conclude to be authorized, proceed to try smart-contract wallet in finally clause.
+    } finally {
+      if (isAuthorizedDirectKey === true) return isAuthorizedDirectKey;
+      // try smart-contract wallet
+      const erc1271CoreContract = new this.web3.eth.Contract(ERC1271, address);
+
+      const magicValue = await erc1271CoreContract.methods
+        .isValidSignature(ethUtil.keccak(challenge), signature) // we send just a regular hash, which then the smart contract hashes ontop to an erc191 hash
+        .call();
+
+      return magicValue === ERC1271_MAGIC_VALUE;
     }
-
-    // try smart-contract wallet
-    const erc1271CoreContract = new this.web3.eth.Contract(ERC1271, address);
-
-    const magicValue = await erc1271CoreContract.methods
-      .isValidSignature(ethUtil.keccak(challenge), signature) // we send just a regular hash, which then the smart contract hashes ontop to an erc191 hash
-      .call();
-
-    return magicValue === ERC1271_MAGIC_VALUE;
   }
 };

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@dapperlabs/dappauth",
-  "version": "2.0.0",
+  "version": "2.0.1",
   "description": "A util to prove actionable control ('ownership') over a public Ethereum address using eth_sign",
   "keywords": [
     "ethereum",

test/contract-mock.js

@@ -46,9 +46,14 @@ module.exports = class MockContract {
     if (this.errorIsValidSignature) {
       throw new Error('isValidSignature call returned an error');
     }
+    // split to 65 bytes (130 hex) chunks
+    const multi_sigs = signature.toString('hex').match(/.{1,130}/g);
+
+    // TODO: is it first or last?
+    const expected_authrorised_sig = multi_sigs[0];
 
     // Get the address of whoever signed this message
-    const { v, r, s } = ethUtil.fromRpcSig(signature);
+    const { v, r, s } = ethUtil.fromRpcSig(`0x${expected_authrorised_sig}`);
     const erc191MessageHash = utils.erc191MessageHash(hash, this.address);
     const recoveredKey = ethUtil.ecrecover(erc191MessageHash, v, r, s);
     const recoveredAddress = ethUtil.publicToAddress(recoveredKey);

test/test.js

@@ -16,7 +16,7 @@ describe('dappauth', function() {
       isEOA: true,
       challenge: 'foo',
       challengeSign: 'foo',
-      signingKey: keyA,
+      signingKeys: [keyA],
       authAddr: utils.keyToAddress(keyA),
       mockContract: {
         authorizedKey: null,
@@ -33,7 +33,7 @@ describe('dappauth', function() {
       isEOA: true,
       challenge: 'foo',
       challengeSign: 'bar',
-      signingKey: keyA,
+      signingKeys: [keyA],
       authAddr: utils.keyToAddress(keyA),
       mockContract: {
         authorizedKey: ethUtil.privateToPublic(keyC),
@@ -49,7 +49,7 @@ describe('dappauth', function() {
       isEOA: true,
       challenge: 'foo',
       challengeSign: 'foo',
-      signingKey: keyA,
+      signingKeys: [keyA],
       authAddr: utils.keyToAddress(keyB),
       mockContract: {
         authorizedKey: ethUtil.privateToPublic(keyC),
@@ -65,7 +65,7 @@ describe('dappauth', function() {
       isEOA: false,
       challenge: 'foo',
       challengeSign: 'foo',
-      signingKey: keyB,
+      signingKeys: [keyB],
       authAddr: utils.keyToAddress(keyA),
       mockContract: {
         authorizedKey: ethUtil.privateToPublic(keyB),
@@ -75,13 +75,30 @@ describe('dappauth', function() {
       expectedAuthorizedSignerError: false,
       expectedAuthorizedSigner: true,
     },
+    {
+      title:
+        'Smart-contract wallets with a 1-of-2 (multi-sig) correct internal key should be authorized signers over their address',
+      isEOA: false,
+      challenge: 'foo',
+      challengeSign: 'foo',
+      signingKeys: [keyB, keyC],
+      authAddr: utils.keyToAddress(keyA),
+      mockContract: {
+        authorizedKey: ethUtil.privateToPublic(keyB),
+        address: utils.keyToAddress(keyA),
+        errorIsValidSignature: false,
+      },
+      expectedAuthorizedSignerError: false,
+      expectedAuthorizedSigner: true,
+    },
+
     {
       title:
         'Smart-contract wallets with a 1-of-1 incorrect internal key should NOT be authorized signers over their address',
       isEOA: false,
       challenge: 'foo',
       challengeSign: 'foo',
-      signingKey: keyB,
+      signingKeys: [keyB],
       authAddr: utils.keyToAddress(keyA),
       mockContract: {
         authorizedKey: ethUtil.privateToPublic(keyC),
@@ -96,7 +113,7 @@ describe('dappauth', function() {
       isEOA: false,
       challenge: 'foo',
       challengeSign: 'foo',
-      signingKey: keyB,
+      signingKeys: [keyB],
       authAddr: utils.keyToAddress(keyA),
       mockContract: {
         authorizedKey: ethUtil.privateToPublic(keyB),
@@ -118,18 +135,20 @@ describe('dappauth', function() {
         ? utils.signEOAPersonalMessage
         : utils.signERC1654PersonalMessage;
 
-      const signature = signatureFunc(
-        test.challengeSign,
-        test.signingKey,
-        test.authAddr,
-      );
+      const signatures = `0x${test.signingKeys
+        .map((signingKey) =>
+          ethUtil.stripHexPrefix(
+            signatureFunc(test.challengeSign, signingKey, test.authAddr),
+          ),
+        )
+        .join('')}`;
 
       let isError = false;
       let isAuthorizedSigner = false;
       try {
         isAuthorizedSigner = await dappAuth.isAuthorizedSigner(
           test.challenge,
-          signature,
+          signatures,
           test.authAddr,
         );
       } catch (error) {