From e98de8f4e7794f45de1db3cb1354d9494b1b4791 Mon Sep 17 00:00:00 2001 From: Karl Ranna Date: Sun, 25 Aug 2019 09:35:05 +0000 Subject: [PATCH] fix(swaps): validate Raiden's resolve request In this commit we validate Raiden's resolve request to make sure the payment we're receiving has the previously agreed upon `deal.makerCltvDelta` as the minimum duration of the lock. In addition we change the error handling to return a status code 400 (Bad Request) to prevent Raiden from crashing. Previously we returned a status code of 200 with the response `{ secret: 'error message' }`. --- lib/http/HttpServer.ts | 24 +++++++++++++++--------- lib/http/HttpService.ts | 2 +- lib/p2p/Peer.ts | 1 + lib/raidenclient/types.ts | 11 +++-------- lib/swaps/Swaps.ts | 22 ++++++++++------------ lib/swaps/errors.ts | 7 ++++++- lib/swaps/types.ts | 1 + test/jest/HttpService.spec.ts | 6 ++++-- 8 files changed, 41 insertions(+), 33 deletions(-) diff --git a/lib/http/HttpServer.ts b/lib/http/HttpServer.ts index 77648c287..18794bf7e 100644 --- a/lib/http/HttpServer.ts +++ b/lib/http/HttpServer.ts @@ -63,19 +63,25 @@ class HttpServer { default: res.writeHead(404); res.end(); + break; } res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(resJson)); } catch (err) { - if (err.code === errorCodes.INVALID_ARGUMENT) { - res.writeHead(400); - res.end(err.message); - } else if (err.code === swapErrorCodes.PAYMENT_HASH_NOT_FOUND) { - res.writeHead(404); - res.end(err.message); - } else { - res.writeHead(500); - res.end(JSON.stringify(err)); + switch (err.code) { + case swapErrorCodes.INVALID_RESOLVE_REQUEST: + case errorCodes.INVALID_ARGUMENT: + res.writeHead(400); + res.end(err.message); + break; + case swapErrorCodes.PAYMENT_HASH_NOT_FOUND: + res.writeHead(404); + res.end(err.message); + break; + default: + res.writeHead(500); + res.end(JSON.stringify(err)); + break; } } } diff --git a/lib/http/HttpService.ts b/lib/http/HttpService.ts index 88784a79a..4d61625d8 100644 --- a/lib/http/HttpService.ts +++ b/lib/http/HttpService.ts @@ -5,12 +5,12 @@ class HttpService { constructor(private service: Service) {} public resolveHashRaiden = async (resolveRequest: RaidenResolveRequest): Promise => { - // TODO: add settle time out, etc const secret = await this.service.resolveHash({ rHash: resolveRequest.secrethash.slice(2), amount: resolveRequest.amount, tokenAddress: resolveRequest.token, expiration: resolveRequest.expiration, + chain_height: resolveRequest.chain_height, }); return { secret }; } diff --git a/lib/p2p/Peer.ts b/lib/p2p/Peer.ts index bdf7bd4d4..055c8a3dd 100644 --- a/lib/p2p/Peer.ts +++ b/lib/p2p/Peer.ts @@ -696,6 +696,7 @@ class Peer extends EventEmitter { this.logger.warn(`Peer (${this.label}): ${err.message}`); this.emit('reputation', ReputationEvent.WireProtocolErr); await this.close(DisconnectionReason.WireProtocolErr, err.message); + break; } }); } diff --git a/lib/raidenclient/types.ts b/lib/raidenclient/types.ts index 97170a90d..5e49968a5 100644 --- a/lib/raidenclient/types.ts +++ b/lib/raidenclient/types.ts @@ -64,15 +64,10 @@ export type RaidenResolveRequest = { secrethash: string; /** The amount of the incoming payment pending resolution, in the smallest units supported by the token. */ amount: number; - /** The maximum number of blocks allowed between the setting of a hashlock and the revealing of the related secret. */ - reveal_timeout: number; - /** The lock expiration for the incoming payment. */ + /** The lock expiration for the incoming payment (absolute block number). */ expiration: number; - // 'settle_timeout': raiden.config['settle_timeout'], - // unused fields on the raiden request listed below, taken from raiden codebase - // 'payment_identifier': secret_request_event.payment_identifier, - // 'payment_sender': to_hex(secret_request_event.recipient), - // 'payment_recipient': to_hex(raiden.address), + /** The current height of the chain */ + chain_height: number; }; export type RaidenResolveResponse = { diff --git a/lib/swaps/Swaps.ts b/lib/swaps/Swaps.ts index 089da7521..2d1e8f27e 100644 --- a/lib/swaps/Swaps.ts +++ b/lib/swaps/Swaps.ts @@ -727,25 +727,22 @@ class Swaps extends EventEmitter { * @returns `true` if the resolve request is valid, `false` otherwise */ private validateResolveRequest = (deal: SwapDeal, resolveRequest: ResolveRequest) => { - const { amount, tokenAddress, expiration } = resolveRequest; + const { amount, tokenAddress, expiration, chain_height } = resolveRequest; let expectedAmount: number; let expectedTokenAddress: string | undefined; let source: string; let destination: string; - // TODO: check cltv value switch (deal.role) { case SwapRole.Maker: expectedAmount = deal.makerUnits; expectedTokenAddress = this.swapClientManager.raidenClient.tokenAddresses.get(deal.makerCurrency); source = 'Taker'; destination = 'Maker'; - if (deal.makerCltvDelta! > expiration) { - // temporary code to relax the lock time check for raiden - this.logger.warn(`lock expiration of ${expiration} does not meet ${deal.makerCltvDelta} minimum for ${deal.rHash}`); - // end temp code - // this.logger.error(`cltvDelta of ${expiration} does not meet ${deal.makerCltvDelta!} minimum`); - // this.failDeal(deal, SwapFailureReason.InvalidResolveRequest, 'Insufficient CLTV received on first leg'); - // return false; + const lockExpirationDelta = expiration - chain_height; + if (deal.makerCltvDelta! > lockExpirationDelta) { + this.logger.error(`cltvDelta of ${lockExpirationDelta} does not meet ${deal.makerCltvDelta!} minimum`); + this.failDeal(deal, SwapFailureReason.InvalidResolveRequest, 'Insufficient CLTV received on first leg'); + return false; } break; case SwapRole.Taker: @@ -870,10 +867,10 @@ class Swaps extends EventEmitter { if (deal) { if (!this.validateResolveRequest(deal, resolveRequest)) { - return deal.errorMessage || ''; + throw errors.INVALID_RESOLVE_REQUEST(rHash, deal.errorMessage || ''); } } else { - return 'swap deal not found'; + throw errors.PAYMENT_HASH_NOT_FOUND(rHash); } try { @@ -886,7 +883,7 @@ class Swaps extends EventEmitter { return preimage; } catch (err) { this.logger.error(err.message); - return err.message; + throw err; } } @@ -1001,6 +998,7 @@ class Swaps extends EventEmitter { break; default: assert.fail('unknown deal phase'); + break; } deal.phase = newPhase; diff --git a/lib/swaps/errors.ts b/lib/swaps/errors.ts index e652ccae0..da0b5410f 100644 --- a/lib/swaps/errors.ts +++ b/lib/swaps/errors.ts @@ -6,7 +6,8 @@ const errorCodes = { SWAP_CLIENT_NOT_CONFIGURED: codesPrefix.concat('.2'), PAYMENT_HASH_NOT_FOUND: codesPrefix.concat('.3'), PAYMENT_ERROR: codesPrefix.concat('.4'), - PAYMENT_REJECTED: codesPrefix.concat('.4'), + PAYMENT_REJECTED: codesPrefix.concat('.5'), + INVALID_RESOLVE_REQUEST: codesPrefix.concat('.6'), }; const errors = { @@ -30,6 +31,10 @@ const errors = { message: 'the recipient rejected our payment for the swap', code: errorCodes.PAYMENT_REJECTED, }, + INVALID_RESOLVE_REQUEST: (rHash: string, errorMessage: string) => ({ + message: `invalid resolve request for rHash ${rHash}: ${errorMessage}`, + code: errorCodes.INVALID_RESOLVE_REQUEST, + }), }; export { errorCodes, errors }; diff --git a/lib/swaps/types.ts b/lib/swaps/types.ts index 1d769101c..90bf59f49 100644 --- a/lib/swaps/types.ts +++ b/lib/swaps/types.ts @@ -105,4 +105,5 @@ export type ResolveRequest = { tokenAddress: string, /** The number of blocks before the incoming payment expires. */ expiration: number, + chain_height: number, }; diff --git a/test/jest/HttpService.spec.ts b/test/jest/HttpService.spec.ts index 0ce16b251..e114a9b77 100644 --- a/test/jest/HttpService.spec.ts +++ b/test/jest/HttpService.spec.ts @@ -6,14 +6,15 @@ jest.mock('../../lib/service/Service'); const mockedService = >Service; const token = '0x4c354c76d5f73a63a90be776897dc81fb6238772'; -const expiration = 75; +const expiration = 7360; +const chain_height = 1000; const secretHash = '2ea852a816e4390f1468b9b1389be14e3a965479beb2c97354a409993eb52e46'; const resolveRequest: RaidenResolveRequest = { token, expiration, + chain_height, secrethash: `0x${secretHash}`, amount: 1, - reveal_timeout: 50, }; describe('HttpService', () => { @@ -35,6 +36,7 @@ describe('HttpService', () => { expect(service.resolveHash) .toHaveBeenCalledWith({ expiration, + chain_height, amount: resolveRequest.amount, rHash: secretHash, tokenAddress: token,