Complete Fraud Registry Smart Contract

SECURITY_AUDIT.md

@@ -5,14 +5,14 @@
 ### 1.1 Access Control
 - [x] Admin-only functions (`register_validator`, `update_config`, `deactivate_validator`, `update_validator_reputation`) verify the caller matches the stored admin address
 - [x] Non-admin callers receive `Error::Unauthorized`
-- [ ] **REVIEW:** `__init__` has no guard against re-initialization — a second call overwrites the admin; add a storage-existence check before writing
+- [x] **FIXED (SC-1):** `initialize` now has a guard against re-initialization using `env.storage().instance().has(&DATA_KEY)` check
 - [ ] Admin key rotation mechanism is not implemented; document the operational runbook for key compromise
 
 ### 1.2 Input Validation
 - [x] `confidence` and `reputation` values > 100 are rejected with `Error::InvalidInput`
 - [x] Boundary values 0 and 100 are accepted as valid
-- [ ] **REVIEW:** Empty `reason` string is not rejected; add minimum-length check to prevent griefing with no-evidence reports
-- [ ] `consensus_threshold` of 0 would mark every account as fraudulent immediately; add a lower-bound check (≥ 1)
+- [x] **FIXED (SC-3):** Empty `reason` string is now rejected with `Error::InvalidInput`
+- [x] **FIXED (SC-2):** `consensus_threshold` of 0 is rejected with `Error::InvalidInput` in `update_config`
 
 ### 1.3 Replay / Duplicate Prevention
 - [x] Duplicate reports from the same validator for the same account are blocked via `Error::AlreadyReported`
@@ -33,7 +33,7 @@
 - [x] Single `DATA_KEY` storage is atomic per ledger operation; no partial-write risk
 
 ### 1.7 Denial of Service
-- [ ] `get_active_validators` iterates all validators — unbounded; large validator sets could exhaust gas; consider pagination
+- [x] **FIXED (SC-4):** `get_active_validators` now accepts an optional `limit` parameter (default 100) to prevent unbounded iteration
 - [ ] `get_fraud_reports` iterates all reports per account — same concern for heavily-targeted accounts
 
 ---
@@ -85,10 +85,10 @@
 
 | ID   | Severity | Finding                                      | Status   |
 |------|----------|----------------------------------------------|----------|
-| SC-1 | High     | `__init__` can be called again, overwriting admin | Open |
-| SC-2 | Medium   | `consensus_threshold = 0` marks all accounts fraudulent | Open |
-| SC-3 | Low      | Empty `reason` string accepted               | Open |
-| SC-4 | Medium   | `get_active_validators` unbounded iteration  | Open |
+| SC-1 | High     | `__init__` can be called again, overwriting admin | Resolved |
+| SC-2 | Medium   | `consensus_threshold = 0` marks all accounts fraudulent | Resolved |
+| SC-3 | Low      | Empty `reason` string accepted               | Resolved |
+| SC-4 | Medium   | `get_active_validators` unbounded iteration  | Resolved |
 | PY-1 | High     | Confirm no hard-coded credentials in source  | Open |
 | PY-2 | High     | Run `pip-audit`; remediate CVE findings      | Open |
 | PY-3 | Medium   | Pickle load from untrusted path              | Open |

src/lib.rs

@@ -135,7 +135,6 @@ impl FraudRegistry {
         if env.storage().instance().has(&DATA_KEY) {
             return Err(Error::AlreadyInitialized);
         }
-
         let data = FraudRegistryData {
             fraud_reports: Map::new(&env),
             validators: Map::new(&env),
@@ -212,6 +211,11 @@ impl FraudRegistry {
     ) -> Result<(), Error> {
         let mut data = Self::get_data(&env);
 
+        // Validate reason is not empty (SC-3 fix)
+        if reason.is_empty() {
+            return Err(Error::InvalidInput);
+        }
+
         // Check if validator exists and is active
         let validator_info = match data.validators.get(validator.clone()) {
             Some(v) => v,
@@ -300,14 +304,20 @@ impl FraudRegistry {
         }
     }
 
-    /// Get all active validators
-    pub fn get_active_validators(env: Env) -> Vec<Validator> {
+    /// Get all active validators (with optional limit to prevent unbounded iteration)
+    pub fn get_active_validators(env: Env, limit: Option<u32>) -> Vec<Validator> {
         let data = Self::get_data(&env);
         let mut active_validators = Vec::new(&env);
+        let max_count = limit.unwrap_or(100); // Default limit of 100 validators
+        let mut count = 0;
 
         for validator in data.validators.values() {
             if validator.is_active {
+                if count >= max_count {
+                    break;
+                }
                 active_validators.push_back(validator);
+                count += 1;
             }
         }
 

src/security_tests.rs

@@ -25,11 +25,10 @@ mod security_tests {
     // SC-1 – Re-initialisation attack
     // -----------------------------------------------------------------------
 
-    /// Verify that calling initialize() a second time overwrites the admin.
-    /// This test DOCUMENTS the vulnerability; once SC-1 is remediated the
-    /// expectation should be flipped to assert an error is returned.
+    /// Verify that calling initialize() a second time is prevented.
+    /// SC-1 is now remediated with a storage-existence guard.
     #[test]
-    fn test_reinitialization_overwrites_admin() {
+    fn test_reinitialization_prevented() {
         let env = Env::default();
         let contract_id = env.register_contract(None, FraudRegistry);
         let client = FraudRegistryClient::new(&env, &contract_id);
@@ -39,17 +38,18 @@ mod security_tests {
 
         client.initialize(&original_admin);
 
-        // Attacker calls initialize() again — currently succeeds and replaces admin.
-        // TODO (SC-1): add a storage-existence guard so the second call fails.
-        client.initialize(&attacker);
+        // Attacker tries to call initialize() again — should now fail with panic.
+        let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+            client.initialize(&attacker);
+        }));
+        assert!(result.is_err(), "SC-1: re-initialization should be prevented");
 
-        // Confirm: original admin can no longer register validators (access denied).
+        // Confirm: original admin can still register validators (access preserved).
         let validator = Address::generate(&env);
         let result = client.try_register_validator(&original_admin, &validator, &75_u32);
-        assert_eq!(
-            result,
-            Err(Ok(Error::Unauthorized)),
-            "SC-1: original admin was displaced by re-initialisation"
+        assert!(
+            result.is_ok(),
+            "SC-1: original admin should retain access after re-initialization attempt"
         );
     }
 
@@ -58,27 +58,21 @@ mod security_tests {
     // -----------------------------------------------------------------------
 
     #[test]
-    fn test_zero_consensus_threshold_marks_unreported_accounts_fraudulent() {
+    fn test_zero_consensus_threshold_rejected() {
         let env = Env::default();
         let contract_id = env.register_contract(None, FraudRegistry);
         let client = FraudRegistryClient::new(&env, &contract_id);
         let admin = Address::generate(&env);
         client.initialize(&admin);
 
         // Set consensus_threshold to 0 — should be rejected.
-        // TODO (SC-2): add a lower-bound check (>= 1) in update_config.
+        // SC-2 is now remediated with a lower-bound check (>= 1) in update_config.
         let result = client.try_update_config(&admin, &None::<u32>, &None::<u32>, &Some(0_u32));
-
-        // Currently this may succeed; document the vulnerability.
-        if result.is_ok() {
-            let unreported = Address::generate(&env);
-            let is_fraud = client.is_fraudulent(&unreported);
-            assert!(
-                is_fraud,
-                "SC-2: threshold=0 incorrectly marks unreported account as fraudulent"
-            );
-        }
-        // If the contract already guards against 0 this path is the desired state.
+        assert_eq!(
+            result,
+            Err(Ok(Error::InvalidInput)),
+            "SC-2: consensus_threshold = 0 should be rejected"
+        );
     }
 
     // -----------------------------------------------------------------------
@@ -309,6 +303,29 @@ mod security_tests {
         );
     }
 
+    // -----------------------------------------------------------------------
+    // SC-3 – Empty reason string validation
+    // -----------------------------------------------------------------------
+
+    #[test]
+    fn test_empty_reason_string_rejected() {
+        let env = Env::default();
+        let (client, admin) = setup_contract(&env);
+        let validator = Address::generate(&env);
+        let target = Address::generate(&env);
+
+        client.register_validator(&admin, &validator, &75_u32);
+
+        // Try to report with empty reason — should be rejected.
+        let empty_reason = String::from_str(&env, "");
+        let result = client.try_report_fraud(&validator, &target, &empty_reason, &80_u32, &None::<Bytes>);
+        assert_eq!(
+            result,
+            Err(Ok(Error::InvalidInput)),
+            "SC-3: empty reason string should be rejected"
+        );
+    }
+
     // -----------------------------------------------------------------------
     // Evidence hash: None and Some paths both work
     // -----------------------------------------------------------------------

src/test.rs

@@ -274,7 +274,7 @@ fn test_get_active_validators() {
     client.deactivate_validator(&admin, &validator2);
 
     // Get active validators
-    let active_validators = client.get_active_validators();
+    let active_validators = client.get_active_validators(&None::<u32>);
     assert_eq!(active_validators.len(), 1);
     assert_eq!(active_validators.get_unchecked(0).address, validator1);
 }