fix: disallow signers to be authorized for different payers (TRST-M10)

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

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

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.
  */
@@ -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

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);

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,14 +209,46 @@ 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 {
         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));
+        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);