From 5cb5b9c8f125c8c92f9f0c6d1e53008e4770f383 Mon Sep 17 00:00:00 2001 From: George Avsetsin Date: Thu, 14 Jul 2022 09:56:38 +0300 Subject: [PATCH] feat: exclude eligible intersections --- src/bls/bls.service.ts | 1 + src/contracts/deposit/deposit.service.spec.ts | 5 ++ src/contracts/lido/index.ts | 2 + src/contracts/lido/lido.module.ts | 8 ++ src/contracts/lido/lido.service.spec.ts | 47 +++++++++++ src/contracts/lido/lido.service.ts | 19 +++++ src/guardian/guardian.module.ts | 9 ++- src/guardian/guardian.service.spec.ts | 78 ++++++++++++++++++- src/guardian/guardian.service.ts | 67 +++++++++++----- 9 files changed, 213 insertions(+), 23 deletions(-) create mode 100644 src/contracts/lido/index.ts create mode 100644 src/contracts/lido/lido.module.ts create mode 100644 src/contracts/lido/lido.service.spec.ts create mode 100644 src/contracts/lido/lido.service.ts diff --git a/src/bls/bls.service.ts b/src/bls/bls.service.ts index e2a88bb5..f2d69dcf 100644 --- a/src/bls/bls.service.ts +++ b/src/bls/bls.service.ts @@ -64,6 +64,7 @@ export class BlsService implements OnModuleInit { return blst.verify(signingRoot, blsPublicKey, blsSignature); } catch (error) { + this.logger.warn('Failed to verify deposit data', depositData); this.logger.error(error); return false; diff --git a/src/contracts/deposit/deposit.service.spec.ts b/src/contracts/deposit/deposit.service.spec.ts index 1df31ee8..52bbea77 100644 --- a/src/contracts/deposit/deposit.service.spec.ts +++ b/src/contracts/deposit/deposit.service.spec.ts @@ -22,6 +22,7 @@ import { PrometheusModule } from 'common/prometheus'; import { LoggerModule } from 'common/logger'; import { ConfigModule } from 'common/config'; import { APP_VERSION } from 'app.constants'; +import { BlsService } from 'bls'; const mockSleep = sleep as jest.MockedFunction; @@ -31,6 +32,7 @@ describe('DepositService', () => { let depositService: DepositService; let loggerService: LoggerService; let repositoryService: RepositoryService; + let blsService: BlsService; const depositAddress = '0x' + '1'.repeat(40); @@ -50,6 +52,7 @@ describe('DepositService', () => { cacheService = moduleRef.get(CacheService); depositService = moduleRef.get(DepositService); repositoryService = moduleRef.get(RepositoryService); + blsService = moduleRef.get(BlsService); loggerService = moduleRef.get(WINSTON_MODULE_NEST_PROVIDER); jest.spyOn(loggerService, 'log').mockImplementation(() => undefined); @@ -251,6 +254,8 @@ describe('DepositService', () => { .spyOn(providerService.provider, 'getBlockNumber') .mockImplementation(async () => endBlock); + jest.spyOn(blsService, 'verify').mockImplementation(() => true); + const mockProviderCall = jest .spyOn(providerService.provider, 'getLogs') .mockImplementation(async () => { diff --git a/src/contracts/lido/index.ts b/src/contracts/lido/index.ts new file mode 100644 index 00000000..9272f996 --- /dev/null +++ b/src/contracts/lido/index.ts @@ -0,0 +1,2 @@ +export * from './lido.module'; +export * from './lido.service'; diff --git a/src/contracts/lido/lido.module.ts b/src/contracts/lido/lido.module.ts new file mode 100644 index 00000000..a430066d --- /dev/null +++ b/src/contracts/lido/lido.module.ts @@ -0,0 +1,8 @@ +import { Module } from '@nestjs/common'; +import { LidoService } from './lido.service'; + +@Module({ + providers: [LidoService], + exports: [LidoService], +}) +export class LidoModule {} diff --git a/src/contracts/lido/lido.service.spec.ts b/src/contracts/lido/lido.service.spec.ts new file mode 100644 index 00000000..94eae293 --- /dev/null +++ b/src/contracts/lido/lido.service.spec.ts @@ -0,0 +1,47 @@ +import { Test } from '@nestjs/testing'; +import { ConfigModule } from 'common/config'; +import { LoggerModule } from 'common/logger'; +import { MockProviderModule, ProviderService } from 'provider'; +import { LidoAbi__factory } from 'generated'; +import { RepositoryModule } from 'contracts/repository'; +import { Interface } from '@ethersproject/abi'; +import { LidoService } from './lido.service'; +import { LidoModule } from './lido.module'; + +describe('SecurityService', () => { + let lidoService: LidoService; + let providerService: ProviderService; + + beforeEach(async () => { + const moduleRef = await Test.createTestingModule({ + imports: [ + ConfigModule.forRoot(), + MockProviderModule.forRoot(), + LoggerModule, + LidoModule, + RepositoryModule, + ], + }).compile(); + + lidoService = moduleRef.get(LidoService); + providerService = moduleRef.get(ProviderService); + }); + + describe('getWithdrawalCredentials', () => { + it('should return withdrawal credentials', async () => { + const expected = '0x' + '1'.repeat(64); + + const mockProviderCall = jest + .spyOn(providerService.provider, 'call') + .mockImplementation(async () => { + const iface = new Interface(LidoAbi__factory.abi); + const result = [expected]; + return iface.encodeFunctionResult('getWithdrawalCredentials', result); + }); + + const wc = await lidoService.getWithdrawalCredentials(); + expect(wc).toBe(expected); + expect(mockProviderCall).toBeCalledTimes(1); + }); + }); +}); diff --git a/src/contracts/lido/lido.service.ts b/src/contracts/lido/lido.service.ts new file mode 100644 index 00000000..a7cd8c98 --- /dev/null +++ b/src/contracts/lido/lido.service.ts @@ -0,0 +1,19 @@ +import { Injectable } from '@nestjs/common'; +import { RepositoryService } from 'contracts/repository'; +import { BlockTag } from 'provider'; + +@Injectable() +export class LidoService { + constructor(private repositoryService: RepositoryService) {} + + /** + * Returns withdrawal credentials from the contract + */ + public async getWithdrawalCredentials(blockTag?: BlockTag): Promise { + const contract = await this.repositoryService.getCachedLidoContract(); + + return await contract.getWithdrawalCredentials({ + blockTag: blockTag as any, + }); + } +} diff --git a/src/guardian/guardian.module.ts b/src/guardian/guardian.module.ts index b6b1e59b..a7f67b1f 100644 --- a/src/guardian/guardian.module.ts +++ b/src/guardian/guardian.module.ts @@ -2,11 +2,18 @@ import { Module } from '@nestjs/common'; import { DepositModule } from 'contracts/deposit'; import { RegistryModule } from 'contracts/registry'; import { SecurityModule } from 'contracts/security'; +import { LidoModule } from 'contracts/lido'; import { MessagesModule } from 'messages'; import { GuardianService } from './guardian.service'; @Module({ - imports: [RegistryModule, DepositModule, SecurityModule, MessagesModule], + imports: [ + RegistryModule, + DepositModule, + SecurityModule, + LidoModule, + MessagesModule, + ], providers: [GuardianService], exports: [GuardianService], }) diff --git a/src/guardian/guardian.service.spec.ts b/src/guardian/guardian.service.spec.ts index 1bd58a9e..e2aed6da 100644 --- a/src/guardian/guardian.service.spec.ts +++ b/src/guardian/guardian.service.spec.ts @@ -11,6 +11,7 @@ import { GuardianModule } from 'guardian'; import { DepositService } from 'contracts/deposit'; import { RegistryService } from 'contracts/registry'; import { SecurityService } from 'contracts/security'; +import { LidoService } from 'contracts/lido'; import { RepositoryModule, RepositoryService } from 'contracts/repository'; import { MessagesService, MessageType } from 'messages'; @@ -20,6 +21,7 @@ describe('GuardianService', () => { let loggerService: LoggerService; let depositService: DepositService; let registryService: RegistryService; + let lidoService: LidoService; let messagesService: MessagesService; let securityService: SecurityService; let repositoryService: RepositoryService; @@ -40,6 +42,7 @@ describe('GuardianService', () => { guardianService = moduleRef.get(GuardianService); depositService = moduleRef.get(DepositService); registryService = moduleRef.get(RegistryService); + lidoService = moduleRef.get(LidoService); messagesService = moduleRef.get(MessagesService); securityService = moduleRef.get(SecurityService); repositoryService = moduleRef.get(RepositoryService); @@ -79,7 +82,7 @@ describe('GuardianService', () => { expect(matched).toBeInstanceOf(Array); expect(matched).toHaveLength(1); - expect(matched).toContain('0x1'); + expect(matched).toContainEqual({ pubkey: '0x1' }); }); it('should not find the keys when they don’t match', () => { @@ -131,7 +134,7 @@ describe('GuardianService', () => { expect(matched).toBeInstanceOf(Array); expect(matched).toHaveLength(1); - expect(matched).toContain(pubkey); + expect(matched).toContainEqual({ pubkey }); }); it('should not find the keys when they don’t match', () => { @@ -231,11 +234,15 @@ describe('GuardianService', () => { }); describe('checkKeysIntersections', () => { + const lidoWC = '0x12'; + const attackerWC = '0x23'; const depositedPubKeys = ['0x1234', '0x5678']; const depositedEvents = { startBlock: 1, endBlock: 5, - events: depositedPubKeys.map((pubkey) => ({ pubkey } as any)), + events: depositedPubKeys.map( + (pubkey) => ({ pubkey, valid: true } as any), + ), }; const nodeOperatorsCache = { depositRoot: '0x2345', @@ -260,7 +267,15 @@ describe('GuardianService', () => { it('should call handleKeysIntersections if next keys are found in the deposit contract', async () => { const depositedKey = depositedPubKeys[0]; const nextSigningKeys = [depositedKey]; - const blockData = { ...currentBlockData, nextSigningKeys }; + const events = currentBlockData.depositedEvents.events.map( + ({ ...data }) => ({ ...data, wc: attackerWC } as any), + ); + + const blockData = { + ...currentBlockData, + depositedEvents: { ...currentBlockData.depositedEvents, events }, + nextSigningKeys, + }; const mockHandleCorrectKeys = jest .spyOn(guardianService, 'handleCorrectKeys') @@ -270,11 +285,16 @@ describe('GuardianService', () => { .spyOn(guardianService, 'handleKeysIntersections') .mockImplementation(async () => undefined); + const mockGetWithdrawalCredentials = jest + .spyOn(lidoService, 'getWithdrawalCredentials') + .mockImplementation(async () => lidoWC); + await guardianService.checkKeysIntersections(blockData); expect(mockHandleCorrectKeys).not.toBeCalled(); expect(mockHandleKeysIntersections).toBeCalledTimes(1); expect(mockHandleKeysIntersections).toBeCalledWith(blockData); + expect(mockGetWithdrawalCredentials).toBeCalledTimes(1); }); it('should call handleCorrectKeys if Lido next keys are not found in the deposit contract', async () => { @@ -349,6 +369,56 @@ describe('GuardianService', () => { }); }); + describe('excludeEligibleIntersections', () => { + const pubkey = '0x1234'; + const lidoWC = '0x12'; + const attackerWC = '0x23'; + const blockData = { blockHash: '0x1234' } as any; + + beforeEach(async () => { + jest + .spyOn(lidoService, 'getWithdrawalCredentials') + .mockImplementation(async () => lidoWC); + }); + + it('should exclude invalid intersections', async () => { + const intersections = [{ valid: false, pubkey, wc: lidoWC } as any]; + + const filteredIntersections = + await guardianService.excludeEligibleIntersections( + intersections, + blockData, + ); + + expect(filteredIntersections).toHaveLength(0); + }); + + it('should exclude intersections with lido WC', async () => { + const intersections = [{ valid: true, pubkey, wc: lidoWC } as any]; + + const filteredIntersections = + await guardianService.excludeEligibleIntersections( + intersections, + blockData, + ); + + expect(filteredIntersections).toHaveLength(0); + }); + + it('should not exclude intersections with attacker WC', async () => { + const intersections = [{ valid: true, pubkey, wc: attackerWC } as any]; + + const filteredIntersections = + await guardianService.excludeEligibleIntersections( + intersections, + blockData, + ); + + expect(filteredIntersections).toHaveLength(1); + expect(filteredIntersections).toEqual(intersections); + }); + }); + describe('handleKeysIntersections', () => { const signature = {} as any; const blockData = { blockNumber: 1 } as any; diff --git a/src/guardian/guardian.service.ts b/src/guardian/guardian.service.ts index 229918a4..b09cd961 100644 --- a/src/guardian/guardian.service.ts +++ b/src/guardian/guardian.service.ts @@ -6,9 +6,10 @@ import { } from '@nestjs/common'; import { Block } from '@ethersproject/providers'; import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston'; -import { DepositService } from 'contracts/deposit'; +import { DepositService, VerifiedDepositEvent } from 'contracts/deposit'; import { RegistryService } from 'contracts/registry'; import { SecurityService } from 'contracts/security'; +import { LidoService } from 'contracts/lido'; import { RepositoryService } from 'contracts/repository'; import { ProviderService } from 'provider'; import { @@ -60,6 +61,7 @@ export class GuardianService implements OnModuleInit { private providerService: ProviderService, private messagesService: MessagesService, private repositoryService: RepositoryService, + private lidoService: LidoService, ) {} public async onModuleInit(): Promise { @@ -205,9 +207,13 @@ export class GuardianService implements OnModuleInit { const nextKeysIntersections = this.getNextKeysIntersections(blockData); const cachedKeysIntersections = this.getCachedKeysIntersections(blockData); const intersections = nextKeysIntersections.concat(cachedKeysIntersections); - const isIntersectionsFound = intersections.length > 0; + const filteredIntersections = await this.excludeEligibleIntersections( + intersections, + blockData, + ); + const isFilteredIntersectionsFound = filteredIntersections.length > 0; - if (isIntersectionsFound) { + if (isFilteredIntersectionsFound) { await this.handleKeysIntersections(blockData); } else { await this.handleCorrectKeys(blockData); @@ -220,16 +226,16 @@ export class GuardianService implements OnModuleInit { * @param blockData - collected data from the current block * @returns list of keys that were deposited earlier */ - public getNextKeysIntersections(blockData: BlockData): string[] { + public getNextKeysIntersections( + blockData: BlockData, + ): VerifiedDepositEvent[] { const { blockHash } = blockData; const { depositedEvents, nextSigningKeys } = blockData; const { depositRoot, keysOpIndex } = blockData; - const depositedKeys = depositedEvents.events.map(({ pubkey }) => pubkey); - const depositedKeysSet = new Set(depositedKeys); - - const intersections = nextSigningKeys.filter((nextSigningKey) => - depositedKeysSet.has(nextSigningKey), + const nextSigningKeysSet = new Set(nextSigningKeys); + const intersections = depositedEvents.events.filter(({ pubkey }) => + nextSigningKeysSet.has(pubkey), ); if (intersections.length) { @@ -250,7 +256,9 @@ export class GuardianService implements OnModuleInit { * @param blockData - collected data from the current block * @returns list of keys that were deposited earlier */ - public getCachedKeysIntersections(blockData: BlockData): string[] { + public getCachedKeysIntersections( + blockData: BlockData, + ): VerifiedDepositEvent[] { const { blockHash } = blockData; const { keysOpIndex, depositRoot, depositedEvents } = blockData; const cache = blockData.nodeOperatorsCache; @@ -262,14 +270,13 @@ export class GuardianService implements OnModuleInit { // Skip checking until the next cache update if (!isCacheUpToDate) return []; - const depositedKeys = depositedEvents.events.map(({ pubkey }) => pubkey); - const depositedKeysSet = new Set(depositedKeys); + const unusedOperatorsKeys = cache.operators.flatMap((operator) => + operator.keys.filter(({ used }) => used === false).map(({ key }) => key), + ); + const unusedOperatorsKeysSet = new Set(unusedOperatorsKeys); - const intersections = cache.operators.flatMap((operator) => - operator.keys - .filter(({ used }) => used === false) - .filter(({ key }) => depositedKeysSet.has(key)) - .map(({ key }) => key), + const intersections = depositedEvents.events.filter(({ pubkey }) => + unusedOperatorsKeysSet.has(pubkey), ); if (intersections.length) { @@ -284,10 +291,34 @@ export class GuardianService implements OnModuleInit { return intersections; } + /** + * Excludes invalid deposits and deposits with Lido WC from intersections + * @param intersections - list of deposits with keys that were deposited earlier + * @param blockData - collected data from the current block + */ + public async excludeEligibleIntersections( + intersections: VerifiedDepositEvent[], + blockData: BlockData, + ): Promise { + // Exclude deposits with invalid signature over the deposit data + const validIntersections = intersections.filter(({ valid }) => valid); + if (!validIntersections.length) return []; + + // Exclude deposits with Lido withdrawal credentials + const { blockHash } = blockData; + const lidoWC = await this.lidoService.getWithdrawalCredentials({ + blockHash, + }); + const attackIntersections = validIntersections.filter( + (deposit) => deposit.wc !== lidoWC, + ); + + return attackIntersections; + } + /** * Handles the situation when keys have previously deposited copies * @param blockData - collected data from the current block - * @param intersections - list of keys that were deposited earlier */ public async handleKeysIntersections(blockData: BlockData): Promise { const {