Merge pull request #221 from identity-com/bugfix/FYEO-CRY-01-extension

Security Audit Feedback - Bugfix

Author: rado0x54

packages/client/core/src/api/unauthorizedCryptidClient.ts

@@ -1,7 +1,7 @@
 import { CryptidClient, CryptidOptions } from "./cryptidClient";
 import { Wallet } from "../types/crypto";
 import { CryptidAccountDetails } from "../lib/CryptidAccountDetails";
-import { PublicKey, Transaction } from "@solana/web3.js";
+import { Transaction } from "@solana/web3.js";
 import { ProposalResult, TransactionState } from "../types/cryptid";
 import { AbstractCryptidClient } from "./abstractCryptidClient";
 import { ControlledCryptidClient } from "./controlledCryptidClient";
@@ -42,22 +42,6 @@ export class UnauthorizedCryptidClient extends AbstractCryptidClient {
     );
   }
 
-  async extend(
-    transactionAccountAddress: PublicKey,
-    transaction: Transaction,
-    state?: TransactionState
-  ): Promise<Transaction> {
-    return this.service().then((service) =>
-      service.extend(
-        this.details,
-        transactionAccountAddress,
-        transaction,
-        state,
-        true
-      )
-    );
-  }
-
   unauthorized(): CryptidClient {
     return this;
   }

packages/client/core/src/lib/CryptidTransaction.ts

@@ -298,15 +298,13 @@ export class CryptidTransaction {
    * @param program
    * @param transactionAccountAddress
    * @param state
-   * @param allowUnauthorized
    */
   // TODO move transactionAccountAddress into constructor?
   extend(
     program: Program<Cryptid>,
     transactionAccountAddress: PublicKey,
     // by default, extend the transaction and seal it at the same time
-    state = TransactionState.Ready,
-    allowUnauthorized = false
+    state = TransactionState.Ready
   ) {
     return (
       program.methods
@@ -316,7 +314,6 @@ export class CryptidTransaction {
           this.cryptidAccount.index,
           this.cryptidAccount.didAccountBump,
           TransactionState.toBorsh(state),
-          allowUnauthorized,
           this.instructions,
           this.accountMetas.length
         )

packages/client/core/src/service/cryptid.ts

@@ -314,8 +314,7 @@ export class CryptidService {
     account: CryptidAccountDetails,
     transactionAccountAddress: PublicKey,
     transaction: Transaction,
-    state?: TransactionState,
-    allowUnauthorized = false
+    state?: TransactionState
   ): Promise<Transaction> {
     const cryptidTransaction = CryptidTransaction.fromSolanaInstructions(
       account,
@@ -327,8 +326,7 @@ export class CryptidService {
     let builder = cryptidTransaction.extend(
       this.program,
       transactionAccountAddress,
-      state,
-      allowUnauthorized
+      state
     );
 
     if (state === TransactionState.Ready) {

packages/client/idl/src/cryptid.ts

@@ -298,10 +298,6 @@ export type Cryptid = {
             "defined": "TransactionState"
           }
         },
-        {
-          "name": "allowUnauthorized",
-          "type": "bool"
-        },
         {
           "name": "instructions",
           "type": {
@@ -853,6 +849,11 @@ export type Cryptid = {
       "code": 6016,
       "name": "InvalidMiddlewareAccount",
       "msg": "Approve Execution needs to be called with a valid Middleware Account."
+    },
+    {
+      "code": 6017,
+      "name": "AlreadyAuthorizedTransactionAccount",
+      "msg": "Already authorized Transaction Account."
     }
   ]
 };
@@ -1157,10 +1158,6 @@ export const IDL: Cryptid = {
             "defined": "TransactionState"
           }
         },
-        {
-          "name": "allowUnauthorized",
-          "type": "bool"
-        },
         {
           "name": "instructions",
           "type": {
@@ -1712,6 +1709,11 @@ export const IDL: Cryptid = {
       "code": 6016,
       "name": "InvalidMiddlewareAccount",
       "msg": "Approve Execution needs to be called with a valid Middleware Account."
+    },
+    {
+      "code": 6017,
+      "name": "AlreadyAuthorizedTransactionAccount",
+      "msg": "Already authorized Transaction Account."
     }
   ]
 };

packages/tests/src/middleware/superuserCheckSigner.ts

@@ -1,11 +1,16 @@
-import { Keypair, LAMPORTS_PER_SOL, PublicKey } from "@solana/web3.js";
+import {
+  Keypair,
+  LAMPORTS_PER_SOL,
+  PublicKey,
+  SystemProgram,
+} from "@solana/web3.js";
 import chai from "chai";
 import chaiAsPromised from "chai-as-promised";
-import { makeTransfer } from "../util/cryptid";
+import { makeTransfer, toAccountMeta } from "../util/cryptid";
 import { initializeDIDAccount } from "../util/did";
 import { balanceOf, createTestContext, fund } from "../util/anchorUtils";
-import { DID_SOL_PREFIX } from "@identity.com/sol-did-client";
-import { Cryptid } from "@identity.com/cryptid";
+import { DID_SOL_PREFIX, DID_SOL_PROGRAM } from "@identity.com/sol-did-client";
+import { Cryptid, TransactionState } from "@identity.com/cryptid";
 import {
   SuperuserCheckSignerMiddleware,
   deriveMiddlewareAccountAddress,
@@ -17,13 +22,16 @@ const { expect } = chai;
 
 describe("Middleware: superuserCheckSigner", () => {
   const {
+    program,
     keypair,
     provider,
     authority,
     middleware: { superuserCheckSigner: superuserCheckSignerMiddlewareProgram },
   } = createTestContext();
 
   let cryptid: CryptidClient;
+  let authorizedCryptid: CryptidClient;
+
   const cryptidIndex = 1;
 
   let middlewareAccount: PublicKey;
@@ -33,6 +41,31 @@ describe("Middleware: superuserCheckSigner", () => {
   const makeTransaction = (to = signer.publicKey) =>
     makeTransfer(cryptid.address(), to);
 
+  const execute = (transactionAccount: PublicKey) =>
+    // execute the Cryptid transaction
+    program.methods
+      .executeTransaction(
+        [], // no controller chain
+        cryptid.details.bump,
+        cryptid.details.index,
+        cryptid.details.didAccountBump,
+        0
+      )
+      .accounts({
+        cryptidAccount: cryptid.address(),
+        didProgram: DID_SOL_PROGRAM,
+        did: cryptid.details.didAccount,
+        authority: signer.publicKey,
+        destination: signer.publicKey,
+        transactionAccount,
+      })
+      .remainingAccounts([
+        toAccountMeta(signer.publicKey, true, false),
+        toAccountMeta(SystemProgram.programId),
+      ])
+      .signers([signer])
+      .rpc();
+
   before("Fund the authority and signer", async () => {
     await Promise.all([
       fund(authority.publicKey, LAMPORTS_PER_SOL),
@@ -99,6 +132,22 @@ describe("Middleware: superuserCheckSigner", () => {
         }
       )
     ).unauthorized();
+
+    authorizedCryptid = await Cryptid.buildFromDID(
+      DID_SOL_PREFIX + ":" + authority.publicKey,
+      authority,
+      {
+        connection: provider.connection,
+        accountIndex: cryptidIndex,
+        middlewares: [
+          {
+            programId: superuserCheckSignerMiddlewareProgram.programId,
+            address: middlewareAccount,
+            isSuperuser: true,
+          },
+        ],
+      }
+    );
   };
 
   before(
@@ -159,6 +208,37 @@ describe("Middleware: superuserCheckSigner", () => {
     );
   });
 
+  it("cannot execute with an unauthorized signer if the transaction was proposed by none (middleware error)", async () => {
+    // propose the Cryptid transaction
+    const { proposeTransaction, transactionAccount, proposeSigners } =
+      await authorizedCryptid.propose(makeTransaction());
+    // this will be signed by a DID authority
+    await authorizedCryptid.send(proposeTransaction, proposeSigners);
+
+    // This should fail!
+    const { transactions, signers } = await cryptid.execute(transactionAccount);
+
+    const shouldFail = cryptid.send(transactions[0], signers);
+
+    return expect(shouldFail).to.be.rejectedWith(
+      "Error Code: AlreadyAuthorizedTransactionAccount"
+    );
+  });
+
+  it("cannot execute with an unauthorized signer if the transaction was proposed by none (execute error)", async () => {
+    // propose the Cryptid transaction
+    const { proposeTransaction, transactionAccount, proposeSigners } =
+      await authorizedCryptid.propose(makeTransaction());
+    // this will be signed by a DID authority
+    await authorizedCryptid.send(proposeTransaction, proposeSigners);
+
+    // This should fail!
+    // Manual execute skips the middleware execution. (which would fail)
+    const shouldFail = execute(transactionAccount);
+
+    return expect(shouldFail).to.be.rejectedWith("Error Code: KeyMustBeSigner");
+  });
+
   it("blocks a transfer not signed by the signer", async () => {
     // change the signer
     signer = Keypair.generate();
@@ -177,4 +257,26 @@ describe("Middleware: superuserCheckSigner", () => {
 
     return expect(shouldFail).to.be.rejectedWith("Error Code: InvalidSigner.");
   });
+
+  it("cannot extend with an unauthorized signer of the transaction was proposed by none", async () => {
+    // propose the Cryptid transaction
+    const { proposeTransaction, transactionAccount, proposeSigners } =
+      await authorizedCryptid.propose(
+        makeTransaction(),
+        TransactionState.NotReady
+      );
+    // this will be signed by the non-did signer
+    await authorizedCryptid.send(proposeTransaction, proposeSigners);
+
+    // This should fail!
+    const extendTx = await cryptid.extend(
+      transactionAccount,
+      makeTransaction()
+    );
+    const shouldFail = cryptid.send(extendTx, []);
+
+    return expect(shouldFail).to.be.rejectedWith(
+      "Error Code: KeyMustBeSigner."
+    );
+  });
 });

programs/cryptid/src/error.rs

@@ -58,4 +58,7 @@ pub enum CryptidError {
     /// An invalid Middleware Account was passed.
     #[msg("Approve Execution needs to be called with a valid Middleware Account.")]
     InvalidMiddlewareAccount,
+    /// Already authorized Transaction Account.
+    #[msg("Transaction Account is already authorized and cannot be authorized again.")]
+    AlreadyAuthorizedTransactionAccount,
 }

programs/cryptid/src/instructions/approve_execution.rs

@@ -15,8 +15,8 @@ pub struct ApproveExecution<'info> {
 }
 
 /// Executes a transaction directly if all required keys sign
-pub fn approve_execution<'a, 'b, 'c, 'info>(
-    ctx: Context<'a, 'b, 'c, 'info, ApproveExecution<'info>>,
+pub fn approve_execution<'info>(
+    ctx: Context<'_, '_, '_, 'info, ApproveExecution<'info>>,
 ) -> Result<()> {
     // Make sure owner is NOT the System Program
     // TODO(ticket): Consider implementing an approval registry on middleware

programs/cryptid/src/instructions/close_transaction.rs

@@ -77,8 +77,8 @@ impl<'a, 'b, 'c, 'info> AllAccounts<'a, 'b, 'c, 'info>
 }
 
 /// Close an existing transaction account
-pub fn close_transaction<'a, 'b, 'c, 'info>(
-    ctx: Context<'a, 'b, 'c, 'info, CloseTransaction<'info>>,
+pub fn close_transaction<'info>(
+    ctx: Context<'_, '_, '_, 'info, CloseTransaction<'info>>,
     controller_chain: Vec<DIDReference>,
     cryptid_account_bump: u8,
     cryptid_account_index: u32,

programs/cryptid/src/instructions/direct_execute.rs

@@ -61,8 +61,8 @@ impl<'a, 'b, 'c, 'info> AllAccounts<'a, 'b, 'c, 'info>
 }
 
 /// Executes a transaction directly if all required keys sign
-pub fn direct_execute<'a, 'b, 'c, 'info>(
-    ctx: Context<'a, 'b, 'c, 'info, DirectExecute<'info>>,
+pub fn direct_execute<'info>(
+    ctx: Context<'_, '_, '_, 'info, DirectExecute<'info>>,
     controller_chain: Vec<DIDReference>,
     instructions: Vec<AbbreviatedInstructionData>,
     cryptid_account_bump: u8,

programs/cryptid/src/instructions/execute_transaction.rs

@@ -86,8 +86,8 @@ impl<'a, 'b, 'c, 'info> AllAccounts<'a, 'b, 'c, 'info>
 }
 
 /// Executes a transaction directly if all required keys sign
-pub fn execute_transaction<'a, 'b, 'c, 'info>(
-    ctx: Context<'a, 'b, 'c, 'info, ExecuteTransaction<'info>>,
+pub fn execute_transaction<'info>(
+    ctx: Context<'_, '_, '_, 'info, ExecuteTransaction<'info>>,
     controller_chain: Vec<DIDReference>,
     cryptid_account_bump: u8,
     cryptid_account_index: u32,