fix(pulse): validate freshness using the latest timestamp in the update data (#2635)

* fix: validate min ts using highest ts in the update data

* doc: comment

* merge

* test: fix tests after merge

Author: tejasbadadare

apps/argus/Cargo.lock

@@ -271,18 +271,20 @@ abstract contract Scheduler is IScheduler, SchedulerState {
             revert InsufficientBalance();
         }
 
-        // Parse the price feed updates with an acceptable timestamp range of [-1h, +10s] from now.
-        // We will validate the trigger conditions ourselves.
+        // Parse the price feed updates with an acceptable timestamp range of [0, now+10s].
+        // Note: We don't want to reject update data if it contains a price
+        // from a market that closed a few days ago, since it will contain a timestamp
+        // from the last trading period. Thus, we use a minimum timestamp of zero while parsing,
+        // and we enforce the past max validity ourselves in _validateShouldUpdatePrices using
+        // the highest timestamp in the update data.
         uint64 curTime = SafeCast.toUint64(block.timestamp);
         (
             PythStructs.PriceFeed[] memory priceFeeds,
             uint64[] memory slots
         ) = pyth.parsePriceFeedUpdatesWithSlots{value: pythFee}(
                 updateData,
                 params.priceIds,
-                curTime > PAST_TIMESTAMP_MAX_VALIDITY_PERIOD
-                    ? curTime - PAST_TIMESTAMP_MAX_VALIDITY_PERIOD
-                    : 0,
+                0, // We enforce the past max validity ourselves in _validateShouldUpdatePrices
                 curTime + FUTURE_TIMESTAMP_MAX_VALIDITY_PERIOD
             );
         status.balanceInWei -= pythFee;
@@ -299,15 +301,14 @@ abstract contract Scheduler is IScheduler, SchedulerState {
 
         // Verify that update conditions are met, and that the timestamp
         // is more recent than latest stored update's. Reverts if not.
-        _validateShouldUpdatePrices(subscriptionId, params, status, priceFeeds);
+        uint256 latestPublishTime = _validateShouldUpdatePrices(
+            subscriptionId,
+            params,
+            status,
+            priceFeeds
+        );
 
         // Update status and store the updates
-        uint256 latestPublishTime = 0; // Use the most recent publish time from the validated feeds
-        for (uint8 i = 0; i < priceFeeds.length; i++) {
-            if (priceFeeds[i].price.publishTime > latestPublishTime) {
-                latestPublishTime = priceFeeds[i].price.publishTime;
-            }
-        }
         status.priceLastUpdatedAt = latestPublishTime;
         status.totalUpdates += priceFeeds.length;
 
@@ -324,13 +325,14 @@ abstract contract Scheduler is IScheduler, SchedulerState {
      * @param params The subscription's parameters struct.
      * @param status The subscription's status struct.
      * @param priceFeeds The array of price feeds to validate.
+     * @return The timestamp of the update if the trigger criteria is met, reverts if not met.
      */
     function _validateShouldUpdatePrices(
         uint256 subscriptionId,
         SubscriptionParams storage params,
         SubscriptionStatus storage status,
         PythStructs.PriceFeed[] memory priceFeeds
-    ) internal view returns (bool) {
+    ) internal view returns (uint256) {
         // Use the most recent timestamp, as some asset markets may be closed.
         // Closed markets will have a publishTime from their last trading period.
         // Since we verify all updates share the same Pythnet slot, we still ensure
@@ -342,6 +344,18 @@ abstract contract Scheduler is IScheduler, SchedulerState {
             }
         }
 
+        // Calculate the minimum acceptable timestamp (clamped at 0)
+        // The maximum acceptable timestamp is enforced by the parsePriceFeedUpdatesWithSlots call
+        uint256 minAllowedTimestamp = (block.timestamp >
+            PAST_TIMESTAMP_MAX_VALIDITY_PERIOD)
+            ? (block.timestamp - PAST_TIMESTAMP_MAX_VALIDITY_PERIOD)
+            : 0;
+
+        // Validate that the update timestamp is not too old
+        if (updateTimestamp < minAllowedTimestamp) {
+            revert TimestampTooOld(updateTimestamp, block.timestamp);
+        }
+
         // Reject updates if they're older than the latest stored ones
         if (
             status.priceLastUpdatedAt > 0 &&
@@ -362,7 +376,7 @@ abstract contract Scheduler is IScheduler, SchedulerState {
                 updateTimestamp >=
                 lastUpdateTime + params.updateCriteria.heartbeatSeconds
             ) {
-                return true;
+                return updateTimestamp;
             }
         }
 
@@ -375,7 +389,7 @@ abstract contract Scheduler is IScheduler, SchedulerState {
 
                 // If there's no previous price, this is the first update
                 if (previousFeed.id == bytes32(0)) {
-                    return true;
+                    return updateTimestamp;
                 }
 
                 // Calculate the deviation percentage
@@ -402,7 +416,7 @@ abstract contract Scheduler is IScheduler, SchedulerState {
                 if (
                     deviationBps >= params.updateCriteria.deviationThresholdBps
                 ) {
-                    return true;
+                    return updateTimestamp;
                 }
             }
         }

target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol

@@ -21,6 +21,10 @@ error PriceSlotMismatch();
 // Update criteria errors
 error InvalidUpdateCriteria();
 error UpdateConditionsNotMet();
+error TimestampTooOld(
+    uint256 providedUpdateTimestamp,
+    uint256 currentTimestamp
+);
 error TimestampOlderThanLastUpdate(
     uint256 providedUpdateTimestamp,
     uint256 lastUpdatedAt

target_chains/ethereum/contracts/contracts/pulse/SchedulerErrors.sol

@@ -10,10 +10,16 @@ contract SchedulerState {
     /// Maximum number of addresses in the reader whitelist
     uint8 public constant MAX_READER_WHITELIST_SIZE = 255;
 
-    // TODO: make these updateable via governance
     /// Maximum time in the past (relative to current block timestamp)
     /// for which a price update timestamp is considered valid
+    /// when validating the update conditions.
+    /// @dev Note: We don't use this when parsing update data from the Pyth contract
+    /// because don't want to reject update data if it contains a price from a market
+    /// that closed a few days ago, since it will contain a timestamp from the last
+    /// trading period. We enforce this value ourselves against the maximum
+    /// timestamp in the provided update data.
     uint64 public constant PAST_TIMESTAMP_MAX_VALIDITY_PERIOD = 1 hours;
+
     /// Maximum time in the future (relative to current block timestamp)
     /// for which a price update timestamp is considered valid
     uint64 public constant FUTURE_TIMESTAMP_MAX_VALIDITY_PERIOD = 10 seconds;

target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol

@@ -1825,6 +1825,106 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils {
         scheduler.updateSubscription(initialSubId, invalidDeviationParams);
     }
 
+    function testUpdatePriceFeedsSucceedsWithStaleFeedIfLatestIsValid() public {
+        // Add a subscription and funds
+        uint256 subscriptionId = addTestSubscription(
+            scheduler,
+            address(reader)
+        );
+
+        // Advance time past the validity period
+        vm.warp(
+            block.timestamp +
+                scheduler.PAST_TIMESTAMP_MAX_VALIDITY_PERIOD() +
+                600
+        ); // Warp 1 hour 10 mins
+
+        uint64 currentTime = SafeCast.toUint64(block.timestamp);
+        uint64 validPublishTime = currentTime - 1800; // 30 mins ago (within 1 hour validity)
+        uint64 stalePublishTime = currentTime -
+            (scheduler.PAST_TIMESTAMP_MAX_VALIDITY_PERIOD() + 300); // 1 hour 5 mins ago (outside validity)
+
+        PythStructs.PriceFeed[] memory priceFeeds = new PythStructs.PriceFeed[](
+            2
+        );
+        priceFeeds[0] = createSingleMockPriceFeed(stalePublishTime);
+        priceFeeds[1] = createSingleMockPriceFeed(validPublishTime);
+
+        uint64[] memory slots = new uint64[](2);
+        slots[0] = 100;
+        slots[1] = 100; // Same slot
+
+        // Mock Pyth response (should succeed in the real world as minValidTime is 0)
+        mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots);
+        bytes[] memory updateData = createMockUpdateData(priceFeeds);
+
+        // Expect PricesUpdated event with the latest valid timestamp
+        vm.expectEmit();
+        emit PricesUpdated(subscriptionId, validPublishTime);
+
+        // Perform update - should succeed because the latest timestamp in the update data is valid
+        vm.prank(pusher);
+        scheduler.updatePriceFeeds(subscriptionId, updateData);
+
+        // Verify last updated timestamp
+        (, SchedulerState.SubscriptionStatus memory status) = scheduler
+            .getSubscription(subscriptionId);
+        assertEq(
+            status.priceLastUpdatedAt,
+            validPublishTime,
+            "Last updated timestamp should be the latest valid one"
+        );
+    }
+
+    function testUpdatePriceFeedsRevertsIfLatestTimestampIsTooOld() public {
+        // Add a subscription and funds
+        uint256 subscriptionId = addTestSubscription(
+            scheduler,
+            address(reader)
+        );
+
+        // Advance time past the validity period
+        vm.warp(
+            block.timestamp +
+                scheduler.PAST_TIMESTAMP_MAX_VALIDITY_PERIOD() +
+                600
+        ); // Warp 1 hour 10 mins
+
+        uint64 currentTime = SafeCast.toUint64(block.timestamp);
+        // Make the *latest* timestamp too old
+        uint64 stalePublishTime1 = currentTime -
+            (scheduler.PAST_TIMESTAMP_MAX_VALIDITY_PERIOD() + 300); // 1 hour 5 mins ago
+        uint64 stalePublishTime2 = currentTime -
+            (scheduler.PAST_TIMESTAMP_MAX_VALIDITY_PERIOD() + 600); // 1 hour 10 mins ago
+
+        PythStructs.PriceFeed[] memory priceFeeds = new PythStructs.PriceFeed[](
+            2
+        );
+        priceFeeds[0] = createSingleMockPriceFeed(stalePublishTime2); // Oldest
+        priceFeeds[1] = createSingleMockPriceFeed(stalePublishTime1); // Latest, but still too old
+
+        uint64[] memory slots = new uint64[](2);
+        slots[0] = 100;
+        slots[1] = 100; // Same slot
+
+        // Mock Pyth response (should succeed in the real world as minValidTime is 0)
+        mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots);
+        bytes[] memory updateData = createMockUpdateData(priceFeeds);
+
+        // Expect revert with TimestampTooOld (checked in _validateShouldUpdatePrices)
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                TimestampTooOld.selector,
+                stalePublishTime1, // The latest timestamp from the update
+                currentTime
+            )
+        );
+
+        // Attempt to update price feeds
+        vm.prank(pusher);
+        scheduler.updatePriceFeeds(subscriptionId, updateData);
+    }
+
     // Required to receive ETH when withdrawing funds
     receive() external payable {}
 }