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

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

packages/horizon/contracts/interfaces/ITAPCollector.sol

@@ -18,6 +18,8 @@ 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
@@ -119,6 +121,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
@@ -159,7 +167,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

@@ -79,6 +79,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 +111,7 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector {
             TAPCollectorSignerStillThawing(block.timestamp, authorization.thawEndTimestamp)
         );
 
-        delete authorizedSigners[signer];
+        authorization.revoked = true;
         emit SignerRevoked(msg.sender, signer);
     }
 
@@ -129,7 +130,10 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector {
         );
 
         address signer = _recoverRAVSigner(signedRAV);
-        require(authorizedSigners[signer].payer != address(0), TAPCollectorInvalidRAVSigner());
+        require(
+            authorizedSigners[signer].payer != address(0) && !authorizedSigners[signer].revoked,
+            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

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

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