Address code review feedback: improve error handling, security, and validation

Co-authored-by: Steake <530040+Steake@users.noreply.github.com>

Author: Copilot

crates/bitcell-admin/src/hsm.rs

@@ -70,7 +70,11 @@ pub struct HsmConfig {
 }
 
 /// HSM authentication credentials
-#[derive(Debug, Clone, Default, Serialize, Deserialize)]
+/// 
+/// # Security
+/// Credentials are automatically zeroed when dropped to prevent
+/// sensitive data from remaining in memory.
+#[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct HsmCredentials {
     /// API token (for Vault)
     #[serde(skip_serializing)]
@@ -87,6 +91,46 @@ pub struct HsmCredentials {
     pub client_key: Option<String>,
 }
 
+impl Default for HsmCredentials {
+    fn default() -> Self {
+        Self {
+            token: None,
+            access_key: None,
+            secret_key: None,
+            client_cert: None,
+            client_key: None,
+        }
+    }
+}
+
+impl Drop for HsmCredentials {
+    fn drop(&mut self) {
+        // Securely zero out sensitive credential data
+        // Note: This provides basic protection but for production use
+        // consider using the `secrecy` crate for guaranteed secure zeroing
+        if let Some(ref mut token) = self.token {
+            // Safety: We're writing zeros to memory that will be dropped
+            // This helps prevent secrets from lingering in memory
+            let bytes = unsafe { token.as_bytes_mut() };
+            for byte in bytes {
+                unsafe { std::ptr::write_volatile(byte, 0) };
+            }
+        }
+        if let Some(ref mut key) = self.access_key {
+            let bytes = unsafe { key.as_bytes_mut() };
+            for byte in bytes {
+                unsafe { std::ptr::write_volatile(byte, 0) };
+            }
+        }
+        if let Some(ref mut key) = self.secret_key {
+            let bytes = unsafe { key.as_bytes_mut() };
+            for byte in bytes {
+                unsafe { std::ptr::write_volatile(byte, 0) };
+            }
+        }
+    }
+}
+
 impl HsmConfig {
     /// Create configuration for HashiCorp Vault
     pub fn vault(endpoint: &str, token: &str, key_name: &str) -> Self {
@@ -95,7 +139,10 @@ impl HsmConfig {
             endpoint: endpoint.to_string(),
             credentials: HsmCredentials {
                 token: Some(token.to_string()),
-                ..Default::default()
+                access_key: None,
+                secret_key: None,
+                client_cert: None,
+                client_key: None,
             },
             default_key: key_name.to_string(),
             timeout_secs: 30,
@@ -109,9 +156,11 @@ impl HsmConfig {
             provider: HsmProvider::AwsCloudHsm,
             endpoint: endpoint.to_string(),
             credentials: HsmCredentials {
+                token: None,
                 access_key: Some(access_key.to_string()),
                 secret_key: Some(secret_key.to_string()),
-                ..Default::default()
+                client_cert: None,
+                client_key: None,
             },
             default_key: key_name.to_string(),
             timeout_secs: 30,
@@ -124,7 +173,7 @@ impl HsmConfig {
         Self {
             provider: HsmProvider::Mock,
             endpoint: "mock://localhost".to_string(),
-            credentials: Default::default(),
+            credentials: HsmCredentials::default(),
             default_key: key_name.to_string(),
             timeout_secs: 5,
             audit_logging: false,
@@ -185,6 +234,10 @@ pub trait HsmBackend: Send + Sync {
     async fn list_keys(&self) -> HsmResult<Vec<String>>;
 }
 
+/// Maximum number of audit log entries to keep in memory
+/// Older entries are automatically rotated out
+const MAX_AUDIT_LOG_ENTRIES: usize = 10_000;
+
 /// HSM client for secure key management
 pub struct HsmClient {
     config: HsmConfig,
@@ -309,6 +362,9 @@ impl HsmClient {
     }
 
     /// Log an operation
+    /// 
+    /// The audit log is bounded to MAX_AUDIT_LOG_ENTRIES entries.
+    /// When the limit is reached, the oldest entries are removed.
     async fn log_operation(&self, operation: &str, key_name: &str, success: bool, error: Option<&HsmError>) {
         if !self.config.audit_logging {
             return;
@@ -325,7 +381,14 @@ impl HsmClient {
             error: error.map(|e| e.to_string()),
         };
 
-        self.audit_log.write().await.push(entry);
+        let mut log = self.audit_log.write().await;
+        log.push(entry);
+        
+        // Enforce maximum size by removing oldest entries
+        if log.len() > MAX_AUDIT_LOG_ENTRIES {
+            let excess = log.len() - MAX_AUDIT_LOG_ENTRIES;
+            log.drain(0..excess);
+        }
     }
 }
 
@@ -438,7 +501,9 @@ mod tests {
 
     #[tokio::test]
     async fn test_mock_hsm_audit_log() {
-        let config = HsmConfig::mock("audit-test");
+        // Create mock config with audit logging enabled
+        let mut config = HsmConfig::mock("audit-test");
+        config.audit_logging = true;
         let hsm = HsmClient::connect(config).await.unwrap();
 
         // Perform some operations

crates/bitcell-crypto/src/poseidon.rs

@@ -81,9 +81,20 @@ impl PoseidonParams<Fr> {
         let total_constants = t * (full_rounds + partial_rounds);
         let mut constants = Vec::with_capacity(total_constants);
 
+        // Maximum iterations to prevent theoretical infinite loop
+        const MAX_ITERATIONS: u64 = 1_000_000;
+        
         // Use domain-separated SHA-256 as PRF
         let mut counter = 0u64;
         while constants.len() < total_constants {
+            if counter >= MAX_ITERATIONS {
+                panic!(
+                    "Round constant generation exceeded {} iterations. \
+                     This should never happen with SHA-256 PRF - please report this bug.",
+                    MAX_ITERATIONS
+                );
+            }
+            
             let mut hasher = Sha256::new();
             hasher.update(b"BitCell_Poseidon_RC");
             hasher.update(&counter.to_le_bytes());
@@ -120,7 +131,10 @@ impl PoseidonParams<Fr> {
             for j in 0..t {
                 // M[i][j] = 1 / (x[i] + y[j])
                 let sum = x[i] + y[j];
-                matrix[i][j] = sum.inverse().expect("x[i] + y[j] should be non-zero");
+                matrix[i][j] = sum.inverse().expect(
+                    "Cauchy MDS construction: x[i] and y[j] are distinct elements, \
+                     so x[i] + y[j] != 0 and is always invertible in a prime field"
+                );
             }
         }
 

crates/bitcell-wallet-gui/src/main.rs

@@ -391,7 +391,7 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
             let amount: f64 = match amount_str.parse() {
                 Ok(a) if a > 0.0 => a,
                 _ => {
-                    wallet_state.set_status_message("Invalid amount: must be a positive number".into());
+                    wallet_state.set_status_message("Invalid amount format: expected a positive number (e.g., 1.23)".into());
                     return;
                 }
             };
@@ -403,6 +403,16 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
 
             let chain = parse_chain(&chain_str);
 
+            // Validate amount before conversion to prevent overflow
+            // Max safe value: u64::MAX / 100_000_000 ≈ 184 billion
+            const MAX_AMOUNT: f64 = 184_467_440_737.0; // u64::MAX / 100_000_000
+            if amount > MAX_AMOUNT {
+                wallet_state.set_status_message(format!(
+                    "Amount too large. Maximum: {} CELL", MAX_AMOUNT
+                ).into());
+                return;
+            }
+            
             // Convert to smallest units (1 CELL = 100_000_000 units)
             let amount_units = (amount * 100_000_000.0) as u64;
 

crates/bitcell-wallet/src/hardware.rs

@@ -84,40 +84,41 @@ pub struct HardwareWallet {
 
 impl HardwareWallet {
     /// Connect to a hardware wallet
-    #[allow(unreachable_code)]
     pub fn connect(wallet_type: HardwareWalletType) -> Result<Self> {
         let device: Arc<dyn HardwareWalletDevice> = match wallet_type {
             HardwareWalletType::Ledger => {
                 #[cfg(feature = "ledger")]
                 {
-                    Arc::new(LedgerDevice::connect()?)
+                    return Ok(Self {
+                        device: Arc::new(LedgerDevice::connect()?),
+                        derivation_path: "m/44'/60'/0'/0/0".to_string(),
+                    });
                 }
                 #[cfg(not(feature = "ledger"))]
                 {
-                    return Err(Error::UnsupportedChain("Ledger support not compiled in".into()));
+                    return Err(Error::HardwareWallet("Ledger support not compiled in. Enable the 'ledger' feature.".into()));
                 }
             }
             HardwareWalletType::Trezor => {
                 #[cfg(feature = "trezor")]
                 {
-                    Arc::new(TrezorDevice::connect()?)
+                    return Ok(Self {
+                        device: Arc::new(TrezorDevice::connect()?),
+                        derivation_path: "m/44'/60'/0'/0/0".to_string(),
+                    });
                 }
                 #[cfg(not(feature = "trezor"))]
                 {
-                    return Err(Error::UnsupportedChain("Trezor support not compiled in".into()));
+                    return Err(Error::HardwareWallet("Trezor support not compiled in. Enable the 'trezor' feature.".into()));
                 }
             }
             HardwareWalletType::Generic => {
-                return Err(Error::UnsupportedChain("Generic hardware wallet not implemented".into()));
+                return Err(Error::HardwareWallet("Generic hardware wallet is not yet implemented".into()));
             }
             #[cfg(test)]
             HardwareWalletType::Mock => {
                 Arc::new(MockHardwareWallet::new())
             }
-            #[cfg(not(test))]
-            _ => {
-                return Err(Error::UnsupportedChain("Unknown hardware wallet type".into()));
-            }
         };
 
         Ok(Self {

crates/bitcell-wallet/src/lib.rs

@@ -61,6 +61,9 @@ pub enum Error {
     #[error("Chain not supported: {0}")]
     UnsupportedChain(String),
 
+    #[error("Hardware wallet error: {0}")]
+    HardwareWallet(String),
+
     #[error("Wallet locked")]
     WalletLocked,
 

crates/bitcell-zkp/src/poseidon_merkle.rs

@@ -134,7 +134,10 @@ impl<F: PrimeField> PoseidonMerkleGadget<F> {
         for i in 0..t {
             for j in 0..t {
                 let sum = x[i] + y[j];
-                matrix[i][j] = sum.inverse().expect("x[i] + y[j] should be non-zero");
+                matrix[i][j] = sum.inverse().expect(
+                    "MDS matrix Cauchy construction guarantees non-zero inverse: \
+                     x[i] and y[j] are chosen as distinct elements so x[i] + y[j] != 0"
+                );
             }
         }
 
@@ -340,7 +343,9 @@ fn poseidon_hash_native<F: PrimeField>(left: F, right: F) -> F {
     for i in 0..t {
         for j in 0..t {
             let sum = x[i] + y[j];
-            mds_matrix[i][j] = sum.inverse().unwrap();
+            mds_matrix[i][j] = sum.inverse().expect(
+                "MDS matrix Cauchy construction guarantees non-zero inverse for distinct x_i, y_j"
+            );
         }
     }