Handle malformed multiSend decoding errors (#1984)

Wraps the decoding/mapping logic for `multiSend` transactions in a `try / catch`, logging the error and returning an empty array instead: - Add `try / catch` to `mapMultiSendTransactions` and log thrown error. - Add/update tests accordingly.
safe-global · Oct 1, 2024 · 0b8a6ae · 0b8a6ae
1 parent 4e37803
commit 0b8a6ae
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 46 deletions.
diff --git a/src/domain/contracts/decoders/__tests__/multi-send-decoder.helper.spec.ts b/src/domain/contracts/decoders/__tests__/multi-send-decoder.helper.spec.ts
@@ -13,12 +13,17 @@ import {
   multiSendTransactionsEncoder,
 } from '@/domain/contracts/__tests__/encoders/multi-send-encoder.builder';
 import { safeBuilder } from '@/domain/safe/entities/__tests__/safe.builder';
+import { ILoggingService } from '@/logging/logging.interface';
+
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
 
 describe('MultiSendDecoder', () => {
   let target: MultiSendDecoder;
 
   beforeEach(() => {
-    target = new MultiSendDecoder();
+    target = new MultiSendDecoder(mockLoggingService);
   });
 
   describe('mapMultiSendTransactions', () => {
@@ -100,6 +105,15 @@ describe('MultiSendDecoder', () => {
         },
       ]);
     });
+
+    it("doesn't map malformed multiSend transactions (real world edge case)", () => {
+      // Ethereum safeTxHash 0x1f6e641ba0d13906745eddfd54731319e5837f4bec5c804a693e35a54d5bbea9
+      // (this is the same behavior as the Transaction Service /data-decoder endpoint)
+      const data =
+        '0x8d80ff0a00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000014d6c123eb801f385f401825e5e6f61bd5b2d44caf000000000000000000000000';
+
+      expect(target.mapMultiSendTransactions(data)).toStrictEqual([]);
+    });
   });
 
   describe('isMultiSend', () => {

diff --git a/src/domain/contracts/decoders/multi-send-decoder.helper.ts b/src/domain/contracts/decoders/multi-send-decoder.helper.ts
@@ -1,7 +1,8 @@
 import { AbiDecoder } from '@/domain/contracts/decoders/abi-decoder.helper';
-import { Injectable } from '@nestjs/common';
+import { Inject, Injectable } from '@nestjs/common';
 import { getAddress, Hex, hexToBigInt, hexToNumber, size, slice } from 'viem';
 import MultiSendCallOnly130 from '@/abis/safe/v1.3.0/MultiSendCallOnly.abi';
+import { ILoggingService, LoggingService } from '@/logging/logging.interface';
 
 @Injectable()
 export class MultiSendDecoder extends AbiDecoder<typeof MultiSendCallOnly130> {
@@ -11,7 +12,9 @@ export class MultiSendDecoder extends AbiDecoder<typeof MultiSendCallOnly130> {
   private static readonly VALUE_SIZE = 32;
   private static readonly DATA_LENGTH_SIZE = 32;
 
-  constructor() {
+  constructor(
+    @Inject(LoggingService) private readonly loggingService: ILoggingService,
+  ) {
     super(MultiSendCallOnly130);
   }
 
@@ -28,50 +31,54 @@ export class MultiSendDecoder extends AbiDecoder<typeof MultiSendCallOnly130> {
       data: Hex;
     }> = [];
 
-    const multiSend = this.decodeFunctionData({ data: multiSendData });
+    try {
+      const multiSend = this.decodeFunctionData({ data: multiSendData });
 
-    const transactions = multiSend.args[0];
-    const transactionsSize = size(transactions);
+      const transactions = multiSend.args[0];
+      const transactionsSize = size(transactions);
 
-    let cursor = 0;
+      let cursor = 0;
 
-    while (cursor < transactionsSize) {
-      const operation = slice(
-        transactions,
-        cursor,
-        (cursor += MultiSendDecoder.OPERATION_SIZE),
-      );
+      while (cursor < transactionsSize) {
+        const operation = slice(
+          transactions,
+          cursor,
+          (cursor += MultiSendDecoder.OPERATION_SIZE),
+        );
 
-      const to = slice(
-        transactions,
-        cursor,
-        (cursor += MultiSendDecoder.TO_SIZE),
-      );
+        const to = slice(
+          transactions,
+          cursor,
+          (cursor += MultiSendDecoder.TO_SIZE),
+        );
 
-      const value = slice(
-        transactions,
-        cursor,
-        (cursor += MultiSendDecoder.VALUE_SIZE),
-      );
+        const value = slice(
+          transactions,
+          cursor,
+          (cursor += MultiSendDecoder.VALUE_SIZE),
+        );
 
-      const dataLength = slice(
-        transactions,
-        cursor,
-        (cursor += MultiSendDecoder.DATA_LENGTH_SIZE),
-      );
+        const dataLength = slice(
+          transactions,
+          cursor,
+          (cursor += MultiSendDecoder.DATA_LENGTH_SIZE),
+        );
 
-      const dataLengthNumber = hexToNumber(dataLength);
-      const data =
-        dataLengthNumber === 0
-          ? '0x'
-          : slice(transactions, cursor, (cursor += dataLengthNumber));
+        const dataLengthNumber = hexToNumber(dataLength);
+        const data =
+          dataLengthNumber === 0
+            ? '0x'
+            : slice(transactions, cursor, (cursor += dataLengthNumber));
 
-      mapped.push({
-        operation: hexToNumber(operation),
-        to: getAddress(to),
-        value: hexToBigInt(value),
-        data,
-      });
+        mapped.push({
+          operation: hexToNumber(operation),
+          to: getAddress(to),
+          value: hexToBigInt(value),
+          data,
+        });
+      }
+    } catch (e) {
+      this.loggingService.warn(`Error decoding MultiSend transaction: ${e}`);
     }
 
     return mapped;

diff --git a/src/domain/relay/limit-addresses.mapper.spec.ts b/src/domain/relay/limit-addresses.mapper.spec.ts
@@ -38,6 +38,7 @@ import {
 import { getAddress } from 'viem';
 import configuration from '@/config/entities/configuration';
 import { getDeploymentVersionsByChainIds } from '@/__tests__/deployments.helper';
+import { ILoggingService } from '@/logging/logging.interface';
 
 const supportedChainIds = Object.keys(configuration().relay.apiKey);
 
@@ -62,6 +63,10 @@ const PROXY_FACTORY_VERSIONS = getDeploymentVersionsByChainIds(
   supportedChainIds,
 );
 
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
+
 const mockSafeRepository = jest.mocked({
   getSafe: jest.fn(),
 } as jest.MockedObjectDeep<ISafeRepository>);
@@ -74,7 +79,7 @@ describe('LimitAddressesMapper', () => {
 
     const erc20Decoder = new Erc20Decoder();
     const safeDecoder = new SafeDecoder();
-    const multiSendDecoder = new MultiSendDecoder();
+    const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
     const proxyFactoryDecoder = new ProxyFactoryDecoder();
 
     target = new LimitAddressesMapper(

diff --git a/src/routes/transactions/helpers/gp-v2-order.helper.spec.ts b/src/routes/transactions/helpers/gp-v2-order.helper.spec.ts
@@ -1,14 +1,19 @@
 import { MultiSendDecoder } from '@/domain/contracts/decoders/multi-send-decoder.helper';
 import { ComposableCowDecoder } from '@/domain/swaps/contracts/decoders/composable-cow-decoder.helper';
+import { ILoggingService } from '@/logging/logging.interface';
 import { GPv2OrderHelper } from '@/routes/transactions/helpers/gp-v2-order.helper';
 import { TransactionFinder } from '@/routes/transactions/helpers/transaction-finder.helper';
 import { TwapOrderHelper } from '@/routes/transactions/helpers/twap-order.helper';
 import { zeroAddress } from 'viem';
 
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
+
 describe('GPv2OrderHelper', () => {
   const target = new GPv2OrderHelper();
 
-  const multiSendDecoder = new MultiSendDecoder();
+  const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
   const transactionFinder = new TransactionFinder(multiSendDecoder);
   const composableCowDecoder = new ComposableCowDecoder();
   const twapOrderHelper = new TwapOrderHelper(

diff --git a/src/routes/transactions/helpers/kiln-native-staking.helper.spec.ts b/src/routes/transactions/helpers/kiln-native-staking.helper.spec.ts
@@ -13,6 +13,7 @@ import {
 } from '@/domain/staking/contracts/decoders/__tests__/encoders/kiln-encoder.builder';
 import { KilnDecoder } from '@/domain/staking/contracts/decoders/kiln-decoder.helper';
 import { StakingRepository } from '@/domain/staking/staking.repository';
+import { ILoggingService } from '@/logging/logging.interface';
 import { KilnNativeStakingHelper } from '@/routes/transactions/helpers/kiln-native-staking.helper';
 import { TransactionFinder } from '@/routes/transactions/helpers/transaction-finder.helper';
 import { faker } from '@faker-js/faker';
@@ -23,13 +24,17 @@ const mockStakingRepository = jest.mocked({
   getDeployment: jest.fn(),
 } as jest.MockedObjectDeep<StakingRepository>);
 
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
+
 describe('KilnNativeStakingHelper', () => {
   let target: KilnNativeStakingHelper;
 
   beforeEach(() => {
     jest.resetAllMocks();
 
-    const multiSendDecoder = new MultiSendDecoder();
+    const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
     const transactionFinder = new TransactionFinder(multiSendDecoder);
     target = new KilnNativeStakingHelper(
       transactionFinder,

diff --git a/src/routes/transactions/helpers/transaction-data-finder.helper.spec.ts b/src/routes/transactions/helpers/transaction-data-finder.helper.spec.ts
@@ -3,16 +3,21 @@ import {
   multiSendTransactionsEncoder,
 } from '@/domain/contracts/__tests__/encoders/multi-send-encoder.builder';
 import { MultiSendDecoder } from '@/domain/contracts/decoders/multi-send-decoder.helper';
+import { ILoggingService } from '@/logging/logging.interface';
 import { TransactionFinder } from '@/routes/transactions/helpers/transaction-finder.helper';
 import { faker } from '@faker-js/faker';
 import { encodeFunctionData, erc20Abi, getAddress } from 'viem';
 
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
+
 describe('TransactionFinder', () => {
   let target: TransactionFinder;
 
   beforeEach(() => {
     jest.resetAllMocks();
-    const multiSendDecoder = new MultiSendDecoder();
+    const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
     target = new TransactionFinder(multiSendDecoder);
   });
 

diff --git a/src/routes/transactions/helpers/twap-order.helper.spec.ts b/src/routes/transactions/helpers/twap-order.helper.spec.ts
@@ -14,6 +14,12 @@ import { TransactionFinder } from '@/routes/transactions/helpers/transaction-fin
 import { TwapOrderHelper } from '@/routes/transactions/helpers/twap-order.helper';
 import { faker } from '@faker-js/faker';
 import { getAddress, zeroAddress } from 'viem';
+import { ILoggingService } from '@/logging/logging.interface';
+
+const mockLoggingService = {
+  warn: jest.fn(),
+} as jest.MockedObjectDeep<ILoggingService>;
+
 describe('TwapOrderHelper', () => {
   const ComposableCowAddress = '0xfdaFc9d1902f4e0b84f65F49f244b32b31013b74';
 
@@ -29,7 +35,7 @@ describe('TwapOrderHelper', () => {
   const batchedCalldata =
     '0x8d80ff0a000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000003cb0031eac7f0141837b266de30f4dc9af15629bd538100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000024f08a03230000000000000000000000002f55e8b20d0b9fefa187aa7d00b6cbe563605bf50031eac7f0141837b266de30f4dc9af15629bd5381000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000443365582cdaee378bd0eb30ddf479272accf91761e697bc00e067a268f95f1d2732ed230b000000000000000000000000fdafc9d1902f4e0b84f65f49f244b32b31013b7400fdafc9d1902f4e0b84f65f49f244b32b31013b74000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002640d0d9800000000000000000000000000000000000000000000000000000000000000008000000000000000000000000052ed56da04309aca4c3fecc595298d80c2f16bac000000000000000000000000000000000000000000000000000000000000024000000000000000000000000000000000000000000000000000000000000000010000000000000000000000006cf1e9ca41f7611def408122793c358a3d11e5a500000000000000000000000000000000000000000000000000000019011918e600000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000140000000000000000000000000fff9976782d46cc05630d1f6ebab18b2324d6b14000000000000000000000000be72e441bf55620febc26715db68d3494213d8cb00000000000000000000000031eac7f0141837b266de30f4dc9af15629bd538100000000000000000000000000000000000000000000000003782dace9d90000000000000000000000000000000000000000000000000003b1b5fbf83bf2f7160000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000003840000000000000000000000000000000000000000000000000000000000000000f7be7261f56698c258bf75f888d68a00c85b22fb21958b9009c719eb88aebda00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000';
 
-  const multiSendDecoder = new MultiSendDecoder();
+  const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
   const transactionFinder = new TransactionFinder(multiSendDecoder);
   const composableCowDecoder = new ComposableCowDecoder();
   const configurationService = new FakeConfigurationService();

diff --git a/src/routes/transactions/mappers/common/native-staking.mapper.spec.ts b/src/routes/transactions/mappers/common/native-staking.mapper.spec.ts
@@ -76,7 +76,7 @@ describe('NativeStakingMapper', () => {
     jest.resetAllMocks();
     jest.useFakeTimers();
 
-    const multiSendDecoder = new MultiSendDecoder();
+    const multiSendDecoder = new MultiSendDecoder(mockLoggingService);
     const transactionFinder = new TransactionFinder(multiSendDecoder);
     const kilnNativeStakingHelper = new KilnNativeStakingHelper(
       transactionFinder,

diff --git a/src/routes/transactions/mappers/common/twap-order.mapper.spec.ts b/src/routes/transactions/mappers/common/twap-order.mapper.spec.ts
@@ -45,7 +45,7 @@ const mockChainsRepository = {
 
 describe('TwapOrderMapper', () => {
   const configurationService = new FakeConfigurationService();
-  const multiSendDecoder = new MultiSendDecoder();
+  const multiSendDecoder = new MultiSendDecoder(loggingService);
   const transactionFinder = new TransactionFinder(multiSendDecoder);
   const gpv2Decoder = new GPv2Decoder(mockLoggingService);
   const allowedApps = new Set<string>();