diff --git a/packages/horizon/contracts/interfaces/ITAPCollector.sol b/packages/horizon/contracts/interfaces/ITAPCollector.sol index d04f142d6..4d353f39c 100644 --- a/packages/horizon/contracts/interfaces/ITAPCollector.sol +++ b/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 diff --git a/packages/horizon/contracts/payments/collectors/TAPCollector.sol b/packages/horizon/contracts/payments/collectors/TAPCollector.sol index 40a375349..e5c491732 100644 --- a/packages/horizon/contracts/payments/collectors/TAPCollector.sol +++ b/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 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/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); + } }