Fix #15: guard puzzle_verification reward arithmetic against overflow - #3
Closed
Phantomcall wants to merge 1 commit into
Closed
Fix #15: guard puzzle_verification reward arithmetic against overflow#3Phantomcall wants to merge 1 commit into
Phantomcall wants to merge 1 commit into
Conversation
…ssue Riddlrealm#15) Use checked_mul/checked_add for reward scaling and accumulation; abort with a new Error::RewardOverflow variant on overflow instead of silently wrapping. Add a #[should_panic] regression test and cite it in SECURE_CODING_GUIDELINES.
Owner
Author
|
Superseded by Riddlrealm#58 (opened against the main repo, not the fork). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Riddlrealm#15 — Guard puzzle_verification reward arithmetic against overflow
Why it matters
An admin misconfiguring
reward_pointslarge enough, multiplied by thedifficultycap, silently wraps or panics at the wrong layer. Both outcomesbreak ledger invariants that
leaderboard,achievement_nft, andreward_tokenrely on. Becausemeta.reward_pointsisi128anddifficultyis cast to
i128withas, the originalmeta.reward_points * (meta.difficulty as i128)was an unchecked multiplication — overflow was silent in dev unlessoverflow-checkshappened to be on.Technical context
PuzzleMeta.reward_points: i128;difficulty: u32is widened viaas i128.*operator with nochecked_mul, and the accumulatedrewards += scaledused+=with nochecked_add.cargo/CI did not catch this becauseclippyhere only deniesclippy::correctness; overflow-checking is a runtime/overflow-checksconcern, not a lint.
What changed
contracts/puzzle_verification/src/lib.rs#[contracterror]Errorenum (coordinating with Issue Commit-reveal to prevent puzzle-solution front-running Riddlrealm/Mindmint-Contracts#27, Resultrefactor) with the
RewardOverflow = 1variant.verify_solutionnow computesscaledwithchecked_muland the runningbalance with
checked_add; either overflow aborts the call viapanic_with_error!(&env, Error::RewardOverflow)instead of corrupting state.contracts/puzzle_verification/src/test.rslib.rsintosrc/test.rs(matches thefile list for this issue and the repo's
datakey_keys_test.rsconvention).test_reward_overflow_panics(#[should_panic]) thatdrives
reward_points = i128::MAXanddifficulty = u32::MAXsoreward_points * difficultyoverflowsi128;verify_solutionmust abortwith
Error::RewardOverflowrather than wrap.test_large_reward_accruessanity check (1_000_000 × difficulty 3 =3_000_000, no overflow) to confirm the checked path still accrues correctly.
docs/SECURE_CODING_GUIDELINES.md#[should_panic]regressiontest for every overflow fix, citing
contracts/puzzle_verification/src/test.rs::test_reward_overflow_panics(Issue Fix unchecked reward multiplication in
puzzle_verification::verify_solutionRiddlrealm/Mindmint-Contracts#15) as the canonical example.Verification
cargo build -p puzzle-verificationsucceeds.cargo clippy -p puzzle-verification --lib(deniesclippy::correctness)passes with rc=0.
panic_with_error!+#[should_panic]pattern (seecontracts/decentralized_identity).cargo test/--all-targetsjobs currently failto compile
soroban-env-host 21.2.1(a pre-existing, repo-wide dependencybreak unrelated to this change —
ed25519-dalek 3.0.0rand_core 0.10vsrand 0.8.7rand_core 0.6skew). That infra break is tracked separately andis not introduced by this PR; the contract's own build and clippy are clean.
Acceptance criteria checklist
checked_mulused forscaled.Error::RewardOverflow.#[should_panic]test fori128::MAXdifficulty ×MAXreward.docs/SECURE_CODING_GUIDELINES.mdupdated to cite the regression test.Overflowvariant added to the newErrorenum (Issue Commit-reveal to prevent puzzle-solution front-running Riddlrealm/Mindmint-Contracts#27 coordination).Labels
area:security,kind:bug,priority:P0,contract:puzzle_verificationDependencies
Depends on Issue Riddlrealm#27 (Result refactor) — the
Errorenum introduced here is thecontract's half of that refactor; remaining panic-to-
Errorconversions can landin Riddlrealm#27.
closes Riddlrealm#15