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

Signed-off-by: Tomás Migone <[email protected]>

Author: tmigone

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 [email protected] 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

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