fix: maxWithdraw returns 0 instead of reverting on zero share ratio#304
Conversation
ERC-4626 specifies that maxWithdraw MUST NOT revert (https://eips.ethereum.org/EIPS/eip-4626#maxwithdraw). `ERC20PriceOracleReceiptVault._shareRatioUserAgnostic` returns `id` as the share ratio, so `maxWithdraw(owner, 0)` produces a zero share ratio and panics with `Panic(0x12)` from divide-by-zero inside `_calculateRedeem`. A zero share ratio at this surface means no deposit at this id has ever existed, so the max withdrawable is 0. Short-circuit before the math. Part of #303 (full ERC-4626 spec compliance is broader and remains open). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`testMaxWithdrawZeroIdDoesNotRevert` pins the id=0 boundary; this fuzz covers every id including ones the vault never minted a receipt for. 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 (3)
✨ 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 |
`testMintSimpleRealReceiptVault` asserts `ERC20InsufficientBalance` when alice mints without paying first. The upper-half-address `vm.assume` filter is insufficient — fuzzer-generated addresses in that range can have code (e.g. precompiles spilled into the address space), in which case the ERC1155 receipt mint trips `ERC1155InvalidReceiver` before the ERC20 balance check, masking the asserted revert. `maxWithdraw` bytecode change shifted the fuzz seed and surfaced an existing flake; the fix is to filter receivers to EOAs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
ERC20PriceOracleReceiptVault._shareRatioUserAgnosticreturnsid, somaxWithdraw(owner, 0)produces a zero share ratio and panics withPanic(0x12)inside_calculateRedeem. A zero share ratio means no deposit at this id has ever existed; the max withdrawable is 0. Short-circuit before the math.testMaxWithdrawZeroIdDoesNotRevert) and a broad fuzz (testMaxWithdrawAnyIdDoesNotRevert).Part of #303 (full ERC-4626 spec compliance is broader and remains open).
Test plan
Panic(0x12); pass after the fix.🤖 Generated with Claude Code