fix: surface oracle reverts from ERC20PriceOracleReceiptVault._nextId#302
Conversation
Drops the bare `try/catch` around `priceOracle.price()`. The catch returned 0, which collapsed every oracle error into a downstream `Panic(0x12)` from divide-by-zero in `_calculateMint`/`_calculateDeposit`, erasing the actual selector (e.g. Pyth `StalePrice()`) from integrators and on-chain monitors. The ERC4626 "MUST NOT revert" requirement applies to the view-only `max*` functions; `mint`/`deposit` may revert, and `previewMint`/`previewDeposit` may revert "due to other conditions that would also cause deposit/mint to revert" — a reverting oracle qualifies. Closes #299 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Asserts the MUST-NOT-revert clause that PR #302's description references: `max*` view functions don't route through `_nextId` so a reverting oracle can't propagate into them. `id` is bounded > 0 to isolate the oracle hypothesis — `maxWithdraw(owner, 0)` separately panics inside `_calculateRedeem`, which is a pre-existing ERC4626 violation tracked in #303 and out of scope here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`max*` view functions never call `_nextId`, so an oracle revert can't reach them whether or not the try/catch exists. The bounded-id assertion passed equivalently before and after this PR — it pinned nothing this change affects. The MUST-NOT-revert clause is tracked in #303 (full ERC4626 spec compliance), which `maxWithdraw(owner, 0)` already violates independently of oracle state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
try/catcharoundpriceOracle.price()in_nextId. The catch returned 0, collapsing every oracle error into a downstreamPanic(0x12)from divide-by-zero in_calculateMint/_calculateDeposit, erasing the actual selector (e.g. PythStalePrice()).max*) is unaffected — those functions don't route through_nextId.mint/depositmay revert, andpreviewMint/previewDepositmay revert "due to other conditions that would also cause deposit/mint to revert" per spec; a reverting oracle qualifies.max*surface has its own MUST-NOT-revert gap (maxWithdraw(owner, 0)panics) tracked in Add ERC4626 spec compliance test suite #303.Closes #299
Test plan
mint,deposit,previewMint,previewDeposit.Panic(0x12)/ZeroSharesAmount(); pass after the fix.🤖 Generated with Claude Code