TRST audit fixes for Graph Payments contracts

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
      *
@@ -300,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

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
@@ -18,10 +19,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 +124,19 @@ 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 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
@@ -137,6 +155,19 @@ 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
+     */
+    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
@@ -153,7 +184,15 @@ interface ITAPCollector is IPaymentsCollector {
     error TAPCollectorInconsistentRAVTokens(uint256 tokens, uint256 tokensCollected);
 
     /**
-     * @notice Authorize a signer to sign on behalf of the payer
+     * 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.
      * @dev Requirements:
      * - `signer` must not be already authorized
      * - `proofDeadline` must be greater than the current timestamp
@@ -213,4 +252,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);
 }

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));
@@ -228,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;
     }