Skip to content

fix(insurance): checked arithmetic on premium/refund paths (#22) - #59

Merged
merlik787-droi merged 2 commits into
Riddlrealm:mainfrom
XxHugheadxX:fix/22-insurance-checked-math
Jul 29, 2026
Merged

fix(insurance): checked arithmetic on premium/refund paths (#22)#59
merlik787-droi merged 2 commits into
Riddlrealm:mainfrom
XxHugheadxX:fix/22-insurance-checked-math

Conversation

@XxHugheadxX

Copy link
Copy Markdown
Contributor

Closes #22

What

Hardens all arithmetic on the premium/refund paths of the insurance contract
against overflow, underflow, and the pre-existing premium-truncation bug.

  • New InsuranceError (#[contracterror]) with PremiumOverflow = 1 and
    RefundUnderflow = 2.
  • calculate_premium_internal: rewritten to multiply-first, divide-once,
    every step using checked_mul / checked_div, surfacing PremiumOverflow
    via panic_with_error!.
  • Checked accumulation of PremiumPool on purchase_policy / renew_policy
    and of premium_paid on renew_policy (checked_add).
  • Checked refund proration + pool deduction on cancel_policy
    (checked_mul + checked_div + checked_sub); the checked_div also guards
    division-by-zero when total_period == 0.
  • New test module (none existed before).

Why panic_with_error! instead of Result signatures

The public ABI uses panic! throughout. Converting entry points to Result
would be a breaking ABI change, out of scope for this arithmetic-hardening issue.
panic_with_error! surfaces the required variants, keeps the ABI intact (zero
breaking changes), and is assertable in tests via try_* clients. Deliberate
design decision, not an omission.

Behavior change (intentional)

The old code did annual_rate = (base_rate * multiplier) / 100 before the
main multiply, truncating the rate whenever base_rate * multiplier was not a
clean multiple of 100 — systematically underpricing premiums (collapsing to the
minimum of 1 in the worst case) regardless of coverage. Multiply-first fixes it:
premiums now scale correctly. Affected premiums will be higher (correct) than
before. Covered by low_rate_premium_no_longer_floors_to_one.

Bonus: div-by-zero guard

checked_div(total_period) in the refund path also cleanly handles
total_period == 0, which previously would have trapped opaquely — now it
surfaces RefundUnderflow.

Display-side arithmetic

No display/aggregation arithmetic exists today; read getters return stored or
freshly computed values without accumulating. If aggregated views are added
later, they should use saturating ops per this issue.

Clippy cleanup (included)

Three manual underflow guards (if x > y { x - y } else { 0 }) in
cancel_policy and the fraud helpers were rewritten as saturating_sub
behavior-identical, on-theme with the arithmetic hardening, and required for
cargo clippy -- -D warnings to pass.

Tests

  • premium_overflow_surfaces_clean_error_near_i128_max — sweep near
    i128::MAX / 1000 → clean PremiumOverflow, not an opaque host trap.
  • low_rate_premium_no_longer_floors_to_one — truncation regression.
  • cancel_refund_prorates_without_trapping — purchase → advance ledger to
    mid-period → cancel; refund ≈ premium/2 and the pool drops by exactly that.

Verification (local)

  • cargo clippy -p insurance -- -D warnings — clean
  • cargo test -p insurance — 3/3 passing
  • cargo build --target wasm32-unknown-unknown --release -p insurance — ok
  • cargo build --workspace — full workspace compiles
  • cargo fmt — clean

⚠️ Heads-up for maintainers: unrelated dependency-resolution break

While running the test suite locally I hit a pre-existing, repo-wide issue
unrelated to this change: since Cargo.lock is git-ignored, a fresh resolve
pulls in ed25519-dalek 3.0.0, whose changed CryptoRng/TryRng bounds fail to
compile soroban-env-host's testutils — which blocks any cargo test in a
clean environment (likely including CI). I pinned it back to 2.2.0 locally to
run the tests.

This affects the whole workspace, not just insurance, so I've kept it out of
this PR. Suggested fix (happy to add here or open a separate PR — your call):
add ed25519-dalek = "=2.2.0" to [workspace.dependencies] in the root
Cargo.toml so fresh builds/CI resolve deterministically.

@XxHugheadxX

Copy link
Copy Markdown
Contributor Author

Note: CI failures here are unrelated to this change

The CI / build and clippy failures on this PR are a pre-existing,
repo-wide
issue, not caused by this change. Every open PR is currently red
with the same error, and fmt (which doesn't compile) passes.

Root cause: ed25519-dalek 3.0.0 was recently published, and its changed
CryptoRng/rand_core bounds fail to compile soroban-env-host's testutils:

error[E0277]: the trait bound ChaCha20Rng: ed25519_dalek::rand_core::CryptoRng
is not satisfied
error: could not compile soroban-env-host (lib) due to 1 previous error

Because Cargo.lock is git-ignored, every fresh CI resolve pulls the broken
3.0.0 (Adding ed25519-dalek v2.2.0 (available: v3.0.0) in the logs).

This change is verified locally with ed25519-dalek pinned to 2.2.0:

  • cargo clippy -p insurance -- -D warnings — clean
  • cargo test -p insurance — 3/3 passing
  • cargo build --target wasm32-unknown-unknown --release -p insurance — ok
  • cargo build --workspace — compiles

Suggested infra fix (repo-wide, separate from this PR): either commit
Cargo.lock, or pin ed25519-dalek = "=2.2.0". Happy to open a separate PR for
that if useful.

@XxHugheadxX

Copy link
Copy Markdown
Contributor Author

@mainteiners

@merlik787-droi
merlik787-droi merged commit 154f62d into Riddlrealm:main Jul 29, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Checked arithmetic throughout insurance premium math

2 participants