From 5737ec39743cfc08858d37208045010211ce7ab3 Mon Sep 17 00:00:00 2001 From: anifast-123 Date: Sun, 18 Oct 2026 02:36:02 +0000 Subject: [PATCH] fix(devops,contracts): resolve third-wave audit issues #1543-#1546 #1546 (deploy.sh errexit dead code): capture `stellar contract deploy` and `stellar contract invoke` output directly as the `if` condition (`if ! VAR=$(cmd); then`) so the custom "Deployment failed" / "Contract initialization failed" diagnostics and captured output are actually reachable under `set -e`. Verified with mocked stellar/cargo: old script exited 7 with no custom message; new script exits 1 and prints both the custom error block and the captured command output. #1545 (docs Redis vs docker-compose): the Redis service existed but its explanatory comment was attached to pgbouncer, and backend/.env.example had no local Redis URL. Restore the Redis comment, give pgbouncer its own, and pin REDIS_URL to redis://localhost:6379 (the port published by the `redis` compose service) so the documented `docker compose up -d redis` flow works from a fresh clone. #1544 (contracts self-stream tests): add test_withdraw_self_stream_credits_shared_address_once and test_cancel_self_stream_settles_both_transfers, asserting the shared sender==recipient address is credited exactly once and that accrued_amount + refunded_amount sum to the full deposit. #1543 (contracts fully-accrued cancel test): add test_cancel_fully_accrued_stream_without_withdraw_is_cancelled asserting 100% to recipient, 0 refund, and StreamStatus::Cancelled (not Completed). closes #1543 closes #1544 closes #1545 closes #1546 --- backend/.env.example | 8 +- contracts/stream_contract/src/test.rs | 116 ++++++++++++++++++++++++++ docker-compose.yml | 17 +++- docs/DEVELOPMENT.md | 7 +- scripts/deploy.sh | 19 +++-- 5 files changed, 152 insertions(+), 15 deletions(-) diff --git a/backend/.env.example b/backend/.env.example index 2ae56aff..234ad350 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -101,8 +101,12 @@ AUTH_CHALLENGE_SWEEP_INTERVAL_MS=300000 # ─── Redis (optional) ──────────────────────────────────────────────────────── # When set, enables horizontal SSE scaling via Redis pub/sub so events # emitted by any backend instance are broadcast to clients on all instances. -# Leave empty to run in single-instance mode (no Redis required). -REDIS_URL= +# Matches the Redis service published by docker-compose.yml on localhost:6379 +# (start it with `docker compose up -d redis`). If Redis is not reachable the +# backend logs a warning and falls back to single-instance SSE mode, so this is +# safe to leave set even when Redis is not running. Set empty to skip the +# connection attempt entirely. +REDIS_URL=redis://localhost:6379 # ─── Caching (optional) ─────────────────────────────────────────────────────── # Time in milliseconds between periodic sweeps to prune expired memory cache entries (default: 60000) diff --git a/contracts/stream_contract/src/test.rs b/contracts/stream_contract/src/test.rs index 3585d7f5..c1c620b8 100644 --- a/contracts/stream_contract/src/test.rs +++ b/contracts/stream_contract/src/test.rs @@ -1341,6 +1341,45 @@ fn test_cancel_stream_refunds_sender() { assert_eq!(stream.withdrawn_amount, 300); } +#[test] +fn test_cancel_fully_accrued_stream_without_withdraw_is_cancelled() { + // #1543: a stream that has fully accrued but was never withdrawn and is then + // cancelled must pay 100% of the deposit to the recipient with a zero refund + // to the sender, and end as `Cancelled` — never `Completed`, which only + // `apply_withdrawal` can set. + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + let recipient = Address::generate(&env); + mint(&env, &token, &sender, 1_000); + + let contract_id = env.register(StreamContract, ()); + let client = StreamContractClient::new(&env, &contract_id); + let token_client = token::Client::new(&env, &token); + + // 1_000 tokens over 100 seconds = 10 tokens/second. + let stream_id = client.create_stream(&sender, &recipient, &token, &1_000, &100); + + // Advance well past the full duration without ever calling withdraw. + env.ledger().with_mut(|l| l.timestamp += 10_000); + + let sender_balance_before = token_client.balance(&sender); + client.cancel_stream(&sender, &stream_id); + + // Recipient gets the entire deposit; sender gets a zero refund. + assert_eq!(token_client.balance(&recipient), 1_000); + assert_eq!(token_client.balance(&sender) - sender_balance_before, 0); + assert_eq!(token_client.balance(&contract_id), 0); + + // The cancel path must record `Cancelled`, not `Completed`. + let stream = client.get_stream(&stream_id).unwrap(); + assert_eq!(stream.status, StreamStatus::Cancelled); + assert!(!stream.is_active); + assert_eq!(stream.withdrawn_amount, 1_000); +} + #[test] fn test_cancel_stream_after_partial_withdrawal() { let env = Env::default(); @@ -1547,6 +1586,83 @@ fn test_create_stream_self_stream() { assert_eq!(s.recipient, actor); } +#[test] +fn test_withdraw_self_stream_credits_shared_address_once() { + // #1544: with sender == recipient, a withdrawal pays the same Address that + // funded the stream. The accrued amount must be credited exactly once, with + // no double-count, and claiming the remainder must total the deposit. + let env = Env::default(); + env.mock_all_auths(); + let (token, _) = create_token(&env); + let actor = Address::generate(&env); + mint(&env, &token, &actor, 1_000); + + let contract_id = env.register(StreamContract, ()); + let client = StreamContractClient::new(&env, &contract_id); + let token_client = token::Client::new(&env, &token); + + // 1_000 tokens over 1_000 s = 1 token/s. + let id = client.create_stream(&actor, &actor, &token, &1_000, &1_000); + // Creation moved the whole deposit into the contract. + assert_eq!(token_client.balance(&actor), 0); + + env.ledger().with_mut(|l| l.timestamp += 300); + let claimed = client.withdraw(&actor, &id); + + // 300 accrued is credited once — not once per "side" of the stream. + assert_eq!(claimed, 300); + assert_eq!(token_client.balance(&actor), 300); + + let s = client.get_stream(&id).unwrap(); + assert_eq!(s.withdrawn_amount, 300); + assert!(s.is_active); + + // The remaining 700 brings the shared address to the full deposit exactly. + env.ledger().with_mut(|l| l.timestamp += 700); + let rest = client.withdraw(&actor, &id); + assert_eq!(rest, 700); + assert_eq!(token_client.balance(&actor), 1_000); + + let s = client.get_stream(&id).unwrap(); + assert_eq!(s.status, StreamStatus::Completed); + assert_eq!(token_client.balance(&contract_id), 0); +} + +#[test] +fn test_cancel_self_stream_settles_both_transfers() { + // #1544: cancel_stream issues two transfers — accrued to the recipient and + // the refund to the sender. With sender == recipient both legs land on the + // same Address and must sum to exactly the deposit, with nothing lost or + // duplicated. + let env = Env::default(); + env.mock_all_auths(); + let (token, _) = create_token(&env); + let actor = Address::generate(&env); + mint(&env, &token, &actor, 1_000); + + let contract_id = env.register(StreamContract, ()); + let client = StreamContractClient::new(&env, &contract_id); + let token_client = token::Client::new(&env, &token); + + let id = client.create_stream(&actor, &actor, &token, &1_000, &1_000); + assert_eq!(token_client.balance(&actor), 0); + + env.ledger().with_mut(|l| l.timestamp += 300); + + let balance_before = token_client.balance(&actor); + client.cancel_stream(&actor, &id); + let balance_after = token_client.balance(&actor); + + // accrued (300) + refunded (700) both target `actor`: exactly the deposit. + assert_eq!(balance_after - balance_before, 1_000); + assert_eq!(token_client.balance(&contract_id), 0); + + let s = client.get_stream(&id).unwrap(); + assert!(!s.is_active); + assert_eq!(s.status, StreamStatus::Cancelled); + assert_eq!(s.withdrawn_amount, 300); +} + #[test] fn test_create_stream_zero_rate() { // amount < duration → rate_per_second rounds to 0; must now be rejected. diff --git a/docker-compose.yml b/docker-compose.yml index a3a25267..e743cbd9 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -21,10 +21,10 @@ services: timeout: 5s retries: 5 - # ── Redis ────────────────────────────────────────────────────────────────── - # Used for SSE horizontal scaling (pub/sub fan-out across backend instances) - # and rate-limit state sharing. Required for the dev:mock sandbox. - # Pinned to Redis 7 (alpine) — same upgrade policy as postgres above. + # ── PgBouncer ────────────────────────────────────────────────────────────── + # Transaction-pooling proxy in front of Postgres so the backend can hold many + # client connections without exhausting Postgres' own connection limit. The + # backend connects through this service (see DATABASE_URL below). pgbouncer: image: edoburu/pgbouncer:v1.24.1-p1 container_name: flowfi-pgbouncer @@ -52,6 +52,15 @@ services: timeout: 3s retries: 5 restart: unless-stopped + + # ── Redis ────────────────────────────────────────────────────────────────── + # Used for SSE horizontal scaling (pub/sub fan-out across backend instances) + # and rate-limit state sharing. Optional for single-instance dev, but required + # by the dev:mock sandbox. Start it standalone with: + # docker compose up -d redis + # and point the backend at it with REDIS_URL=redis://localhost:6379 + # (the value used by backend/.env.example). Pinned to Redis 7 (alpine) — same + # upgrade policy as postgres above. redis: image: redis:7-alpine@sha256:858f009f9709ce576febc734aa78b8f6d624b82571f9ddb6bda4377c833b3499 container_name: flowfi-redis diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index ede796f4..a6c2abd9 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -123,12 +123,17 @@ cd flowfi docker compose up -d postgres ``` -(Optional for SSE fanout testing) +Optional — start Redis for multi-instance SSE fanout testing: ```bash docker compose up -d redis ``` +The Redis service in `docker-compose.yml` is published on +`localhost:6379`, which is the value set for `REDIS_URL` in +`backend/.env.example` (`redis://localhost:6379`), so no extra configuration +is needed to reach it. + --- ### 3. Backend setup diff --git a/scripts/deploy.sh b/scripts/deploy.sh index 9ccc006e..d6c38e01 100755 --- a/scripts/deploy.sh +++ b/scripts/deploy.sh @@ -149,13 +149,16 @@ echo "" # Step 3: Deploy contract echo -e "${BLUE}Step 3: Deploying contract to $NETWORK...${NC}" -DEPLOY_OUTPUT=$(stellar contract deploy \ +# NOTE: The command substitution is used directly as the `if` condition rather +# than assigned first and checked with `$?`. Under `set -e`, a failing command +# substitution in a *plain* assignment (`VAR=$(cmd)`) aborts the script +# immediately, so a following `if [[ $? -ne 0 ]]` would be dead code and the +# custom diagnostic below would never print. +if ! DEPLOY_OUTPUT=$(stellar contract deploy \ --wasm "$OPTIMIZED_WASM_PATH" \ --source "$DEPLOYER_SECRET" \ --network "$NETWORK" \ - --network-passphrase "$NETWORK_PASSPHRASE" 2>&1) - -if [[ $? -ne 0 ]]; then + --network-passphrase "$NETWORK_PASSPHRASE" 2>&1); then echo -e "${RED}Error: Deployment failed${NC}" echo "$DEPLOY_OUTPUT" exit 1 @@ -174,7 +177,9 @@ echo "" # Step 4: Initialize contract echo -e "${BLUE}Step 4: Initializing contract...${NC}" -INIT_OUTPUT=$(stellar contract invoke \ +# Same errexit-safe pattern as the deploy step above: the assignment is the +# `if` condition, so a failure is caught here instead of aborting the script. +if ! INIT_OUTPUT=$(stellar contract invoke \ --id "$CONTRACT_ID" \ --source "$DEPLOYER_SECRET" \ --network "$NETWORK" \ @@ -182,9 +187,7 @@ INIT_OUTPUT=$(stellar contract invoke \ initialize \ --admin "$ADMIN_ADDRESS" \ --treasury "$TREASURY_ADDRESS" \ - --fee_rate_bps "$FEE_RATE_BPS" 2>&1) - -if [[ $? -ne 0 ]]; then + --fee_rate_bps "$FEE_RATE_BPS" 2>&1); then echo -e "${RED}Error: Contract initialization failed${NC}" echo "$INIT_OUTPUT" exit 1