Skip to content

fix(events): use saturating arithmetic for anchor_idx calculation in select_winners (closes #134) - #145

Open
rushikeshgarad2024-dev wants to merge 2 commits into
boundlessfi:testnetfrom
rushikeshgarad2024-dev:fix/select-winners-saturating-add-134
Open

rushikeshgarad2024-dev wants to merge 2 commits into
boundlessfi:testnetfrom
rushikeshgarad2024-dev:fix/select-winners-saturating-add-134

Conversation

@rushikeshgarad2024-dev

Copy link
Copy Markdown

Summary

Source: docs/threat-model.md v1.0, Tamp.7 (planned hardening).

Replaced bare addition existing_count + (idx as u32) at contracts/events/src/event_ops.rs with existing_count.saturating_add(idx as u32) to strictly comply with the checked/saturating arithmetic policy adopted across the contract.

Closes #134

Comment thread contracts/events/src/event_ops.rs Outdated
let amount = base_escrow.saturating_mul(percent) / 100_i128;

let anchor_idx = existing_count + (idx as u32);
let anchor_idx = existing_count.saturating_add(idx as u32);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Info Severity severity

Arithmetic and Financial Logic: silent index collision risk in winner selection loop

existing_count.saturating_add(idx as u32) can clamp anchor_idx to u32::MAX instead of failing if existing_count + idx overflows. Because anchor_idx is persisted in PrizeAward and likely used later to locate the corresponding Winner, saturation could cause multiple awards to share the same anchor_idx, leading to incorrect lookups (wrong recipient/amount) or permanent inability to claim if the index no longer matches the appended winner row.

For persisted indices/anchors, prefer fail-closed arithmetic: use checked_add and return an explicit error (or enforce existing_count <= u32::MAX - winners.len()) so state cannot be written with a clamped/colliding anchor_idx.


Fix with MCP
Almanax found a vulnerability. Can you take a look and fix it?
Finding ID: 14fe1965-603a-4858-8cd4-84c8baba6cfd
Actions
  • Reply /almanax ask <question> to ask a follow-up question.
  • Reply /almanax dismiss [<reason>] and it won't appear again in future scans.
  • Reply /almanax resolve [<reason>] to mark the finding as resolved.
  • Reply /almanax severity <level> [<reason>] to override the severity.

@rushikeshgarad2024-dev

Copy link
Copy Markdown
Author

Thank you for the review @almanax-ai[bot]!

Updated anchor_idx calculation to use fail-closed checked arithmetic (existing_count.checked_add(idx as u32).ok_or(Error::TooManyContributors)?), preventing any silent collision/clamping at u32::MAX and ensuring the transaction safely reverts on counter overflow.

/almanax resolve

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.

select_winners: use saturating arithmetic for the anchor index (Scout finding)

1 participant