fix(#445): replace unchecked i128/u32 additions with checked_add to prevent overflow - #721
Conversation
|
@dedukpe Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
davidmaronio
left a comment
There was a problem hiding this comment.
good sweep for #445. converting the raw + additions in check_contribution_caps, update_contribution_accounting, the personal-cap checks and the revenue pool/claim paths to checked_add(...).ok_or(Error::Overflow)? is the right pattern, and threading Result through update_contribution_accounting is done cleanly.
blockers:
-
src/revenue.rs:130 - the
set_revenue_claimed(...)call ends with)and no semicolon, right before the comment block and the next statement. that is a syntax error and is why fmt, clippy and test are all red. add the;. -
src/revenue.rs:144 -
claimants_so_far + 1is still an unchecked add while everything around it got converted. it is a claimant counter so overflow is unrealistic, but for consistency with the PR's stated goal make itchecked_add(1).ok_or(Error::Overflow)?too (same for any remaining+ 1counters you touched around). -
after fixing, run
cargo fmtandcargo clippy --features testutilslocally; the branch is also behind main, and part of the red CI likely comes from the stale base.
please fix the semicolon, rebase on main, and get CI green; the substance is otherwise ready.
… prevent overflow
d1dfd93 to
851d2ed
Compare
davidmaronio
left a comment
There was a problem hiding this comment.
all three items addressed: the set_revenue_claimed call is fixed, both claimants_so_far + 1 counters now use checked_add, and fmt/clippy/test are green on a fresh base. consistent overflow handling across the contribution and revenue paths, good work.
Summary
Closes #445
In
src/contributions.rsandsrc/revenue.rs, several uncheckedi128andu32additions could silently overflow on high-balance tokens (e.g., 18-decimal tokens with large supplies). This PR replaces all of them withchecked_add(...).ok_or(Error::Overflow)?to ensure safe overflow handling.Note:
verify_with_votesinsrc/voting.rsalready useschecked_addfortotal_votesandtotal_weight(fixed in prior work). This PR extends the same pattern to the remaining locations.Changes
src/contributions.rs(46 lines changed)check_contribution_caps: cap comparison now useschecked_addupdate_contribution_accounting: return type changed from()toResult<(), Error>; 5 additions (amount_raised, effective_amount_raised, contribution, lifetime, total_raised) all use checked arithmeticcontribute(): personal cap check useschecked_addbatch_contribute(): personal cap check useschecked_addsrc/revenue.rs(31 lines changed)deposit_revenue: pool accumulation useschecked_addclaim_revenue: claimed amount and distributed accumulator usechecked_addclaim_creator_revenue: claimed amount useschecked_addSecurity Considerations
cast_votefor consistencyError::Overflowvariant (discriminant 30) already exists insrc/errors.rsTesting
Error::Overflowinstead of silently wrapping