From 0b8a6aeee887db009ba2062e6d0c1dabf3250385 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Tue, 1 Oct 2024 11:08:39 +0200 Subject: [PATCH] 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. --- .../multi-send-decoder.helper.spec.ts | 16 +++- .../decoders/multi-send-decoder.helper.ts | 83 ++++++++++--------- .../relay/limit-addresses.mapper.spec.ts | 7 +- .../helpers/gp-v2-order.helper.spec.ts | 7 +- .../kiln-native-staking.helper.spec.ts | 7 +- .../transaction-data-finder.helper.spec.ts | 7 +- .../helpers/twap-order.helper.spec.ts | 8 +- .../common/native-staking.mapper.spec.ts | 2 +- .../mappers/common/twap-order.mapper.spec.ts | 2 +- 9 files changed, 93 insertions(+), 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 index 3e0437b548..d4415a194e 100644 --- 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; 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 index 975a5843d7..3fa0a38f36 100644 --- 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 { @@ -11,7 +12,9 @@ export class MultiSendDecoder extends AbiDecoder { 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 { 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 index f4b623dace..a90329b4d1 100644 --- 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; + const mockSafeRepository = jest.mocked({ getSafe: jest.fn(), } as jest.MockedObjectDeep); @@ -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 index 25768877bd..318ab7a8cf 100644 --- 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; + 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 index 925bd5b0bb..0c1577afb2 100644 --- 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); +const mockLoggingService = { + warn: jest.fn(), +} as jest.MockedObjectDeep; + 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 index 3556edddb1..1fcd171311 100644 --- 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; + 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 index 427b3c2c0e..ce3270cab2 100644 --- 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; + 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 index 6d1e98a69d..b95efe4393 100644 --- 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 index e6980541ac..244244a8e6 100644 --- 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();