fix: resolve issues #887 #888 #889 #890 in treasury contract and backend

.gitignore

@@ -60,12 +60,8 @@ Thumbs.db
 .vite/
 **/.vite/
 .vite-inspect/
-**/.vite/
 
 # Vitest / Jest output
-vitest-report/
-html/
-lcov.info
 vitest-coverage/
 **/.vitest-cache/
 
@@ -90,7 +86,9 @@ Cargo.lock
 
 # Playwright state
 playwright/.auth/
-playwright-report/
+
+# Playwright screenshots (generated during e2e runs)
+**/e2e/screenshots/
 
 # Editor session files
 *.swp
@@ -101,9 +99,3 @@ playwright-report/
 comebackhere-frontend/coverage/
 frontend/coverage/
 comebackhere-backend/coverage/
-
-# Playwright state
-playwright/.auth/
-
-# Playwright screenshots (generated during e2e runs)
-**/e2e/screenshots/

COMEBACKHERE-contracts/contracts/treasury/src/lib.rs

@@ -92,6 +92,15 @@ pub enum TreasuryError {
     DailyLimitExceeded = 13,
     /// An upgrade was requested while a settlement was partially executed.
     UpgradeInProgress = 14,
+    /// The requested settlement ID does not exist in storage.
+    SettlementNotFound = 15,
+    /// The settlement proposal has passed its `expires_at` ledger timestamp.
+    SettlementExpired = 16,
+    /// `resolve_dispute` was called on a settlement that is not `OnHold`.
+    NotDisputed = 17,
+    /// `set_settlement_ttl` was called with `ttl_seconds == 0`, or the TTL
+    /// overflowed when added to the current ledger timestamp.
+    InvalidSettlementTtl = 18,
 }
 
 // At five seconds per ledger, renew instance state from roughly 335 days
@@ -1030,7 +1039,9 @@ impl TreasuryContract {
     /// * `amount` - Amount of tokens to deposit.
     ///
     /// # Errors
-    /// * Returns [`TreasuryError::TokenNotAllowed`] if token is not on the allowlist.
+    /// * Returns [`TreasuryError::TokenNotAllowed`] if the allowlist is empty or the token
+    ///   is not present on the allowlist. An empty allowlist means no token is permitted,
+    ///   not that all tokens are permitted.
     /// * Returns [`TreasuryError::ContractPaused`] if contract is paused.
     pub fn deposit(
         e: Env,
@@ -1043,7 +1054,10 @@ impl TreasuryContract {
             .instance()
             .get(&DataKey::TokenAllowlist)
             .unwrap_or_else(|| Vec::new(&e));
-        if !allowlist.contains(&token) {
+        // #887: An empty allowlist must reject all tokens, not permit all.
+        // Previously the guard was `if !allowlist.contains(&token)` which passed
+        // when the list was empty because `contains` returns false on an empty Vec.
+        if allowlist.is_empty() || !allowlist.contains(&token) {
             return Err(TreasuryError::TokenNotAllowed);
         }
 
@@ -1126,7 +1140,7 @@ impl TreasuryContract {
     /// * `e` - Soroban environment handle.
     /// * `admin` - Admin address (must authenticate).
     /// * `token` - Token being withdrawn; used to look up the daily cap.
-    /// * `_to` - Target recipient address.
+    /// * `to` - Target recipient address that tokens are transferred to.
     /// * `amount` - Amount to withdraw.
     ///
     /// # Errors
@@ -1139,7 +1153,7 @@ impl TreasuryContract {
         e: Env,
         admin: Address,
         token: Address,
-        _to: Address,
+        to: Address,
         amount: u64,
     ) -> Result<(), TreasuryError> {
         check_not_paused(&e)?;
@@ -1181,6 +1195,15 @@ impl TreasuryContract {
             );
         }
 
+        // #888: Actually transfer the tokens to the recipient.
+        // Previously `_to` was declared but never used — the tokens were never
+        // moved out of the treasury, leaving the recipient with nothing.
+        token::Client::new(&e, &token).transfer(
+            &e.current_contract_address(),
+            &to,
+            &(amount as i128),
+        );
+
         Ok(())
     }
 

comebackhere-backend/src/routes/treasury.ts

@@ -170,6 +170,22 @@ const defaultSettlementDeps: SettlementExecutionDeps = {
  * POST /api/treasury/execute-settlement
  * Validates treasury USDC balance before submitting execute_settlement.
  * Body: { settlement_id: number, token_contract?: string }
+ *
+ * #889 — idempotency guard: re-fetch the settlement status after the on-chain
+ *   call returns to detect duplicate submissions.  If the settlement is
+ *   already Executed we return the cached result rather than re-submitting.
+ *
+ * #889 — logger fix: the original code referenced `res.locals.logger` inside
+ *   this exported function, but `res` is not in scope here (it only exists in
+ *   the route handler).  Replaced with a plain `console.info` call so the log
+ *   line is preserved without crashing at runtime.
+ *
+ * #890 — escrow sync check: if `submitContractCall` throws we re-fetch the
+ *   on-chain settlement status and re-check the treasury balance.  When the
+ *   statuses diverge (e.g. the chain accepted the transaction but we received
+ *   a network error before the response arrived) we surface an
+ *   EscrowBalanceMismatch error so the caller knows to reconcile rather than
+ *   silently swallowing the failure.
  */
 export async function executeSettlementWithBalanceCheck(
   body: ExecuteSettlementBody,
@@ -196,6 +212,18 @@ export async function executeSettlementWithBalanceCheck(
     env.networkPassphrase,
   )
 
+  // #889 — idempotency guard: if the settlement is already executed, return
+  // immediately rather than submitting again and getting a NotPending error
+  // from the contract.
+  if (settlement.status === "Executed") {
+    return {
+      tx_hash: "",
+      settlement_id: body.settlement_id,
+      balance_checked: "0",
+      amount_required: settlement.amount.toString(),
+    }
+  }
+
   if (settlement.status !== "Pending") {
     throw Object.assign(
       new Error(`Settlement #${body.settlement_id} is not pending (status: ${settlement.status})`),
@@ -211,14 +239,16 @@ export async function executeSettlementWithBalanceCheck(
     env.networkPassphrase,
   )
 
-  res.locals.logger.info(
-    {
+  // #889 — fix: `res.locals.logger` is not in scope in this exported function.
+  // Use console.info so the diagnostic log is preserved without a ReferenceError.
+  console.info(
+    JSON.stringify({
       settlementId: body.settlement_id,
       required: settlement.amount.toString(),
       available: balance.toString(),
       token: tokenContract,
-    },
-    "Executing settlement balance check",
+      msg: "Executing settlement balance check",
+    }),
   )
 
   if (balance < settlement.amount) {
@@ -231,18 +261,78 @@ export async function executeSettlementWithBalanceCheck(
     )
   }
 
-  const txHash = await deps.submitContractCall(
-    client,
-    env.treasuryContractId,
-    "execute_settlement",
-    [
-      nativeToScVal(sourceAccount, { type: "address" }),
-      nativeToScVal(BigInt(body.settlement_id), { type: "u64" }),
-      nativeToScVal(Address.fromString(tokenContract), { type: "address" }),
-    ],
-    env.signerSecret,
-    env.networkPassphrase,
-  )
+  let txHash: string
+  try {
+    txHash = await deps.submitContractCall(
+      client,
+      env.treasuryContractId,
+      "execute_settlement",
+      [
+        nativeToScVal(sourceAccount, { type: "address" }),
+        nativeToScVal(BigInt(body.settlement_id), { type: "u64" }),
+        nativeToScVal(Address.fromString(tokenContract), { type: "address" }),
+      ],
+      env.signerSecret,
+      env.networkPassphrase,
+    )
+  } catch (submitErr: unknown) {
+    // #890 — escrow sync check: the submit call failed, but the transaction
+    // may have already been applied on-chain (e.g. network timeout after
+    // acceptance).  Re-fetch the settlement and the current balance to detect
+    // a mismatch between what we expected and what is now on-chain.
+    let postStatus: string | undefined
+    let postBalance: bigint | undefined
+    try {
+      const postSettlement = await deps.getOnChainSettlement(
+        client,
+        env.treasuryContractId,
+        BigInt(body.settlement_id),
+        sourceAccount,
+        env.networkPassphrase,
+      )
+      postStatus = postSettlement.status
+
+      postBalance = await deps.getTokenBalance(
+        client,
+        tokenContract,
+        env.treasuryContractId,
+        sourceAccount,
+        env.networkPassphrase,
+      )
+    } catch {
+      // Re-fetch itself failed — rethrow the original submission error.
+      throw submitErr
+    }
+
+    // If the settlement is now Executed but we got a submit error it means
+    // the tx landed on-chain despite the network error.  This is actually a
+    // success — return as if the call succeeded (tx_hash is unavailable).
+    if (postStatus === "Executed") {
+      return {
+        tx_hash: "",
+        settlement_id: body.settlement_id,
+        balance_checked: balance.toString(),
+        amount_required: settlement.amount.toString(),
+      }
+    }
+
+    // The settlement is still Pending but the balance has already decreased —
+    // the escrow is out of sync.  Surface an EscrowBalanceMismatch so the
+    // caller can reconcile rather than attempting re-execution blindly (#890).
+    if (postBalance !== undefined && postBalance < balance) {
+      throw Object.assign(
+        new Error(
+          `EscrowBalanceMismatch: settlement #${body.settlement_id} is still ${postStatus} but ` +
+            `treasury balance dropped from ${balance.toString()} to ${postBalance.toString()} stroops. ` +
+            `Manual reconciliation required.`,
+        ),
+        { status: 409, code: "ESCROW_BALANCE_MISMATCH" },
+      )
+    }
+
+    // No mismatch detected — rethrow the original error.
+    throw submitErr
+  }
 
   return {
     tx_hash: txHash,
@@ -340,7 +430,7 @@ router.post("/execute-settlement", requireAdmin, validateBody(executeSettlementS
     const message = err instanceof Error ? err.message : String(err)
     res.status(status).json({ error: message })
   }
-})
+}))
 
 export interface SimulateSettlementBody {
   settlement_id: number