fix(app): reject future-dated Chainlink rounds in feedUsd - #29
Conversation
feedUsd accepted updatedAt in the future (nowS - updatedAt negative never exceeds maxAgeS), so a skewed clock or bad RPC round served as a price. feedEthUsd delegates to the same helper, so ETH/USD had the same hole. Reject r.updatedAt > nowS and cover it in baseStocks + ethPrice tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Limit details: You’ve used the included review currently available. 📝 WalkthroughWalkthroughThe change rejects future-dated oracle readings in ChangesOracle timestamp validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Future-dated oracle readings are rejected while delayed fallback rounds remain valid when read. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Usage-based review receipt
Note This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing. Comment |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The future-date guard is useful, but its ETH fallback caller currently supplies a clock captured before the network requests. That makes this reject valid rounds published while the request is running. Please update that integration before merging.
I ran the 21 stock/ETH tests successfully, then reproduced the regression with an advancing-clock test: the existing implementation returns 2441.34, while this head returns null for the same valid round.
| export function feedUsd(r: { answer: bigint; updatedAt: number } | null, nowS: number, feedDecimals = 8, maxAgeS = BASE_STOCK_MAX_FEED_AGE_S): number | null { | ||
| if (!r || r.answer <= 0n) return null; | ||
| if (!(r.updatedAt > 0) || nowS - r.updatedAt > maxAgeS) return null; | ||
| if (!(r.updatedAt > 0) || r.updatedAt > nowS || nowS - r.updatedAt > maxAgeS) return null; |
There was a problem hiding this comment.
[P2] Compare the feed timestamp with the clock after the read completes. ethUsd() captures t before awaiting Coinbase, then passes it through refresh() and fromChainlink(). If Coinbase fails after 5 seconds and Chainlink returns a round published 3 seconds after the request started, this guard rejects it even though it is already 2 seconds old. I reproduced a cold fallback returning null instead of 2441.34; the previous implementation accepts it. Pass the current/injected clock through the fallback and sample it after await feedFn(), then add an integration test with elapsed time during the failed Coinbase request.
…are not rejected as future (PR Gitlawb#29) - fromChainlink takes injected now() and samples after await feedFn() - refresh stamps cache from completion time - regression test: slow Coinbase failure + round published mid-request returns 2441.34
|
Thanks @Vasanthdev2004 for the review! Addressed the fallback-clock review: |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for fixing the clock handling. The refresh now samples time after the feed request, so a valid round published while the request is in flight is accepted, while genuinely future-dated rounds are still rejected.
Rechecked the current head: all 22 stock/ETH tests passed, along with focused checks for both timing cases. These were isolated tests, not live oracle requests. Current app and contract CI are green.
My earlier requested change is addressed. Approving this version; the price-validation fix makes sense as its own PR.
2 participants
feedUsd accepted updatedAt in the future: nowS - updatedAt goes negative and never exceeds maxAgeS, so a skewed clock or bad RPC round was served as a live price. feedEthUsd delegates to the same helper, so ETH/USD had the same hole.
Before (baseStocks.ts): if (!(r.updatedAt > 0) || nowS - r.updatedAt > maxAgeS) — future passes.
After: if (!(r.updatedAt > 0) || r.updatedAt > nowS || nowS - r.updatedAt > maxAgeS) — future is null, stocks and ETH both covered. Adds regression cases in baseStocks.test.ts and ethPrice.test.ts.
Verified locally on upstream/main base:
Summary by CodeRabbit
Bug Fixes
Tests