Skip to content

fix(profile,events): replace saturating_add with checked_add + typed errors - #101

Merged
0xdevcollins merged 3 commits into
boundlessfi:testnetfrom
Shadow-MMN:fix/issue-72-arithmetic-hardening
Jul 27, 2026
Merged

fix(profile,events): replace saturating_add with checked_add + typed errors#101
0xdevcollins merged 3 commits into
boundlessfi:testnetfrom
Shadow-MMN:fix/issue-72-arithmetic-hardening

Conversation

@Shadow-MMN

@Shadow-MMN Shadow-MMN commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Closes #72

Problem

Two unbounded accumulators use saturating_add, which silently clamps on overflow rather than reverting:

  1. boundless-profile earnings (earnings.rs:26): per-user per-token earnings silently pin at i128::MAX on overflow, causing irrecoverable accounting drift. Later increments become no-ops.

  2. boundless-events event ID counter (idempotency.rs:30-31): the counter freezes at u64::MAX, returning the same ID forever — causing ID collisions and event-state aliasing.

Fix

Use checked arithmetic that reverts with a typed error:

  • current.checked_add(amount).ok_or(Error::EarningsOverflow)?
  • id.checked_add(1).ok_or(Error::EventIdOverflow)?

This aligns with the repo's hard rule: no unwrap on host-returned
Option/Result — return a typed Error instead.

Changes

File Change
contracts/profile/src/errors.rs Added EarningsOverflow = 21
contracts/profile/src/earnings.rs saturating_addchecked_add + error
contracts/profile/src/tests/earnings.rs Updated test to expect EarningsOverflow instead of clamped value
contracts/events/src/errors.rs Added EventIdOverflow = 71
contracts/events/src/idempotency.rs next_event_id returns Result<u64, Error>, uses checked_add
contracts/events/src/event_ops.rs Propagated ? from new next_event_id return type
contracts/events/src/tests/op_id_security.rs Added event_id_overflow_reverts test

Testing

  • cargo test --release: 287/287 passed (221 events + 66 profile)
  • cargo fmt -- --check: clean
  • WASM builds: events 55,676 bytes / profile 15,907 bytes (both under 64 KB ceiling)

Summary by CodeRabbit

  • Bug Fixes
    • Event creation now reverts when the next event ID would overflow, preventing invalid/duplicate IDs.
    • Earnings updates now revert on arithmetic overflow instead of clamping to the maximum value.
  • Tests
    • Added a security regression test ensuring event ID overflow reverts and rolls back balances and stored IDs.
    • Updated earnings tests to verify overflow reverts and preserves the previously stored earnings value.

@almanax-ai

almanax-ai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Quota reached

Your plan allows 300 CI/CD file units per month. You've used 297 and this scan would add 7 more (total: 304).

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d165fe23-77ca-4828-9f33-ffc04a829a1b

📥 Commits

Reviewing files that changed from the base of the PR and between 608515d and 5f13fe1.

📒 Files selected for processing (3)
  • contracts/events/src/errors.rs
  • contracts/events/src/tests/op_id_security.rs
  • contracts/profile/src/tests/earnings.rs

Disabled knowledge base sources:

  • Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.


📝 Walkthrough

Walkthrough

Event ID and profile earnings arithmetic now use checked addition with typed overflow errors. Event creation propagates event ID failures, and tests verify both overflow cases revert without invalid state updates.

Changes

Event ID overflow handling

Layer / File(s) Summary
Checked event ID generation
contracts/events/src/errors.rs, contracts/events/src/idempotency.rs, contracts/events/src/event_ops.rs, contracts/events/src/tests/op_id_security.rs
Event ID increments use checked arithmetic, return Error::EventIdOverflow, propagate through event creation, and are covered by rollback assertions at the maximum ID.

Earnings overflow handling

Layer / File(s) Summary
Checked earnings accumulation
contracts/profile/src/errors.rs, contracts/profile/src/earnings.rs, contracts/profile/src/tests/earnings.rs
Earnings accumulation returns Error::EarningsOverflow on overflow, with tests confirming the stored value remains unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Poem

I’m a rabbit who checks every sum,
No wrapped-up numbers shall come.
IDs hop with care,
Earnings stay fair,
Overflow? A typed error will come!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly summarizes the main change: replacing saturating_add with checked arithmetic and typed errors in profile/events.
Linked Issues check ✅ Passed Changes match issue #72 by switching both counters to checked_add, adding typed overflow errors, and updating tests.
Out of Scope Changes check ✅ Passed No unrelated code changes are evident; edits stay focused on overflow handling and tests for profile and events.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
contracts/events/src/tests/op_id_security.rs (1)

213-247: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Also assert rollback of pre-ID state changes.

create_event deposits escrow before next_event_id can return EventIdOverflow. This test checks only the error code; also assert that the stored counter remains u64::MAX, no event is persisted, and owner/fee-account balances are unchanged after the failed call.

🤖 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 `@contracts/events/src/tests/op_id_security.rs` around lines 213 - 247, Extend
event_id_overflow_reverts to verify transaction rollback after the failed
create_event call: capture the owner and fee-account balances and event
count/state before invocation, then assert they are unchanged afterward, the
stored next_event_id remains u64::MAX, and no event was persisted. Reuse the
existing storage and balance/event inspection helpers visible in the test
context.
contracts/profile/src/tests/earnings.rs (1)

193-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove narration-only comments from the test.

The test name and assertions already convey that the second registration must revert and that state remains unchanged. Keep comments only for non-obvious rationale.

As per coding guidelines, comments should explain information the code cannot convey; these comments restate self-evident behavior.

🤖 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 `@contracts/profile/src/tests/earnings.rs` around lines 193 - 202, Remove the
narration-only comments surrounding the second registration assertion in the
earnings test, including the comments describing overflow/revert and unchanged
state. Keep the test name, setup, and assertions unchanged.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@contracts/events/src/tests/op_id_security.rs`:
- Around line 213-247: Extend event_id_overflow_reverts to verify transaction
rollback after the failed create_event call: capture the owner and fee-account
balances and event count/state before invocation, then assert they are unchanged
afterward, the stored next_event_id remains u64::MAX, and no event was
persisted. Reuse the existing storage and balance/event inspection helpers
visible in the test context.

In `@contracts/profile/src/tests/earnings.rs`:
- Around line 193-202: Remove the narration-only comments surrounding the second
registration assertion in the earnings test, including the comments describing
overflow/revert and unchanged state. Keep the test name, setup, and assertions
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ba20f7b-366b-4a9c-90d8-fc3ea3ca081d

📥 Commits

Reviewing files that changed from the base of the PR and between 8e15770 and 608515d.

📒 Files selected for processing (7)
  • contracts/events/src/errors.rs
  • contracts/events/src/event_ops.rs
  • contracts/events/src/idempotency.rs
  • contracts/events/src/tests/op_id_security.rs
  • contracts/profile/src/earnings.rs
  • contracts/profile/src/errors.rs
  • contracts/profile/src/tests/earnings.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 23, 2026
…-cap comments

- rustfmt (rust 1.93) wants the assert_eq! args in event_id_overflow_reverts
  split one-per-line; apply it (CI rustfmt was failing on this).
- The errors enum is at 48/50 after boundlessfi#99 removed the three deadline variants,
  so the three 'enum is at the 50-case cap / consolidate before adding'
  comments were stale. Reword them: state the real reuse rationale, and the
  true current count.
@0xdevcollins
0xdevcollins merged commit 9bdb45b into boundlessfi:testnet Jul 27, 2026
3 of 4 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.

Arithmetic hardening: saturating_add on earnings and next_event_id should be checked

2 participants