From 65e99f902786a252381b850a23f73a7c1e12a111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Tue, 26 Nov 2024 16:18:12 -0300 Subject: [PATCH] fix: prevent payers from being able to bypass escrow mechanism (TRST-H01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/interfaces/ITAPCollector.sol | 6 + .../payments/collectors/TAPCollector.sol | 11 ++ .../tap-collector/collect/collect.t.sol | 141 ++++++++++++++---- 3 files changed, 131 insertions(+), 27 deletions(-) diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index dd557de53..d04f142d6 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/packages/horizon/contracts/interfaces/ITAPCollector.sol @@ -137,6 +137,12 @@ interface ITAPCollector is IPaymentsCollector { */ error TAPCollectorInvalidRAVSigner(); + /** + * Thrown when the RAV is for a data service the service provider has no provision for + * @param dataService The address of the data service + */ + error TAPCollectorUnauthorizedDataService(address dataService); + /** * Thrown when the caller is not the data service the RAV was issued to * @param caller The address of the caller diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index 57588a042..6bae89fa4 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity 0.8.27; +import { IHorizonStaking } from "../../interfaces/IHorizonStaking.sol"; import { IGraphPayments } from "../../interfaces/IGraphPayments.sol"; import { ITAPCollector } from "../../interfaces/ITAPCollector.sol"; @@ -118,6 +119,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { * @notice Initiate a payment collection through the payments protocol * See {IGraphPayments.collect}. * @dev Caller must be the data service the RAV was issued to. + * @dev Service provider must have an active provision with the data service to collect payments * @notice REVERT: This function may revert if ECDSA.recover fails, check ECDSA library for details. */ function collect(IGraphPayments.PaymentTypes paymentType, bytes memory data) external override returns (uint256) { @@ -130,6 +132,15 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { address signer = _recoverRAVSigner(signedRAV); require(authorizedSigners[signer].payer != address(0), TAPCollectorInvalidRAVSigner()); + // Check the service provider has an active provision with the data service + // This prevents an attack where the payer can deny the service provider from collecting payments + // by using a signer as data service to syphon off the tokens in the escrow to an account they control + uint256 tokensAvailable = _graphStaking().getProviderTokensAvailable( + signedRAV.rav.serviceProvider, + signedRAV.rav.dataService + ); + require(tokensAvailable > 0, TAPCollectorUnauthorizedDataService(signedRAV.rav.dataService)); + return _collect(paymentType, authorizedSigners[signer].payer, signedRAV, dataServiceCut); } diff --git a/packages/horizon/test/payments/tap-collector/collect/collect.t.sol b/packages/horizon/test/payments/tap-collector/collect/collect.t.sol index 06b0e027a..555c9c50e 100644 --- a/packages/horizon/test/payments/tap-collector/collect/collect.t.sol +++ b/packages/horizon/test/payments/tap-collector/collect/collect.t.sol @@ -9,10 +9,9 @@ import { IGraphPayments } from "../../../../contracts/interfaces/IGraphPayments. import { TAPCollectorTest } from "../TAPCollector.t.sol"; contract TAPCollectorCollectTest is TAPCollectorTest { - /* - * HELPERS - */ + * HELPERS + */ function _getQueryFeeEncodedData( uint256 _signerPrivateKey, @@ -47,22 +46,27 @@ contract TAPCollectorCollectTest is TAPCollectorTest { * TESTS */ - function testTAPCollector_Collect(uint256 tokens) public useGateway useSigner { + function testTAPCollector_Collect( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); } - function testTAPCollector_Collect_Multiple(uint256 tokens, uint8 steps) public useGateway useSigner { + function testTAPCollector_Collect_Multiple( + uint256 tokens, + uint8 steps + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { steps = uint8(bound(steps, 1, 100)); tokens = bound(tokens, steps, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); @@ -70,15 +74,94 @@ contract TAPCollectorCollectTest is TAPCollectorTest { uint256 payed = 0; uint256 tokensPerStep = tokens / steps; for (uint256 i = 0; i < steps; i++) { - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(payed + tokensPerStep)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.indexer, + users.verifier, + uint128(payed + tokensPerStep) + ); _collect(IGraphPayments.PaymentTypes.QueryFee, data); payed += tokensPerStep; } } + function testTAPCollector_Collect_RevertWhen_NoProvision(uint256 tokens) public useGateway useSigner { + tokens = bound(tokens, 1, type(uint128).max); + + _approveCollector(address(tapCollector), tokens); + _depositTokens(address(tapCollector), users.indexer, tokens); + + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + + resetPrank(users.verifier); + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorUnauthorizedDataService.selector, + users.verifier + ); + vm.expectRevert(expectedError); + tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); + } + + function testTAPCollector_Collect_RevertWhen_ProvisionEmpty( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { + // thaw tokens from the provision + resetPrank(users.indexer); + staking.thaw(users.indexer, users.verifier, 100); + + tokens = bound(tokens, 1, type(uint128).max); + + resetPrank(users.gateway); + _approveCollector(address(tapCollector), tokens); + _depositTokens(address(tapCollector), users.indexer, tokens); + + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + + resetPrank(users.verifier); + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorUnauthorizedDataService.selector, + users.verifier + ); + vm.expectRevert(expectedError); + tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); + } + + function testTAPCollector_Collect_PreventSignerAttack( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { + tokens = bound(tokens, 1, type(uint128).max); + + resetPrank(users.gateway); + _approveCollector(address(tapCollector), tokens); + _depositTokens(address(tapCollector), users.indexer, tokens); + + // The sender authorizes another signer + (address anotherSigner, uint256 anotherSignerPrivateKey) = makeAddrAndKey("anotherSigner"); + uint256 proofDeadline = block.timestamp + 1; + bytes memory anotherSignerProof = _getSignerProof(proofDeadline, anotherSignerPrivateKey); + _authorizeSigner(anotherSigner, proofDeadline, anotherSignerProof); + + // And crafts a RAV using the new signer as the data service + bytes memory data = _getQueryFeeEncodedData( + anotherSignerPrivateKey, + users.indexer, + anotherSigner, + uint128(tokens) + ); + + // the call should revert because the service provider has no provision with the "data service" + resetPrank(anotherSigner); + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorUnauthorizedDataService.selector, + anotherSigner + ); + vm.expectRevert(expectedError); + tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); + } + function testTAPCollector_Collect_RevertWhen_CallerNotDataService(uint256 tokens) public useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + resetPrank(users.gateway); _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); @@ -95,9 +178,11 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } - function testTAPCollector_Collect_RevertWhen_InconsistentRAVTokens(uint256 tokens) public useGateway useSigner { + function testTAPCollector_Collect_RevertWhen_InconsistentRAVTokens( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); @@ -106,20 +191,18 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _collect(IGraphPayments.PaymentTypes.QueryFee, data); // Attempt to collect again - vm.expectRevert(abi.encodeWithSelector( - ITAPCollector.TAPCollectorInconsistentRAVTokens.selector, - tokens, - tokens - )); + vm.expectRevert( + abi.encodeWithSelector(ITAPCollector.TAPCollectorInconsistentRAVTokens.selector, tokens, tokens) + ); tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } function testTAPCollector_Collect_RevertWhen_SignerNotAuthorized(uint256 tokens) public useGateway { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); resetPrank(users.verifier); @@ -127,16 +210,18 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } - function testTAPCollector_Collect_ThawingSigner(uint256 tokens) public useGateway useSigner { + function testTAPCollector_Collect_ThawingSigner( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); // Start thawing signer _thawSigner(signer); skip(revokeSignerThawingPeriod + 1); - + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); resetPrank(users.verifier); @@ -145,7 +230,7 @@ contract TAPCollectorCollectTest is TAPCollectorTest { function testTAPCollector_Collect_RevertIf_SignerWasRevoked(uint256 tokens) public useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); @@ -153,7 +238,7 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _thawSigner(signer); skip(revokeSignerThawingPeriod + 1); _revokeAuthorizedSigner(signer); - + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); resetPrank(users.verifier); @@ -161,9 +246,11 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } - function testTAPCollector_Collect_ThawingSignerCanceled(uint256 tokens) public useGateway useSigner { + function testTAPCollector_Collect_ThawingSignerCanceled( + uint256 tokens + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { tokens = bound(tokens, 1, type(uint128).max); - + _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); @@ -171,7 +258,7 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _thawSigner(signer); skip(revokeSignerThawingPeriod + 1); _cancelThawSigner(signer); - + bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); resetPrank(users.verifier);