From 0c649fd2164f04d5893e8badf1f572612bdac839 Mon Sep 17 00:00:00 2001 From: ahmadrabiumustapha Date: Mon, 19 Oct 2026 09:06:54 +0000 Subject: [PATCH] fix: resolve issues #887 #888 #889 #890 in treasury contract and backend #887 - deposit() now rejects all tokens when the allowlist is empty. Previously the guard was `if !allowlist.contains(&token)` which passed on an empty Vec, allowing any token. Changed to `if allowlist.is_empty() || !allowlist.contains(&token)`. #888 - withdraw() now actually transfers tokens to the recipient. The `_to` parameter was declared but never used; the token transfer call was missing entirely. Renamed to `to` and added `token::Client::new(&e, &token).transfer(..., &to, &(amount as i128))`. #889 - executeSettlementWithBalanceCheck: fixed runtime crash caused by `res.locals.logger` reference in the exported function (res is not in scope outside the route handler). Replaced with `console.info`. Added idempotency guard: if settlement.status === 'Executed' return early instead of re-submitting. #890 - After a failed submitContractCall the handler now re-fetches the on-chain settlement status and treasury balance. If the balance dropped but the settlement is still Pending it surfaces an EscrowBalanceMismatch error (409) so the caller can reconcile rather than blindly re-executing. Also: added missing TreasuryError variants to the enum (SettlementNotFound=15, SettlementExpired=16, NotDisputed=17, InvalidSettlementTtl=18) that were referenced in contract code but absent from the declaration, causing a compile-time error. Fixed missing closing paren on asyncHandler call in the execute-settlement route. Deduplicated .gitignore (removed repeated entries for playwright-report, playwright/.auth, vitest-report, .vite/, coverage/, html/, lcov.info). --- .gitignore | 14 +- .../contracts/treasury/src/lib.rs | 31 ++++- comebackhere-backend/src/routes/treasury.ts | 124 +++++++++++++++--- 3 files changed, 137 insertions(+), 32 deletions(-) diff --git a/.gitignore b/.gitignore index aed438a..e0a8906 100644 --- a/.gitignore +++ b/.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/ diff --git a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs b/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs index 5d1e097..44d8329 100644 --- a/COMEBACKHERE-contracts/contracts/treasury/src/lib.rs +++ b/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(()) } diff --git a/comebackhere-backend/src/routes/treasury.ts b/comebackhere-backend/src/routes/treasury.ts index 5929ffe..1153a9b 100644 --- a/comebackhere-backend/src/routes/treasury.ts +++ b/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