Audit 4 minor fixes (#65)

* fix issue 9: add comment above paymentInfo.payer = payer

* fix issue 7: comment about failure to reset preapproval

* fix issue 5: simplify imports and remappings

* use standard ReentrancyGuard

* add check for preApprovalExpiry in preApprove

* use transient reentrancy guard on all chains

* add comment to preapproval expiry check

* remove triple slash

Author: amiecorso

remappings.txt

@@ -1,5 +1,4 @@
 @openzeppelin/contracts/=lib/spend-permissions/lib/openzeppelin-contracts/contracts/
-@openzeppelin/contracts/security/=lib/spend-permissions/lib/magicspend/lib/openzeppelin-contracts/contracts/utils/
 FreshCryptoLib/=lib/spend-permissions/lib/webauthn-sol/lib/FreshCryptoLib/solidity/src/
 account-abstraction/=lib/spend-permissions/lib/account-abstraction/contracts/
 ds-test/=lib/spend-permissions/lib/webauthn-sol/lib/forge-std/lib/ds-test/src/

src/PaymentEscrow.sol

@@ -498,4 +498,9 @@ contract PaymentEscrow is ReentrancyGuardTransient {
             revert InvalidFeeReceiver(feeReceiver, configuredFeeReceiver);
         }
     }
+
+    /// @dev Override to use transient reentrancy guard on all chains
+    function _useTransientReentrancyGuardOnlyOnMainnet() internal view virtual override returns (bool) {
+        return false;
+    }
 }

src/TokenStore.sol

@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity ^0.8.28;
 
-import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /// @title TokenStore
 /// @notice Holds funds for a single operator's payments

src/collectors/PreApprovalPaymentCollector.sol

@@ -40,6 +40,11 @@ contract PreApprovalPaymentCollector is TokenCollector {
         // Check sender is buyer
         if (msg.sender != paymentInfo.payer) revert PaymentEscrow.InvalidSender(msg.sender, paymentInfo.payer);
 
+        // Check pre-approval expiry has not passed
+        if (block.timestamp >= paymentInfo.preApprovalExpiry) {
+            revert PaymentEscrow.AfterPreApprovalExpiry(uint48(block.timestamp), paymentInfo.preApprovalExpiry);
+        }
+
         // Check has not already pre-approved
         bytes32 paymentInfoHash = paymentEscrow.getHash(paymentInfo);
         if (isPreApproved[paymentInfoHash]) revert PaymentAlreadyPreApproved(paymentInfoHash);
@@ -63,8 +68,8 @@ contract PreApprovalPaymentCollector is TokenCollector {
     ) internal override {
         // Check payment pre-approved
         bytes32 paymentInfoHash = paymentEscrow.getHash(paymentInfo);
+        // Skip resetting pre-approval to save gas as the `PaymentEscrow` enforces unique, single-lifecycle payments
         if (!isPreApproved[paymentInfoHash]) revert PaymentNotPreApproved(paymentInfoHash);
-
         // Transfer tokens from payer directly to token store
         SafeERC20.safeTransferFrom(IERC20(paymentInfo.token), paymentInfo.payer, tokenStore, amount);
     }

src/collectors/TokenCollector.sol

@@ -57,13 +57,13 @@ abstract contract TokenCollector {
     ) internal virtual;
 
     /// @notice Get hash for PaymentInfo with null payer address
-    /// @dev Proactively setting payer back to original value covers accidental bugs of memory location being used elsewhere
     /// @param paymentInfo PaymentInfo struct with non-null payer address
     /// @return hash Hash of PaymentInfo with payer replaced with zero address
     function _getHashPayerAgnostic(PaymentEscrow.PaymentInfo memory paymentInfo) internal view returns (bytes32) {
         address payer = paymentInfo.payer;
         paymentInfo.payer = address(0);
         bytes32 hashPayerAgnostic = paymentEscrow.getHash(paymentInfo);
+        // Proactively setting payer back to original value covers accidental bugs if memory location is then used elsewhere
         paymentInfo.payer = payer;
         return hashPayerAgnostic;
     }

test/src/collectors/PreApprovalPaymentCollector.t.sol

@@ -58,6 +58,22 @@ contract PreApprovalPaymentCollectorTest is PaymentEscrowBase {
         PreApprovalPaymentCollector(address(preApprovalPaymentCollector)).preApprove(paymentInfo);
     }
 
+    function test_preApprove_reverts_ifAfterPreApprovalExpiry(uint120 amount) public {
+        vm.assume(amount > 0);
+
+        // Create payment info with a pre-approval expiry in the past
+        PaymentEscrow.PaymentInfo memory paymentInfo = _createPaymentInfo({payer: payerEOA, maxAmount: amount});
+        paymentInfo.preApprovalExpiry = uint48(block.timestamp - 1);
+
+        vm.prank(payerEOA);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                PaymentEscrow.AfterPreApprovalExpiry.selector, uint48(block.timestamp), paymentInfo.preApprovalExpiry
+            )
+        );
+        PreApprovalPaymentCollector(address(preApprovalPaymentCollector)).preApprove(paymentInfo);
+    }
+
     function test_preApprove_emitsExpectedEvents(uint120 amount) public {
         vm.assume(amount > 0);