diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md new file mode 100644 index 0000000..eb80bf4 --- /dev/null +++ b/PR_DESCRIPTION.md @@ -0,0 +1,82 @@ +# Fix #15 — Guard puzzle_verification reward arithmetic against overflow + +## Why it matters +An admin misconfiguring `reward_points` large enough, multiplied by the +`difficulty` cap, silently wraps or panics at the wrong layer. Both outcomes +break ledger invariants that `leaderboard`, `achievement_nft`, and +`reward_token` rely on. Because `meta.reward_points` is `i128` and `difficulty` +is cast to `i128` with `as`, the original `meta.reward_points * (meta.difficulty +as i128)` was an **unchecked** multiplication — overflow was silent in dev unless +`overflow-checks` happened to be on. + +## Technical context +- `PuzzleMeta.reward_points: i128`; `difficulty: u32` is widened via `as i128`. +- The old code used the `*` operator with no `checked_mul`, and the accumulated + `rewards += scaled` used `+=` with no `checked_add`. +- `cargo`/CI did not catch this because `clippy` here only denies + `clippy::correctness`; overflow-checking is a runtime/`overflow-checks` + concern, not a lint. + +## What changed + +### `contracts/puzzle_verification/src/lib.rs` +- Added a `#[contracterror]` `Error` enum (coordinating with Issue #27, Result + refactor) with the `RewardOverflow = 1` variant. +- `verify_solution` now computes `scaled` with `checked_mul` and the running + balance with `checked_add`; either overflow aborts the call via + `panic_with_error!(&env, Error::RewardOverflow)` instead of corrupting state. + ```rust + let scaled = match meta.reward_points.checked_mul((meta.difficulty as i128).max(1)) { + Some(v) => v, + None => panic_with_error!(&env, Error::RewardOverflow), + }; + let rewards = match rewards.checked_add(scaled) { + Some(v) => v, + None => panic_with_error!(&env, Error::RewardOverflow), + }; + ``` + +### `contracts/puzzle_verification/src/test.rs` +- Extracted the test module out of `lib.rs` into `src/test.rs` (matches the + file list for this issue and the repo's `datakey_keys_test.rs` convention). +- Added regression test `test_reward_overflow_panics` (`#[should_panic]`) that + drives `reward_points = i128::MAX` and `difficulty = u32::MAX` so + `reward_points * difficulty` overflows `i128`; `verify_solution` must abort + with `Error::RewardOverflow` rather than wrap. +- Added `test_large_reward_accrues` sanity check (1_000_000 × difficulty 3 = + 3_000_000, no overflow) to confirm the checked path still accrues correctly. + +### `docs/SECURE_CODING_GUIDELINES.md` +- Extended the **Arithmetic** section to mandate a `#[should_panic]` regression + test for every overflow fix, citing + `contracts/puzzle_verification/src/test.rs::test_reward_overflow_panics` + (Issue #15) as the canonical example. + +## Verification +- `cargo build -p puzzle-verification` succeeds. +- `cargo clippy -p puzzle-verification --lib` (denies `clippy::correctness`) + passes with rc=0. +- Test logic follows the repo's established `panic_with_error!` + + `#[should_panic]` pattern (see `contracts/decentralized_identity`). +- NOTE: the workspace-wide `cargo test` / `--all-targets` jobs currently fail + to compile `soroban-env-host 21.2.1` (a pre-existing, repo-wide dependency + break unrelated to this change — `ed25519-dalek 3.0.0` `rand_core 0.10` vs + `rand 0.8.7` `rand_core 0.6` skew). That infra break is tracked separately and + is not introduced by this PR; the contract's own build and clippy are clean. + +## Acceptance criteria checklist +- [x] `checked_mul` used for `scaled`. +- [x] Overflow returns `Error::RewardOverflow`. +- [x] `#[should_panic]` test for `i128::MAX` difficulty × `MAX` reward. +- [x] `docs/SECURE_CODING_GUIDELINES.md` updated to cite the regression test. +- [x] `Overflow` variant added to the new `Error` enum (Issue #27 coordination). + +## Labels +`area:security`, `kind:bug`, `priority:P0`, `contract:puzzle_verification` + +## Dependencies +Depends on Issue #27 (Result refactor) — the `Error` enum introduced here is the +contract's half of that refactor; remaining panic-to-`Error` conversions can land +in #27. + +closes #15 diff --git a/contracts/puzzle_verification/src/lib.rs b/contracts/puzzle_verification/src/lib.rs index 0ae709b..8b71430 100644 --- a/contracts/puzzle_verification/src/lib.rs +++ b/contracts/puzzle_verification/src/lib.rs @@ -1,6 +1,9 @@ #![no_std] -use soroban_sdk::{contract, contractimpl, contracttype, Address, Bytes, BytesN, Env, Symbol}; +use soroban_sdk::{ + contract, contracterror, contractimpl, contracttype, panic_with_error, Address, Bytes, BytesN, + Env, Symbol, +}; #[contracttype] #[derive(Clone)] @@ -21,6 +24,15 @@ pub enum DataKey { Rewards(Address), } +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + /// Reward point arithmetic overflowed (reward_points × difficulty, or + /// accumulated rewards). See Issue #15. + RewardOverflow = 1, +} + #[contract] pub struct PuzzleVerification; @@ -106,15 +118,24 @@ impl PuzzleVerification { .instance() .set(&DataKey::Completed(player.clone(), puzzle_id), &true); - let scaled = meta.reward_points * (meta.difficulty as i128).max(1); + let scaled = match meta + .reward_points + .checked_mul((meta.difficulty as i128).max(1)) + { + Some(v) => v, + None => panic_with_error!(&env, Error::RewardOverflow), + }; - let mut rewards: i128 = env + let rewards: i128 = env .storage() .instance() .get(&DataKey::Rewards(player.clone())) .unwrap_or(0); - rewards += scaled; + let rewards = match rewards.checked_add(scaled) { + Some(v) => v, + None => panic_with_error!(&env, Error::RewardOverflow), + }; env.storage() .instance() @@ -148,60 +169,4 @@ impl PuzzleVerification { } #[cfg(test)] -mod test { - use super::*; - use soroban_sdk::testutils::Address as _; - use soroban_sdk::testutils::Ledger as _; - - #[test] - fn test_verification_flow() { - let env = Env::default(); - let contract_id = env.register_contract(None, PuzzleVerification); - let client = PuzzleVerificationClient::new(&env, &contract_id); - - let admin = Address::generate(&env); - let player = Address::generate(&env); - - env.mock_all_auths(); - client.initialize(&admin); - - env.ledger().set_timestamp(1_000); - - let preimage = Bytes::from_array(&env, &[7u8; 5]); - let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); - let now = env.ledger().timestamp(); - - client.set_puzzle(&1, &hash, &(now - 1), &(now + 1000), &2, &50); - - let wrong = Bytes::from_array(&env, &[8u8; 5]); - assert_eq!(client.verify_solution(&player, &1, &wrong), false); - - assert_eq!(client.verify_solution(&player, &1, &preimage), true); - assert_eq!(client.is_completed(&player, &1), true); - assert_eq!(client.rewards_of(&player), 100); - } - - #[test] - #[should_panic(expected = "puzzle not active")] - fn test_expiration_enforced() { - let env = Env::default(); - let contract_id = env.register_contract(None, PuzzleVerification); - let client = PuzzleVerificationClient::new(&env, &contract_id); - - let admin = Address::generate(&env); - let player = Address::generate(&env); - - env.mock_all_auths(); - client.initialize(&admin); - - env.ledger().set_timestamp(1_000); - - let preimage = Bytes::from_array(&env, &[1u8; 3]); - let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); - let now = env.ledger().timestamp(); - - client.set_puzzle(&42, &hash, &(now - 100), &(now - 50), &1, &10); - - let _ = client.verify_solution(&player, &42, &preimage); - } -} +mod test; diff --git a/contracts/puzzle_verification/src/test.rs b/contracts/puzzle_verification/src/test.rs new file mode 100644 index 0000000..f02fe0e --- /dev/null +++ b/contracts/puzzle_verification/src/test.rs @@ -0,0 +1,117 @@ +use super::*; +use soroban_sdk::testutils::Address as _; +use soroban_sdk::testutils::Ledger as _; + +#[test] +fn test_verification_flow() { + let env = Env::default(); + let contract_id = env.register_contract(None, PuzzleVerification); + let client = PuzzleVerificationClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let player = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + + env.ledger().set_timestamp(1_000); + + let preimage = Bytes::from_array(&env, &[7u8; 5]); + let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); + let now = env.ledger().timestamp(); + + client.set_puzzle(&1, &hash, &(now - 1), &(now + 1000), &2, &50); + + let wrong = Bytes::from_array(&env, &[8u8; 5]); + assert_eq!(client.verify_solution(&player, &1, &wrong), false); + + assert_eq!(client.verify_solution(&player, &1, &preimage), true); + assert_eq!(client.is_completed(&player, &1), true); + assert_eq!(client.rewards_of(&player), 100); +} + +#[test] +#[should_panic(expected = "puzzle not active")] +fn test_expiration_enforced() { + let env = Env::default(); + let contract_id = env.register_contract(None, PuzzleVerification); + let client = PuzzleVerificationClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let player = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + + env.ledger().set_timestamp(1_000); + + let preimage = Bytes::from_array(&env, &[1u8; 3]); + let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); + let now = env.ledger().timestamp(); + + client.set_puzzle(&42, &hash, &(now - 100), &(now - 50), &1, &10); + + let _ = client.verify_solution(&player, &42, &preimage); +} + +/// Regression test for Issue #15: reward arithmetic must not silently wrap. +/// `i128::MAX` reward points multiplied by `u32::MAX` difficulty overflows +/// `i128`, so `verify_solution` must abort with `Error::RewardOverflow` +/// (manifested here as a panic) rather than corrupting ledger state. +#[test] +#[should_panic] +fn test_reward_overflow_panics() { + let env = Env::default(); + let contract_id = env.register_contract(None, PuzzleVerification); + let client = PuzzleVerificationClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let player = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + + env.ledger().set_timestamp(1_000); + + let preimage = Bytes::from_array(&env, &[3u8; 4]); + let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); + let now = env.ledger().timestamp(); + + // MAX reward points × MAX difficulty → checked_mul overflows i128. + client.set_puzzle( + &7, + &hash, + &(now - 1), + &(now + 1000), + &u32::MAX, + &i128::MAX, + ); + + let _ = client.verify_solution(&player, &7, &preimage); +} + +/// Sanity check that a large-but-safe reward still accrues correctly. +#[test] +fn test_large_reward_accrues() { + let env = Env::default(); + let contract_id = env.register_contract(None, PuzzleVerification); + let client = PuzzleVerificationClient::new(&env, &contract_id); + + let admin = Address::generate(&env); + let player = Address::generate(&env); + + env.mock_all_auths(); + client.initialize(&admin); + + env.ledger().set_timestamp(1_000); + + let preimage = Bytes::from_array(&env, &[5u8; 6]); + let hash: BytesN<32> = env.crypto().sha256(&preimage).into(); + let now = env.ledger().timestamp(); + + // 1_000_000 reward points × difficulty 3 = 3_000_000 (no overflow). + client.set_puzzle(&9, &hash, &(now - 1), &(now + 1000), &3, &1_000_000); + + assert_eq!(client.verify_solution(&player, &9, &preimage), true); + assert_eq!(client.rewards_of(&player), 3_000_000); +} diff --git a/docs/SECURE_CODING_GUIDELINES.md b/docs/SECURE_CODING_GUIDELINES.md index 6f4734f..7e44093 100644 --- a/docs/SECURE_CODING_GUIDELINES.md +++ b/docs/SECURE_CODING_GUIDELINES.md @@ -14,6 +14,13 @@ - Use checked math everywhere totals could overflow. - Avoid float. +- Regression test every overflow fix with a `#[should_panic]` case that drives + the inputs to the `i128`/`u128` extremes. Example: + `contracts/puzzle_verification/src/test.rs::test_reward_overflow_panics` + (Issue #15) feeds `reward_points = i128::MAX` and `difficulty = u32::MAX` so + `reward_points * difficulty` overflows `i128`; `verify_solution` must abort + with `Error::RewardOverflow` instead of silently wrapping ledger state relied + on by `leaderboard`, `achievement_nft`, and `reward_token`. ## State