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