Merge pull request #2543 from AmbireTech/fix-typed-data-block-bypassed

Fix / block AmbireOperation typed-data bypass after account switch

Author: superKalo

src/controllers/requests/requests.test.ts

@@ -688,14 +688,15 @@ describe('RequestsController ', () => {
 
     const buildSignTypedDataRequest = (
       controller: Awaited<ReturnType<typeof prepareTest>>['controller'],
-      typedData: object
+      typedData: object,
+      signerAddress: string = FROM
     ) =>
       controller.build({
         type: 'dappRequest',
         params: {
           request: {
             method: 'eth_signTypedData_v4',
-            params: [FROM, JSON.stringify(typedData)],
+            params: [signerAddress, JSON.stringify(typedData)],
             session: MOCK_SESSION
           },
           dappPromise: {
@@ -779,5 +780,65 @@ describe('RequestsController ', () => {
       expect(req.kind).toBe('typedMessage')
       expect(req.meta.params.domain.chainId).toBe(1n)
     })
+
+    const SELECTED_ACCOUNT = FROM
+    const OTHER_ACCOUNT = '0xa07D75aacEFd11b425AF7181958F0F85c312f143'
+
+    const AMBIRE_OPERATION_TYPED_DATA = {
+      types: {
+        EIP712Domain: [
+          { name: 'name', type: 'string' },
+          { name: 'version', type: 'string' },
+          { name: 'chainId', type: 'uint256' },
+          { name: 'verifyingContract', type: 'address' },
+          { name: 'salt', type: 'bytes32' }
+        ],
+        AmbireOperation: [
+          { name: 'account', type: 'address' },
+          { name: 'hash', type: 'bytes32' }
+        ]
+      },
+      primaryType: 'AmbireOperation',
+      domain: {
+        name: 'Ambire',
+        version: '1',
+        chainId: 1,
+        verifyingContract: SELECTED_ACCOUNT,
+        salt: '0x0000000000000000000000000000000000000000000000000000000000000000'
+      },
+      message: {
+        account: SELECTED_ACCOUNT,
+        hash: '0x1111111111111111111111111111111111111111111111111111111111111111'
+      }
+    }
+
+    test('rejects AmbireOperation typed data for the selected account', async () => {
+      const { controller } = await prepareTest(true)
+      await expect(
+        buildSignTypedDataRequest(controller, AMBIRE_OPERATION_TYPED_DATA, SELECTED_ACCOUNT)
+      ).rejects.toThrow('Signing an AmbireOperation is not allowed')
+      expect(controller.userRequests.length).toBe(0)
+    })
+
+    test('rejects AmbireOperation typed data for a non-selected account', async () => {
+      const { controller } = await prepareTest(true)
+      const otherAccountTypedData = {
+        ...AMBIRE_OPERATION_TYPED_DATA,
+        domain: {
+          ...AMBIRE_OPERATION_TYPED_DATA.domain,
+          verifyingContract: OTHER_ACCOUNT
+        },
+        message: {
+          account: OTHER_ACCOUNT,
+          hash: '0x1111111111111111111111111111111111111111111111111111111111111111'
+        }
+      }
+
+      await expect(
+        buildSignTypedDataRequest(controller, otherAccountTypedData, OTHER_ACCOUNT)
+      ).rejects.toThrow('Signing an AmbireOperation is not allowed')
+      expect(controller.userRequests.length).toBe(0)
+      expect(controller.userRequestsWaitingAccountSwitch.length).toBe(0)
+    })
   })
 })

src/controllers/requests/requests.ts

@@ -67,6 +67,10 @@ import {
   messageOnNewRequest
 } from '../../libs/requests/requests'
 import { parse } from '../../libs/richJson/richJson'
+import {
+  AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE,
+  isAmbireOperationTypedData
+} from '../../libs/signMessage/signMessage'
 import { getSwapAndBridgeRequestParams } from '../../libs/swapAndBridge/swapAndBridge'
 import {
   getClaimWalletRequestParams,
@@ -396,6 +400,14 @@ export class RequestsController extends EventEmitter implements IRequestsControl
     for (const req of reqs) {
       const { kind, meta, dappPromises } = req
 
+      if (
+        kind === 'typedMessage' &&
+        isAmbireOperationTypedData((meta as TypedMessageUserRequest['meta']).params)
+      ) {
+        this.#rejectAmbireOperationTypedDataRequest(req as TypedMessageUserRequest)
+        continue
+      }
+
       if (allowAccountSwitch && isSignRequest(kind)) {
         if ((meta as SignUserRequest['meta']).accountAddr !== this.#selectedAccount.account?.addr) {
           await this.#addSwitchAccountUserRequest(req as SignUserRequest)
@@ -913,6 +925,15 @@ export class RequestsController extends EventEmitter implements IRequestsControl
 
         requestsToAddOrRemove.forEach((r) => {
           this.userRequestsWaitingAccountSwitch.splice(this.userRequests.indexOf(r), 1)
+
+          if (
+            r.kind === 'typedMessage' &&
+            isAmbireOperationTypedData((r as TypedMessageUserRequest).meta.params)
+          ) {
+            this.#rejectAmbireOperationTypedDataRequest(r as TypedMessageUserRequest)
+            return
+          }
+
           userRequestsToAdd.push(r)
         })
       }
@@ -1303,11 +1324,8 @@ export class RequestsController extends EventEmitter implements IRequestsControl
         throw ethErrors.rpc.invalidParams('The message contents did not match the provided types.')
       }
 
-      if (
-        msgAddress === this.#selectedAccount.account.addr &&
-        (typedData.primaryType === 'AmbireOperation' || !!typedData.types.AmbireOperation)
-      ) {
-        throw ethErrors.rpc.methodNotSupported('Signing an AmbireOperation is not allowed')
+      if (isAmbireOperationTypedData(typedData)) {
+        throw ethErrors.rpc.methodNotSupported(AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE)
       }
 
       userRequest = {
@@ -1716,7 +1734,18 @@ export class RequestsController extends EventEmitter implements IRequestsControl
     if (userRequest) await this.addUserRequests([userRequest])
   }
 
+  #rejectAmbireOperationTypedDataRequest(req: TypedMessageUserRequest) {
+    req.dappPromises.forEach((p) => {
+      p.reject(ethErrors.rpc.methodNotSupported(AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE))
+    })
+  }
+
   async #addSwitchAccountUserRequest(req: SignUserRequest) {
+    if (req.kind === 'typedMessage' && isAmbireOperationTypedData(req.meta.params)) {
+      this.#rejectAmbireOperationTypedDataRequest(req)
+      return
+    }
+
     this.userRequestsWaitingAccountSwitch.push(req)
     await this.addUserRequests(
       [

src/controllers/signAccountOp/signAccountOp.ts

@@ -3189,7 +3189,10 @@ export class SignAccountOpController
                 this.account,
                 accountState,
                 signer,
-                this.#network
+                this.#network,
+                false,
+                undefined,
+                true
               )
           )
           if (!this.accountOp.meta) {

src/controllers/signMessage/signMessage.ts

@@ -40,13 +40,15 @@ import {
   getSigningRequestDisplayData
 } from '../../libs/signingRequest/signingRequest'
 import {
+  AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE,
   getAppFormatted,
   getEIP712Hash,
   getEIP712Signature,
   getPlainTextSignature,
   getSafeMessageTypedData,
   getVerifyMessageSignature,
-  verifyMessage
+  verifyMessage,
+  isAmbireOperationTypedData
 } from '../../libs/signMessage/signMessage'
 import hexStringToUint8Array from '../../utils/hexStringToUint8Array'
 import { SignedMessage } from '../activity/types'
@@ -560,6 +562,10 @@ export class SignMessageController
         }
 
         if (this.messageToSign.content.kind === 'typedMessage') {
+          if (isAmbireOperationTypedData(this.messageToSign.content)) {
+            throw new Error(AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE)
+          }
+
           if (this.#account.creation && this.messageToSign.content.primaryType === 'Permit') {
             throw new Error(
               'It looks like that this app doesn\'t detect Smart Account wallets, and requested incompatible approval type. Please, go back to the app and change the approval type to "Transaction", which is supported by Smart Account wallets.'

src/libs/signMessage/signMessage.test.ts

@@ -359,6 +359,8 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       accountState,
       signer,
       polygonNetwork,
+      true,
+      undefined,
       true
     )
 
@@ -658,7 +660,10 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       smartAccount,
       accountState,
       signer,
-      polygonNetwork
+      polygonNetwork,
+      false,
+      undefined,
+      true
     )
     // the key should be dedicatedToOneSA, so we expect the signature to end in 00
     expect(eip712Sig.signature.slice(-2)).toEqual('00')
@@ -697,7 +702,10 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       v1Account,
       accountState,
       signer,
-      ethereumNetwork
+      ethereumNetwork,
+      false,
+      undefined,
+      true
     )
     // the key is for a v1 acc so it should be 00
     expect(eip712Sig.signature.slice(-2)).toEqual('00')
@@ -752,7 +760,10 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       v1Account,
       accountState,
       signer,
-      ethereumNetwork
+      ethereumNetwork,
+      false,
+      undefined,
+      true
     )
     // the key is for a v1 acc so it should be 00
     expect(eip712Sig.signature.slice(-2)).toEqual('00')
@@ -777,7 +788,16 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       hashMessage('test')
     )
     try {
-      await getEIP712Signature(typedData, v1Account, accountState, signer, polygonNetwork)
+      await getEIP712Signature(
+        typedData,
+        v1Account,
+        accountState,
+        signer,
+        polygonNetwork,
+        false,
+        undefined,
+        true
+      )
       console.log('No error was thrown for [V1 SA]: eip-712, but it should have')
       expect(true).toEqual(false)
     } catch (e: any) {
@@ -855,7 +875,10 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: true ', () => {
       smartAccount,
       v2AccountState,
       signer,
-      polygonNetwork
+      polygonNetwork,
+      false,
+      undefined,
+      true
     )
     expect(eip712Sig.signature.slice(-2)).toEqual('00')
 
@@ -1056,7 +1079,16 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: false', () => {
       hashMessage('test')
     )
     try {
-      await getEIP712Signature(typedData, smartAccount, accountState, signer, polygonNetwork)
+      await getEIP712Signature(
+        typedData,
+        smartAccount,
+        accountState,
+        signer,
+        polygonNetwork,
+        false,
+        undefined,
+        true
+      )
       console.log('No error was thrown for [Not dedicated to one SA]: eip-712, but it should have')
       expect(true).toEqual(false)
     } catch (e: any) {
@@ -1065,6 +1097,21 @@ describe('Sign Message, Keystore with key dedicatedToOneSA: false', () => {
       )
     }
   })
+  test('Signing [V2 SA]: rejects AmbireOperation typed data from untrusted sources by default', async () => {
+    const accountStates = await getAccountsInfo([smartAccount])
+    const accountState = accountStates[smartAccount.addr]![polygonNetwork.chainId.toString()]!
+    const signer = await keystore.getSigner(eoaSigner.keyPublicAddress, 'internal')
+
+    const typedData = getTypedData(
+      polygonNetwork.chainId,
+      accountState.accountAddr,
+      hashMessage('test')
+    )
+
+    await expect(
+      getEIP712Signature(typedData, smartAccount, accountState, signer, polygonNetwork)
+    ).rejects.toThrow('Signing an AmbireOperation is not allowed')
+  })
 })
 
 describe('Sign Message, Safe accounts', () => {

src/libs/signMessage/signMessage.ts

@@ -63,6 +63,18 @@ export const EIP_1271_NOT_SUPPORTED_BY = [
   'bitrefill.com'
 ]
 
+export const AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE =
+  'Signing an AmbireOperation is not allowed'
+
+export const isAmbireOperationTypedData = (typedData: {
+  primaryType: string
+  types: Record<string, unknown>
+}) => {
+  if ('AmbireReadableOperation' in typedData.types) return false
+
+  return typedData.primaryType === 'AmbireOperation' || 'AmbireOperation' in typedData.types
+}
+
 /**
  * For Unprotected signatures, we need to append 00 at the end
  * for ambire to recognize it
@@ -607,7 +619,8 @@ export async function getEIP712Signature(
   signer: KeystoreSignerInterface,
   network: Network,
   isOG = false,
-  withHardwareWalletSigningRequest?: WithHardwareWalletSigningRequest
+  withHardwareWalletSigningRequest?: WithHardwareWalletSigningRequest,
+  allowAmbireOperation = false
 ): Promise<{ signature: Hex; hash?: Hex }> {
   if (!message.types.EIP712Domain) {
     throw new Error(
@@ -642,6 +655,10 @@ export async function getEIP712Signature(
       )) as Hex
     }
 
+  if (isAmbireOperationTypedData(message) && !allowAmbireOperation) {
+    throw new Error(AMBIRE_OPERATION_SIGNING_NOT_ALLOWED_MESSAGE)
+  }
+
   if (!accountState.isV2) {
     if (
       // @NOTE: isOG is to allow tem members to sign anything with v1 accounts regardless of safety and security