From 1fb9f5ca03267d84d5a107c39e7b372f1fdce949 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Wed, 13 Mar 2024 12:28:08 +0100 Subject: [PATCH] Only allow enabling/disabling recovery alerts for modules of given Safe (#1267) This includes a check that the specified `moduleAddress` is enabled on that `safeAddress` to the `EnableRecoveryAlertsGuard`/`DisableRecoveryAlertsGuard` guards: - Add check that `moduleAddress` is enabled on `safeAddress` in `EnableRecoveryAlertsGuard`/`DisableRecoveryAlertsGuard` - Add test coverage --- .../disable-recovery-alerts.guard.spec.ts | 34 ++- .../guards/disable-recovery-alerts.guard.ts | 17 +- .../enable-recovery-alerts.guard.spec.ts | 34 ++- .../guards/enable-recovery-alerts.guard.ts | 17 +- .../recovery/recovery.controller.spec.ts | 197 +++++++++++++++++- 5 files changed, 292 insertions(+), 7 deletions(-) diff --git a/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts b/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts index ce016753ae..e4c23fd691 100644 --- a/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts +++ b/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts @@ -14,6 +14,12 @@ import { faker } from '@faker-js/faker'; import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts'; import { Hash } from 'viem'; import { DisableRecoveryAlertsGuard } from '@/routes/recovery/guards/disable-recovery-alerts.guard'; +import { ISafeRepository } from '@/domain/safe/safe.repository.interface'; + +const safeRepository = { + getSafesByModule: jest.fn(), +} as jest.MockedObjectDeep; +const safeRepositoryMock = jest.mocked(safeRepository); @Controller() class TestController { @@ -54,6 +60,12 @@ describe('DisableRecoveryAlertsGuard guard tests', () => { const moduleFixture: TestingModule = await Test.createTestingModule({ imports: [TestLoggingModule, ConfigurationModule.register(configuration)], controllers: [TestController], + providers: [ + { + provide: ISafeRepository, + useValue: safeRepositoryMock, + }, + ], }).compile(); app = await new TestAppProvider().provide(moduleFixture); await app.init(); @@ -63,7 +75,11 @@ describe('DisableRecoveryAlertsGuard guard tests', () => { await app.close(); }); - it('returns 200 for a valid signature', async () => { + it('returns 200 for a valid signature for module on given Safe', async () => { + safeRepositoryMock.getSafesByModule.mockResolvedValue({ + safes: [safeAddress], + }); + await request(app.getHttpServer()) .delete(`/test/${chainId}/${safeAddress}/${moduleAddress}`) .set('Safe-Wallet-Signature', signature) @@ -75,6 +91,22 @@ describe('DisableRecoveryAlertsGuard guard tests', () => { .expect(200); }); + it('returns 403 for a valid signature for module not on given Safe', async () => { + safeRepositoryMock.getSafesByModule.mockResolvedValue({ + safes: [], + }); + + await request(app.getHttpServer()) + .delete(`/test/${chainId}/${safeAddress}/${moduleAddress}`) + .set('Safe-Wallet-Signature', signature) + .set('Safe-Wallet-Signature-Timestamp', timestamp.toString()) + .send({ + moduleAddress, + signer: signer.address, + }) + .expect(403); + }); + it('returns 403 for an invalid signature', async () => { const invalidSignature = await signer.signMessage({ message: 'some invalid message', diff --git a/src/routes/recovery/guards/disable-recovery-alerts.guard.ts b/src/routes/recovery/guards/disable-recovery-alerts.guard.ts index 6031ee809f..549663fc27 100644 --- a/src/routes/recovery/guards/disable-recovery-alerts.guard.ts +++ b/src/routes/recovery/guards/disable-recovery-alerts.guard.ts @@ -5,7 +5,8 @@ import { Injectable, } from '@nestjs/common'; import { ILoggingService, LoggingService } from '@/logging/logging.interface'; -import { verifyMessage } from 'viem'; +import { getAddress, verifyMessage } from 'viem'; +import { ISafeRepository } from '@/domain/safe/safe.repository.interface'; /** * The DisableRecoveryAlertsGuard guard should be used on routes that require @@ -29,6 +30,7 @@ import { verifyMessage } from 'viem'; @Injectable() export class DisableRecoveryAlertsGuard implements CanActivate { constructor( + @Inject(ISafeRepository) private readonly safeRepository: ISafeRepository, @Inject(LoggingService) private readonly loggingService: ILoggingService, ) {} @@ -58,11 +60,22 @@ export class DisableRecoveryAlertsGuard implements CanActivate { const message = `${DisableRecoveryAlertsGuard.ACTION_PREFIX}-${chainId}-${safeAddress}-${moduleAddress}-${signer}-${timestamp}`; try { - return await verifyMessage({ + const isValid = await verifyMessage({ address: signer, message, signature, }); + + if (!isValid) { + return false; + } + + const { safes } = await this.safeRepository.getSafesByModule({ + chainId, + moduleAddress, + }); + + return safes.some((safe) => getAddress(safe) === getAddress(safeAddress)); } catch (e) { this.loggingService.debug(e); return false; diff --git a/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts b/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts index f787f77547..5adfb3c28c 100644 --- a/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts +++ b/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts @@ -9,6 +9,12 @@ import { faker } from '@faker-js/faker'; import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts'; import { Hash } from 'viem'; import { EnableRecoveryAlertsGuard } from '@/routes/recovery/guards/enable-recovery-alerts.guard'; +import { ISafeRepository } from '@/domain/safe/safe.repository.interface'; + +const safeRepository = { + getSafesByModule: jest.fn(), +} as jest.MockedObjectDeep; +const safeRepositoryMock = jest.mocked(safeRepository); @Controller() class TestController { @@ -45,6 +51,12 @@ describe('EnableRecoveryAlertsGuard guard tests', () => { const moduleFixture: TestingModule = await Test.createTestingModule({ imports: [TestLoggingModule, ConfigurationModule.register(configuration)], controllers: [TestController], + providers: [ + { + provide: ISafeRepository, + useValue: safeRepositoryMock, + }, + ], }).compile(); app = await new TestAppProvider().provide(moduleFixture); await app.init(); @@ -54,7 +66,11 @@ describe('EnableRecoveryAlertsGuard guard tests', () => { await app.close(); }); - it('returns 201 for a valid signature', async () => { + it('returns 201 for a valid signature for module on given Safe', async () => { + safeRepositoryMock.getSafesByModule.mockResolvedValue({ + safes: [safeAddress], + }); + await request(app.getHttpServer()) .post(`/test/${chainId}/${safeAddress}`) .set('Safe-Wallet-Signature', signature) @@ -66,6 +82,22 @@ describe('EnableRecoveryAlertsGuard guard tests', () => { .expect(201); }); + it('returns 403 for a valid signature for module not on given Safe', async () => { + safeRepositoryMock.getSafesByModule.mockResolvedValue({ + safes: [], + }); + + await request(app.getHttpServer()) + .post(`/test/${chainId}/${safeAddress}`) + .set('Safe-Wallet-Signature', signature) + .set('Safe-Wallet-Signature-Timestamp', timestamp.toString()) + .send({ + moduleAddress, + signer: signer.address, + }) + .expect(403); + }); + it('returns 403 for an invalid signature', async () => { const invalidSignature = await signer.signMessage({ message: 'some invalid message', diff --git a/src/routes/recovery/guards/enable-recovery-alerts.guard.ts b/src/routes/recovery/guards/enable-recovery-alerts.guard.ts index 318b249682..793fd2a463 100644 --- a/src/routes/recovery/guards/enable-recovery-alerts.guard.ts +++ b/src/routes/recovery/guards/enable-recovery-alerts.guard.ts @@ -5,7 +5,8 @@ import { Injectable, } from '@nestjs/common'; import { ILoggingService, LoggingService } from '@/logging/logging.interface'; -import { verifyMessage } from 'viem'; +import { getAddress, verifyMessage } from 'viem'; +import { ISafeRepository } from '@/domain/safe/safe.repository.interface'; /** * The EnableRecoveryAlertsGuard guard should be used on routes that require @@ -29,6 +30,7 @@ import { verifyMessage } from 'viem'; @Injectable() export class EnableRecoveryAlertsGuard implements CanActivate { constructor( + @Inject(ISafeRepository) private readonly safeRepository: ISafeRepository, @Inject(LoggingService) private readonly loggingService: ILoggingService, ) {} @@ -58,11 +60,22 @@ export class EnableRecoveryAlertsGuard implements CanActivate { const message = `${EnableRecoveryAlertsGuard.ACTION_PREFIX}-${chainId}-${safeAddress}-${moduleAddress}-${signer}-${timestamp}`; try { - return await verifyMessage({ + const isValid = await verifyMessage({ address: signer, message, signature, }); + + if (!isValid) { + return false; + } + + const { safes } = await this.safeRepository.getSafesByModule({ + chainId, + moduleAddress, + }); + + return safes.some((safe) => getAddress(safe) === getAddress(safeAddress)); } catch (e) { this.loggingService.debug(e); return false; diff --git a/src/routes/recovery/recovery.controller.spec.ts b/src/routes/recovery/recovery.controller.spec.ts index c3e79f64fe..b2e5c9e20d 100644 --- a/src/routes/recovery/recovery.controller.spec.ts +++ b/src/routes/recovery/recovery.controller.spec.ts @@ -97,6 +97,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.post.mockImplementation(({ url }) => @@ -112,12 +121,59 @@ describe('Recovery (Unit)', () => { .set('Safe-Wallet-Signature-Timestamp', timestamp.toString()) .send({ ...addRecoveryModuleDto, - // TODO: Add to request signer: signer.address, }) .expect(200); }); + it('should prevent requests for modules not on specified Safe', async () => { + const addRecoveryModuleDto = addRecoveryModuleDtoBuilder().build(); + const privateKey = generatePrivateKey(); + const signer = privateKeyToAccount(privateKey); + const chain = chainBuilder().build(); + const safe = safeBuilder().with('owners', [signer.address]).build(); + const timestamp = jest.now(); + const message = `enable-recovery-alerts-${chain.chainId}-${safe.address}-${addRecoveryModuleDto.moduleAddress}-${signer.address}-${timestamp}`; + const signature = await signer.signMessage({ message }); + + networkService.get.mockImplementation(({ url }) => { + if (url === `${safeConfigUrl}/api/v1/chains/${chain.chainId}`) { + return Promise.resolve({ status: 200, data: chain }); + } + if ( + url === `${chain.transactionService}/api/v1/safes/${safe.address}` + ) { + return Promise.resolve({ status: 200, data: safe }); + } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [] }, + }); + } + return Promise.reject(`No matching rule for url: ${url}`); + }); + networkService.post.mockImplementation(({ url }) => + url === + `${alertsUrl}/api/v1/account/${alertsAccount}/project/${alertsProject}/address` + ? Promise.resolve({ status: 200, data: {} }) + : Promise.reject(`No matching rule for url: ${url}`), + ); + + await request(app.getHttpServer()) + .post(`/v1/chains/${chain.chainId}/safes/${safe.address}/recovery`) + .set('Safe-Wallet-Signature', signature) + .set('Safe-Wallet-Signature-Timestamp', timestamp.toString()) + .send({ + ...addRecoveryModuleDto, + signer: signer.address, + }) + .expect(403); + }); + it('should prevent requests older than 5 minutes', async () => { const addRecoveryModuleDto = addRecoveryModuleDtoBuilder().build(); const privateKey = generatePrivateKey(); @@ -137,6 +193,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); @@ -172,6 +237,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); @@ -207,6 +281,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.post.mockImplementation(({ url }) => @@ -259,6 +342,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.post.mockImplementation(({ url }) => @@ -308,6 +400,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${addRecoveryModuleDto.moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.post.mockImplementation(({ url }) => @@ -348,6 +449,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.delete.mockImplementation(({ url }) => @@ -369,6 +479,55 @@ describe('Recovery (Unit)', () => { .expect(204); }); + it('should prevent requests for modules not on specified Safe', async () => { + const moduleAddress = faker.finance.ethereumAddress(); + const privateKey = generatePrivateKey(); + const signer = privateKeyToAccount(privateKey); + const chain = chainBuilder().build(); + const safe = safeBuilder().with('owners', [signer.address]).build(); + const timestamp = jest.now(); + const message = `disable-recovery-alerts-${chain.chainId}-${safe.address}-${moduleAddress}-${signer.address}-${timestamp}`; + const signature = await signer.signMessage({ message }); + + networkService.get.mockImplementation(({ url }) => { + if (url === `${safeConfigUrl}/api/v1/chains/${chain.chainId}`) { + return Promise.resolve({ status: 200, data: chain }); + } + if ( + url === `${chain.transactionService}/api/v1/safes/${safe.address}` + ) { + return Promise.resolve({ status: 200, data: safe }); + } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [] }, + }); + } + return Promise.reject(`No matching rule for url: ${url}`); + }); + networkService.delete.mockImplementation(({ url }) => + url === + `${alertsUrl}/api/v1/account/${alertsAccount}/project/${alertsProject}/contract/${chain.chainId}/${moduleAddress}` + ? Promise.resolve({ status: 204, data: {} }) + : Promise.reject(`No matching rule for url: ${url}`), + ); + + await request(app.getHttpServer()) + .delete( + `/v1/chains/${chain.chainId}/safes/${safe.address}/recovery/${moduleAddress}`, + ) + .set('Safe-Wallet-Signature', signature) + .set('Safe-Wallet-Signature-Timestamp', timestamp.toString()) + .send({ + signer: signer.address, + }) + .expect(403); + }); + it('should prevent requests older than 5 minutes', async () => { const moduleAddress = faker.finance.ethereumAddress(); const privateKey = generatePrivateKey(); @@ -388,6 +547,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.delete.mockImplementation(({ url }) => @@ -430,6 +598,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.delete.mockImplementation(({ url }) => @@ -482,6 +659,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.delete.mockImplementation(({ url }) => @@ -537,6 +723,15 @@ describe('Recovery (Unit)', () => { ) { return Promise.resolve({ status: 200, data: safe }); } + if ( + url === + `${chain.transactionService}/api/v1/modules/${moduleAddress}/safes/` + ) { + return Promise.resolve({ + status: 200, + data: { safes: [safe.address] }, + }); + } return Promise.reject(`No matching rule for url: ${url}`); }); networkService.delete.mockImplementation(({ url }) =>