From c8efe89b533f47c892d23ea923912111254f7853 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 1/9] 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 | 10 ++ .../tap-collector/collect/collect.t.sol | 141 ++++++++++++++---- 3 files changed, 130 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..40a375349 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -118,6 +118,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 +131,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); From 9dc58d00554aa3ccc8ab10e249ef3fba5cc3ce2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Wed, 27 Nov 2024 11:16:26 -0300 Subject: [PATCH 2/9] fix: disallow signers to be authorized for different payers (TRST-M10) 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 | 20 +++- .../payments/collectors/TAPCollector.sol | 21 +++- .../payments/tap-collector/TAPCollector.t.sol | 21 ++-- .../tap-collector/collect/collect.t.sol | 113 ++++++++++++++++-- .../signer/authorizeSigner.t.sol | 21 ++++ .../tap-collector/signer/thawSigner.t.sol | 14 +++ 6 files changed, 186 insertions(+), 24 deletions(-) diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index d04f142d6..b364135c3 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/packages/horizon/contracts/interfaces/ITAPCollector.sol @@ -18,10 +18,14 @@ interface ITAPCollector is IPaymentsCollector { address payer; // Timestamp at which thawing period ends (zero if not thawing) uint256 thawEndTimestamp; + // Whether the signer authorization was revoked + bool revoked; } /// @notice The Receipt Aggregate Voucher (RAV) struct struct ReceiptAggregateVoucher { + // The address of the payer the RAV was issued by + address payer; // The address of the data service the RAV was issued to address dataService; // The address of the service provider the RAV was issued to @@ -119,6 +123,12 @@ interface ITAPCollector is IPaymentsCollector { */ error TAPCollectorSignerNotAuthorizedByPayer(address payer, address signer); + /** + * Thrown when the attempting to revoke a signer that was already revoked + * @param signer The address of the signer + */ + error TAPCollectorAuthorizationAlreadyRevoked(address payer, address signer); + /** * Thrown when the signer is not thawing * @param signer The address of the signer @@ -137,6 +147,13 @@ interface ITAPCollector is IPaymentsCollector { */ error TAPCollectorInvalidRAVSigner(); + /** + * Thrown when the RAV payer does not match the signers authorized payer + * @param authorizedPayer The address of the authorized payer + * @param ravPayer The address of the RAV payer + */ + error TAPCollectorInvalidRAVPayer(address authorizedPayer, address ravPayer); + /** * Thrown when the RAV is for a data service the service provider has no provision for * @param dataService The address of the data service @@ -159,7 +176,8 @@ interface ITAPCollector is IPaymentsCollector { error TAPCollectorInconsistentRAVTokens(uint256 tokens, uint256 tokensCollected); /** - * @notice Authorize a signer to sign on behalf of the payer + * @notice Authorize a signer to sign on behalf of the payer. + * A signer can not be authorized for multiple payers even after revoking previous authorizations. * @dev Requirements: * - `signer` must not be already authorized * - `proofDeadline` must be greater than the current timestamp diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index 40a375349..a5449e1cc 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -18,6 +18,8 @@ import { MessageHashUtils } from "@openzeppelin/contracts/utils/cryptography/Mes * @dev Note that the contract expects the RAV aggregate value to be monotonically increasing, each successive RAV for the same * (data service-payer-receiver) tuple should have a value greater than the previous one. The contract will keep track of the tokens * already collected and calculate the difference to collect. + * @dev The contract also implements a mechanism to authorize signers to sign RAVs on behalf of a payer. Signers cannot be reused + * for different payers. * @custom:security-contact Please email security+contracts@thegraph.com if you find any * bugs. We may have an active bug bounty program. */ @@ -27,7 +29,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { /// @notice The EIP712 typehash for the ReceiptAggregateVoucher struct bytes32 private constant EIP712_RAV_TYPEHASH = keccak256( - "ReceiptAggregateVoucher(address dataService,address serviceProvider,uint64 timestampNs,uint128 valueAggregate,bytes metadata)" + "ReceiptAggregateVoucher(address payer,address dataService,address serviceProvider,uint64 timestampNs,uint128 valueAggregate,bytes metadata)" ); /// @notice Authorization details for payer-signer pairs @@ -79,6 +81,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { PayerAuthorization storage authorization = authorizedSigners[signer]; require(authorization.payer == msg.sender, TAPCollectorSignerNotAuthorizedByPayer(msg.sender, signer)); + require(!authorization.revoked, TAPCollectorAuthorizationAlreadyRevoked(msg.sender, signer)); authorization.thawEndTimestamp = block.timestamp + REVOKE_SIGNER_THAWING_PERIOD; emit SignerThawing(msg.sender, signer, authorization.thawEndTimestamp); @@ -110,7 +113,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { TAPCollectorSignerStillThawing(block.timestamp, authorization.thawEndTimestamp) ); - delete authorizedSigners[signer]; + authorization.revoked = true; emit SignerRevoked(msg.sender, signer); } @@ -122,14 +125,26 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { * @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) { + // Ensure caller is the RAV data service (SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(data, (SignedRAV, uint256)); require( signedRAV.rav.dataService == msg.sender, TAPCollectorCallerNotDataService(msg.sender, signedRAV.rav.dataService) ); + // Ensure RAV signer is authorized for a payer address signer = _recoverRAVSigner(signedRAV); - require(authorizedSigners[signer].payer != address(0), TAPCollectorInvalidRAVSigner()); + require( + authorizedSigners[signer].payer != address(0) && !authorizedSigners[signer].revoked, + TAPCollectorInvalidRAVSigner() + ); + + // Ensure RAV payer matches the authorized payer + address payer = signedRAV.rav.payer; + require( + authorizedSigners[signer].payer == payer, + TAPCollectorInvalidRAVPayer(authorizedSigners[signer].payer, payer) + ); // 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 diff --git a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol index 25b4c901c..1120c5b92 100644 --- a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol +++ b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol @@ -66,9 +66,10 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest tapCollector.authorizeSigner(_signer, _proofDeadline, _proof); - (address _payer, uint256 thawEndTimestamp) = tapCollector.authorizedSigners(_signer); + (address _payer, uint256 thawEndTimestamp, bool revoked) = tapCollector.authorizedSigners(_signer); assertEq(_payer, msgSender); assertEq(thawEndTimestamp, 0); + assertEq(revoked, false); } function _thawSigner(address _signer) internal { @@ -80,9 +81,10 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest tapCollector.thawSigner(_signer); - (address _payer, uint256 thawEndTimestamp) = tapCollector.authorizedSigners(_signer); + (address _payer, uint256 thawEndTimestamp, bool revoked) = tapCollector.authorizedSigners(_signer); assertEq(_payer, msgSender); assertEq(thawEndTimestamp, expectedThawEndTimestamp); + assertEq(revoked, false); } function _cancelThawSigner(address _signer) internal { @@ -93,29 +95,34 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest tapCollector.cancelThawSigner(_signer); - (address _payer, uint256 thawEndTimestamp) = tapCollector.authorizedSigners(_signer); + (address _payer, uint256 thawEndTimestamp, bool revoked) = tapCollector.authorizedSigners(_signer); assertEq(_payer, msgSender); assertEq(thawEndTimestamp, 0); + assertEq(revoked, false); } function _revokeAuthorizedSigner(address _signer) internal { (, address msgSender, ) = vm.readCallers(); + (address beforePayer, uint256 beforeThawEndTimestamp, ) = tapCollector.authorizedSigners(_signer); + vm.expectEmit(address(tapCollector)); emit ITAPCollector.SignerRevoked(msgSender, _signer); tapCollector.revokeAuthorizedSigner(_signer); - (address _payer, uint256 thawEndTimestamp) = tapCollector.authorizedSigners(_signer); - assertEq(_payer, address(0)); - assertEq(thawEndTimestamp, 0); + (address afterPayer, uint256 afterThawEndTimestamp, bool afterRevoked) = tapCollector.authorizedSigners(_signer); + + assertEq(beforePayer, afterPayer); + assertEq(beforeThawEndTimestamp, afterThawEndTimestamp); + assertEq(afterRevoked, true); } function _collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data) internal { (ITAPCollector.SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (ITAPCollector.SignedRAV, uint256)); bytes32 messageHash = tapCollector.encodeRAV(signedRAV.rav); address _signer = ECDSA.recover(messageHash, signedRAV.signature); - (address _payer, ) = tapCollector.authorizedSigners(_signer); + (address _payer, , ) = tapCollector.authorizedSigners(_signer); uint256 tokensAlreadyCollected = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); uint256 tokensToCollect = signedRAV.rav.valueAggregate - tokensAlreadyCollected; uint256 tokensDataService = tokensToCollect.mulPPM(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 555c9c50e..dabc56592 100644 --- a/packages/horizon/test/payments/tap-collector/collect/collect.t.sol +++ b/packages/horizon/test/payments/tap-collector/collect/collect.t.sol @@ -15,11 +15,12 @@ contract TAPCollectorCollectTest is TAPCollectorTest { function _getQueryFeeEncodedData( uint256 _signerPrivateKey, + address _payer, address _indexer, address _collector, uint128 _tokens ) private view returns (bytes memory) { - ITAPCollector.ReceiptAggregateVoucher memory rav = _getRAV(_indexer, _collector, _tokens); + ITAPCollector.ReceiptAggregateVoucher memory rav = _getRAV(_payer, _indexer, _collector, _tokens); bytes32 messageHash = tapCollector.encodeRAV(rav); (uint8 v, bytes32 r, bytes32 s) = vm.sign(_signerPrivateKey, messageHash); bytes memory signature = abi.encodePacked(r, s, v); @@ -28,12 +29,14 @@ contract TAPCollectorCollectTest is TAPCollectorTest { } function _getRAV( + address _payer, address _indexer, address _collector, uint128 _tokens ) private pure returns (ITAPCollector.ReceiptAggregateVoucher memory rav) { return ITAPCollector.ReceiptAggregateVoucher({ + payer: _payer, dataService: _collector, serviceProvider: _indexer, timestampNs: 0, @@ -54,7 +57,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); @@ -76,6 +85,7 @@ contract TAPCollectorCollectTest is TAPCollectorTest { for (uint256 i = 0; i < steps; i++) { bytes memory data = _getQueryFeeEncodedData( signerPrivateKey, + users.gateway, users.indexer, users.verifier, uint128(payed + tokensPerStep) @@ -91,7 +101,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); bytes memory expectedError = abi.encodeWithSelector( @@ -115,7 +131,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); bytes memory expectedError = abi.encodeWithSelector( @@ -137,13 +159,16 @@ contract TAPCollectorCollectTest is TAPCollectorTest { // 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); + { + 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.gateway, users.indexer, anotherSigner, uint128(tokens) @@ -166,7 +191,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.indexer); bytes memory expectedError = abi.encodeWithSelector( @@ -178,6 +209,32 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } + function testTAPCollector_Collect_RevertWhen_PayerMismatch(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); + + (address anotherPayer, ) = makeAddrAndKey("anotherPayer"); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + anotherPayer, + users.indexer, + users.verifier, + uint128(tokens) + ); + + resetPrank(users.verifier); + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorInvalidRAVPayer.selector, + users.gateway, + anotherPayer + ); + vm.expectRevert(expectedError); + tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); + } + function testTAPCollector_Collect_RevertWhen_InconsistentRAVTokens( uint256 tokens ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { @@ -185,7 +242,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); @@ -203,7 +266,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); vm.expectRevert(abi.encodeWithSelector(ITAPCollector.TAPCollectorInvalidRAVSigner.selector)); @@ -222,7 +291,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { _thawSigner(signer); skip(revokeSignerThawingPeriod + 1); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); @@ -239,7 +314,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { skip(revokeSignerThawingPeriod + 1); _revokeAuthorizedSigner(signer); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); vm.expectRevert(abi.encodeWithSelector(ITAPCollector.TAPCollectorInvalidRAVSigner.selector)); @@ -259,7 +340,13 @@ contract TAPCollectorCollectTest is TAPCollectorTest { skip(revokeSignerThawingPeriod + 1); _cancelThawSigner(signer); - bytes memory data = _getQueryFeeEncodedData(signerPrivateKey, users.indexer, users.verifier, uint128(tokens)); + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); diff --git a/packages/horizon/test/payments/tap-collector/signer/authorizeSigner.t.sol b/packages/horizon/test/payments/tap-collector/signer/authorizeSigner.t.sol index b337c48c7..30ffd4610 100644 --- a/packages/horizon/test/payments/tap-collector/signer/authorizeSigner.t.sol +++ b/packages/horizon/test/payments/tap-collector/signer/authorizeSigner.t.sol @@ -49,6 +49,27 @@ contract TAPCollectorAuthorizeSignerTest is TAPCollectorTest { tapCollector.authorizeSigner(signer, proofDeadline, signerProof); } + function testTAPCollector_AuthorizeSigner_RevertWhen_AlreadyAuthroizedAfterRevoking() public useGateway { + // Authorize signer + uint256 proofDeadline = block.timestamp + 1; + bytes memory signerProof = _getSignerProof(proofDeadline, signerPrivateKey); + _authorizeSigner(signer, proofDeadline, signerProof); + + // Revoke signer + _thawSigner(signer); + skip(revokeSignerThawingPeriod + 1); + _revokeAuthorizedSigner(signer); + + // Attempt to authorize signer again + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorSignerAlreadyAuthorized.selector, + users.gateway, + signer + ); + vm.expectRevert(expectedError); + tapCollector.authorizeSigner(signer, proofDeadline, signerProof); + } + function testTAPCollector_AuthorizeSigner_RevertWhen_ProofExpired() public useGateway { // Sign proof with payer uint256 proofDeadline = block.timestamp - 1; diff --git a/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol b/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol index fb47c37fb..db9d99040 100644 --- a/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol +++ b/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol @@ -26,4 +26,18 @@ contract TAPCollectorThawSignerTest is TAPCollectorTest { vm.expectRevert(expectedError); tapCollector.thawSigner(signer); } + + function testTAPCollector_ThawSigner_RevertWhen_AlreadyRevoked() public useGateway useSigner { + _thawSigner(signer); + skip(revokeSignerThawingPeriod + 1); + _revokeAuthorizedSigner(signer); + + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorAuthorizationAlreadyRevoked.selector, + users.gateway, + signer + ); + vm.expectRevert(expectedError); + tapCollector.thawSigner(signer); + } } From 65f4d688e68a24bda2734947d2fbff087a878fb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Wed, 27 Nov 2024 12:17:28 -0300 Subject: [PATCH 3/9] fix: remove collector allowance feature from payments escrow (TRST-CL1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/interfaces/IPaymentsEscrow.sol | 101 ---------------- .../contracts/payments/PaymentsEscrow.sol | 75 +----------- .../ignition/configs/horizon.hardhat.json | 1 - .../ignition/modules/core/PaymentsEscrow.ts | 3 +- packages/horizon/test/GraphBase.t.sol | 2 +- .../horizon/test/escrow/GraphEscrow.t.sol | 6 - packages/horizon/test/escrow/collect.t.sol | 36 +----- packages/horizon/test/escrow/collector.t.sol | 108 ------------------ packages/horizon/test/escrow/paused.t.sol | 22 ---- .../tap-collector/collect/collect.t.sol | 11 -- .../PaymentsEscrowShared.t.sol | 16 --- packages/horizon/test/utils/Constants.sol | 1 - .../test/SubgraphBaseTest.t.sol | 2 - .../subgraphService/SubgraphService.t.sol | 2 +- .../subgraphService/collect/query/query.t.sol | 7 +- .../subgraph-service/test/utils/Constants.sol | 1 - 16 files changed, 10 insertions(+), 384 deletions(-) delete mode 100644 packages/horizon/test/escrow/collector.t.sol diff --git a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol index c1eb7707a..4d7207481 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol @@ -25,50 +25,6 @@ interface IPaymentsEscrow { uint256 thawEndTimestamp; } - /// @notice Details for a payer-collector pair - /// @dev Collectors can be removed only after a thawing period - struct Collector { - // Amount of tokens the collector is allowed to collect - uint256 allowance; - // Timestamp at which the collector thawing period ends (zero if not thawing) - uint256 thawEndTimestamp; - } - - /** - * @notice Emitted when a payer authorizes a collector to collect funds - * @param payer The address of the payer - * @param collector The address of the collector - * @param addedAllowance The amount of tokens added to the collector's allowance - * @param newTotalAllowance The new total allowance after addition - */ - event AuthorizedCollector( - address indexed payer, - address indexed collector, - uint256 addedAllowance, - uint256 newTotalAllowance - ); - - /** - * @notice Emitted when a payer thaws a collector - * @param payer The address of the payer - * @param collector The address of the collector - */ - event ThawCollector(address indexed payer, address indexed collector); - - /** - * @notice Emitted when a payer cancels the thawing of a collector - * @param payer The address of the payer - * @param collector The address of the collector - */ - event CancelThawCollector(address indexed payer, address indexed collector); - - /** - * @notice Emitted when a payer revokes a collector authorization. - * @param payer The address of the payer - * @param collector The address of the collector - */ - event RevokeCollector(address indexed payer, address indexed collector); - /** * @notice Emitted when a payer deposits funds into the escrow for a payer-collector-receiver tuple * @param payer The address of the payer @@ -152,13 +108,6 @@ interface IPaymentsEscrow { */ error PaymentsEscrowThawingPeriodTooLong(uint256 thawingPeriod, uint256 maxWaitPeriod); - /** - * @notice Thrown when a collector has insufficient allowance to collect funds - * @param allowance The current allowance - * @param minAllowance The minimum required allowance - */ - error PaymentsEscrowInsufficientAllowance(uint256 allowance, uint256 minAllowance); - /** * @notice Thrown when the contract balance is not consistent with the collection amount * @param balanceBefore The balance before the collection @@ -172,54 +121,6 @@ interface IPaymentsEscrow { */ error PaymentsEscrowInvalidZeroTokens(); - /** - * @notice Authorize a collector to collect funds from the payer's escrow - * @dev This function can only be used to increase the allowance of a collector. - * To reduce it the authorization must be revoked and a new one must be created. - * - * Requirements: - * - `allowance` must be greater than zero - * - * Emits an {AuthorizedCollector} event - * - * @param collector The address of the collector - * @param allowance The amount of tokens to add to the collector's allowance - */ - function approveCollector(address collector, uint256 allowance) external; - - /** - * @notice Thaw a collector's collector authorization - * @dev The thawing period is defined by the `REVOKE_COLLECTOR_THAWING_PERIOD` constant - * - * Emits a {ThawCollector} event - * - * @param collector The address of the collector - */ - function thawCollector(address collector) external; - - /** - * @notice Cancel a collector's authorization thawing - * @dev Requirements: - * - `collector` must be thawing - * - * Emits a {CancelThawCollector} event - * - * @param collector The address of the collector - */ - function cancelThawCollector(address collector) external; - - /** - * @notice Revoke a collector's authorization. - * Removes the collector from the list of authorized collectors. - * @dev Requirements: - * - `collector` must have thawed - * - * Emits a {RevokeCollector} event - * - * @param collector The address of the collector - */ - function revokeCollector(address collector) external; - /** * @notice Deposits funds into the escrow for a payer-collector-receiver tuple, where * the payer is the transaction caller. @@ -277,8 +178,6 @@ interface IPaymentsEscrow { * @notice Collects funds from the payer-collector-receiver's escrow and sends them to {GraphPayments} for * distribution using the Graph Horizon Payments protocol. * The function will revert if there are not enough funds in the escrow. - * @dev Requirements: - * - `collector` needs to be authorized by the payer and have enough allowance * * Emits an {EscrowCollected} event * diff --git a/packages/horizon/contracts/payments/PaymentsEscrow.sol b/packages/horizon/contracts/payments/PaymentsEscrow.sol index 7cf7e9e38..5643d4a5b 100644 --- a/packages/horizon/contracts/payments/PaymentsEscrow.sol +++ b/packages/horizon/contracts/payments/PaymentsEscrow.sol @@ -23,10 +23,6 @@ import { GraphDirectory } from "../utilities/GraphDirectory.sol"; contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, IPaymentsEscrow { using TokenUtils for IGraphToken; - /// @notice Authorization details for payer-collector pairs - mapping(address payer => mapping(address collector => IPaymentsEscrow.Collector collectorDetails)) - public authorizedCollectors; - /// @notice Escrow account details for payer-collector-receiver tuples mapping(address payer => mapping(address collector => mapping(address receiver => IPaymentsEscrow.EscrowAccount escrowAccount))) public escrowAccounts; @@ -35,9 +31,6 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, /// @dev This is a precautionary measure to avoid inadvertedly locking funds for too long uint256 public constant MAX_WAIT_PERIOD = 90 days; - /// @notice Thawing period in seconds for authorized collectors - uint256 public immutable REVOKE_COLLECTOR_THAWING_PERIOD; - /// @notice Thawing period in seconds for escrow funds withdrawal uint256 public immutable WITHDRAW_ESCROW_THAWING_PERIOD; @@ -49,24 +42,14 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, /** * @notice Construct the PaymentsEscrow contract * @param controller The address of the controller - * @param revokeCollectorThawingPeriod Thawing period in seconds for authorized collectors * @param withdrawEscrowThawingPeriod Thawing period in seconds for escrow funds withdrawal */ - constructor( - address controller, - uint256 revokeCollectorThawingPeriod, - uint256 withdrawEscrowThawingPeriod - ) GraphDirectory(controller) { - require( - revokeCollectorThawingPeriod <= MAX_WAIT_PERIOD, - PaymentsEscrowThawingPeriodTooLong(revokeCollectorThawingPeriod, MAX_WAIT_PERIOD) - ); + constructor(address controller, uint256 withdrawEscrowThawingPeriod) GraphDirectory(controller) { require( withdrawEscrowThawingPeriod <= MAX_WAIT_PERIOD, PaymentsEscrowThawingPeriodTooLong(withdrawEscrowThawingPeriod, MAX_WAIT_PERIOD) ); - REVOKE_COLLECTOR_THAWING_PERIOD = revokeCollectorThawingPeriod; WITHDRAW_ESCROW_THAWING_PERIOD = withdrawEscrowThawingPeriod; } @@ -77,52 +60,6 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, __Multicall_init(); } - /** - * @notice See {IPaymentsEscrow-approveCollector} - */ - function approveCollector(address collector_, uint256 allowance) external override notPaused { - require(allowance != 0, PaymentsEscrowInvalidZeroTokens()); - Collector storage collector = authorizedCollectors[msg.sender][collector_]; - collector.allowance += allowance; - emit AuthorizedCollector(msg.sender, collector_, allowance, collector.allowance); - } - - /** - * @notice See {IPaymentsEscrow-thawCollector} - */ - function thawCollector(address collector) external override notPaused { - authorizedCollectors[msg.sender][collector].thawEndTimestamp = - block.timestamp + - REVOKE_COLLECTOR_THAWING_PERIOD; - emit ThawCollector(msg.sender, collector); - } - - /** - * @notice See {IPaymentsEscrow-cancelThawCollector} - */ - function cancelThawCollector(address collector) external override notPaused { - require(authorizedCollectors[msg.sender][collector].thawEndTimestamp != 0, PaymentsEscrowNotThawing()); - - authorizedCollectors[msg.sender][collector].thawEndTimestamp = 0; - emit CancelThawCollector(msg.sender, collector); - } - - /** - * @notice See {IPaymentsEscrow-revokeCollector} - */ - function revokeCollector(address collector_) external override notPaused { - Collector storage collector = authorizedCollectors[msg.sender][collector_]; - - require(collector.thawEndTimestamp != 0, PaymentsEscrowNotThawing()); - require( - collector.thawEndTimestamp < block.timestamp, - PaymentsEscrowStillThawing(block.timestamp, collector.thawEndTimestamp) - ); - - delete authorizedCollectors[msg.sender][collector_]; - emit RevokeCollector(msg.sender, collector_); - } - /** * @notice See {IPaymentsEscrow-deposit} */ @@ -194,19 +131,11 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, address dataService, uint256 tokensDataService ) external override notPaused { - // Check if collector is authorized and has enough funds - Collector storage collectorDetails = authorizedCollectors[payer][msg.sender]; - require( - collectorDetails.allowance >= tokens, - PaymentsEscrowInsufficientAllowance(collectorDetails.allowance, tokens) - ); - // Check if there are enough funds in the escrow account EscrowAccount storage account = escrowAccounts[payer][msg.sender][receiver]; require(account.balance >= tokens, PaymentsEscrowInsufficientBalance(account.balance, tokens)); - // Reduce amount from approved collector and account balance - collectorDetails.allowance -= tokens; + // Reduce amount from account balance account.balance -= tokens; uint256 balanceBefore = _graphToken().balanceOf(address(this)); diff --git a/packages/horizon/ignition/configs/horizon.hardhat.json b/packages/horizon/ignition/configs/horizon.hardhat.json index 894a2ed59..7c4214d37 100644 --- a/packages/horizon/ignition/configs/horizon.hardhat.json +++ b/packages/horizon/ignition/configs/horizon.hardhat.json @@ -36,7 +36,6 @@ "protocolPaymentCut": 10000 }, "PaymentsEscrow": { - "revokeCollectorThawingPeriod": 10000, "withdrawEscrowThawingPeriod": 10000 }, "TAPCollector": { diff --git a/packages/horizon/ignition/modules/core/PaymentsEscrow.ts b/packages/horizon/ignition/modules/core/PaymentsEscrow.ts index 1dbd07088..7c7948a7e 100644 --- a/packages/horizon/ignition/modules/core/PaymentsEscrow.ts +++ b/packages/horizon/ignition/modules/core/PaymentsEscrow.ts @@ -10,13 +10,12 @@ export default buildModule('PaymentsEscrow', (m) => { const { Controller, PeripheryRegistered } = m.useModule(GraphPeripheryModule) const { PaymentsEscrowProxyAdmin, PaymentsEscrowProxy, HorizonRegistered } = m.useModule(HorizonProxiesModule) - const revokeCollectorThawingPeriod = m.getParameter('revokeCollectorThawingPeriod') const withdrawEscrowThawingPeriod = m.getParameter('withdrawEscrowThawingPeriod') // Deploy PaymentsEscrow implementation const PaymentsEscrowImplementation = m.contract('PaymentsEscrow', PaymentsEscrowArtifact, - [Controller, revokeCollectorThawingPeriod, withdrawEscrowThawingPeriod], + [Controller, withdrawEscrowThawingPeriod], { after: [PeripheryRegistered, HorizonRegistered], }, diff --git a/packages/horizon/test/GraphBase.t.sol b/packages/horizon/test/GraphBase.t.sol index 7aa44d6f5..b2d43ba63 100644 --- a/packages/horizon/test/GraphBase.t.sol +++ b/packages/horizon/test/GraphBase.t.sol @@ -116,7 +116,7 @@ abstract contract GraphBaseTest is IHorizonStakingTypes, Utils, Constants { // PaymentsEscrow bytes memory escrowImplementationParameters = abi.encode( address(controller), - revokeCollectorThawingPeriod,withdrawEscrowThawingPeriod + withdrawEscrowThawingPeriod ); bytes memory escrowImplementationBytecode = abi.encodePacked( type(PaymentsEscrow).creationCode, diff --git a/packages/horizon/test/escrow/GraphEscrow.t.sol b/packages/horizon/test/escrow/GraphEscrow.t.sol index d3ffd21da..421bbfdd2 100644 --- a/packages/horizon/test/escrow/GraphEscrow.t.sol +++ b/packages/horizon/test/escrow/GraphEscrow.t.sol @@ -24,12 +24,6 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { _; } - modifier useCollector(uint256 tokens) { - vm.assume(tokens > 0); - escrow.approveCollector(users.verifier, tokens); - _; - } - modifier depositAndThawTokens(uint256 amount, uint256 thawAmount) { vm.assume(thawAmount > 0); vm.assume(amount > thawAmount); diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol index 72b795ee9..254d8738d 100644 --- a/packages/horizon/test/escrow/collect.t.sol +++ b/packages/horizon/test/escrow/collect.t.sol @@ -106,47 +106,17 @@ contract GraphEscrowCollectTest is GraphEscrowTest { _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0); resetPrank(users.gateway); - escrow.approveCollector(users.verifier, tokens); _depositTokens(users.verifier, users.indexer, tokens); resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, tokens, subgraphDataServiceAddress, tokensDataService); } - function testCollect_RevertWhen_CollectorNotAuthorized(uint256 amount) public { - vm.assume(amount > 0); - vm.startPrank(users.verifier); - uint256 dataServiceCut = 30000; // 3% - bytes memory expectedError = abi.encodeWithSelector( - IPaymentsEscrow.PaymentsEscrowInsufficientAllowance.selector, - 0, - amount - ); - vm.expectRevert(expectedError); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, dataServiceCut); - vm.stopPrank(); - } - - function testCollect_RevertWhen_CollectorHasInsufficientAmount( - uint256 amount, - uint256 insufficientAmount - ) public useGateway useCollector(insufficientAmount) useDeposit(amount) { - vm.assume(insufficientAmount < amount); - - vm.startPrank(users.verifier); - bytes memory expectedError = abi.encodeWithSignature( - "PaymentsEscrowInsufficientAllowance(uint256,uint256)", - insufficientAmount, - amount - ); - vm.expectRevert(expectedError); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 0); - } - function testCollect_RevertWhen_SenderHasInsufficientAmountInEscrow( uint256 amount, uint256 insufficientAmount - ) public useGateway useCollector(amount) useDeposit(insufficientAmount) { + ) public useGateway useDeposit(insufficientAmount) { + vm.assume(amount > 0); vm.assume(insufficientAmount < amount); vm.startPrank(users.verifier); @@ -162,7 +132,6 @@ contract GraphEscrowCollectTest is GraphEscrowTest { vm.assume(amount > 1 ether); resetPrank(users.gateway); - escrow.approveCollector(users.verifier, amount); _depositTokens(users.verifier, users.indexer, amount); resetPrank(users.verifier); @@ -181,7 +150,6 @@ contract GraphEscrowCollectTest is GraphEscrowTest { vm.assume(amount <= MAX_STAKING_TOKENS); resetPrank(users.gateway); - escrow.approveCollector(users.verifier, amount); _depositTokens(users.verifier, users.indexer, amount); resetPrank(users.verifier); diff --git a/packages/horizon/test/escrow/collector.t.sol b/packages/horizon/test/escrow/collector.t.sol deleted file mode 100644 index d6cb3bc0f..000000000 --- a/packages/horizon/test/escrow/collector.t.sol +++ /dev/null @@ -1,108 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.27; - -import "forge-std/Test.sol"; - -import { IPaymentsEscrow } from "../../contracts/interfaces/IPaymentsEscrow.sol"; - -import { GraphEscrowTest } from "./GraphEscrow.t.sol"; - -contract GraphEscrowCollectorTest is GraphEscrowTest { - - /* - * HELPERS - */ - - function _thawCollector() internal { - (uint256 beforeAllowance,) = escrow.authorizedCollectors(users.gateway, users.verifier); - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.ThawCollector(users.gateway, users.verifier); - escrow.thawCollector(users.verifier); - - (uint256 allowance, uint256 thawEndTimestamp) = escrow.authorizedCollectors(users.gateway, users.verifier); - assertEq(allowance, beforeAllowance); - assertEq(thawEndTimestamp, block.timestamp + revokeCollectorThawingPeriod); - } - - function _cancelThawCollector() internal { - (uint256 beforeAllowance, uint256 beforeThawEndTimestamp) = escrow.authorizedCollectors(users.gateway, users.verifier); - assertTrue(beforeThawEndTimestamp != 0, "Collector should be thawing"); - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.CancelThawCollector(users.gateway, users.verifier); - escrow.cancelThawCollector(users.verifier); - - (uint256 allowance, uint256 thawEndTimestamp) = escrow.authorizedCollectors(users.gateway, users.verifier); - assertEq(allowance, beforeAllowance); - assertEq(thawEndTimestamp, 0); - } - - function _revokeCollector() internal { - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.RevokeCollector(users.gateway, users.verifier); - escrow.revokeCollector(users.verifier); - - (uint256 allowance, uint256 thawEndTimestamp) = escrow.authorizedCollectors(users.gateway, users.verifier); - assertEq(allowance, 0); - assertEq(thawEndTimestamp, 0); - } - - /* - * TESTS - */ - - function testCollector_Approve( - uint256 tokens, - uint256 approveSteps - ) public useGateway { - approveSteps = bound(approveSteps, 1, 100); - vm.assume(tokens > approveSteps); - - uint256 approveTokens = tokens / approveSteps; - for (uint i = 0; i < approveSteps; i++) { - _approveCollector(users.verifier, approveTokens); - } - } - - function testCollector_RevertWhen_ApprovingForZeroAllowance( - uint256 amount - ) public useGateway useCollector(amount) { - bytes memory expectedError = abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowInvalidZeroTokens.selector); - vm.expectRevert(expectedError); - escrow.approveCollector(users.verifier, 0); - } - - function testCollector_Thaw(uint256 amount) public useGateway useCollector(amount) { - _thawCollector(); - } - - function testCollector_CancelThaw(uint256 amount) public useGateway useCollector(amount) { - _thawCollector(); - _cancelThawCollector(); - } - - function testCollector_RevertWhen_CancelThawIsNotThawing(uint256 amount) public useGateway useCollector(amount) { - bytes memory expectedError = abi.encodeWithSignature("PaymentsEscrowNotThawing()"); - vm.expectRevert(expectedError); - escrow.cancelThawCollector(users.verifier); - vm.stopPrank(); - } - - function testCollector_Revoke(uint256 amount) public useGateway useCollector(amount) { - _thawCollector(); - skip(revokeCollectorThawingPeriod + 1); - _revokeCollector(); - } - - function testCollector_RevertWhen_RevokeIsNotThawing(uint256 amount) public useGateway useCollector(amount) { - bytes memory expectedError = abi.encodeWithSignature("PaymentsEscrowNotThawing()"); - vm.expectRevert(expectedError); - escrow.revokeCollector(users.verifier); - } - - function testCollector_RevertWhen_RevokeIsStillThawing(uint256 amount) public useGateway useCollector(amount) { - escrow.thawCollector(users.verifier); - bytes memory expectedError = abi.encodeWithSignature("PaymentsEscrowStillThawing(uint256,uint256)", block.timestamp, block.timestamp + revokeCollectorThawingPeriod); - vm.expectRevert(expectedError); - escrow.revokeCollector(users.verifier); - } -} \ No newline at end of file diff --git a/packages/horizon/test/escrow/paused.t.sol b/packages/horizon/test/escrow/paused.t.sol index a993da883..6019b5c15 100644 --- a/packages/horizon/test/escrow/paused.t.sol +++ b/packages/horizon/test/escrow/paused.t.sol @@ -63,26 +63,4 @@ contract GraphEscrowPausedTest is GraphEscrowTest { vm.expectRevert(abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowIsPaused.selector)); escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, tokens, subgraphDataServiceAddress, tokensDataService); } - - // Collectors - - function testPaused_RevertWhen_ApproveCollector(uint256 tokens) public useGateway usePaused(true) { - vm.expectRevert(abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowIsPaused.selector)); - escrow.approveCollector(users.verifier, tokens); - } - - function testPaused_RevertWhen_ThawCollector(uint256 tokens) public useGateway useCollector(tokens) usePaused(true) { - vm.expectRevert(abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowIsPaused.selector)); - escrow.thawCollector(users.verifier); - } - - function testPaused_RevertWhen_CancelThawCollector(uint256 tokens) public useGateway useCollector(tokens) usePaused(true) { - vm.expectRevert(abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowIsPaused.selector)); - escrow.cancelThawCollector(users.verifier); - } - - function testPaused_RevertWhen_RevokeCollector(uint256 tokens) public useGateway useCollector(tokens) usePaused(true) { - vm.expectRevert(abi.encodeWithSelector(IPaymentsEscrow.PaymentsEscrowIsPaused.selector)); - escrow.revokeCollector(users.verifier); - } } \ No newline at end of file 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 dabc56592..ddb76b919 100644 --- a/packages/horizon/test/payments/tap-collector/collect/collect.t.sol +++ b/packages/horizon/test/payments/tap-collector/collect/collect.t.sol @@ -54,7 +54,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { ) 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( @@ -76,7 +75,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { steps = uint8(bound(steps, 1, 100)); tokens = bound(tokens, steps, type(uint128).max); - _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); resetPrank(users.verifier); @@ -98,7 +96,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { 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( @@ -128,7 +125,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tokens = bound(tokens, 1, type(uint128).max); resetPrank(users.gateway); - _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); bytes memory data = _getQueryFeeEncodedData( @@ -154,7 +150,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { 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 @@ -188,7 +183,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tokens = bound(tokens, 1, type(uint128).max); resetPrank(users.gateway); - _approveCollector(address(tapCollector), tokens); _depositTokens(address(tapCollector), users.indexer, tokens); bytes memory data = _getQueryFeeEncodedData( @@ -240,7 +234,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { ) 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, @@ -263,7 +256,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { 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( @@ -284,7 +276,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { ) 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 @@ -306,7 +297,6 @@ 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); // Start thawing signer @@ -332,7 +322,6 @@ contract TAPCollectorCollectTest is TAPCollectorTest { ) 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 diff --git a/packages/horizon/test/shared/payments-escrow/PaymentsEscrowShared.t.sol b/packages/horizon/test/shared/payments-escrow/PaymentsEscrowShared.t.sol index 2bc435f7a..c3714dfd6 100644 --- a/packages/horizon/test/shared/payments-escrow/PaymentsEscrowShared.t.sol +++ b/packages/horizon/test/shared/payments-escrow/PaymentsEscrowShared.t.sol @@ -22,22 +22,6 @@ abstract contract PaymentsEscrowSharedTest is GraphBaseTest { * HELPERS */ - function _approveCollector(address _verifier, uint256 _tokens) internal { - (, address msgSender, ) = vm.readCallers(); - (uint256 beforeAllowance,) = escrow.authorizedCollectors(msgSender, _verifier); - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.AuthorizedCollector( - msgSender, // payer - _verifier, // collector - _tokens, // addedAllowance - beforeAllowance + _tokens // newTotalAllowance after the added allowance - ); - escrow.approveCollector(_verifier, _tokens); - (uint256 allowance, uint256 thawEndTimestamp) = escrow.authorizedCollectors(msgSender, _verifier); - assertEq(allowance - beforeAllowance, _tokens); - assertEq(thawEndTimestamp, 0); - } - function _depositTokens(address _collector, address _receiver, uint256 _tokens) internal { (, address msgSender, ) = vm.readCallers(); (uint256 escrowBalanceBefore,,) = escrow.escrowAccounts(msgSender, _collector, _receiver); diff --git a/packages/horizon/test/utils/Constants.sol b/packages/horizon/test/utils/Constants.sol index cd5cc2bfb..e9ad5c2e9 100644 --- a/packages/horizon/test/utils/Constants.sol +++ b/packages/horizon/test/utils/Constants.sol @@ -7,7 +7,6 @@ abstract contract Constants { uint256 internal constant MAX_STAKING_TOKENS = 10_000_000_000 ether; // GraphEscrow parameters uint256 internal constant withdrawEscrowThawingPeriod = 60; - uint256 internal constant revokeCollectorThawingPeriod = 60; // GraphPayments parameters uint256 internal constant protocolPaymentCut = 10000; // Staking constants diff --git a/packages/subgraph-service/test/SubgraphBaseTest.t.sol b/packages/subgraph-service/test/SubgraphBaseTest.t.sol index 099164473..663442e1f 100644 --- a/packages/subgraph-service/test/SubgraphBaseTest.t.sol +++ b/packages/subgraph-service/test/SubgraphBaseTest.t.sol @@ -113,7 +113,6 @@ abstract contract SubgraphBaseTest is Utils, Constants { vm.getCode("PaymentsEscrow.sol:PaymentsEscrow"), abi.encode( address(controller), - revokeCollectorThawingPeriod, withdrawEscrowThawingPeriod ) )); @@ -174,7 +173,6 @@ abstract contract SubgraphBaseTest is Utils, Constants { ); escrow = new PaymentsEscrow{salt: saltEscrow}( address(controller), - revokeCollectorThawingPeriod, withdrawEscrowThawingPeriod ); diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index b417f30bf..64842b1f2 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -232,7 +232,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { ITAPCollector.SignedRAV memory signedRav = abi.decode(_data, (ITAPCollector.SignedRAV)); allocationId = abi.decode(signedRav.rav.metadata, (address)); allocation = subgraphService.getAllocation(allocationId); - (address payer, ) = tapCollector.authorizedSigners(_recoverRAVSigner(signedRav)); + (address payer, , ) = tapCollector.authorizedSigners(_recoverRAVSigner(signedRav)); // Total amount of tokens collected for indexer uint256 tokensCollected = tapCollector.tokensCollected(address(subgraphService), _indexer, payer); diff --git a/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol b/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol index 93972679f..65d7e23d7 100644 --- a/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol @@ -54,8 +54,7 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest { }); } - function _approveCollector(uint256 tokens) private { - escrow.approveCollector(address(tapCollector), tokens); + function _deposit(uint256 tokens) private { token.approve(address(escrow), tokens); escrow.deposit(address(tapCollector), users.indexer, tokens); } @@ -91,7 +90,7 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest { tokensPayment = bound(tokensPayment, minimumProvisionTokens, maxTokensPayment); resetPrank(users.gateway); - _approveCollector(tokensPayment); + _deposit(tokensPayment); _authorizeSigner(); resetPrank(users.indexer); @@ -108,7 +107,7 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest { uint256 tokensPayment = tokensAllocated / stakeToFeesRatio / numPayments; resetPrank(users.gateway); - _approveCollector(tokensAllocated); + _deposit(tokensAllocated); _authorizeSigner(); resetPrank(users.indexer); diff --git a/packages/subgraph-service/test/utils/Constants.sol b/packages/subgraph-service/test/utils/Constants.sol index 76f864da1..025396ea8 100644 --- a/packages/subgraph-service/test/utils/Constants.sol +++ b/packages/subgraph-service/test/utils/Constants.sol @@ -21,7 +21,6 @@ abstract contract Constants { uint64 internal constant MAX_WAIT_PERIOD = 28 days; // GraphEscrow parameters uint256 internal constant withdrawEscrowThawingPeriod = 60; - uint256 internal constant revokeCollectorThawingPeriod = 60; // GraphPayments parameters uint256 internal constant protocolPaymentCut = 10000; // RewardsMananger parameters From 86b998ba6d83e388c8c85fd049ca032122cf8df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Wed, 27 Nov 2024 14:43:48 -0300 Subject: [PATCH 4/9] fix: make getBalance return 0 if balance is less than thawing amount (TRST-L10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/interfaces/IPaymentsEscrow.sol | 1 + .../contracts/payments/PaymentsEscrow.sol | 3 + .../horizon/test/escrow/GraphEscrow.t.sol | 92 +++++++++- packages/horizon/test/escrow/collect.t.sol | 167 +++++++----------- packages/horizon/test/escrow/getters.t.sol | 67 +++++++ packages/horizon/test/escrow/thaw.t.sol | 11 +- 6 files changed, 229 insertions(+), 112 deletions(-) create mode 100644 packages/horizon/test/escrow/getters.t.sol diff --git a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol index 4d7207481..32f83d597 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol @@ -199,6 +199,7 @@ interface IPaymentsEscrow { /** * @notice Get the balance of a payer-collector-receiver tuple + * This function will return 0 if the current balance is less than the amount of funds being thawed. * @param payer The address of the payer * @param collector The address of the collector * @param receiver The address of the receiver diff --git a/packages/horizon/contracts/payments/PaymentsEscrow.sol b/packages/horizon/contracts/payments/PaymentsEscrow.sol index 5643d4a5b..108197fb3 100644 --- a/packages/horizon/contracts/payments/PaymentsEscrow.sol +++ b/packages/horizon/contracts/payments/PaymentsEscrow.sol @@ -157,6 +157,9 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, */ function getBalance(address payer, address collector, address receiver) external view override returns (uint256) { EscrowAccount storage account = escrowAccounts[payer][collector][receiver]; + if (account.balance <= account.tokensThawing) { + return 0; + } return account.balance - account.tokensThawing; } diff --git a/packages/horizon/test/escrow/GraphEscrow.t.sol b/packages/horizon/test/escrow/GraphEscrow.t.sol index 421bbfdd2..b2c4438bc 100644 --- a/packages/horizon/test/escrow/GraphEscrow.t.sol +++ b/packages/horizon/test/escrow/GraphEscrow.t.sol @@ -2,12 +2,13 @@ pragma solidity 0.8.27; import "forge-std/Test.sol"; +import { IPaymentsEscrow } from "../../contracts/interfaces/IPaymentsEscrow.sol"; +import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStakingShared.t.sol"; import { PaymentsEscrowSharedTest } from "../shared/payments-escrow/PaymentsEscrowShared.t.sol"; contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { - /* * MODIFIERS */ @@ -39,4 +40,91 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { function _approveEscrow(uint256 tokens) internal { token.approve(address(escrow), tokens); } -} \ No newline at end of file + + function _thawEscrow(address collector, address receiver, uint256 amount) internal { + (, address msgSender, ) = vm.readCallers(); + uint256 expectedThawEndTimestamp = block.timestamp + withdrawEscrowThawingPeriod; + vm.expectEmit(address(escrow)); + emit IPaymentsEscrow.Thaw(msgSender, collector, receiver, amount, expectedThawEndTimestamp); + escrow.thaw(collector, receiver, amount); + + (, uint256 amountThawing, uint256 thawEndTimestamp) = escrow.escrowAccounts(msgSender, collector, receiver); + assertEq(amountThawing, amount); + assertEq(thawEndTimestamp, expectedThawEndTimestamp); + } + + struct CollectPaymentData { + uint256 escrowBalance; + uint256 paymentsBalance; + uint256 receiverBalance; + uint256 delegationPoolBalance; + uint256 dataServiceBalance; + } + + function _collectEscrow( + IGraphPayments.PaymentTypes _paymentType, + address _payer, + address _receiver, + uint256 _tokens, + address _dataService, + uint256 _tokensDataService + ) internal { + (, address _collector, ) = vm.readCallers(); + + // Previous balances + (uint256 previousPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + CollectPaymentData memory previousBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), + dataServiceBalance: token.balanceOf(_dataService) + }); + + vm.expectEmit(address(escrow)); + emit IPaymentsEscrow.EscrowCollected(_payer, _collector, _receiver, _tokens); + escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _tokensDataService); + + // Calculate cuts + uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); + uint256 delegatorCut = staking.getDelegationFeeCut(_receiver, _dataService, _paymentType); + + // After balances + (uint256 afterPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + CollectPaymentData memory afterBalances = CollectPaymentData({ + escrowBalance: token.balanceOf(address(escrow)), + paymentsBalance: token.balanceOf(address(payments)), + receiverBalance: token.balanceOf(_receiver), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), + dataServiceBalance: token.balanceOf(_dataService) + }); + + // Check receiver balance after payment + uint256 receiverExpectedPayment = _tokens - + _tokensDataService - + (_tokens * protocolPaymentCut) / + MAX_PPM - + (_tokens * delegatorCut) / + MAX_PPM; + assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); + assertEq(token.balanceOf(address(payments)), 0); + + // Check delegation pool balance after payment + assertEq( + afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, + (_tokens * delegatorCut) / MAX_PPM + ); + + // Check that the escrow account has been updated + assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); + + // Check that payments balance didn't change + assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); + + // Check data service balance after payment + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + + // Check payers escrow balance after payment + assertEq(previousPayerEscrowBalance - _tokens, afterPayerEscrowBalance); + } +} diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol index 254d8738d..711dd7d19 100644 --- a/packages/horizon/test/escrow/collect.t.sol +++ b/packages/horizon/test/escrow/collect.t.sol @@ -5,89 +5,10 @@ import "forge-std/Test.sol"; import { IHorizonStakingMain } from "../../contracts/interfaces/internal/IHorizonStakingMain.sol"; import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; -import { IPaymentsEscrow } from "../../contracts/interfaces/IPaymentsEscrow.sol"; import { GraphEscrowTest } from "./GraphEscrow.t.sol"; contract GraphEscrowCollectTest is GraphEscrowTest { - - struct CollectPaymentData { - uint256 escrowBalance; - uint256 paymentsBalance; - uint256 receiverBalance; - uint256 delegationPoolBalance; - uint256 dataServiceBalance; - } - - function _collect( - IGraphPayments.PaymentTypes _paymentType, - address _payer, - address _receiver, - uint256 _tokens, - address _dataService, - uint256 _tokensDataService - ) private { - (, address _collector, ) = vm.readCallers(); - - // Previous balances - (uint256 previousPayerEscrowBalance,,) = escrow.escrowAccounts(_payer, _collector, _receiver); - CollectPaymentData memory previousBalances = CollectPaymentData({ - escrowBalance: token.balanceOf(address(escrow)), - paymentsBalance: token.balanceOf(address(payments)), - receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), - dataServiceBalance: token.balanceOf(_dataService) - }); - - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.EscrowCollected(_payer, _collector, _receiver, _tokens); - escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _tokensDataService); - - // Calculate cuts - uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); - uint256 delegatorCut = staking.getDelegationFeeCut( - _receiver, - _dataService, - _paymentType - ); - - // After balances - (uint256 afterPayerEscrowBalance,,) = escrow.escrowAccounts(_payer, _collector, _receiver); - CollectPaymentData memory afterBalances = CollectPaymentData({ - escrowBalance: token.balanceOf(address(escrow)), - paymentsBalance: token.balanceOf(address(payments)), - receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), - dataServiceBalance: token.balanceOf(_dataService) - }); - - // Check receiver balance after payment - uint256 receiverExpectedPayment = _tokens - _tokensDataService - _tokens * protocolPaymentCut / MAX_PPM - _tokens * delegatorCut / MAX_PPM; - assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); - assertEq(token.balanceOf(address(payments)), 0); - - // Check delegation pool balance after payment - assertEq(afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, _tokens * delegatorCut / MAX_PPM); - - // Check that the escrow account has been updated - assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); - - // Check that payments balance didn't change - assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); - - // Check data service balance after payment - assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); - - // Check payers escrow balance after payment - assertEq(previousPayerEscrowBalance - _tokens, afterPayerEscrowBalance); - } - /* * TESTS */ @@ -96,9 +17,14 @@ contract GraphEscrowCollectTest is GraphEscrowTest { uint256 tokens, uint256 delegationTokens, uint256 tokensDataService - ) public useIndexer useProvision(tokens, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = tokens * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegatoion = tokens * delegationFeeCut / MAX_PPM; + ) + public + useIndexer + useProvision(tokens, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { + uint256 tokensProtocol = (tokens * protocolPaymentCut) / MAX_PPM; + uint256 tokensDelegatoion = (tokens * delegationFeeCut) / MAX_PPM; vm.assume(tokensDataService < tokens - tokensProtocol - tokensDelegatoion); delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS); @@ -109,38 +35,70 @@ contract GraphEscrowCollectTest is GraphEscrowTest { _depositTokens(users.verifier, users.indexer, tokens); resetPrank(users.verifier); - _collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, tokens, subgraphDataServiceAddress, tokensDataService); + _collectEscrow( + IGraphPayments.PaymentTypes.QueryFee, + users.gateway, + users.indexer, + tokens, + subgraphDataServiceAddress, + tokensDataService + ); } function testCollect_RevertWhen_SenderHasInsufficientAmountInEscrow( - uint256 amount, + uint256 amount, uint256 insufficientAmount - ) public useGateway useDeposit(insufficientAmount) { + ) public useGateway useDeposit(insufficientAmount) { vm.assume(amount > 0); vm.assume(insufficientAmount < amount); vm.startPrank(users.verifier); - bytes memory expectedError = abi.encodeWithSignature("PaymentsEscrowInsufficientBalance(uint256,uint256)", insufficientAmount, amount); + bytes memory expectedError = abi.encodeWithSignature( + "PaymentsEscrowInsufficientBalance(uint256,uint256)", + insufficientAmount, + amount + ); vm.expectRevert(expectedError); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 0); + escrow.collect( + IGraphPayments.PaymentTypes.QueryFee, + users.gateway, + users.indexer, + amount, + subgraphDataServiceAddress, + 0 + ); vm.stopPrank(); } function testCollect_RevertWhen_InvalidPool( uint256 amount - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { vm.assume(amount > 1 ether); resetPrank(users.gateway); _depositTokens(users.verifier, users.indexer, amount); resetPrank(users.verifier); - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); + escrow.collect( + IGraphPayments.PaymentTypes.QueryFee, + users.gateway, users.indexer, - subgraphDataServiceAddress - )); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 1); + amount, + subgraphDataServiceAddress, + 1 + ); } function testCollect_RevertWhen_InvalidProvision( @@ -148,16 +106,25 @@ contract GraphEscrowCollectTest is GraphEscrowTest { ) public useIndexer useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { vm.assume(amount > 1 ether); vm.assume(amount <= MAX_STAKING_TOKENS); - + resetPrank(users.gateway); _depositTokens(users.verifier, users.indexer, amount); resetPrank(users.verifier); - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); + escrow.collect( + IGraphPayments.PaymentTypes.QueryFee, + users.gateway, users.indexer, - subgraphDataServiceAddress - )); - escrow.collect(IGraphPayments.PaymentTypes.QueryFee, users.gateway, users.indexer, amount, subgraphDataServiceAddress, 1); + amount, + subgraphDataServiceAddress, + 1 + ); } -} \ No newline at end of file +} diff --git a/packages/horizon/test/escrow/getters.t.sol b/packages/horizon/test/escrow/getters.t.sol new file mode 100644 index 000000000..6434e1b30 --- /dev/null +++ b/packages/horizon/test/escrow/getters.t.sol @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.27; + +import "forge-std/Test.sol"; +import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; + +import { GraphEscrowTest } from "./GraphEscrow.t.sol"; + +contract GraphEscrowGettersTest is GraphEscrowTest { + /* + * TESTS + */ + + function testGetBalance(uint256 amount) public useGateway useDeposit(amount) { + uint256 balance = escrow.getBalance(users.gateway, users.verifier, users.indexer); + assertEq(balance, amount); + } + + function testGetBalance_WhenThawing( + uint256 amountDeposit, + uint256 amountThawing + ) public useGateway useDeposit(amountDeposit) { + vm.assume(amountThawing > 0); + vm.assume(amountDeposit >= amountThawing); + + // thaw some funds + _thawEscrow(users.verifier, users.indexer, amountThawing); + + uint256 balance = escrow.getBalance(users.gateway, users.verifier, users.indexer); + assertEq(balance, amountDeposit - amountThawing); + } + + function testGetBalance_WhenCollectedOverThawing( + uint256 amountDeposit, + uint256 amountThawing, + uint256 amountCollected + ) public useGateway useDeposit(amountDeposit) { + vm.assume(amountThawing > 0); + vm.assume(amountDeposit > 0); + vm.assume(amountDeposit >= amountThawing); + vm.assume(amountDeposit >= amountCollected); + vm.assume(amountDeposit - amountCollected < amountThawing); + + // thaw some funds + _thawEscrow(users.verifier, users.indexer, amountThawing); + + // users start with max uint256 balance so we burn to avoid overflow + // TODO: we should modify all tests to consider users have a max balance thats less than max uint256 + resetPrank(users.indexer); + token.burn(amountCollected); + + // collect some funds to get the balance of the account below the amount thawing + resetPrank(users.verifier); + _collectEscrow( + IGraphPayments.PaymentTypes.QueryFee, + users.gateway, + users.indexer, + amountCollected, + subgraphDataServiceAddress, + 0 + ); + + // balance should always be 0 since thawing funds > available funds + uint256 balance = escrow.getBalance(users.gateway, users.verifier, users.indexer); + assertEq(balance, 0); + } +} diff --git a/packages/horizon/test/escrow/thaw.t.sol b/packages/horizon/test/escrow/thaw.t.sol index 8e2674e00..017c3291f 100644 --- a/packages/horizon/test/escrow/thaw.t.sol +++ b/packages/horizon/test/escrow/thaw.t.sol @@ -3,8 +3,6 @@ pragma solidity 0.8.27; import "forge-std/Test.sol"; -import { IPaymentsEscrow } from "../../contracts/interfaces/IPaymentsEscrow.sol"; - import { GraphEscrowTest } from "./GraphEscrow.t.sol"; contract GraphEscrowThawTest is GraphEscrowTest { @@ -14,14 +12,7 @@ contract GraphEscrowThawTest is GraphEscrowTest { */ function testThaw_Tokens(uint256 amount) public useGateway useDeposit(amount) { - uint256 expectedThawEndTimestamp = block.timestamp + withdrawEscrowThawingPeriod; - vm.expectEmit(address(escrow)); - emit IPaymentsEscrow.Thaw(users.gateway, users.verifier, users.indexer, amount, expectedThawEndTimestamp); - escrow.thaw(users.verifier, users.indexer, amount); - - (, uint256 amountThawing,uint256 thawEndTimestamp) = escrow.escrowAccounts(users.gateway, users.verifier, users.indexer); - assertEq(amountThawing, amount); - assertEq(thawEndTimestamp, expectedThawEndTimestamp); + _thawEscrow(users.verifier, users.indexer, amount); } function testThaw_RevertWhen_InsufficientThawAmount( From a9bf99c851bbfbc6adc32466f326bd7164fa8b19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Wed, 27 Nov 2024 17:26:27 -0300 Subject: [PATCH 5/9] fix: allow partially collecting RAVs (TRST-M05) 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 | 25 ++++ .../payments/collectors/TAPCollector.sol | 119 ++++++++++-------- .../payments/tap-collector/TAPCollector.t.sol | 18 ++- .../tap-collector/collect/collect.t.sol | 56 ++++++++- 4 files changed, 162 insertions(+), 56 deletions(-) diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index b364135c3..347ccf565 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/packages/horizon/contracts/interfaces/ITAPCollector.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.27; import { IPaymentsCollector } from "./IPaymentsCollector.sol"; +import { IGraphPayments } from "./IGraphPayments.sol"; /** * @title Interface for the {TAPCollector} contract @@ -175,6 +176,13 @@ interface ITAPCollector is IPaymentsCollector { */ error TAPCollectorInconsistentRAVTokens(uint256 tokens, uint256 tokensCollected); + /** + * Thrown when the attempting to collect more tokens than what it's owed + * @param tokensToCollect The amount of tokens to collect + * @param maxTokensToCollect The maximum amount of tokens to collect + */ + error TAPCollectorInvalidTokensToCollectAmount(uint256 tokensToCollect, uint256 maxTokensToCollect); + /** * @notice Authorize a signer to sign on behalf of the payer. * A signer can not be authorized for multiple payers even after revoking previous authorizations. @@ -237,4 +245,21 @@ interface ITAPCollector is IPaymentsCollector { * @return The hash of the RAV. */ function encodeRAV(ReceiptAggregateVoucher calldata rav) external view returns (bytes32); + + /** + * @notice See {IPaymentsCollector.collect} + * This variant adds the ability to partially collect a RAV by specifying the amount of tokens to collect. + * + * Requirements: + * - The amount of tokens to collect must be less than or equal to the total amount of tokens in the RAV minus + * the tokens already collected. + * @param paymentType The payment type to collect + * @param data Additional data required for the payment collection + * @param tokensToCollect The amount of tokens to collect + */ + function collect( + IGraphPayments.PaymentTypes paymentType, + bytes calldata data, + uint256 tokensToCollect + ) external returns (uint256); } diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index a5449e1cc..c98ef68a1 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -125,37 +125,15 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { * @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) { - // Ensure caller is the RAV data service - (SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(data, (SignedRAV, uint256)); - require( - signedRAV.rav.dataService == msg.sender, - TAPCollectorCallerNotDataService(msg.sender, signedRAV.rav.dataService) - ); - - // Ensure RAV signer is authorized for a payer - address signer = _recoverRAVSigner(signedRAV); - require( - authorizedSigners[signer].payer != address(0) && !authorizedSigners[signer].revoked, - TAPCollectorInvalidRAVSigner() - ); - - // Ensure RAV payer matches the authorized payer - address payer = signedRAV.rav.payer; - require( - authorizedSigners[signer].payer == payer, - TAPCollectorInvalidRAVPayer(authorizedSigners[signer].payer, payer) - ); - - // 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, data, 0); + } - return _collect(paymentType, authorizedSigners[signer].payer, signedRAV, dataServiceCut); + function collect( + IGraphPayments.PaymentTypes paymentType, + bytes memory data, + uint256 tokensToCollect + ) external override returns (uint256) { + return _collect(paymentType, data, tokensToCollect); } /** @@ -177,28 +155,71 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { */ function _collect( IGraphPayments.PaymentTypes _paymentType, - address _payer, - SignedRAV memory _signedRAV, - uint256 _dataServiceCut + bytes memory _data, + uint256 _tokensToCollect ) private returns (uint256) { - address dataService = _signedRAV.rav.dataService; - address receiver = _signedRAV.rav.serviceProvider; + // Ensure caller is the RAV data service + (SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (SignedRAV, uint256)); + require( + signedRAV.rav.dataService == msg.sender, + TAPCollectorCallerNotDataService(msg.sender, signedRAV.rav.dataService) + ); + + // Ensure RAV signer is authorized for a payer + address signer = _recoverRAVSigner(signedRAV); + require( + authorizedSigners[signer].payer != address(0) && !authorizedSigners[signer].revoked, + TAPCollectorInvalidRAVSigner() + ); - uint256 tokensRAV = _signedRAV.rav.valueAggregate; - uint256 tokensAlreadyCollected = tokensCollected[dataService][receiver][_payer]; + // Ensure RAV payer matches the authorized payer + address payer = authorizedSigners[signer].payer; require( - tokensRAV > tokensAlreadyCollected, - TAPCollectorInconsistentRAVTokens(tokensRAV, tokensAlreadyCollected) + signedRAV.rav.payer == payer, + TAPCollectorInvalidRAVPayer(payer, signedRAV.rav.payer) ); - uint256 tokensToCollect = tokensRAV - tokensAlreadyCollected; - uint256 tokensDataService = tokensToCollect.mulPPM(_dataServiceCut); + address dataService = signedRAV.rav.dataService; + address receiver = signedRAV.rav.serviceProvider; + + // 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)); + } + + uint256 tokensToCollect = 0; + { + uint256 tokensRAV = signedRAV.rav.valueAggregate; + uint256 tokensAlreadyCollected = tokensCollected[dataService][receiver][payer]; + require( + tokensRAV > tokensAlreadyCollected, + TAPCollectorInconsistentRAVTokens(tokensRAV, tokensAlreadyCollected) + ); + + if (_tokensToCollect == 0) { + tokensToCollect = tokensRAV - tokensAlreadyCollected; + } else { + require( + _tokensToCollect <= tokensRAV - tokensAlreadyCollected, + TAPCollectorInvalidTokensToCollectAmount(_tokensToCollect, tokensRAV - tokensAlreadyCollected) + ); + tokensToCollect = _tokensToCollect; + } + } + + uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); if (tokensToCollect > 0) { - tokensCollected[dataService][receiver][_payer] = tokensRAV; + tokensCollected[dataService][receiver][payer] += tokensToCollect; _graphPaymentsEscrow().collect( _paymentType, - _payer, + payer, receiver, tokensToCollect, dataService, @@ -206,15 +227,15 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { ); } - emit PaymentCollected(_paymentType, _payer, receiver, tokensToCollect, dataService, tokensDataService); + emit PaymentCollected(_paymentType, payer, receiver, tokensToCollect, dataService, tokensDataService); emit RAVCollected( - _payer, + payer, dataService, receiver, - _signedRAV.rav.timestampNs, - _signedRAV.rav.valueAggregate, - _signedRAV.rav.metadata, - _signedRAV.signature + signedRAV.rav.timestampNs, + signedRAV.rav.valueAggregate, + signedRAV.rav.metadata, + signedRAV.signature ); return tokensToCollect; } diff --git a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol index 1120c5b92..ac67d6552 100644 --- a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol +++ b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol @@ -119,12 +119,20 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest } function _collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data) internal { + __collect(_paymentType, _data, 0); + } + + function _collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data, uint256 _tokensToCollect) internal { + __collect(_paymentType, _data, _tokensToCollect); + } + + function __collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data, uint256 _tokensToCollect) internal { (ITAPCollector.SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (ITAPCollector.SignedRAV, uint256)); bytes32 messageHash = tapCollector.encodeRAV(signedRAV.rav); address _signer = ECDSA.recover(messageHash, signedRAV.signature); (address _payer, , ) = tapCollector.authorizedSigners(_signer); uint256 tokensAlreadyCollected = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); - uint256 tokensToCollect = signedRAV.rav.valueAggregate - tokensAlreadyCollected; + uint256 tokensToCollect = _tokensToCollect == 0 ? signedRAV.rav.valueAggregate - tokensAlreadyCollected : _tokensToCollect; uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); vm.expectEmit(address(tapCollector)); @@ -136,6 +144,7 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest signedRAV.rav.dataService, tokensDataService ); + vm.expectEmit(address(tapCollector)); emit ITAPCollector.RAVCollected( _payer, signedRAV.rav.dataService, @@ -145,11 +154,10 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest signedRAV.rav.metadata, signedRAV.signature ); - - uint256 tokensCollected = tapCollector.collect(_paymentType, _data); - assertEq(tokensCollected, tokensToCollect); + uint256 tokensCollected = _tokensToCollect == 0 ? tapCollector.collect(_paymentType, _data) : tapCollector.collect(_paymentType, _data, _tokensToCollect); uint256 tokensCollectedAfter = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); - assertEq(tokensCollectedAfter, signedRAV.rav.valueAggregate); + assertEq(tokensCollected, tokensToCollect); + assertEq(tokensCollectedAfter, _tokensToCollect == 0 ? signedRAV.rav.valueAggregate : tokensAlreadyCollected + _tokensToCollect); } } 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 ddb76b919..a4e1eafa7 100644 --- a/packages/horizon/test/payments/tap-collector/collect/collect.t.sol +++ b/packages/horizon/test/payments/tap-collector/collect/collect.t.sol @@ -203,11 +203,12 @@ contract TAPCollectorCollectTest is TAPCollectorTest { tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data); } - function testTAPCollector_Collect_RevertWhen_PayerMismatch(uint256 tokens) public useGateway useSigner { + function testTAPCollector_Collect_RevertWhen_PayerMismatch( + 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); (address anotherPayer, ) = makeAddrAndKey("anotherPayer"); @@ -340,4 +341,55 @@ contract TAPCollectorCollectTest is TAPCollectorTest { resetPrank(users.verifier); _collect(IGraphPayments.PaymentTypes.QueryFee, data); } + + function testTAPCollector_CollectPartial( + uint256 tokens, + uint256 tokensToCollect + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { + tokens = bound(tokens, 1, type(uint128).max); + tokensToCollect = bound(tokensToCollect, 1, tokens); + + _depositTokens(address(tapCollector), users.indexer, tokens); + + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); + + resetPrank(users.verifier); + _collect(IGraphPayments.PaymentTypes.QueryFee, data, tokensToCollect); + } + + function testTAPCollector_CollectPartial_RevertWhen_AmountTooHigh( + uint256 tokens, + uint256 tokensToCollect + ) public useIndexer useProvisionDataService(users.verifier, 100, 0, 0) useGateway useSigner { + tokens = bound(tokens, 1, type(uint128).max - 1); + + _depositTokens(address(tapCollector), users.indexer, tokens); + + bytes memory data = _getQueryFeeEncodedData( + signerPrivateKey, + users.gateway, + users.indexer, + users.verifier, + uint128(tokens) + ); + + resetPrank(users.verifier); + uint256 tokensAlreadyCollected = tapCollector.tokensCollected(users.verifier, users.indexer, users.gateway); + tokensToCollect = bound(tokensToCollect, tokens - tokensAlreadyCollected + 1, type(uint128).max); + + vm.expectRevert( + abi.encodeWithSelector( + ITAPCollector.TAPCollectorInvalidTokensToCollectAmount.selector, + tokensToCollect, + tokens - tokensAlreadyCollected + ) + ); + tapCollector.collect(IGraphPayments.PaymentTypes.QueryFee, data, tokensToCollect); + } } From 670fba7cf1538e30a5b38465c51ed30402310e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Thu, 28 Nov 2024 12:28:49 -0300 Subject: [PATCH 6/9] fix: subgraph service test broke after rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../test/subgraphService/collect/query/query.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol b/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol index 65d7e23d7..d42c36f65 100644 --- a/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol +++ b/packages/subgraph-service/test/subgraphService/collect/query/query.t.sol @@ -46,6 +46,7 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest { ) private view returns (ITAPCollector.ReceiptAggregateVoucher memory rav) { return ITAPCollector.ReceiptAggregateVoucher({ + payer: users.gateway, dataService: address(subgraphService), serviceProvider: indexer, timestampNs: 0, From 2bf44562faeace3cc021758081cd1368b872fe02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Tue, 3 Dec 2024 12:47:55 -0300 Subject: [PATCH 7/9] fix: verify state transition for tap collector thawing signers (TRST-R03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../horizon/contracts/interfaces/ITAPCollector.sol | 7 +++++++ .../contracts/payments/collectors/TAPCollector.sol | 9 +++++---- .../payments/tap-collector/signer/thawSigner.t.sol | 13 +++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index 347ccf565..194edb11a 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/packages/horizon/contracts/interfaces/ITAPCollector.sol @@ -130,6 +130,13 @@ interface ITAPCollector is IPaymentsCollector { */ error TAPCollectorAuthorizationAlreadyRevoked(address payer, address signer); + /** + * Thrown when attempting to thaw a signer that is already thawing + * @param signer The address of the signer + * @param thawEndTimestamp The timestamp at which the thawing period ends + */ + error TAPCollectorSignerAlreadyThawing(address signer, uint256 thawEndTimestamp); + /** * Thrown when the signer is not thawing * @param signer The address of the signer diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index c98ef68a1..e23d16486 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -82,6 +82,10 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { require(authorization.payer == msg.sender, TAPCollectorSignerNotAuthorizedByPayer(msg.sender, signer)); require(!authorization.revoked, TAPCollectorAuthorizationAlreadyRevoked(msg.sender, signer)); + require( + authorization.thawEndTimestamp == 0, + TAPCollectorSignerAlreadyThawing(signer, authorization.thawEndTimestamp) + ); authorization.thawEndTimestamp = block.timestamp + REVOKE_SIGNER_THAWING_PERIOD; emit SignerThawing(msg.sender, signer, authorization.thawEndTimestamp); @@ -174,10 +178,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { // Ensure RAV payer matches the authorized payer address payer = authorizedSigners[signer].payer; - require( - signedRAV.rav.payer == payer, - TAPCollectorInvalidRAVPayer(payer, signedRAV.rav.payer) - ); + require(signedRAV.rav.payer == payer, TAPCollectorInvalidRAVPayer(payer, signedRAV.rav.payer)); address dataService = signedRAV.rav.dataService; address receiver = signedRAV.rav.serviceProvider; diff --git a/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol b/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol index db9d99040..def79d831 100644 --- a/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol +++ b/packages/horizon/test/payments/tap-collector/signer/thawSigner.t.sol @@ -40,4 +40,17 @@ contract TAPCollectorThawSignerTest is TAPCollectorTest { vm.expectRevert(expectedError); tapCollector.thawSigner(signer); } + + function testTAPCollector_ThawSigner_RevertWhen_AlreadyThawing() public useGateway useSigner { + _thawSigner(signer); + + (,uint256 thawEndTimestamp,) = tapCollector.authorizedSigners(signer); + bytes memory expectedError = abi.encodeWithSelector( + ITAPCollector.TAPCollectorSignerAlreadyThawing.selector, + signer, + thawEndTimestamp + ); + vm.expectRevert(expectedError); + tapCollector.thawSigner(signer); + } } From a06580420f485169c42b17df6b2109a340f3a23f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Tue, 10 Dec 2024 11:05:53 -0300 Subject: [PATCH 8/9] fix: refactor payments cut distribution (TRST-L12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/interfaces/IGraphPayments.sol | 35 +++-- .../interfaces/IPaymentsCollector.sol | 6 +- .../contracts/interfaces/IPaymentsEscrow.sol | 4 +- .../contracts/payments/GraphPayments.sol | 49 +++--- .../contracts/payments/PaymentsEscrow.sol | 4 +- .../payments/collectors/TAPCollector.sol | 13 +- .../horizon/test/escrow/GraphEscrow.t.sol | 53 ++++--- packages/horizon/test/escrow/collect.t.sol | 10 +- .../horizon/test/payments/GraphPayments.t.sol | 144 ++++++++++-------- .../payments/tap-collector/TAPCollector.t.sol | 54 +++++-- .../contracts/SubgraphService.sol | 7 +- .../contracts/interfaces/ISubgraphService.sol | 6 +- .../subgraphService/SubgraphService.t.sol | 7 +- 13 files changed, 227 insertions(+), 165 deletions(-) diff --git a/packages/horizon/contracts/interfaces/IGraphPayments.sol b/packages/horizon/contracts/interfaces/IGraphPayments.sol index f446d6f52..eaac08ae4 100644 --- a/packages/horizon/contracts/interfaces/IGraphPayments.sol +++ b/packages/horizon/contracts/interfaces/IGraphPayments.sol @@ -23,27 +23,30 @@ interface IGraphPayments { * @param payer The address of the payer * @param receiver The address of the receiver * @param dataService The address of the data service - * @param tokensReceiver Amount of tokens for the receiver - * @param tokensDelegationPool Amount of tokens for delegators - * @param tokensDataService Amount of tokens for the data service + * @param tokens The total amount of tokens being collected * @param tokensProtocol Amount of tokens charged as protocol tax + * @param tokensDataService Amount of tokens for the data service + * @param tokensDelegationPool Amount of tokens for delegators + * @param tokensReceiver Amount of tokens for the receiver */ - event PaymentCollected( + event GraphPaymentCollected( address indexed payer, address indexed receiver, address indexed dataService, - uint256 tokensReceiver, - uint256 tokensDelegationPool, + uint256 tokens, + uint256 tokensProtocol, uint256 tokensDataService, - uint256 tokensProtocol + uint256 tokensDelegationPool, + uint256 tokensReceiver ); /** - * @notice Thrown when there are insufficient tokens to pay the required amount - * @param tokens The amount of tokens available - * @param minTokens The amount of tokens being collected + * @notice Thrown when the calculated amount of tokens to be paid out to all parties is + * not the same as the amount of tokens being collected + * @param tokens The amount of tokens being collected + * @param tokensCalculated The sum of all the tokens to be paid out */ - error GraphPaymentsInsufficientTokens(uint256 tokens, uint256 minTokens); + error GraphPaymentsBadAccounting(uint256 tokens, uint256 tokensCalculated); /** * @notice Thrown when the protocol payment cut is invalid @@ -51,6 +54,12 @@ interface IGraphPayments { */ error GraphPaymentsInvalidProtocolPaymentCut(uint256 protocolPaymentCut); + /** + * @notice Thrown when trying to use a cut that is not expressed in PPM + * @param cut The cut + */ + error GraphPaymentsInvalidCut(uint256 cut); + /** * @notice Collects funds from a payer. * It will pay cuts to all relevant parties and forward the rest to the receiver. @@ -58,13 +67,13 @@ interface IGraphPayments { * @param receiver The address of the receiver * @param tokens The amount of tokens being collected * @param dataService The address of the data service - * @param tokensDataService The amount of tokens that should be sent to the data service + * @param dataServiceCut The data service cut in PPM */ function collect( PaymentTypes paymentType, address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external; } diff --git a/packages/horizon/contracts/interfaces/IPaymentsCollector.sol b/packages/horizon/contracts/interfaces/IPaymentsCollector.sol index 85d09d59f..61854bb71 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsCollector.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsCollector.sol @@ -19,17 +19,15 @@ interface IPaymentsCollector { * @param paymentType The payment type collected as defined by {IGraphPayments} * @param payer The address of the payer * @param receiver The address of the receiver - * @param tokensReceiver The amount of tokens received by the receiver * @param dataService The address of the data service - * @param tokensDataService The amount of tokens received by the data service + * @param tokens The amount of tokens being collected */ event PaymentCollected( IGraphPayments.PaymentTypes indexed paymentType, address indexed payer, address receiver, - uint256 tokensReceiver, address indexed dataService, - uint256 tokensDataService + uint256 tokens ); /** diff --git a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol index 32f83d597..760a086a7 100644 --- a/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol +++ b/packages/horizon/contracts/interfaces/IPaymentsEscrow.sol @@ -186,7 +186,7 @@ interface IPaymentsEscrow { * @param receiver The address of the receiver * @param tokens The amount of tokens to collect * @param dataService The address of the data service - * @param tokensDataService The amount of tokens that {GraphPayments} should send to the data service + * @param dataServiceCut The data service cut in PPM that {GraphPayments} should send */ function collect( IGraphPayments.PaymentTypes paymentType, @@ -194,7 +194,7 @@ interface IPaymentsEscrow { address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external; /** diff --git a/packages/horizon/contracts/payments/GraphPayments.sol b/packages/horizon/contracts/payments/GraphPayments.sol index 0ebe566a5..0dd06ef72 100644 --- a/packages/horizon/contracts/payments/GraphPayments.sol +++ b/packages/horizon/contracts/payments/GraphPayments.sol @@ -32,7 +32,7 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I * @param protocolPaymentCut The protocol tax in PPM */ constructor(address controller, uint256 protocolPaymentCut) GraphDirectory(controller) { - require(PPMMath.isValidPPM(protocolPaymentCut), GraphPaymentsInvalidProtocolPaymentCut(protocolPaymentCut)); + require(PPMMath.isValidPPM(protocolPaymentCut), GraphPaymentsInvalidCut(protocolPaymentCut)); PROTOCOL_PAYMENT_CUT = protocolPaymentCut; _disableInitializers(); } @@ -52,41 +52,54 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external { + require(PPMMath.isValidPPM(dataServiceCut), GraphPaymentsInvalidCut(dataServiceCut)); + + // Pull tokens from the sender _graphToken().pullTokens(msg.sender, tokens); - // Calculate cuts - uint256 tokensProtocol = tokens.mulPPM(PROTOCOL_PAYMENT_CUT); - uint256 delegationFeeCut = _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType); - uint256 tokensDelegationPool = tokens.mulPPM(delegationFeeCut); - uint256 totalCut = tokensProtocol + tokensDataService + tokensDelegationPool; - require(tokens >= totalCut, GraphPaymentsInsufficientTokens(tokens, totalCut)); + // Calculate token amounts for each party + // Order matters: protocol -> data service -> delegators -> receiver + // Note the substractions should not underflow as we are only deducting a percentage of the remainder + uint256 tokensRemaining = tokens; + + uint256 tokensProtocol = tokensRemaining.mulPPMRoundUp(PROTOCOL_PAYMENT_CUT); + tokensRemaining = tokensRemaining - tokensProtocol; + + uint256 tokensDataService = tokensRemaining.mulPPMRoundUp(dataServiceCut); + tokensRemaining = tokensRemaining - tokensDataService; + + uint256 tokensDelegationPool = tokensRemaining.mulPPMRoundUp( + _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType) + ); + tokensRemaining = tokensRemaining - tokensDelegationPool; + + // Ensure accounting is correct + uint256 tokensTotal = tokensProtocol + tokensDataService + tokensDelegationPool + tokensRemaining; + require(tokens == tokensTotal, GraphPaymentsBadAccounting(tokens, tokensTotal)); - // Pay protocol cut + // Pay all parties _graphToken().burnTokens(tokensProtocol); - // Pay data service cut _graphToken().pushTokens(dataService, tokensDataService); - // Pay delegators if (tokensDelegationPool > 0) { _graphToken().approve(address(_graphStaking()), tokensDelegationPool); _graphStaking().addToDelegationPool(receiver, dataService, tokensDelegationPool); } - // Pay receiver - uint256 tokensReceiverRemaining = tokens - totalCut; - _graphToken().pushTokens(receiver, tokensReceiverRemaining); + _graphToken().pushTokens(receiver, tokensRemaining); - emit PaymentCollected( + emit GraphPaymentCollected( msg.sender, receiver, dataService, - tokensReceiverRemaining, - tokensDelegationPool, + tokens, + tokensProtocol, tokensDataService, - tokensProtocol + tokensDelegationPool, + tokensRemaining ); } } diff --git a/packages/horizon/contracts/payments/PaymentsEscrow.sol b/packages/horizon/contracts/payments/PaymentsEscrow.sol index 108197fb3..6f4252873 100644 --- a/packages/horizon/contracts/payments/PaymentsEscrow.sol +++ b/packages/horizon/contracts/payments/PaymentsEscrow.sol @@ -129,7 +129,7 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, address receiver, uint256 tokens, address dataService, - uint256 tokensDataService + uint256 dataServiceCut ) external override notPaused { // Check if there are enough funds in the escrow account EscrowAccount storage account = escrowAccounts[payer][msg.sender][receiver]; @@ -141,7 +141,7 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, uint256 balanceBefore = _graphToken().balanceOf(address(this)); _graphToken().approve(address(_graphPayments()), tokens); - _graphPayments().collect(paymentType, receiver, tokens, dataService, tokensDataService); + _graphPayments().collect(paymentType, receiver, tokens, dataService, dataServiceCut); uint256 balanceAfter = _graphToken().balanceOf(address(this)); require( diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index e23d16486..8d15ff5c8 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -214,21 +214,12 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { } } - uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); - if (tokensToCollect > 0) { tokensCollected[dataService][receiver][payer] += tokensToCollect; - _graphPaymentsEscrow().collect( - _paymentType, - payer, - receiver, - tokensToCollect, - dataService, - tokensDataService - ); + _graphPaymentsEscrow().collect(_paymentType, payer, receiver, tokensToCollect, dataService, dataServiceCut); } - emit PaymentCollected(_paymentType, payer, receiver, tokensToCollect, dataService, tokensDataService); + emit PaymentCollected(_paymentType, payer, receiver, dataService, tokensToCollect); emit RAVCollected( payer, dataService, diff --git a/packages/horizon/test/escrow/GraphEscrow.t.sol b/packages/horizon/test/escrow/GraphEscrow.t.sol index b2c4438bc..120713c6c 100644 --- a/packages/horizon/test/escrow/GraphEscrow.t.sol +++ b/packages/horizon/test/escrow/GraphEscrow.t.sol @@ -7,8 +7,11 @@ import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStakingShared.t.sol"; import { PaymentsEscrowSharedTest } from "../shared/payments-escrow/PaymentsEscrowShared.t.sol"; +import { PPMMath } from "../../contracts/libraries/PPMMath.sol"; contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { + using PPMMath for uint256; + /* * MODIFIERS */ @@ -59,6 +62,7 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { uint256 receiverBalance; uint256 delegationPoolBalance; uint256 dataServiceBalance; + uint256 payerEscrowBalance; } function _collectEscrow( @@ -67,53 +71,62 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { address _receiver, uint256 _tokens, address _dataService, - uint256 _tokensDataService + uint256 _dataServiceCut ) internal { (, address _collector, ) = vm.readCallers(); // Previous balances - (uint256 previousPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); CollectPaymentData memory previousBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), - dataServiceBalance: token.balanceOf(_dataService) + dataServiceBalance: token.balanceOf(_dataService), + payerEscrowBalance: 0 }); + { + (uint256 payerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + previousBalances.payerEscrowBalance = payerEscrowBalance; + } + vm.expectEmit(address(escrow)); emit IPaymentsEscrow.EscrowCollected(_payer, _collector, _receiver, _tokens); - escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _tokensDataService); + escrow.collect(_paymentType, _payer, _receiver, _tokens, _dataService, _dataServiceCut); // Calculate cuts - uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); - uint256 delegatorCut = staking.getDelegationFeeCut(_receiver, _dataService, _paymentType); + // this is nasty but stack is indeed too deep + uint256 tokensDataService = (_tokens - _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT())).mulPPMRoundUp( + _dataServiceCut + ); + uint256 tokensDelegation = (_tokens - + _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()) - + tokensDataService).mulPPMRoundUp(staking.getDelegationFeeCut(_receiver, _dataService, _paymentType)); + uint256 receiverExpectedPayment = _tokens - + _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()) - + tokensDataService - + tokensDelegation; // After balances - (uint256 afterPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); CollectPaymentData memory afterBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), - dataServiceBalance: token.balanceOf(_dataService) + dataServiceBalance: token.balanceOf(_dataService), + payerEscrowBalance: 0 }); + { + (uint256 afterPayerEscrowBalance, , ) = escrow.escrowAccounts(_payer, _collector, _receiver); + afterBalances.payerEscrowBalance = afterPayerEscrowBalance; + } // Check receiver balance after payment - uint256 receiverExpectedPayment = _tokens - - _tokensDataService - - (_tokens * protocolPaymentCut) / - MAX_PPM - - (_tokens * delegatorCut) / - MAX_PPM; assertEq(afterBalances.receiverBalance - previousBalances.receiverBalance, receiverExpectedPayment); assertEq(token.balanceOf(address(payments)), 0); // Check delegation pool balance after payment - assertEq( - afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, - (_tokens * delegatorCut) / MAX_PPM - ); + assertEq(afterBalances.delegationPoolBalance - previousBalances.delegationPoolBalance, tokensDelegation); // Check that the escrow account has been updated assertEq(previousBalances.escrowBalance, afterBalances.escrowBalance + _tokens); @@ -122,9 +135,9 @@ contract GraphEscrowTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest { assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); // Check data service balance after payment - assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, tokensDataService); // Check payers escrow balance after payment - assertEq(previousPayerEscrowBalance - _tokens, afterPayerEscrowBalance); + assertEq(previousBalances.payerEscrowBalance - _tokens, afterBalances.payerEscrowBalance); } } diff --git a/packages/horizon/test/escrow/collect.t.sol b/packages/horizon/test/escrow/collect.t.sol index 711dd7d19..106582beb 100644 --- a/packages/horizon/test/escrow/collect.t.sol +++ b/packages/horizon/test/escrow/collect.t.sol @@ -16,18 +16,16 @@ contract GraphEscrowCollectTest is GraphEscrowTest { function testCollect_Tokens( uint256 tokens, uint256 delegationTokens, - uint256 tokensDataService + uint256 dataServiceCut ) public useIndexer useProvision(tokens, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = (tokens * protocolPaymentCut) / MAX_PPM; - uint256 tokensDelegatoion = (tokens * delegationFeeCut) / MAX_PPM; - vm.assume(tokensDataService < tokens - tokensProtocol - tokensDelegatoion); - + dataServiceCut = bound(dataServiceCut, 0, MAX_PPM); delegationTokens = bound(delegationTokens, 1, MAX_STAKING_TOKENS); + resetPrank(users.delegator); _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0); @@ -41,7 +39,7 @@ contract GraphEscrowCollectTest is GraphEscrowTest { users.indexer, tokens, subgraphDataServiceAddress, - tokensDataService + dataServiceCut ); } diff --git a/packages/horizon/test/payments/GraphPayments.t.sol b/packages/horizon/test/payments/GraphPayments.t.sol index 19028bde1..559180bf1 100644 --- a/packages/horizon/test/payments/GraphPayments.t.sol +++ b/packages/horizon/test/payments/GraphPayments.t.sol @@ -8,8 +8,10 @@ import { IGraphPayments } from "../../contracts/interfaces/IGraphPayments.sol"; import { GraphPayments } from "../../contracts/payments/GraphPayments.sol"; import { HorizonStakingSharedTest } from "../shared/horizon-staking/HorizonStakingShared.t.sol"; +import { PPMMath } from "../../contracts/libraries/PPMMath.sol"; contract GraphPaymentsTest is HorizonStakingSharedTest { + using PPMMath for uint256; struct CollectPaymentData { uint256 escrowBalance; @@ -24,54 +26,52 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { address _receiver, uint256 _tokens, address _dataService, - uint256 _tokensDataService + uint256 _dataServiceCut ) private { // Previous balances CollectPaymentData memory previousBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), dataServiceBalance: token.balanceOf(_dataService) }); // Calculate cuts - uint256 protocolPaymentCut = payments.PROTOCOL_PAYMENT_CUT(); - uint256 delegatorCut = staking.getDelegationFeeCut( - _receiver, - _dataService, - _paymentType + uint256 tokensProtocol = _tokens.mulPPMRoundUp(payments.PROTOCOL_PAYMENT_CUT()); + uint256 tokensDataService = (_tokens - tokensProtocol).mulPPMRoundUp(_dataServiceCut); + uint256 tokensDelegation = (_tokens - tokensProtocol - tokensDataService).mulPPMRoundUp( + staking.getDelegationFeeCut(_receiver, _dataService, _paymentType) ); - uint256 tokensProtocol = _tokens * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegation = _tokens * delegatorCut / MAX_PPM; - uint256 receiverExpectedPayment = _tokens - _tokensDataService - tokensProtocol - tokensDelegation; + uint256 receiverExpectedPayment = _tokens - tokensProtocol - tokensDataService - tokensDelegation; - (,address msgSender, ) = vm.readCallers(); + (, address msgSender, ) = vm.readCallers(); vm.expectEmit(address(payments)); - emit IGraphPayments.PaymentCollected( + emit IGraphPayments.GraphPaymentCollected( msgSender, _receiver, _dataService, - receiverExpectedPayment, + _tokens, + tokensProtocol, + tokensDataService, tokensDelegation, - _tokensDataService, - tokensProtocol + receiverExpectedPayment + ); + payments.collect( + _paymentType, + _receiver, + _tokens, + _dataService, + _dataServiceCut ); - payments.collect(_paymentType, _receiver, _tokens, _dataService, _tokensDataService); // After balances CollectPaymentData memory afterBalances = CollectPaymentData({ escrowBalance: token.balanceOf(address(escrow)), paymentsBalance: token.balanceOf(address(payments)), receiverBalance: token.balanceOf(_receiver), - delegationPoolBalance: staking.getDelegatedTokensAvailable( - _receiver, - _dataService - ), + delegationPoolBalance: staking.getDelegatedTokensAvailable(_receiver, _dataService), dataServiceBalance: token.balanceOf(_dataService) }); @@ -89,7 +89,7 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { assertEq(previousBalances.paymentsBalance, afterBalances.paymentsBalance); // Check data service balance after payment - assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, _tokensDataService); + assertEq(afterBalances.dataServiceBalance - previousBalances.dataServiceBalance, tokensDataService); } /* @@ -97,11 +97,11 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { */ function testConstructor_RevertIf_InvalidProtocolPaymentCut(uint256 protocolPaymentCut) public { - protocolPaymentCut = bound(protocolPaymentCut, MAX_PPM + 1, MAX_PPM + 100); + protocolPaymentCut = bound(protocolPaymentCut, MAX_PPM + 1, type(uint256).max); resetPrank(users.deployer); bytes memory expectedError = abi.encodeWithSelector( - IGraphPayments.GraphPaymentsInvalidProtocolPaymentCut.selector, + IGraphPayments.GraphPaymentsInvalidCut.selector, protocolPaymentCut ); vm.expectRevert(expectedError); @@ -110,12 +110,15 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { function testCollect( uint256 amount, - uint256 tokensDataService, + uint256 dataServiceCut, uint256 tokensDelegate - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegation = amount * delegationFeeCut / MAX_PPM; - vm.assume(tokensDataService < amount - tokensProtocol - tokensDelegation); + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { + dataServiceCut = bound(dataServiceCut, 0, MAX_PPM); address escrowAddress = address(escrow); // Delegate tokens @@ -129,35 +132,50 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { approve(address(payments), amount); // Collect payments through GraphPayments - _collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + _collect( + IGraphPayments.PaymentTypes.QueryFee, + users.indexer, + amount, + subgraphDataServiceAddress, + dataServiceCut + ); vm.stopPrank(); } - function testCollect_RevertWhen_InsufficientAmount( + function testCollect_RevertWhen_InvalidDataServiceCut( uint256 amount, - uint256 tokensDataService - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { - tokensDataService = bound(tokensDataService, amount + 1, MAX_STAKING_TOKENS + 1); - - address escrowAddress = address(escrow); - mint(escrowAddress, amount); - vm.startPrank(escrowAddress); - approve(address(payments), amount); + uint256 dataServiceCut + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { + dataServiceCut = bound(dataServiceCut, MAX_PPM + 1, type(uint256).max); - bytes memory expectedError; - { - uint256 tokensProtocol = amount * protocolPaymentCut / MAX_PPM; - uint256 tokensDelegatoion = amount * delegationFeeCut / MAX_PPM; - uint256 requiredAmount = tokensDataService + tokensProtocol + tokensDelegatoion; - expectedError = abi.encodeWithSignature("GraphPaymentsInsufficientTokens(uint256,uint256)", amount, requiredAmount); - } + resetPrank(users.deployer); + bytes memory expectedError = abi.encodeWithSelector( + IGraphPayments.GraphPaymentsInvalidCut.selector, + dataServiceCut + ); vm.expectRevert(expectedError); - payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, tokensDataService); + payments.collect( + IGraphPayments.PaymentTypes.QueryFee, + users.indexer, + amount, + subgraphDataServiceAddress, + dataServiceCut + ); } function testCollect_RevertWhen_InvalidPool( uint256 amount - ) public useIndexer useProvision(amount, 0, 0) useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) { + ) + public + useIndexer + useProvision(amount, 0, 0) + useDelegationFeeCut(IGraphPayments.PaymentTypes.QueryFee, delegationFeeCut) + { vm.assume(amount > 1 ether); address escrowAddress = address(escrow); @@ -167,11 +185,13 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { approve(address(payments), amount); // Collect payments through GraphPayments - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, - users.indexer, - subgraphDataServiceAddress - )); + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidDelegationPool.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); } @@ -181,18 +201,20 @@ contract GraphPaymentsTest is HorizonStakingSharedTest { vm.assume(amount > 1 ether); vm.assume(amount <= MAX_STAKING_TOKENS); address escrowAddress = address(escrow); - + // Add tokens in escrow mint(escrowAddress, amount); vm.startPrank(escrowAddress); approve(address(payments), amount); // Collect payments through GraphPayments - vm.expectRevert(abi.encodeWithSelector( - IHorizonStakingMain.HorizonStakingInvalidProvision.selector, - users.indexer, - subgraphDataServiceAddress - )); + vm.expectRevert( + abi.encodeWithSelector( + IHorizonStakingMain.HorizonStakingInvalidProvision.selector, + users.indexer, + subgraphDataServiceAddress + ) + ); payments.collect(IGraphPayments.PaymentTypes.QueryFee, users.indexer, amount, subgraphDataServiceAddress, 1); } } diff --git a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol index ac67d6552..362cff8af 100644 --- a/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol +++ b/packages/horizon/test/payments/tap-collector/TAPCollector.t.sol @@ -60,12 +60,12 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest function _authorizeSigner(address _signer, uint256 _proofDeadline, bytes memory _proof) internal { (, address msgSender, ) = vm.readCallers(); - + vm.expectEmit(address(tapCollector)); emit ITAPCollector.SignerAuthorized(msgSender, _signer); - + tapCollector.authorizeSigner(_signer, _proofDeadline, _proof); - + (address _payer, uint256 thawEndTimestamp, bool revoked) = tapCollector.authorizedSigners(_signer); assertEq(_payer, msgSender); assertEq(thawEndTimestamp, 0); @@ -111,7 +111,9 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest tapCollector.revokeAuthorizedSigner(_signer); - (address afterPayer, uint256 afterThawEndTimestamp, bool afterRevoked) = tapCollector.authorizedSigners(_signer); + (address afterPayer, uint256 afterThawEndTimestamp, bool afterRevoked) = tapCollector.authorizedSigners( + _signer + ); assertEq(beforePayer, afterPayer); assertEq(beforeThawEndTimestamp, afterThawEndTimestamp); @@ -126,23 +128,34 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest __collect(_paymentType, _data, _tokensToCollect); } - function __collect(IGraphPayments.PaymentTypes _paymentType, bytes memory _data, uint256 _tokensToCollect) internal { - (ITAPCollector.SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (ITAPCollector.SignedRAV, uint256)); + function __collect( + IGraphPayments.PaymentTypes _paymentType, + bytes memory _data, + uint256 _tokensToCollect + ) internal { + (ITAPCollector.SignedRAV memory signedRAV, ) = abi.decode( + _data, + (ITAPCollector.SignedRAV, uint256) + ); bytes32 messageHash = tapCollector.encodeRAV(signedRAV.rav); address _signer = ECDSA.recover(messageHash, signedRAV.signature); (address _payer, , ) = tapCollector.authorizedSigners(_signer); - uint256 tokensAlreadyCollected = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); - uint256 tokensToCollect = _tokensToCollect == 0 ? signedRAV.rav.valueAggregate - tokensAlreadyCollected : _tokensToCollect; - uint256 tokensDataService = tokensToCollect.mulPPM(dataServiceCut); - + uint256 tokensAlreadyCollected = tapCollector.tokensCollected( + signedRAV.rav.dataService, + signedRAV.rav.serviceProvider, + _payer + ); + uint256 tokensToCollect = _tokensToCollect == 0 + ? signedRAV.rav.valueAggregate - tokensAlreadyCollected + : _tokensToCollect; + vm.expectEmit(address(tapCollector)); emit IPaymentsCollector.PaymentCollected( - _paymentType, + _paymentType, _payer, signedRAV.rav.serviceProvider, - tokensToCollect, signedRAV.rav.dataService, - tokensDataService + tokensToCollect ); vm.expectEmit(address(tapCollector)); emit ITAPCollector.RAVCollected( @@ -154,10 +167,19 @@ contract TAPCollectorTest is HorizonStakingSharedTest, PaymentsEscrowSharedTest signedRAV.rav.metadata, signedRAV.signature ); - uint256 tokensCollected = _tokensToCollect == 0 ? tapCollector.collect(_paymentType, _data) : tapCollector.collect(_paymentType, _data, _tokensToCollect); + uint256 tokensCollected = _tokensToCollect == 0 + ? tapCollector.collect(_paymentType, _data) + : tapCollector.collect(_paymentType, _data, _tokensToCollect); - uint256 tokensCollectedAfter = tapCollector.tokensCollected(signedRAV.rav.dataService, signedRAV.rav.serviceProvider, _payer); + uint256 tokensCollectedAfter = tapCollector.tokensCollected( + signedRAV.rav.dataService, + signedRAV.rav.serviceProvider, + _payer + ); assertEq(tokensCollected, tokensToCollect); - assertEq(tokensCollectedAfter, _tokensToCollect == 0 ? signedRAV.rav.valueAggregate : tokensAlreadyCollected + _tokensToCollect); + assertEq( + tokensCollectedAfter, + _tokensToCollect == 0 ? signedRAV.rav.valueAggregate : tokensAlreadyCollected + _tokensToCollect + ); } } diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol index a07463a25..b1403124b 100644 --- a/packages/subgraph-service/contracts/SubgraphService.sol +++ b/packages/subgraph-service/contracts/SubgraphService.sol @@ -556,13 +556,10 @@ contract SubgraphService is IGraphPayments.PaymentTypes.QueryFee, abi.encode(_signedRav, curationCut) ); - uint256 tokensCurators = tokensCollected.mulPPM(curationCut); uint256 balanceAfter = _graphToken().balanceOf(address(this)); - require( - balanceBefore + tokensCurators == balanceAfter, - SubgraphServiceInconsistentCollection(balanceBefore, balanceAfter, tokensCurators) - ); + require(balanceAfter >= balanceBefore, SubgraphServiceInconsistentCollection(balanceBefore, balanceAfter)); + uint256 tokensCurators = balanceAfter - balanceBefore; if (tokensCollected > 0) { // lock stake as economic security for fees diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol index 32ea9e8fb..44b9b3e6d 100644 --- a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol +++ b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol @@ -80,13 +80,11 @@ interface ISubgraphService is IDataServiceFees { error SubgraphServiceInvalidPaymentType(IGraphPayments.PaymentTypes paymentType); /** - * @notice Thrown when the contract GRT balance is inconsistent with the payment amount collected - * from Graph Payments + * @notice Thrown when the contract GRT balance is inconsistent after collecting from Graph Payments * @param balanceBefore The contract GRT balance before the collection * @param balanceAfter The contract GRT balance after the collection - * @param tokensDataService The amount of tokens sent to the subgraph service */ - error SubgraphServiceInconsistentCollection(uint256 balanceBefore, uint256 balanceAfter, uint256 tokensDataService); + error SubgraphServiceInconsistentCollection(uint256 balanceBefore, uint256 balanceAfter); /** * @notice @notice Thrown when the service provider in the RAV does not match the expected indexer. diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol index 64842b1f2..619ad4bab 100644 --- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol +++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol @@ -242,7 +242,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { // Calculate curation cut uint256 curationFeesCut = subgraphService.curationFeesCut(); queryFeeData.curationCut = curation.isCurated(allocation.subgraphDeploymentId) ? curationFeesCut : 0; - uint256 tokensCurators = paymentCollected.mulPPM(queryFeeData.curationCut); + uint256 tokensProtocol = paymentCollected.mulPPMRoundUp(queryFeeData.protocolPaymentCut); + uint256 tokensCurators = (paymentCollected - tokensProtocol).mulPPMRoundUp(queryFeeData.curationCut); vm.expectEmit(address(subgraphService)); emit ISubgraphService.QueryFeesCollected(_indexer, paymentCollected, tokensCurators); @@ -302,8 +303,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest { if (_paymentType == IGraphPayments.PaymentTypes.QueryFee) { // Check indexer got paid the correct amount { - uint256 tokensProtocol = paymentCollected.mulPPM(protocolPaymentCut); - uint256 curationTokens = paymentCollected.mulPPM(queryFeeData.curationCut); + uint256 tokensProtocol = paymentCollected.mulPPMRoundUp(protocolPaymentCut); + uint256 curationTokens = (paymentCollected - tokensProtocol).mulPPMRoundUp(queryFeeData.curationCut); uint256 expectedIndexerTokensPayment = paymentCollected - tokensProtocol - curationTokens; assertEq( collectPaymentDataAfter.indexerBalance, From dc79bd401974a605789f38a5d812b731a9006143 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Fri, 13 Dec 2024 15:45:07 -0300 Subject: [PATCH 9/9] fix: add payer to EIP712 rav type hash (TRST-M15) --- packages/horizon/contracts/payments/collectors/TAPCollector.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index 8d15ff5c8..c8b42b87f 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/packages/horizon/contracts/payments/collectors/TAPCollector.sol @@ -249,6 +249,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector { keccak256( abi.encode( EIP712_RAV_TYPEHASH, + _rav.payer, _rav.dataService, _rav.serviceProvider, _rav.timestampNs,