GraphPayments changes post audit

.gitignore

@@ -34,8 +34,10 @@ bin/
 .vscode
 
 # Coverage and other reports
+coverage/
 reports/
 coverage.json
+lcov.info
 
 # Local test files
 addresses-local.json

packages/horizon/contracts/interfaces/IGraphPayments.sol

@@ -20,6 +20,7 @@ interface IGraphPayments {
 
     /**
      * @notice Emitted when a payment is collected
+     * @param paymentType The type of payment as defined in {IGraphPayments}
      * @param payer The address of the payer
      * @param receiver The address of the receiver
      * @param dataService The address of the data service
@@ -30,8 +31,9 @@ interface IGraphPayments {
      * @param tokensReceiver Amount of tokens for the receiver
      */
     event GraphPaymentCollected(
+        PaymentTypes indexed paymentType,
         address indexed payer,
-        address indexed receiver,
+        address receiver,
         address indexed dataService,
         uint256 tokens,
         uint256 tokensProtocol,
@@ -40,14 +42,6 @@ interface IGraphPayments {
         uint256 tokensReceiver
     );
 
-    /**
-     * @notice Thrown when the calculated amount of tokens to be paid out to all parties is
-     * not the same as the amount of tokens being collected
-     * @param tokens The amount of tokens being collected
-     * @param tokensCalculated The sum of all the tokens to be paid out
-     */
-    error GraphPaymentsBadAccounting(uint256 tokens, uint256 tokensCalculated);
-
     /**
      * @notice Thrown when the protocol payment cut is invalid
      * @param protocolPaymentCut The protocol payment cut
@@ -63,9 +57,10 @@ interface IGraphPayments {
     /**
      * @notice Collects funds from a payer.
      * It will pay cuts to all relevant parties and forward the rest to the receiver.
+     * Note that the collected amount can be zero.
      * @param paymentType The type of payment as defined in {IGraphPayments}
      * @param receiver The address of the receiver
-     * @param tokens The amount of tokens being collected
+     * @param tokens The amount of tokens being collected.
      * @param dataService The address of the data service
      * @param dataServiceCut The data service cut in PPM
      */

packages/horizon/contracts/interfaces/IPaymentsEscrow.sol

@@ -37,9 +37,18 @@ interface IPaymentsEscrow {
     /**
      * @notice Emitted when a payer cancels an escrow thawing
      * @param payer The address of the payer
+     * @param collector The address of the collector
      * @param receiver The address of the receiver
+     * @param tokensThawing The amount of tokens that were being thawed
+     * @param thawEndTimestamp The timestamp at which the thawing period was ending
      */
-    event CancelThaw(address indexed payer, address indexed receiver);
+    event CancelThaw(
+        address indexed payer,
+        address indexed collector,
+        address indexed receiver,
+        uint256 tokensThawing,
+        uint256 thawEndTimestamp
+    );
 
     /**
      * @notice Emitted when a payer thaws funds from the escrow for a payer-collector-receiver tuple
@@ -68,12 +77,19 @@ interface IPaymentsEscrow {
 
     /**
      * @notice Emitted when a collector collects funds from the escrow for a payer-collector-receiver tuple
+     * @param paymentType The type of payment being collected as defined in the {IGraphPayments} interface
      * @param payer The address of the payer
      * @param collector The address of the collector
      * @param receiver The address of the receiver
      * @param tokens The amount of tokens collected
      */
-    event EscrowCollected(address indexed payer, address indexed collector, address indexed receiver, uint256 tokens);
+    event EscrowCollected(
+        IGraphPayments.PaymentTypes indexed paymentType,
+        address indexed payer,
+        address indexed collector,
+        address receiver,
+        uint256 tokens
+    );
 
     // -- Errors --
 
@@ -145,20 +161,29 @@ interface IPaymentsEscrow {
     /**
      * @notice Thaw a specific amount of escrow from a payer-collector-receiver's escrow account.
      * The payer is the transaction caller.
-     * If `tokens` is zero and funds were already thawing it will cancel the thawing.
      * Note that repeated calls to this function will overwrite the previous thawing amount
      * and reset the thawing period.
      * @dev Requirements:
      * - `tokens` must be less than or equal to the available balance
      *
-     * Emits a {Thaw} event. If `tokens` is zero it will emit a {CancelThaw} event.
+     * Emits a {Thaw} event.
      *
      * @param collector The address of the collector
      * @param receiver The address of the receiver
      * @param tokens The amount of tokens to thaw
      */
     function thaw(address collector, address receiver, uint256 tokens) external;
 
+    /**
+     * @notice Cancels the thawing of escrow from a payer-collector-receiver's escrow account.
+     * @param collector The address of the collector
+     * @param receiver The address of the receiver
+     * @dev Requirements:
+     * - The payer must be thawing funds
+     * Emits a {CancelThaw} event.
+     */
+    function cancelThaw(address collector, address receiver) external;
+
     /**
      * @notice Withdraws all thawed escrow from a payer-collector-receiver's escrow account.
      * The payer is the transaction caller.

packages/horizon/contracts/interfaces/ITAPCollector.sol

@@ -29,10 +29,10 @@ interface ITAPCollector is IPaymentsCollector {
         bytes32 collectionId;
         // 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
         address serviceProvider;
+        // The address of the data service the RAV was issued to
+        address dataService;
         // The RAV timestamp, indicating the latest TAP Receipt in the RAV
         uint64 timestampNs;
         // Total amount owed to the service provider since the beginning of the

packages/horizon/contracts/payments/GraphPayments.sol

@@ -3,6 +3,7 @@ pragma solidity 0.8.27;
 
 import { IGraphToken } from "@graphprotocol/contracts/contracts/token/IGraphToken.sol";
 import { IGraphPayments } from "../interfaces/IGraphPayments.sol";
+import { IHorizonStakingTypes } from "../interfaces/internal/IHorizonStakingTypes.sol";
 
 import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import { MulticallUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/MulticallUpgradeable.sol";
@@ -22,6 +23,8 @@ import { GraphDirectory } from "../utilities/GraphDirectory.sol";
 contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, IGraphPayments {
     using TokenUtils for IGraphToken;
     using PPMMath for uint256;
+
+    /// @notice Protocol payment cut in PPM
     uint256 public immutable PROTOCOL_PAYMENT_CUT;
 
     /**
@@ -70,14 +73,14 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I
         uint256 tokensDataService = tokensRemaining.mulPPMRoundUp(dataServiceCut);
         tokensRemaining = tokensRemaining - tokensDataService;
 
-        uint256 tokensDelegationPool = tokensRemaining.mulPPMRoundUp(
-            _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType)
-        );
-        tokensRemaining = tokensRemaining - tokensDelegationPool;
-
-        // Ensure accounting is correct
-        uint256 tokensTotal = tokensProtocol + tokensDataService + tokensDelegationPool + tokensRemaining;
-        require(tokens == tokensTotal, GraphPaymentsBadAccounting(tokens, tokensTotal));
+        uint256 tokensDelegationPool = 0;
+        IHorizonStakingTypes.DelegationPool memory pool = _graphStaking().getDelegationPool(receiver, dataService);
+        if (pool.shares > 0) {
+            tokensDelegationPool = tokensRemaining.mulPPMRoundUp(
+                _graphStaking().getDelegationFeeCut(receiver, dataService, paymentType)
+            );
+            tokensRemaining = tokensRemaining - tokensDelegationPool;
+        }
 
         // Pay all parties
         _graphToken().burnTokens(tokensProtocol);
@@ -92,6 +95,7 @@ contract GraphPayments is Initializable, MulticallUpgradeable, GraphDirectory, I
         _graphToken().pushTokens(receiver, tokensRemaining);
 
         emit GraphPaymentCollected(
+            paymentType,
             msg.sender,
             receiver,
             dataService,

packages/horizon/contracts/payments/PaymentsEscrow.sol

@@ -23,17 +23,21 @@ import { GraphDirectory } from "../utilities/GraphDirectory.sol";
 contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory, IPaymentsEscrow {
     using TokenUtils for IGraphToken;
 
-    /// @notice Escrow account details for payer-collector-receiver tuples
-    mapping(address payer => mapping(address collector => mapping(address receiver => IPaymentsEscrow.EscrowAccount escrowAccount)))
-        public escrowAccounts;
-
     /// @notice The maximum thawing period (in seconds) for both escrow withdrawal and collector revocation
     /// @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 escrow funds withdrawal
     uint256 public immutable WITHDRAW_ESCROW_THAWING_PERIOD;
 
+    /// @notice Escrow account details for payer-collector-receiver tuples
+    mapping(address payer => mapping(address collector => mapping(address receiver => IPaymentsEscrow.EscrowAccount escrowAccount)))
+        public escrowAccounts;
+
+    /**
+     * @notice Modifier to prevent function execution when contract is paused
+     * @dev Reverts if the controller indicates the contract is paused
+     */
     modifier notPaused() {
         require(!_graphController().paused(), PaymentsEscrowIsPaused());
         _;
@@ -78,19 +82,9 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
      * @notice See {IPaymentsEscrow-thaw}
      */
     function thaw(address collector, address receiver, uint256 tokens) external override notPaused {
-        EscrowAccount storage account = escrowAccounts[msg.sender][collector][receiver];
-
-        // if amount thawing is zero and requested amount is zero this is an invalid request.
-        // otherwise if amount thawing is greater than zero and requested amount is zero this
-        // is a cancel thaw request.
-        if (tokens == 0) {
-            require(account.tokensThawing != 0, PaymentsEscrowNotThawing());
-            account.tokensThawing = 0;
-            account.thawEndTimestamp = 0;
-            emit CancelThaw(msg.sender, receiver);
-            return;
-        }
+        require(tokens > 0, PaymentsEscrowInvalidZeroTokens());
 
+        EscrowAccount storage account = escrowAccounts[msg.sender][collector][receiver];
         require(account.balance >= tokens, PaymentsEscrowInsufficientBalance(account.balance, tokens));
 
         account.tokensThawing = tokens;
@@ -99,6 +93,21 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
         emit Thaw(msg.sender, collector, receiver, tokens, account.thawEndTimestamp);
     }
 
+    /**
+     * @notice See {IPaymentsEscrow-cancelThaw}
+     */
+    function cancelThaw(address collector, address receiver) external override notPaused {
+        EscrowAccount storage account = escrowAccounts[msg.sender][collector][receiver];
+        require(account.tokensThawing != 0, PaymentsEscrowNotThawing());
+
+        uint256 tokensThawing = account.tokensThawing;
+        uint256 thawEndTimestamp = account.thawEndTimestamp;
+        account.tokensThawing = 0;
+        account.thawEndTimestamp = 0;
+
+        emit CancelThaw(msg.sender, collector, receiver, tokensThawing, thawEndTimestamp);
+    }
+
     /**
      * @notice See {IPaymentsEscrow-withdraw}
      */
@@ -138,29 +147,27 @@ contract PaymentsEscrow is Initializable, MulticallUpgradeable, GraphDirectory,
         // Reduce amount from account balance
         account.balance -= tokens;
 
-        uint256 balanceBefore = _graphToken().balanceOf(address(this));
+        uint256 escrowBalanceBefore = _graphToken().balanceOf(address(this));
 
         _graphToken().approve(address(_graphPayments()), tokens);
         _graphPayments().collect(paymentType, receiver, tokens, dataService, dataServiceCut);
 
-        uint256 balanceAfter = _graphToken().balanceOf(address(this));
+        // Verify that the escrow balance is consistent with the collected tokens
+        uint256 escrowBalanceAfter = _graphToken().balanceOf(address(this));
         require(
-            balanceBefore == tokens + balanceAfter,
-            PaymentsEscrowInconsistentCollection(balanceBefore, balanceAfter, tokens)
+            escrowBalanceBefore == tokens + escrowBalanceAfter,
+            PaymentsEscrowInconsistentCollection(escrowBalanceBefore, escrowBalanceAfter, tokens)
         );
 
-        emit EscrowCollected(payer, msg.sender, receiver, tokens);
+        emit EscrowCollected(paymentType, payer, msg.sender, receiver, tokens);
     }
 
     /**
      * @notice See {IPaymentsEscrow-getBalance}
      */
     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;
+        return account.balance > account.tokensThawing ? account.balance - account.tokensThawing : 0;
     }
 
     /**

packages/horizon/contracts/payments/collectors/TAPCollector.sol

@@ -29,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 payer,address dataService,address serviceProvider,uint64 timestampNs,uint128 valueAggregate,bytes metadata)"
+            "ReceiptAggregateVoucher(address payer,address serviceProvider,address dataService,uint64 timestampNs,uint128 valueAggregate,bytes metadata)"
         );
 
     /// @notice Authorization details for payer-signer pairs
@@ -166,8 +166,9 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector {
         bytes memory _data,
         uint256 _tokensToCollect
     ) private returns (uint256) {
-        // Ensure caller is the RAV data service
         (SignedRAV memory signedRAV, uint256 dataServiceCut) = abi.decode(_data, (SignedRAV, uint256));
+
+        // Ensure caller is the RAV data service
         require(
             signedRAV.rav.dataService == msg.sender,
             TAPCollectorCallerNotDataService(msg.sender, signedRAV.rav.dataService)
@@ -259,8 +260,8 @@ contract TAPCollector is EIP712, GraphDirectory, ITAPCollector {
                     abi.encode(
                         EIP712_RAV_TYPEHASH,
                         _rav.payer,
-                        _rav.dataService,
                         _rav.serviceProvider,
+                        _rav.dataService,
                         _rav.timestampNs,
                         _rav.valueAggregate,
                         keccak256(_rav.metadata)