Skip to content

fix: enable overflow-checks and use checked counter increments - #833

Open
Martins-594 wants to merge 18 commits into
zintarh:mainfrom
Martins-594:fix/reopen-651
Open

fix: enable overflow-checks and use checked counter increments#833
Martins-594 wants to merge 18 commits into
zintarh:mainfrom
Martins-594:fix/reopen-651

Conversation

@Martins-594

Copy link
Copy Markdown
Contributor

Overview

This PR fixes the silent overflow gap between the debug and release WASM builds. [profile.release] currently leaves overflow-checks unset, so Cargo defaults it to false and the deployed contract can wrap u32 counters and proposal timestamps instead of trapping. The PR enables overflow-checks = true, replaces every listed unchecked + 1 with checked_add(...).unwrap_or_else(|| panic_with_error!(e, ContractError::ArithmeticOverflow)), bounds the admin proposal duration, and adds regression tests at the overflow boundary.

Related Issue

Closes the bounty issue: [Security] Counters use unchecked + 1 in a release profile with overflow checks disabled.

Changes

🔒 Overflow-Safe Arithmetic

  • [MODIFY] src/mint.rs

    • Replaces current_count + 1 and current_total + 1 in both mint and batch paths with checked_add(1).unwrap_or_else(|| panic_with_error!(e, ContractError::ArithmeticOverflow)).
  • [MODIFY] src/bridge.rs

    • Replaces current_nonce + 1 and all matching count/total increments with the same checked-arithmetic pattern.
  • [MODIFY] src/governance.rs

    • Replaces count + 1, votes_for += 1, votes_against += 1, and start_time + duration_seconds with checked arithmetic.
    • Enforces a documented [MIN_PROPOSAL_DURATION_SECONDS, MAX_PROPOSAL_DURATION_SECONDS] range in create_admin_proposal, rejecting caller-supplied durations outside that range.

⚙️ Release Profile & Error Handling

  • [MODIFY] Cargo.toml
    • Adds overflow-checks = true to [profile.release], matching the overflow behavior tested in debug builds.
  • [MODIFY] src/errors.rs
    • Reuses the existing ContractError::ArithmeticOverflow variant for every guarded increment/duration path.

🧪 Tests & WASM Size Limit

  • [ADD] src/security_test.rs
    • Adds boundary tests for the outbound nonce and proposal duration that assert ArithmeticOverflow.
  • [MODIFY] .github/wasm-size-limit
    • Updates the WASM size limit to account for the measured overflow-checks = true delta.

Verification Results

cargo test --test security_test
✅ 12/12 passed

Release WASM verification:
✅ overflow-checks = true in [profile.release]
✅ outbound nonce boundary returns ArithmeticOverflow
✅ proposal duration boundary returns ArithmeticOverflow
✅ wasm size delta: +2.1 KB — recorded in .github/wasm-size-limit
Acceptance Criteria Status
overflow-checks = true in release profile, with WASM size delta recorded ✅ Added to Cargo.toml; delta +2.1 KB, .github/wasm-size-limit updated
Every counter increment listed above uses checked arithmetic ✅ All + 1 increments in mint.rs, bridge.rs, and governance.rs use checked_add + ArithmeticOverflow
create_admin_proposal rejects durations outside a documented [MIN, MAX] range [MIN_PROPOSAL_DURATION_SECONDS, MAX_PROPOSAL_DURATION_SECONDS] enforced
Tests assert ArithmeticOverflow at the boundary for proposal duration and outbound nonce ✅ Boundary tests pass

Closes #651

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.

[Security] Counters use unchecked + 1 in a release profile with overflow checks disabled

1 participant