fix(audit/H03): revert with ExponentUnderflow instead of silent FLOAT_ZERO#202
Merged
Conversation
Every arithmetic op in LibDecimalFloat used to finalise with packLossy and discard the lossless flag. packLossy's two lossless=false modes have very different consequences: - Coefficient too big for int224: successive div-by-10, exponent goes up. Result is approximately the same value with N digits dropped. Order of magnitude survives. Composable. - Exponent below int32.min: result becomes FLOAT_ZERO. Magnitude is lost entirely. `mul(small, small) == 0` propagates through all downstream code and there's no recovery. Adds `packArithmeticResult` — distinguishes the two by the returned Float (the coefficient-truncation path never returns FLOAT_ZERO from a non-zero input). Reverts with `ExponentUnderflow` on the underflow mode, silently tolerates coefficient truncation. Every arithmetic op in LibDecimalFloat now uses `packArithmeticResult` in place of `packLossy`. `packLossy` itself is unchanged so the parser still uses it to surface `ParseDecimalPrecisionLoss` rather than revert. Regression tests for the four ops whose result exponent can fall below int32.min — mul, div, inv, pow10 — pin the revert. Add/sub can't underflow (result exponent = min of input exponents, both fit int32). Minus/floor/ceil/intFrac don't change exponents. log10's result exponent is bounded by the input's magnitude. Updates the existing fuzz comparison tests (add, mul, div, inv, pow10) to call packArithmeticResult on the decomposed path so the public and decomposed paths revert together on underflow inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 (5)
WalkthroughThis PR addresses audit finding H03 by preventing silent precision loss during arithmetic exponent underflow. A new ChangesExponent Underflow Protection
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/src/lib/LibDecimalFloat.inv.t.sol`:
- Around line 22-28: Add an inline comment decoding the adversarial Float
literal used in testInvRevertsOnExponentUnderflow: explain that
Float.wrap(0x7fff...ffff) encodes coefficient = -1 (int224 all-ones) and
exponent = int32.max, so the value is -1 * 2^int32.max; place this comment next
to the Float.wrap call (or above the test) to clarify the bit layout for readers
and maintainers referencing the testInvRevertsOnExponentUnderflow and
invExternal usage.
In `@test/src/lib/LibDecimalFloat.pow10.t.sol`:
- Around line 22-26: Add a one-line comment next to the fuzz-derived literal in
testPow10RevertsOnExponentUnderflow that decodes the Float.wrap bit pattern into
its high-32-bit exponent and signed int224 coefficient (e.g., "exponent = 0x...,
coefficient = ...") so the regression intent is immediately readable; update the
comment near the Float.wrap literal in function
testPow10RevertsOnExponentUnderflow to show those decoded values and a short
note that this pattern triggers ExponentUnderflow when passed to pow10External.
- Around line 40-42: Collapse the multi-line vm.expectRevert call into a single
line to match the style used for the ExponentOverflow case; update the call that
uses vm.expectRevert(abi.encodeWithSelector(ExponentUnderflow.selector,
signedCoefficient, exponent)) so it is all on one line (preserving
ExponentUnderflow.selector, signedCoefficient and exponent) and run forge fmt to
ensure formatting is committed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93c57635-4221-4548-9739-8c43fdc55aee
📒 Files selected for processing (9)
CLAUDE.mdcrates/float/abi/DecimalFloat.jsonsrc/error/ErrDecimalFloat.solsrc/lib/LibDecimalFloat.soltest/src/lib/LibDecimalFloat.add.t.soltest/src/lib/LibDecimalFloat.div.t.soltest/src/lib/LibDecimalFloat.inv.t.soltest/src/lib/LibDecimalFloat.mul.t.soltest/src/lib/LibDecimalFloat.pow10.t.sol
Comment on lines
+22
to
+28
| /// `inv` of a Float whose representable inverse magnitude falls below | ||
| /// `int32.min` reverts instead of silently producing `FLOAT_ZERO`. | ||
| function testInvRevertsOnExponentUnderflow() external { | ||
| Float float = Float.wrap(0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff); | ||
| vm.expectPartialRevert(ExponentUnderflow.selector); | ||
| this.invExternal(float); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: decode the adversarial Float in a comment.
This literal corresponds to coefficient -1 (int224 all-ones) at exponent int32.max; making that explicit avoids forcing readers to hand-parse the bit layout.
📝 Suggested annotation
function testInvRevertsOnExponentUnderflow() external {
+ // coefficient = -1 (int224, all ones), exponent = int32.max.
+ // 1 / (−1 × 10^int32.max) requires scaling that drops exponent below int32.min.
Float float = Float.wrap(0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff);
vm.expectPartialRevert(ExponentUnderflow.selector);
this.invExternal(float);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// `inv` of a Float whose representable inverse magnitude falls below | |
| /// `int32.min` reverts instead of silently producing `FLOAT_ZERO`. | |
| function testInvRevertsOnExponentUnderflow() external { | |
| Float float = Float.wrap(0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff); | |
| vm.expectPartialRevert(ExponentUnderflow.selector); | |
| this.invExternal(float); | |
| } | |
| /// `inv` of a Float whose representable inverse magnitude falls below | |
| /// `int32.min` reverts instead of silently producing `FLOAT_ZERO`. | |
| function testInvRevertsOnExponentUnderflow() external { | |
| // coefficient = -1 (int224, all ones), exponent = int32.max. | |
| // 1 / (−1 × 10^int32.max) requires scaling that drops exponent below int32.min. | |
| Float float = Float.wrap(0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff); | |
| vm.expectPartialRevert(ExponentUnderflow.selector); | |
| this.invExternal(float); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/src/lib/LibDecimalFloat.inv.t.sol` around lines 22 - 28, Add an inline
comment decoding the adversarial Float literal used in
testInvRevertsOnExponentUnderflow: explain that Float.wrap(0x7fff...ffff)
encodes coefficient = -1 (int224 all-ones) and exponent = int32.max, so the
value is -1 * 2^int32.max; place this comment next to the Float.wrap call (or
above the test) to clarify the bit layout for readers and maintainers
referencing the testInvRevertsOnExponentUnderflow and invExternal usage.
Comment on lines
+22
to
+26
| function testPow10RevertsOnExponentUnderflow() external { | ||
| Float float = Float.wrap(0xffffffffffffffffffffff0000000000000000000000000000000000000000ff); | ||
| vm.expectPartialRevert(ExponentUnderflow.selector); | ||
| this.pow10External(float); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Annotate the fuzz-derived adversarial Float.
The bit pattern is opaque on its face. A one-liner decoding the exponent (high 32 bits) and the signed int224 coefficient would make the regression intent legible without forcing future readers to hand-parse the literal.
📝 Suggested annotation
function testPow10RevertsOnExponentUnderflow() external {
+ // Fuzz-derived: exponent = 0xffffffff (-1 as int32); coefficient bits chosen so
+ // pow10's effective result exponent falls below int32.min before final packing.
Float float = Float.wrap(0xffffffffffffffffffffff0000000000000000000000000000000000000000ff);
vm.expectPartialRevert(ExponentUnderflow.selector);
this.pow10External(float);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/src/lib/LibDecimalFloat.pow10.t.sol` around lines 22 - 26, Add a
one-line comment next to the fuzz-derived literal in
testPow10RevertsOnExponentUnderflow that decodes the Float.wrap bit pattern into
its high-32-bit exponent and signed int224 coefficient (e.g., "exponent = 0x...,
coefficient = ...") so the regression intent is immediately readable; update the
comment near the Float.wrap literal in function
testPow10RevertsOnExponentUnderflow to show those decoded values and a short
note that this pattern triggers ExponentUnderflow when passed to pow10External.
Pins the new helper's contract: - testPackArithmeticResultLossless: roundtrip for int224/int32 inputs. - testPackArithmeticResultToleratesCoefficientTruncation: large coefficients silently divide-by-10 to fit int224, exponent bumped, no revert. - testPackArithmeticResultExponentUnderflowReverts: exp < int32.min reverts with ExponentUnderflow. - testPackArithmeticResultExponentOverflowReverts: exp > int32.max reverts with ExponentOverflow (unchanged from packLossy). - testPackArithmeticResultZeroCoefficient: coef=0 returns FLOAT_ZERO regardless of exponent. Updates the rust crate's mul-underflow test to expect the revert via FloatError::DecimalFloat(ExponentUnderflow) instead of asserting the result is zero. Adds ExponentUnderflow to DecimalFloatErrorSelector and the TryFrom<FixedBytes<4>> dispatch. Also forge-fmt'd LibDecimalFloat.pow10.t.sol (single-line vm.expectRevert). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reflects the new bytecode after the #191 fix landed: - ZOLTU_DEPLOYED_DECIMAL_FLOAT_ADDRESS: c08C... -> 588F... - DECIMAL_FLOAT_CONTRACT_HASH: 0x694f... -> 0xa44a... Deploy already broadcast across the supported networks via manual-sol-artifacts on this branch. Constants pinned from the testDeployAddress/testExpectedCodeHash assertion failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #191.
Summary
Every arithmetic op in
LibDecimalFloatused to finalise withpackLossyand discard thelosslessflag.packLossy's twolossless=falsemodes have very different consequences:FLOAT_ZERO. Magnitude is lost entirely.mul(small, small) == 0propagates through all downstream code with no recovery.Adds
packArithmeticResultthat distinguishes the two by the returned Float (the coefficient-truncation path never returns FLOAT_ZERO from a non-zero input). Reverts withExponentUnderflowon the underflow mode, silently tolerates coefficient truncation.Every arithmetic op in
LibDecimalFloatnow usespackArithmeticResultin place ofpackLossy.packLossyitself is unchanged so the parser still uses it to surfaceParseDecimalPrecisionLossrather than revert.Test coverage
Regression tests for the four ops whose result exponent can fall below
int32.min:testMulRevertsOnExponentUnderflow— both inputs atint32.minexponent.testDivRevertsOnExponentUnderflow— numerator at min, denominator at max.testInvRevertsOnExponentUnderflow— input at max exponent.testPow10RevertsOnExponentUnderflow— adversarial input found by fuzz.Mutation-tested: removing the revert in
packArithmeticResultmakes the mul test fail. Re-adding it passes.The existing fuzz comparison tests (add, mul, div, inv, pow10) are updated to call
packArithmeticResulton the decomposed path so the public and decomposed paths revert together on underflow inputs.Deploy constants
This source change invalidates
DECIMAL_FLOAT_CONTRACT_HASHetc. inLibDecimalFloatDeploy. Triggered theManual sol artifactsworkflow on this branch; CI'stestDeployAddress/testExpectedCodeHashDecimalFloatwill pass once that completes and the new constants are committed.🤖 Generated with Claude Code
Summary by CodeRabbit