fix: validate all Chainlink feed timestamps against expiry in ChainlinkRelayerV1#715
fix: validate all Chainlink feed timestamps against expiry in ChainlinkRelayerV1#715
Conversation
…nkRelayerV1 Previously, only the newest aggregator timestamp was checked against the expiry window. A stale older feed could pass through undetected, allowing stale price data to influence the aggregated rate. Fix: validate the oldest aggregator timestamp. If the oldest is within expiry, all feeds are within expiry. If the oldest is expired, at least one feed carries stale data and the relay should fail. Adds a regression test (test_revertsWhenOldestFeedIsExpiredButNewestIsNot) demonstrating the vulnerability: oldest feed 700s old (expired), newest 400s old (within expiry=600s), spread 300s (within maxTimestampSpread=300s). Fixes: MEN-36 (mento-core S-2 from QA audit MEN-5)
|
CI fix pushed — |
…set all aggregator timestamps In relay_triple and relay_full, the inherited test_revertsWhenOldestFeedIsExpiredButNewestIsNot() only set timestamps for aggregators 0 and 1, leaving aggregators 2 (and 3) at their setUp default (block.timestamp=1). After vm.warp(+1000) this caused spread = newTs - 1 = 600 > maxTimestampSpread=300, triggering TimestampSpreadTooHigh instead of the expected ExpiredTimestamp. Fix: override the test in relay_triple and relay_full to explicitly set all aggregator timestamps so the spread stays within the 300s limit and the expired-feed check fires correctly. Co-Authored-By: Paperclip <noreply@paperclip.ing>
mento-val
left a comment
There was a problem hiding this comment.
QA Review — LGTM / Approved (reviewer cannot self-approve)
Fix logic: Correct.
The one-line change from newestChainlinkTs → oldestChainlinkTs in the expiry check closes the security gap cleanly.
Why the logic holds:
maxTimestampSpreadalready constrains all feed timestamps within a window.- If
oldestChainlinkTsis within the expiry window, every feed is guaranteed fresh (all others are newer). - If
oldestChainlinkTsis expired, at least one feed carries stale data → relay correctly reverts. - The
TimestampNotNewcheck (still usingnewestChainlinkTs) is unaffected and correct.
Test coverage: Appropriate.
test_revertsWhenOldestFeedIsExpiredButNewestIsNotinrelay_doubleexercises the exact vulnerability (Feed 0: 700s, Feed 1: 400s, spread: 300s ≤ maxSpread, oldest expired → must revert).- Overrides in
relay_tripleandrelay_fullcorrectly set ALL aggregator timestamps so inherited setUp timestamps don't trigger a spuriousTimestampSpreadTooHigh. These are not masking real failures — they're correctly isolating the expiry assertion from the spread assertion. vm.warp(block.timestamp + 1000)before subtracting timestamps prevents underflow — correct defensive test practice.
CI: All 4 checks passing (Lint & Test, Slither, Echidna).
Remapping addition: foundry-chainlink-toolkit entry is benign test tooling infrastructure.
No issues found. LGTM — ready to merge.
Summary
Fixes a security bug (mento-core S-2 from QA audit MEN-5) where only the newest Chainlink aggregator timestamp was checked against the staleness expiry window.
Problem
In
ChainlinkRelayerV1.relay(), when multiple aggregators are used:With two aggregators where feed 0 is stale but feed 1 is fresh, the relay succeeds with stale data influencing the aggregated rate. Example:
Current code: relay succeeds, stale price from Feed 0 contaminates the result.
Fix
If the oldest timestamp is within the expiry window, all feeds are fresh. If the oldest is expired, at least one feed carries stale data and the relay reverts.
Tests
Adds
test_revertsWhenOldestFeedIsExpiredButNewestIsNottoChainlinkRelayerV1Test_relay_doubledemonstrating the exact vulnerability scenario.This PR was initially closed due to CI failures in
relay_tripleandrelay_fulltest suites. The root cause was that the test function was defined forrelay_double(2 aggregators) and inherited byrelay_triple(3) andrelay_full(4). The inherited setUp timestamps caused a timestamp spread >maxTimestampSpread, firingTimestampSpreadTooHighbeforeExpiredTimestamp.CI fixes (commits
4634545,8d814f0):block.timestampto prevent underflow in the staleness testvirtualand overrode it inrelay_tripleandrelay_fullto set all aggregator timestamps, keeping spread ≤300 and ensuringExpiredTimestampfires correctly🤖 Generated with Claude Code