fix(fee-primitives): overflow and rounding invariant audit for i128 fee arithmetic - #243
Open
Hexor-Hash wants to merge 1 commit into
Open
Conversation
Full audit of every arithmetic expression in fee-primitives/src/lib.rs per Afro-Pay#193. - Add overflow-safe checked_mul at both multiplication sites (calculate_fee, convert_currency). On overflow, calculate_fee clamps to config.max_fee (the true fee is necessarily far above it); convert_currency panics with a descriptive message since it has no safe ceiling to fall back to. - Round fee calculation up (ceiling division) so the protocol never under-collects due to truncation; document why convert_currency intentionally keeps floor division (it's a value transform, not a charge). - Validate amount/min_fee/max_fee/base_fee_rate are non-negative, min_fee <= max_fee, and base_fee_rate <= 10_000 bps (100%). - Guarantee the computed fee never exceeds the principal amount, even if min_fee is configured above it. - Add 16 tests: edge cases (zero amount, i128::MAX, invalid config) and 5 property-based tests covering the acceptance-criteria invariants (zero fee at zero amount, no overflow at i128::MAX, fee+principal well-formed, rounding always >= 0 and favors the protocol, fee never exceeds principal). - Fix pre-existing compile errors that meant this crate had never actually built or run its tests: missing `contractimpl` import, private storage.get/set calls (now via storage().persistent()), and contract-exported functions taking references (Soroban contract functions must take owned Val types). - Add fee-primitives to the Cargo workspace members so `cargo test -p fee-primitives` (the acceptance criterion's exact command) works. - Document the rounding convention and invariants in docs/fee-primitives.md. Closes Afro-Pay#193 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@Hexor-Hash is attempting to deploy a commit to the milah's projects Team on Vercel. A member of the Team first needs to authorize it. |
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
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.
Summary
Closes #193. Full audit of every arithmetic expression in
contracts/fee-primitives/src/lib.rs, covering overflow safety and rounding direction, plus tests proving the invariants the issue calls for.Findings & fixes
amount * base_fee_rateincalculate_fee,amount * rate.rateinconvert_currency) used raw*, which is undefined/incorrect on overflow for a plain (non-checked) build. Both now usechecked_mul:calculate_fee: on overflow, the true fee is necessarily far aboveconfig.max_fee(which already bounds protocol revenue per call), so the result clamps straight tomax_feeinstead of computing an unrepresentable product.convert_currency: there's no safe ceiling to fall back to for a currency conversion, so overflow now panics with a descriptive message instead of silently wrapping.calculate_feenow uses ceiling division — any non-zero remainder rounds the fee up — so the protocol never under-collects due to truncation (documented in code and indocs/fee-primitives.md).convert_currencyintentionally keeps floor division since it's a value transform, not a charge; this is documented too.amount,min_fee,max_fee,base_fee_ratemust be non-negative,min_fee <= max_fee, andbase_fee_rate <= 10_000bps (100%) — all panic with descriptive messages otherwise.amount, even ifmin_feeis configured above it.Pre-existing bugs found while getting
cargo test -p fee-primitivesto actually runThe crate did not compile before this change (so its tests had never run):
#[contractimpl]was used without importing it fromsoroban_sdk.env.storage().get/set(...)called the SDK's now-private raw storage methods; switched toenv.storage().persistent().get/set(...).&FeeConfig,&ConversionRate), which Soroban's#[contractimpl]macro doesn't support for exported functions — changed to owned parameters.fee-primitiveswasn't a member of thecontracts/Cargo workspace, socargo test -p fee-primitives(the acceptance criterion's exact command) couldn't resolve the package; added it toworkspace.members.test_currency_conversiontest asserted a result that was inconsistent with the documented basis-points formula (and had never actually run to catch it).Tests
16 tests total in
fee-primitives/src/lib.rs, including 5 property-based tests (deterministic PRNG-driven, since this is a#![no_std]contract crate andproptest/quickcheckrequirestd) covering the acceptance criteria exactly:i128::MAXdoes not overflow (clamps tomax_fee)Plus edge-case unit tests for invalid config (rate > 100%,
min_fee > max_fee), conversion overflow, and the conversion+fee pipeline.cargo fmt -p fee-primitivesandcargo clippy -p fee-primitives --all-targets -- -D warningsare both clean. Verified the rest of thecontracts/workspace (escrow,governor,payment_registry) still builds after addingfee-primitivesas a workspace member.Docs
docs/fee-primitives.mdnow documents the rounding convention, overflow handling, and invariants.