fix: prevent duplicate loaded contracts

Author: thepastaclaw

src/backend_task/contract.rs

@@ -20,13 +20,37 @@ use dash_sdk::dpp::group::group_action::GroupAction;
 use dash_sdk::dpp::group::group_action_status::GroupActionStatus;
 use dash_sdk::dpp::identity::accessors::IdentityGettersV0;
 use dash_sdk::dpp::platform_value::Value;
+use dash_sdk::dpp::platform_value::string_encoding::Encoding;
 use dash_sdk::drive::query::{WhereClause, WhereOperator};
 use dash_sdk::platform::group_actions::GroupActionsQuery;
 use dash_sdk::platform::{
     DataContract, Document, DocumentQuery, Fetch, FetchMany, Identifier, IdentityPublicKey,
 };
 use dash_sdk::query_types::IndexMap;
 
+/// Returns the first identifier in `identifiers` that appears earlier in the
+/// same slice, or `None` if every entry is unique.
+///
+/// Used as a task-boundary duplicate guard for [`ContractTask::FetchContracts`].
+fn first_duplicate_id(identifiers: &[Identifier]) -> Option<Identifier> {
+    identifiers
+        .iter()
+        .enumerate()
+        .find_map(|(index, id)| identifiers[..index].contains(id).then_some(*id))
+}
+
+/// Returns the first identifier in `requested` that is also present in
+/// `existing`, or `None` when none of the requested IDs are already loaded.
+fn first_already_loaded_id(
+    requested: &[Identifier],
+    existing: &[Identifier],
+) -> Option<Identifier> {
+    requested
+        .iter()
+        .find(|identifier| existing.contains(identifier))
+        .copied()
+}
+
 #[derive(Debug, Clone, PartialEq)]
 pub enum ContractTask {
     FetchContracts(Vec<Identifier>),
@@ -47,6 +71,28 @@ impl AppContext {
     ) -> Result<BackendTaskSuccessResult, crate::backend_task::error::TaskError> {
         match task {
             ContractTask::FetchContracts(identifiers) => {
+                // Task-boundary duplicate / already-loaded enforcement.
+                //
+                // The `AddContractsScreen` UI also performs these checks, but
+                // duplicating them here protects non-UI callers (e.g. MCP tools)
+                // and closes the TOCTOU window between the screen check and
+                // the network fetch / persistence below.
+                if let Some(duplicate) = first_duplicate_id(&identifiers) {
+                    return Err(
+                        crate::backend_task::error::TaskError::DuplicateContractInRequest {
+                            contract_id: duplicate.to_string(Encoding::Base58),
+                        },
+                    );
+                }
+                let existing_ids = self.loaded_contract_ids()?;
+                if let Some(already_loaded) = first_already_loaded_id(&identifiers, &existing_ids) {
+                    return Err(
+                        crate::backend_task::error::TaskError::ContractAlreadyLoaded {
+                            contract_id: already_loaded.to_string(Encoding::Base58),
+                        },
+                    );
+                }
+
                 match DataContract::fetch_many(sdk, identifiers).await {
                     Ok(data_contracts) => {
                         let mut results = vec![];
@@ -208,9 +254,30 @@ impl AppContext {
             }
             ContractTask::RemoveContract(identifier) => self
                 .remove_contract(&identifier)
-                .map(|_| BackendTaskSuccessResult::RemovedContract)
-                .map_err(crate::backend_task::error::TaskError::from),
+                .map(|_| BackendTaskSuccessResult::RemovedContract),
             ContractTask::SaveDataContract(data_contract, alias, insert_tokens_too) => {
+                // Task-boundary enforcement: the local DB layer silently
+                // skips system contract IDs and `INSERT OR IGNORE` makes a
+                // duplicate user-contract insert a no-op, so without this
+                // check non-UI callers could appear to "save" a contract
+                // when the database state did not actually change.
+                let contract_id = data_contract.id();
+                if self.is_system_contract_id(&contract_id) {
+                    return Err(
+                        crate::backend_task::error::TaskError::SystemContractImmutable {
+                            contract_id: contract_id.to_string(Encoding::Base58),
+                        },
+                    );
+                }
+                let existing_ids = self.loaded_contract_ids()?;
+                if existing_ids.contains(&contract_id) {
+                    return Err(
+                        crate::backend_task::error::TaskError::ContractAlreadyLoaded {
+                            contract_id: contract_id.to_string(Encoding::Base58),
+                        },
+                    );
+                }
+
                 self.db.insert_contract_if_not_exists(
                     &data_contract,
                     alias.as_deref(),
@@ -222,3 +289,37 @@ impl AppContext {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn id(byte: u8) -> Identifier {
+        Identifier::from_bytes(&[byte; 32]).expect("32 bytes is a valid identifier")
+    }
+
+    #[test]
+    fn first_duplicate_id_returns_none_for_unique_inputs() {
+        assert!(first_duplicate_id(&[id(1), id(2), id(3)]).is_none());
+    }
+
+    #[test]
+    fn first_duplicate_id_returns_first_repeat() {
+        assert_eq!(
+            first_duplicate_id(&[id(1), id(2), id(1), id(3)]),
+            Some(id(1))
+        );
+    }
+
+    #[test]
+    fn first_already_loaded_id_returns_first_match_in_request_order() {
+        let requested = [id(7), id(2), id(9)];
+        let existing = [id(9), id(2)];
+        assert_eq!(first_already_loaded_id(&requested, &existing), Some(id(2)));
+    }
+
+    #[test]
+    fn first_already_loaded_id_returns_none_when_no_overlap() {
+        assert!(first_already_loaded_id(&[id(1), id(2)], &[id(3), id(4)]).is_none());
+    }
+}

src/backend_task/error.rs

@@ -458,6 +458,39 @@ pub enum TaskError {
     )]
     DataContractNotFound,
 
+    /// A user-driven mutation targeted a built-in system contract. System
+    /// contracts (DPNS, DashPay, withdrawals, token history, keyword search)
+    /// are managed by the application and cannot be modified or removed.
+    #[error(
+        "Contract {contract_id} is a built-in system contract and cannot be modified or removed. \
+         Use a different contract."
+    )]
+    SystemContractImmutable {
+        /// Base58-encoded ID of the system contract whose mutation was rejected.
+        contract_id: String,
+    },
+
+    /// The same contract identifier appeared more than once in a single
+    /// add-contracts request.
+    #[error(
+        "Contract {contract_id} was entered more than once. Remove the duplicate entry before adding contracts."
+    )]
+    DuplicateContractInRequest {
+        /// Base58-encoded ID of the duplicated contract.
+        contract_id: String,
+    },
+
+    /// An add-contracts request referenced a contract that is already loaded
+    /// (either persisted in the local database or one of the built-in system
+    /// contracts).
+    #[error(
+        "Contract {contract_id} is already loaded. Select it from the existing contracts list or enter a different contract ID."
+    )]
+    ContractAlreadyLoaded {
+        /// Base58-encoded ID of the already-loaded contract.
+        contract_id: String,
+    },
+
     // ──────────────────────────────────────────────────────────────────────────
     // Serialization errors
     // ──────────────────────────────────────────────────────────────────────────

src/backend_task/update_data_contract.rs

@@ -108,8 +108,7 @@ impl AppContext {
 
         match state_transition.broadcast_and_wait(sdk, None).await {
             Ok(returned_contract) => {
-                self.db
-                    .replace_contract(data_contract.id(), &returned_contract, self)?;
+                self.replace_contract(data_contract.id(), &returned_contract)?;
                 let fee_result = FeeResult::new(estimated_fee, estimated_fee);
                 Ok(BackendTaskSuccessResult::UpdatedContract(fee_result))
             }
@@ -148,9 +147,7 @@ impl AppContext {
                             _ => sleep(Duration::from_secs(10)).await,
                         }
                         if let Ok(Some(contract)) = DataContract::fetch(sdk, id).await {
-                            self.db
-                                .replace_contract(contract.id(), &contract, self)
-                                .ok();
+                            self.replace_contract(contract.id(), &contract).ok();
 
                             return Ok(BackendTaskSuccessResult::ContractSavedAfterProofError);
                         }