Fix #15: guard puzzle_verification reward arithmetic against overflow

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

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;

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);
+}

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