Fixed permissionless middleware approval

It was discovered that a permission middleware approval process, while NOT allowing transactions by an unauthorized signer, would however allow to BLOCK an authorized signer from execution. This PR introduces a new state variable on transaction account that whitelists all middleware programs that can approve middleware. Furthermore, this PR removes a legacy slot parameter.
identity-com · Feb 23, 2023 · 8fafeff · 8fafeff
1 parent e30f06e
commit 8fafeff
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 38 deletions.
diff --git a/packages/client/core/src/lib/CryptidTransaction.ts b/packages/client/core/src/lib/CryptidTransaction.ts
@@ -278,6 +278,7 @@ export class CryptidTransaction {
           this.cryptidAccount.didAccountBump,
           TransactionState.toBorsh(state),
           allowUnauthorized,
+          this.cryptidAccount.middlewares.map((m) => m.programId),
           this.instructions,
           this.accountMetas.length
         )

diff --git a/packages/client/idl/src/cryptid.ts b/packages/client/idl/src/cryptid.ts
@@ -214,6 +214,12 @@ export type Cryptid = {
           "name": "allowUnauthorized",
           "type": "bool"
         },
+        {
+          "name": "whitelistedMiddlewarePrograms",
+          "type": {
+            "vec": "publicKey"
+          }
+        },
         {
           "name": "instructions",
           "type": {
@@ -586,14 +592,6 @@ export type Cryptid = {
               "option": "publicKey"
             }
           },
-          {
-            "name": "slot",
-            "docs": [
-              "The slot in which the transaction was proposed",
-              "This is used to prevent replay attacks"
-            ],
-            "type": "u8"
-          },
           {
             "name": "state",
             "docs": [
@@ -617,6 +615,17 @@ export type Cryptid = {
               "option": "publicKey"
             }
           },
+          {
+            "name": "whitelistedMiddlewarePrograms",
+            "docs": [
+              "This vector contains a list of middleware program ids that are allowed to",
+              "approve the execution. Important, is not used for passing transactions execution",
+              "checks. (approved_middleware: Option<Pubkey>) is used for that."
+            ],
+            "type": {
+              "vec": "publicKey"
+            }
+          },
           {
             "name": "authorized",
             "type": "bool"
@@ -853,7 +862,7 @@ export type Cryptid = {
     {
       "code": 6017,
       "name": "AlreadyAuthorizedTransactionAccount",
-      "msg": "Already authorized Transaction Account."
+      "msg": "Transaction Account is already authorized and cannot be authorized again."
     }
   ]
 };
@@ -1074,6 +1083,12 @@ export const IDL: Cryptid = {
           "name": "allowUnauthorized",
           "type": "bool"
         },
+        {
+          "name": "whitelistedMiddlewarePrograms",
+          "type": {
+            "vec": "publicKey"
+          }
+        },
         {
           "name": "instructions",
           "type": {
@@ -1446,14 +1461,6 @@ export const IDL: Cryptid = {
               "option": "publicKey"
             }
           },
-          {
-            "name": "slot",
-            "docs": [
-              "The slot in which the transaction was proposed",
-              "This is used to prevent replay attacks"
-            ],
-            "type": "u8"
-          },
           {
             "name": "state",
             "docs": [
@@ -1477,6 +1484,17 @@ export const IDL: Cryptid = {
               "option": "publicKey"
             }
           },
+          {
+            "name": "whitelistedMiddlewarePrograms",
+            "docs": [
+              "This vector contains a list of middleware program ids that are allowed to",
+              "approve the execution. Important, is not used for passing transactions execution",
+              "checks. (approved_middleware: Option<Pubkey>) is used for that."
+            ],
+            "type": {
+              "vec": "publicKey"
+            }
+          },
           {
             "name": "authorized",
             "type": "bool"
@@ -1713,7 +1731,7 @@ export const IDL: Cryptid = {
     {
       "code": 6017,
       "name": "AlreadyAuthorizedTransactionAccount",
-      "msg": "Already authorized Transaction Account."
+      "msg": "Transaction Account is already authorized and cannot be authorized again."
     }
   ]
 };
diff --git a/packages/tests/src/middleware/checkPass.ts b/packages/tests/src/middleware/checkPass.ts
@@ -143,7 +143,8 @@ describe("Middleware: checkPass", () => {
 
   const propose = async (
     transactionAccount: Keypair,
-    instruction: InstructionData = transferInstructionData
+    instruction: InstructionData = transferInstructionData,
+    whitelistedMiddleware: PublicKey[] = [checkPassMiddlewareProgram.programId]
   ) =>
     program.methods
       .proposeTransaction(
@@ -153,6 +154,7 @@ describe("Middleware: checkPass", () => {
         cryptid.details.didAccountBump,
         TransactionState.toBorsh(TransactionState.Ready),
         false,
+        whitelistedMiddleware,
         [instruction],
         2
       )
@@ -446,6 +448,22 @@ describe("Middleware: checkPass", () => {
       expect(previousBalance - currentBalance).to.equal(LAMPORTS_PER_SOL); // Now the tx has been executed
     });
 
+    it("fails a transfer if the middleware program has not been whitelisted in the propose", async () => {
+      const transactionAccount = Keypair.generate();
+
+      // issue a gateway token to the authority
+      const gatewayToken = await createGatewayToken(authority.publicKey);
+
+      // propose the Cryptid transaction without whitelisting the middleware
+      await propose(transactionAccount, transferInstructionData, []);
+
+      // pass through the middleware
+      const shouldFail = checkPass(transactionAccount, gatewayToken.publicKey);
+      return expect(shouldFail).to.be.rejectedWith(
+        "Error Code: InvalidMiddlewareAccount"
+      );
+    });
+
     it("allows a transfer if the DID account has a valid gateway token", async () => {
       // the difference between this one and the previous one is that it shows that
       // the gateway token can be associated with the DID account itself rather than the authority wallet

diff --git a/packages/tests/src/proposeExecute.ts b/packages/tests/src/proposeExecute.ts
@@ -56,6 +56,7 @@ didTestCases.forEach(({ didType, getDidAccount }) => {
             cryptid.details.didAccountBump,
             TransactionState.toBorsh(TransactionState.Ready),
             false,
+            [], // no whitelisted middleware programs
             [instruction],
             2
           )
@@ -178,6 +179,7 @@ didTestCases.forEach(({ didType, getDidAccount }) => {
             cryptid.details.didAccountBump,
             TransactionState.toBorsh(TransactionState.Ready),
             false,
+            [], // no whitelisted middleware programs
             [transferInstructionData],
             2
           )
@@ -337,6 +339,7 @@ didTestCases.forEach(({ didType, getDidAccount }) => {
             cryptid.details.didAccountBump,
             TransactionState.toBorsh(TransactionState.Ready),
             false,
+            [], // no whitelisted middleware programs
             [transferInstructionData],
             2
           )

diff --git a/programs/cryptid/src/instructions/approve_execution.rs b/programs/cryptid/src/instructions/approve_execution.rs
@@ -18,12 +18,11 @@ pub struct 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
     require!(
-        !anchor_lang::solana_program::system_program::check_id(
-            ctx.accounts.middleware_account.owner
-        ),
+        ctx.accounts
+            .transaction_account
+            .whitelisted_middleware_programs
+            .contains(ctx.accounts.middleware_account.owner),
         CryptidError::InvalidMiddlewareAccount
     );
 

diff --git a/programs/cryptid/src/instructions/extend_transaction.rs b/programs/cryptid/src/instructions/extend_transaction.rs
@@ -54,7 +54,8 @@ pub struct ExtendTransaction<'info> {
                 transaction_account.accounts.len() + num_accounts as usize,
                 InstructionSize::from_iter_to_iter(
                     instructions.iter().chain(transaction_account.instructions.iter())
-                )
+                ),
+                transaction_account.whitelisted_middleware_programs.len()
             ),
         realloc::payer = authority,
         realloc::zero = false,

diff --git a/programs/cryptid/src/instructions/propose_transaction.rs b/programs/cryptid/src/instructions/propose_transaction.rs
@@ -22,6 +22,8 @@ did_account_bump: u8,
 state: TransactionState,
 /// True if the transaction account is being proposed by a non-authority on the DID
 allow_unauthorized: bool,
+/// A list of middleware programs that are allowed to approve this transaction account.
+whitelisted_middleware_programs: Vec<Pubkey>,
 /// The instructions to execute
 instructions: Vec<AbbreviatedInstructionData>,
 num_accounts: u8,
@@ -50,7 +52,8 @@ pub struct ProposeTransaction<'info> {
             num_accounts.into(),
             InstructionSize::from_iter_to_iter(
                 instructions.iter()
-            )
+            ),
+            whitelisted_middleware_programs.len()
        ))
     ]
     transaction_account: Account<'info, TransactionAccount>,
@@ -89,6 +92,7 @@ pub fn propose_transaction<'info>(
     did_account_bump: u8,
     state: TransactionState,
     allow_unauthorized: bool,
+    whitelisted_middleware_programs: Vec<Pubkey>,
     instructions: Vec<AbbreviatedInstructionData>,
 ) -> Result<()> {
     let all_accounts = ctx.all_accounts();
@@ -122,8 +126,6 @@ pub fn propose_transaction<'info>(
     // despite being index 0 in the remaining accounts.
     ctx.accounts.transaction_account.accounts = all_accounts.iter().map(|a| *a.key).collect();
 
-    // TODO: Set slot OR move any Slot / Expiry constraints to middleware
-    // ctx.accounts.transaction_account.slot = Clock::get()?.slot;
     ctx.accounts.transaction_account.did = *ctx.accounts.did.key;
     ctx.accounts.transaction_account.instructions = instructions;
     ctx.accounts.transaction_account.cryptid_account = *ctx.accounts.cryptid_account.key;
@@ -134,6 +136,9 @@ pub fn propose_transaction<'info>(
         None
     };
     ctx.accounts.transaction_account.authorized = !allow_unauthorized;
+    ctx.accounts
+        .transaction_account
+        .whitelisted_middleware_programs = whitelisted_middleware_programs;
 
     // if the transaction is being created by an unauthorized signer,
     // then the cryptid account must have superuser middleware registered

diff --git a/programs/cryptid/src/instructions/superuser_approve_execution.rs b/programs/cryptid/src/instructions/superuser_approve_execution.rs
@@ -25,12 +25,11 @@ pub struct SuperuserApproveExecution<'info> {
 pub fn superuser_approve_execution<'info>(
     ctx: Context<'_, '_, '_, 'info, SuperuserApproveExecution<'info>>,
 ) -> Result<()> {
-    // Make sure owner is NOT the System Program
-    // TODO(ticket): Consider implementing an approval registry on middleware
     require!(
-        !anchor_lang::solana_program::system_program::check_id(
-            ctx.accounts.middleware_account.owner
-        ),
+        ctx.accounts
+            .transaction_account
+            .whitelisted_middleware_programs
+            .contains(ctx.accounts.middleware_account.owner),
         CryptidError::InvalidMiddlewareAccount
     );
 

diff --git a/programs/cryptid/src/lib.rs b/programs/cryptid/src/lib.rs
@@ -68,6 +68,7 @@ pub mod cryptid {
         did_account_bump: u8,
         state: TransactionState,
         allow_unauthorized: bool,
+        whitelisted_middleware_programs: Vec<Pubkey>,
         instructions: Vec<AbbreviatedInstructionData>,
         _num_accounts: u8,
     ) -> Result<()> {
@@ -79,6 +80,7 @@ pub mod cryptid {
             did_account_bump,
             state,
             allow_unauthorized,
+            whitelisted_middleware_programs,
             instructions,
         )
     }

diff --git a/programs/cryptid/src/state/transaction_account.rs b/programs/cryptid/src/state/transaction_account.rs
@@ -20,9 +20,6 @@ pub struct TransactionAccount {
     pub instructions: Vec<AbbreviatedInstructionData>,
     /// The most recent middleware PDA that approved the transaction
     pub approved_middleware: Option<Pubkey>,
-    /// The slot in which the transaction was proposed
-    /// This is used to prevent replay attacks
-    pub slot: u8,
     /// The transaction state, to prevent replay attacks
     /// in case an executed transaction account is not immediately
     /// garbage-collected by the runtime
@@ -32,23 +29,28 @@ pub struct TransactionAccount {
     /// If the transaction account is proposed by an unauthorized cryptid client, then
     /// it is set to to that signer, and only a `superUser` middleware can approve it.
     pub unauthorized_signer: Option<Pubkey>,
+    /// This vector contains a list of middleware program ids that are allowed to
+    /// approve the execution. Important, is not used for passing transactions execution
+    /// checks. (approved_middleware: Option<Pubkey>) is used for that.
+    pub whitelisted_middleware_programs: Vec<Pubkey>,
     pub authorized: bool,
 }
 impl TransactionAccount {
     /// Calculates the on-chain size of a [`TransactionAccount`]
     pub fn calculate_size(
         num_accounts: usize,
         instruction_sizes: impl Iterator<Item = InstructionSize>,
+        num_whitelisted_middleware_programs: usize,
     ) -> usize {
         DISCRIMINATOR_SIZE
             + 32 // cryptid_account
             + 32 // did (owner)
             + 4 + 32 * (num_accounts + 4) //accounts (+4 for the named accounts)
             + 4 + instruction_sizes.into_iter().map(AbbreviatedInstructionData::calculate_size).sum::<usize>() //transaction_instructions
             + 1 + 32 // approved_middleware
-            + 1 // slot
             + 1 // state
             + 1 + 32 // unauthorized signer
+            + 4 + 32 * num_whitelisted_middleware_programs // whitelisted_middleware_programs
             + 1 // authorized
     }
 
@@ -91,6 +93,7 @@ mod test {
                 accounts: 1,
                 data_len: 1,
             }),
+            1,
         );
         println!("Size: {size}");
 
@@ -104,9 +107,9 @@ mod test {
                 data: vec![0],
             }],
             approved_middleware: None,
-            slot: 0,
             state: TransactionState::Ready,
             unauthorized_signer: None,
+            whitelisted_middleware_programs: vec![Default::default()],
             authorized: true,
         };
         let ser_size = BorshSerialize::try_to_vec(&account).unwrap().len();