From f0aa51410029c9f3e53cfc7249f081657e0f456e Mon Sep 17 00:00:00 2001 From: dnnyorji Date: Tue, 20 Oct 2026 02:10:15 +0100 Subject: [PATCH 1/4] docs(contracts): add state archival and TTL management reference Documents that none of the three contracts ever extend TTLs, so every instance-storage key is at risk of archival, and gives operators the stellar CLI commands to extend or restore before/after it happens. --- contracts/docs/concepts-state-archival.md | 227 ++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 contracts/docs/concepts-state-archival.md diff --git a/contracts/docs/concepts-state-archival.md b/contracts/docs/concepts-state-archival.md new file mode 100644 index 0000000..7ab83d6 --- /dev/null +++ b/contracts/docs/concepts-state-archival.md @@ -0,0 +1,227 @@ +# State Archival & TTL Management + +Soroban rents ledger state. Every storage entry a contract writes carries a **time-to-live +(TTL)**, measured in ledgers remaining until it is eligible for **archival** — removal from +the live ledger's BucketList into cold storage, from which it must be explicitly **restored** +before it can be read or written again. A contract that lets a live key's TTL run out does not +lose the data (archival is not deletion), but it does become unusable: any call that touches +an archived entry fails until an operator restores it. + +This matters most for `group_treasury`: if `DataKey::Members` or an open `DataKey::Proposal` +archives, the treasury cannot authorize withdrawals, cannot accept votes, and cannot even +answer `get_members()` until someone restores the entry. This document covers how each of the +three contracts in this workspace uses storage, how (or whether) each extends TTLs, and what an +operator does when something archives anyway. + +Source reviewed: `contracts/contracts/{token_transfer,group_treasury,proposals}/src/*.rs`, +verified by searching every contract source file for `extend_ttl`, `.bump(`, and `restore` — +none of the three contracts call any of them (see [Findings](#findings-no-contract-extends-any-ttl)). + +Related: [`contracts-token-transfer-storage.md`](contracts-token-transfer-storage.md), +[`contracts-group-treasury-storage.md`](contracts-group-treasury-storage.md), +[`concepts-resource-budget.md`](concepts-resource-budget.md) (TTL/rent as one of several +metered resources), [`api-deployment-invocation.md`](api-deployment-invocation.md) (the +Stellar CLI conventions this document extends). + +--- + +## Soroban storage and archival, briefly + +Soroban's `env.storage()` exposes three *storage types*, but only **two archival durability +classes**: + +| Storage type (`env.storage().X()`) | Archival durability | TTL behaviour | +| --- | --- | --- | +| `instance()` | **Persistent** | Shares one TTL with the contract's instance entry (and the contract's Wasm code entry). If it expires, the *entire* contract instance — every key stored via `instance()` — archives together. | +| `persistent()` | **Persistent** | Each keyed entry has its own independent TTL. | +| `temporary()` | **Temporary** | Expires and is permanently deleted (not archived-but-recoverable) once its TTL reaches zero. Never used to recover old data. | + +A new entry starts with a TTL set by the network's minimum-TTL configuration for its +durability class. Every ledger that passes without action decrements that countdown. A +contract extends an entry's TTL explicitly, by calling `extend_ttl` (on a `Persistent`/` +Temporary` key via `env.storage().persistent().extend_ttl(key, threshold, extend_to)`, or on +the whole instance via `env.storage().instance().extend_ttl(threshold, extend_to)`) — there is +no automatic extension on read or write. If the TTL reaches zero before anything extends it, +the entry **archives**: it is still provably on the ledger (restorable, not deleted), but any +transaction that reads or writes it fails until it is restored. `temporary()` entries do not +archive — they are gone for good once their TTL expires, which is why nothing in this workspace +uses temporary storage for data that must survive. + +## Findings: no contract extends any TTL + +All three contracts store every key through `env.storage().instance()` exclusively. None of +them call `.extend_ttl()`, `.bump()`, or any TTL-related API, anywhere in `lib.rs` or +`storage.rs`: + +```bash +grep -rniE "extend_ttl|\.bump\(|restore" contracts/{token_transfer,group_treasury,proposals}/src/*.rs +# (no matches) +``` + +That means TTL management for every key documented below is **entirely external to the +contract code** — it depends on whatever extends the contract's instance entry from outside +(an operator's maintenance transaction, a deploy/invoke call that happens to touch the +instance, or nothing at all). None of the deploy scripts in `contracts/scripts/` call +`extend` either, so a freshly deployed contract's only TTL activity is whatever minimum the +network assigns at creation and whatever `stellar contract invoke` calls happen to do +implicitly (simulated-and-submitted invocations typically refresh the footprint's TTL as part +of normal fee/resource accounting on write, but this is network/RPC behaviour, not something +this codebase controls or can rely on at a chosen interval). + +## Per-contract storage key inventory + +**Durability column meaning:** every key below is `instance()` storage, so "Durability" is +`Persistent` for all of them per the table above — the column is kept to make it explicit this +was checked per key, not assumed from the contract once. + +### `token_transfer` + +| Key | Durability | Where TTL is extended | +| --- | --- | --- | +| `DataKey::Admin` | Persistent (instance) | **Nowhere.** No call in `lib.rs` extends it. | +| `DataKey::TokenContract` | Persistent (instance) | **Nowhere.** | + +Source: `contracts/token_transfer/src/{lib,storage}.rs`. Two keys, both instance, both never +explicitly extended — see [`contracts-token-transfer-storage.md`](contracts-token-transfer-storage.md#storage-type-usage) +for the storage-tier rationale (not the TTL gap, which that document does not cover). + +### `group_treasury` + +| Key | Durability | Where TTL is extended | +| --- | --- | --- | +| `DataKey::Admin` | Persistent (instance) | **Nowhere.** | +| `DataKey::Balances` | Persistent (instance) | **Nowhere.** | +| `DataKey::Members` | Persistent (instance) | **Nowhere.** | +| `DataKey::Threshold` | Persistent (instance) | **Nowhere.** | +| `DataKey::ProposalCount` | Persistent (instance) | **Nowhere.** | +| `DataKey::Proposal(u32)` | Persistent (instance) | **Nowhere.** | +| `DataKey::Vote(u32, Address)` | Persistent (instance) | **Nowhere.** | + +Source: `contracts/group_treasury/src/{lib,storage}.rs`, per +[`contracts-group-treasury-storage.md`](contracts-group-treasury-storage.md#storage-tier-everything-is-instance-storage). +Because every key shares the single instance TTL, **there is no per-key risk gradient here** — +`Members` and `Proposal(id)` archive at exactly the same moment as `Admin`, because they are +the same ledger entry underneath `DataKey` variants. This is the contract named in the task +description: if the instance entry archives, `get_members()`, `propose_withdraw()`, and every +vote function fail until restored, because none of them can be called without first reading +`DataKey::Admin` or `DataKey::Members` from the same archived instance. + +### `proposals` + +| Key | Durability | Where TTL is extended | +| --- | --- | --- | +| `DataKey::Admin` | Persistent (instance) | **Nowhere.** | +| `DataKey::NextProposalId` | Persistent (instance) | **Nowhere.** | +| `DataKey::Proposal(u64)` | Persistent (instance) | **Nowhere.** | +| `DataKey::Vote(u64, Address)` | Persistent (instance) | **Nowhere.** | + +Source: `contracts/proposals/src/{lib,storage}.rs`. + +## TTL thresholds and extension amounts: none are configured + +There are **no TTL threshold or extension-amount constants anywhere in this codebase** — no +`extend_ttl` call exists to carry them. This section exists to document that absence +explicitly, and to head off a likely point of confusion: + +**Do not confuse storage TTL with `group_treasury`'s `ttl_ledgers` parameter.** +`propose_withdraw(proposer, to, token, amount, ttl_ledgers)` takes a caller-supplied +`ttl_ledgers: u32` and computes `expires_at = env.ledger().timestamp() + (ttl_ledgers as u64 * +5)` — this is **proposal voting-window duration**, a business-logic field on +`WithdrawProposal`, converted from ledgers to seconds at the contract's documented ~5 +seconds/ledger assumption. It has nothing to do with Soroban's storage-entry TTL or archival. +The backend's `apps/backend/docs/api-treasury.md` uses the same "TTL" word for the same +ledger→seconds conversion on its own `/treasury/propose` endpoint, which reinforces the +naming collision rather than resolving it — when reading "TTL" anywhere in this codebase, +confirm from context which of the two unrelated concepts is meant. + +Because no contract calls `extend_ttl`, there is no "amount this workspace extends by" to +report, and no reasoning to document for a choice that was never made. What *would* need +choosing, if this gap were closed, and the reasoning each choice should weigh: + +- **Threshold** (the `threshold` argument to `extend_ttl`, i.e. "extend once the entry has + this many ledgers of TTL left") — should be comfortably larger than the longest expected gap + between transactions that touch the instance, so routine inactivity never crosses it + unnoticed. +- **`extend_to`** (how far out to push the TTL) — trades off against cost: Soroban charges + rent proportional to entry size × extension length, so extending `group_treasury`'s instance + (which grows without bound as proposals and votes accumulate, per + [`concepts-resource-budget.md`](concepts-resource-budget.md#size-is-not-the-only-budget)) by + a large amount becomes proportionally more expensive as the treasury accumulates history. +- Network-level minimum/maximum TTL bounds for persistent entries are a live, queryable + network parameter (`ConfigSettingStateArchival` in Soroban's ledger config), not a constant + safe to hard-code in a doc — query the current values from the target network's RPC rather + than trusting a number written here, which could drift out of date. + +## Risk: keys written but never extended + +Every key in every contract in this workspace falls into this bucket — there is no subset to +carve out as "the risky ones," because none of the three contracts extend anything. Stated as +risks: + +| Risk | Contract(s) affected | Consequence | +| --- | --- | --- | +| Instance entry (all storage) archives from pure inactivity | `token_transfer`, `group_treasury`, `proposals` | Every function panics/fails at the host level until an operator restores the instance. For `token_transfer`, this stops all transfers routed through it. For `group_treasury`, members cannot vote, deposit, or withdraw, and `is_member`/`get_members` — called by the external `proposals` contract's `execute_withdraw` — also fail, so a stalled treasury instance can block the `proposals` contract's execution path too. | +| No code path re-extends TTL on the operations that matter most | `group_treasury` especially | A treasury with no activity for a network-defined minimum period (an inactive group, or a group that only reads balances via simulated/`--send=no` calls, which do not extend TTL) drifts toward archival with no in-contract signal. There is no `ping()`/`keep_alive()` function a cron job could call to safely extend TTL without otherwise mutating state. | +| Unbounded instance growth raises the cost of recovery | `group_treasury`, `proposals` | Per `concepts-resource-budget.md`, both contracts keep every proposal and every vote in the same instance entry forever. If that entry archives, the restore transaction's cost scales with the *entire* accumulated entry size, not just the keys currently needed — a long-lived, heavily-used treasury is the most valuable to keep alive and the most expensive to recover if it lapses. | +| Deploy scripts do not establish a baseline extension | `contracts/scripts/deploy_*.sh` | A freshly deployed contract starts at whatever TTL the network assigns on creation and is never pushed further out by the deploy flow itself — the "someone extends this on a schedule" assumption has no owner anywhere in this repo today. | + +**There is no mitigation in this codebase today.** This is an open gap, not a documented +design trade-off — nothing in `lib.rs`, `storage.rs`, or `contracts/scripts/` suggests this was +a deliberate choice. Closing it needs either an admin-gated `extend_ttl` entrypoint on each +contract (callable by automation without otherwise mutating state) or an operational process +that invokes the CLI commands below on a schedule tighter than the network's minimum persistent +TTL. + +## Operator recovery: extending and restoring with the Stellar CLI + +These commands follow the same invocation conventions as +[`api-deployment-invocation.md`](api-deployment-invocation.md) — `--source`, `--network`, and +`--id` resolve identities and the target network the same way. Soroban's archival durability +only has two classes (`persistent`, which covers both `instance()` and `persistent()` storage, +and `temporary()`); every key in this workspace is `persistent`. + +**Extend before archival** (routine maintenance — run this periodically, well before the +network's minimum TTL would otherwise be reached, since nothing in the contracts does this +automatically): + +```bash +stellar contract extend \ + --id $GROUP_TREASURY_CONTRACT_ID \ + --source deployer \ + --network testnet \ + --durability persistent \ + --ledgers-to-extend 535679 +``` + +Omitting `--key`/`--key-xdr` extends the contract's own instance entry (and its Wasm code +entry) — the right target here, since every `DataKey` in this workspace lives inside that one +instance entry, not as independent `persistent()` keys. + +**Restore after archival** (the entry has already lapsed and every call is failing): + +```bash +stellar contract restore \ + --id $GROUP_TREASURY_CONTRACT_ID \ + --source deployer \ + --network testnet \ + --durability persistent +``` + +This resubmits the archived entry's current (last-known) state to the ledger and resets its +TTL, after which normal `invoke` calls succeed again. The account passed via `--source` pays +the restoration fee, which scales with the size of what is being restored — see the +unbounded-growth risk above for why this cost is not fixed over a treasury's lifetime. + +Both subcommands' exact flags have changed across Stellar CLI releases (notably around how +`--ledgers-to-extend` and `--durability` are specified); confirm against the installed CLI +with `stellar contract extend --help` / `stellar contract restore --help` before relying on +the invocations above verbatim, rather than assuming they match every CLI version. + +## What this document does not claim + +- It does not assert specific network minimum/maximum TTL ledger counts — those are live + network configuration, not a constant of this codebase, and are called out above as + something to query rather than hard-code. +- It does not claim archival has been observed in testing or production for any of these + contracts — the finding here is a static-analysis result (no `extend_ttl` call exists in the + source), not an incident report. From 4d74a64f7837cec6b24bf3efa9a7f4e198474a76 Mon Sep 17 00:00:00 2001 From: dnnyorji Date: Tue, 20 Oct 2026 02:11:37 +0100 Subject: [PATCH 2/4] docs(user-guide): add devices, safety numbers, and privacy guide Plain-language companion to threat-model.md covering device linking and revocation, safety-number verification, why a newly linked device can't read old group history, and the AI assistant's E2EE exception. --- docs/user-guide/devices-and-privacy.md | 224 +++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 docs/user-guide/devices-and-privacy.md diff --git a/docs/user-guide/devices-and-privacy.md b/docs/user-guide/devices-and-privacy.md new file mode 100644 index 0000000..3e965dd --- /dev/null +++ b/docs/user-guide/devices-and-privacy.md @@ -0,0 +1,224 @@ +# Devices, safety numbers, and privacy + +A plain-language guide to what Clicked's encryption actually protects, how to manage the +devices signed in to your account, how to confirm you're really talking to who you think you +are, and what to do when something looks off. + +This is the non-technical companion to [`docs/threat-model.md`](../threat-model.md), which is +written for engineers and auditors. Everything in the first section below is meant to agree +with that document — if the two ever seem to say different things, the threat model is the +more precise, more current source, and this page should be corrected to match it. + +--- + +## What Clicked can and cannot see + +Clicked encrypts your messages end-to-end. In practice that means two very different things +are true at the same time, and it matters which one applies to any given piece of information. + +### Clicked cannot read your messages + +When you send a text message, a photo, a video, or a file, it is scrambled (encrypted) on +your own device before it ever leaves it. It only becomes readable again on the recipient's +device. Clicked's servers move that scrambled data around, but they never hold the key needed +to unscramble it — that key only ever exists on your devices and the devices of the people +you're talking to. + +This covers: + +- The actual words, images, and files in your messages. +- The keys that would be needed to decrypt them. Clicked's servers have no field in any + database table that could even hold one of your private keys — it isn't a policy decision + that could be changed by a setting, it's that the capability was never built. +- The internal "session" state your app keeps to encrypt and decrypt — that lives only on + your device. + +### Clicked does see some things — this is normal for any messaging app + +For Clicked to deliver a message to the right person, at the right time, on the right device, +it has to know some things in plain, unencrypted form. This is true of every messaging service +that routes messages between people, not a Clicked-specific gap: + +| What Clicked sees | Why it has to | +| --- | --- | +| Who you're messaging, and who's in a group with you | It has to know where to deliver a message | +| When you send a message, and roughly how big it is | Delivery and storage both need this; a large file is visibly large even though its contents aren't readable | +| Whether a message is text, an image, a file, etc. | Used to show the right icon/preview before anything is decrypted | +| Whether you're online, and when you were last active | This is how "online" status and delivery receipts work — you can turn your own visibility off in your privacy settings, but Clicked's servers still see it to run the feature | +| The public keys your devices use to start encrypted conversations | These are *public* by design — sharing them is how two devices agree on a shared secret in the first place. Your *private* keys never leave your device | +| Your device names and device type (phone, web, etc.) | Shown to you on the [devices page](#linking-and-revoking-devices) so you can recognize your own devices | +| Your wallet address | Used to sign you in and to identify you for on-chain treasury and payment features | + +None of this lets Clicked read what you actually said. But taken together, this kind of +information — called **metadata** — can still reveal a lot: who you talk to, how often, and +roughly when, even without anyone reading a single message. If someone fully compromised +Clicked's servers, this is the kind of information they would get access to; they still would +not get your past message contents without separately compromising one of your devices too. + +### The one deliberate exception: the AI assistant + +Clicked's AI assistant is a chat feature you can ask questions. **Anything you send to the AI +assistant is not end-to-end encrypted.** The question you type is sent as plain, readable text +to Clicked's backend and from there to an outside AI provider (OpenAI) to generate a reply — +the same way the question would be visible if you emailed it to a support team. The assistant's +reply is also stored in a way Clicked's own servers can read (#363). + +Use the AI assistant for general questions about how Clicked works — don't use it to ask +questions that contain anything you wouldn't want outside the encrypted part of the app, the +same way you wouldn't paste a sensitive message into a generic search engine. + +### What this doesn't protect you from + +Encryption protects the content of your conversations. It does not: + +- Hide *that* you're using Clicked, or *who* you talk to, from Clicked itself (see the metadata + table above). +- Protect you if someone else has physical access to your already-unlocked device — anyone + looking at your unlocked phone can read messages the same way you can. +- Protect messages after they're decrypted and displayed — a screenshot, or someone reading + over your shoulder, bypasses encryption entirely, because at that point the message is just + text on a screen. + +--- + +## Linking and revoking devices + +You can be signed in to Clicked on more than one device at once (phone, web browser, etc.). +Each one is tracked separately so you can see exactly what's signed in and shut off any device +you don't recognize or no longer use. + +### Linking a new device + +There's no separate "pairing" step — open Clicked on the new device and connect the same +wallet you use everywhere else. The device registers itself automatically and appears in your +device list (**Settings → Linked Devices**) the moment it signs in. + +### Seeing what's linked + +Your device list shows, for every device signed in to your account: + +- A name and device type (web, iOS, Android) +- When it was last active +- Whether it's the device you're currently using + +### Revoking a device + +If you lose a device, retire an old one, or see one you don't recognize, revoke it from +**Settings → Linked Devices**. Revoking: + +- Signs that device out immediately. +- Rotates the keys that device was using, so it can no longer decrypt any *new* messages sent + to you after the revocation. + +You can also **log out everywhere else** in one action, which revokes every device except the +one you're currently using — useful if you're not sure which specific device to revoke, or +you believe your account was accessed from somewhere you don't recognize. + +**A key-change notice afterward is expected, not a sign of a problem.** Revoking a device +changes the keys associated with your account. The next time someone messages you, their app +may show them a notice that your keys changed — that's the system working correctly, confirming +the old, revoked device genuinely can no longer read new messages. See +[Verifying a contact's safety number](#verifying-a-contacts-safety-number) below for what to do +if *you* see that notice about someone else. + +### Why a newly linked device can't read older group history + +This surprises people the first time they see it: link a new device, open a group conversation +you were already part of, and the messages from before you linked that device aren't there — +they show as unavailable on that device specifically, not as an error. + +This is a direct consequence of how group encryption works, not a bug or a missing sync step. +Group conversations derive a fresh encryption key every time the group's membership changes +(someone joins, someone leaves, a device is added). A device only receives the key material +starting from the point it joined — there is no copy of the old keys sitting on Clicked's +servers that could be handed to it, because Clicked's servers never had them either. The same +property is what makes removing a device from a group meaningful: if a newly added device +could reach backward and decrypt everything before it joined, so could a device that was +removed and then sneakily re-added, which would defeat the point of removing it. + +In short: a new device sees everything from the moment it joined onward, and nothing before — +by design, as the price of the removal/rotation step actually being secure. Your other +devices, and the other group members, still have that older history; it isn't lost, just not +visible on that particular new device. + +### After losing a device + +If a phone or laptop with Clicked signed in is lost or stolen: + +1. **Revoke it immediately** from another device, via **Settings → Linked Devices**, or use + **log out everywhere else** if you're not sure the lost device is the only one you no longer + trust. +2. If you can't get to another signed-in device, sign in again on a device you do trust — this + issues you a new device identity, and you can revoke the lost one from there once you can + see it in the list. +3. Expect the people you talk to most to briefly see a "security code changed" notice the next + time you message them from your remaining devices — this is them seeing evidence that your + device set changed, which is exactly what should happen. Nothing further is required on + their end beyond the ordinary verification described below, and it does not mean they were + compromised. +4. Revoking does not retroactively protect messages the lost device already decrypted and + displayed before you revoked it — if someone has the unlocked device itself, revocation + stops *new* messages, not what was already on the screen. Treat a lost, unlocked device the + same way you'd treat losing an unlocked phone for any other sensitive account. + +--- + +## Verifying a contact's safety number + +A **safety number** is a long string of digits that's computed from the encryption keys of a +specific person's devices. Its only job is to answer one question: *am I really encrypting to +the devices this person controls, or has something changed underneath me?* + +### Why this exists + +Every messaging app that does key exchange through a central server has the same theoretical +weak point: the server is the one telling you which public keys belong to your contact. If a +server were ever compromised in a very specific way, it could in principle try to hand you the +wrong keys without you noticing. Comparing safety numbers closes that gap, because it gives you +a way to confirm, independently of the server, that you and your contact are both looking at +the same keys. + +For most people, most of the time, this isn't something you need to actively think about — +Clicked checks it automatically on your behalf (see the next section). Explicit verification +matters most for conversations where confirming identity really counts. + +### How to verify + +1. Open the conversation with the contact you want to verify. +2. Open **Safety numbers** for that conversation. +3. Compare the number shown with the same number on your contact's device — read it aloud on a + call, compare it in person, or send it over a different channel than Clicked itself (the + point is to use a path other than the one you're trying to verify). +4. If it matches, tap **Mark verified**. + +### "Safety number changed" / "security code changed" + +Clicked automatically remembers the keys it has seen for each of your contacts. If those keys +ever change — because your contact linked a new device, revoked an old one, or (far less +likely) something is actually wrong — you'll see a **"Safety number changed"** notice the next +time you'd otherwise send them a message, and sending is paused until you deal with it. + +What to do: + +1. **Don't panic — check the ordinary explanation first.** The overwhelmingly common cause is + that your contact linked or revoked a device, which is completely normal account maintenance + on their end. Ask them, through any channel, whether they recently added or removed a + device. +2. **Re-verify the safety number** using the steps above, against their *current* devices. +3. Once it matches, tap **Mark verified** (or re-verify) to clear the warning and resume + sending. Until you do, Clicked deliberately keeps refusing to send — it fails closed rather + than silently encrypting to keys it hasn't confirmed, because a changed key is exactly the + situation this check exists to catch. + +If your contact says they *didn't* link or revoke anything and the number still doesn't match +after checking again, treat that as a real signal something is wrong and stop sending sensitive +information in that conversation until it's resolved. + +--- + +## See also + +- [`docs/threat-model.md`](../threat-model.md) — the technical version of the "what Clicked can + and cannot see" section above, written for engineers. +- [`docs/data-retention.md`](../data-retention.md) — how long your messages, devices, and other + data are kept, and what deleting something does and doesn't remove. From 266830f600cce3cd00e1b585153ecb94c146a0dd Mon Sep 17 00:00:00 2001 From: dnnyorji Date: Tue, 20 Oct 2026 02:14:23 +0100 Subject: [PATCH 3/4] docs(contracts): add security review checklist, link from PR template Walks through require_auth coverage, init front-running vs. re-initialization, admin key powers, i128 overflow, zero/negative amounts, threshold edge cases, double voting, and storage archival for all three contracts, citing the exact code or test behind each answer and marking unproven items as open findings. Adds a contracts checklist section to the PR template that links it. --- .github/pull_request_template.md | 5 + contracts/docs/security-checklist.md | 256 +++++++++++++++++++++++++++ 2 files changed, 261 insertions(+) create mode 100644 contracts/docs/security-checklist.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 11284f5..6731736 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -14,3 +14,8 @@ - [ ] I have read the contributing guidelines - [ ] I have tested my changes locally - [ ] My code follows the project's coding standards + +## Contracts (only if this PR touches `contracts/`) + +- [ ] I have worked through [`contracts/docs/security-checklist.md`](../contracts/docs/security-checklist.md) for every entry point this PR adds or changes +- [ ] Any item the checklist marks as an open finding that this PR touches is called out above, not silently re-approved diff --git a/contracts/docs/security-checklist.md b/contracts/docs/security-checklist.md new file mode 100644 index 0000000..f35318b --- /dev/null +++ b/contracts/docs/security-checklist.md @@ -0,0 +1,256 @@ +# Contract Security Review Checklist + +The checklist a reviewer works through on any PR that touches +`contracts/contracts/{token_transfer,group_treasury,proposals}`. Each item below is answered +for all three contracts as they stand today, with a link to the exact code or test that proves +the answer — or, where nothing proves it, marked **Open finding** rather than assumed safe. + +This is a point-in-time audit, not a standing guarantee. Re-check an item against the diff +whenever a PR touches the function(s) it covers; a passing answer here does not transfer to +code written after this document. + +**Read this first if you're about to write or review a test:** a test built on +`env.mock_all_auths()` proves that *some* authorization mock exists for the call — not that the +*correct* address is the one required. A `require_auth()` call can be deleted entirely from a +function and every `mock_all_auths()`-based test will keep passing. Every "Pass" below that +relies on a test cites either a scoped `mock_auths()` / `env.auths()` assertion, an unmocked +`should_panic` test, or (for the init-front-running items) a test that demonstrates the +*absence* of a check by succeeding with no mocked auth at all — never a blanket +`mock_all_auths()` test standing alone. See +[`testing.md`](testing.md#testing-authorization) (#348) for the full explanation and the +three-tier pattern this review follows. + +--- + +## 1. `require_auth` on every state-changing entry point + +| Function | `token_transfer` | `group_treasury` | `proposals` | +| --- | --- | --- | --- | +| `initialize` | ❌ No auth at all — see [§2](#2-initialization-front-running-and-re-initialization) | ❌ No auth at all — see [§2](#2-initialization-front-running-and-re-initialization) | ✅ `admin.require_auth()` — [`lib.rs:45`](../contracts/proposals/src/lib.rs#L45) | +| `transfer` | ✅ `from.require_auth()` — [`lib.rs:38`](../contracts/token_transfer/src/lib.rs#L38), proven by `test_transfer_requires_sender_auth` ([`test.rs:124`](../contracts/token_transfer/src/test.rs#L124)) | — | — | +| `set_token_contract` | ✅ `admin.require_auth()` via loaded admin — [`lib.rs:86`](../contracts/token_transfer/src/lib.rs#L86) | — | — | +| `upgrade` | ✅ `admin.require_auth()` — [`lib.rs:104`](../contracts/token_transfer/src/lib.rs#L104), proven by `test_upgrade_non_admin_panics` ([`test.rs:174`](../contracts/token_transfer/src/test.rs#L174)) | — | — | +| `add_member` / `remove_member` | — | ✅ `require_admin()` → `admin.require_auth()` — [`lib.rs:21`](../contracts/group_treasury/src/lib.rs#L21), proven by `test_non_admin_cannot_add_member` ([`test.rs:258`](../contracts/group_treasury/src/test.rs#L258)) | — | +| `deposit` | — | ✅ `from.require_auth()` — [`lib.rs:154`](../contracts/group_treasury/src/lib.rs#L154) | — | +| `withdraw` | — | ✅ `require_admin()` — [`lib.rs:180`](../contracts/group_treasury/src/lib.rs#L180), proven by `test_non_admin_cannot_withdraw` ([`test.rs:158`](../contracts/group_treasury/src/test.rs#L158)) | — | +| `propose_withdraw` | — | ✅ `proposer.require_auth()` — [`lib.rs:225`](../contracts/group_treasury/src/lib.rs#L225), proven by `test_propose_withdraw_non_member_panics` ([`test.rs:784`](../contracts/group_treasury/src/test.rs#L784)) (membership check; see note below on what this proves) | — | +| `approve_withdraw` / `reject_withdraw` | — | ✅ voter `require_auth()` via `require_votable` — [`lib.rs:434`](../contracts/group_treasury/src/lib.rs#L434), proven unmocked by `test_vote_without_auth_panics` ([`test.rs:579`](../contracts/group_treasury/src/test.rs#L579)) | — | +| `create_proposal` | — | — | ✅ `proposer.require_auth()` — [`lib.rs:64`](../contracts/proposals/src/lib.rs#L64) | +| `vote` | — | — | ✅ `voter.require_auth()` — [`lib.rs:119`](../contracts/proposals/src/lib.rs#L119), proven by `double_vote_panics` style tests combined with the auth-presence pattern in `testing.md` | +| `finalize_proposal` / `finalize_expired_proposal` | — | — | ⚪ **Intentionally no auth.** Doc comment states "callable by anyone" — [`lib.rs:154`](../contracts/proposals/src/lib.rs#L154), [`lib.rs:192`](../contracts/proposals/src/lib.rs#L192). Gated by time (`expires_at`) and by the existing vote tally, not by caller identity, and moves no funds — reviewed as a deliberate design choice, not a gap. Confirm this reasoning still holds if either function is ever changed to do more than flip a status. | +| `execute_proposal` | — | — | ✅ `executor.require_auth()` — [`lib.rs:219`](../contracts/proposals/src/lib.rs#L219). Note: authenticates *that someone signed*, not that the signer is privileged — anyone may execute a `Passed` proposal. This is load-bearing only for the MVP path, which moves no funds (see [§3](#3-admin-key-powers)). | +| `execute_withdraw` | — | — | ✅ `caller.require_auth()` plus a separate `treasury.is_member(caller)` check — [`lib.rs:246-263`](../contracts/proposals/src/lib.rs#L246-L263), proven by `execute_withdraw_non_member_panics` ([`test.rs:450`](../contracts/proposals/src/test.rs#L450)) | + +**Note on `propose_withdraw`'s proof:** `test_propose_withdraw_non_member_panics` proves the +*membership* check panics for a non-member — it does not, by itself, distinguish "no auth +check exists" from "auth check exists but membership failed first." Combined with +`test_vote_without_auth_panics` existing as a *separate, deliberately unmocked* test for the +sibling voting functions, the pattern in this contract's test suite is consistent enough to +trust, but a reviewer adding a new `require_auth()`-gated function here should add an unmocked +test for it specifically, not rely on the membership check to stand in for one. + +--- + +## 2. Initialization front-running and re-initialization + +**Re-initialization** (calling `initialize` a second time) is guarded in all three contracts +by the same pattern — `if env.storage().instance().has(&DataKey::Admin) { panic!("already +initialized"); }` — and verified for two of the three: + +| Contract | Re-init guard | Test | +| --- | --- | --- | +| `token_transfer` | ✅ [`lib.rs:19-21`](../contracts/token_transfer/src/lib.rs#L19-L21) | ✅ `test_double_initialize_panics` ([`test.rs:78`](../contracts/token_transfer/src/test.rs#L78)) | +| `group_treasury` | ✅ [`lib.rs:34-36`](../contracts/group_treasury/src/lib.rs#L34-L36) | ✅ `test_double_initialize_panics` ([`test.rs:80`](../contracts/group_treasury/src/test.rs#L80)) | +| `proposals` | ✅ [`lib.rs:42-44`](../contracts/proposals/src/lib.rs#L42-L44) | ⚠️ **Open finding** — no test in `proposals/src/test.rs` calls `initialize` twice; the guard exists but is unverified by this suite. | + +**Initialization front-running** (someone other than the intended deployer calling +`initialize` first) is the more serious half of this item, and the three contracts are **not** +equally protected — `require_auth` on the `admin` parameter is a weaker mitigation than it +looks: + +| Contract | Front-running exposure | +| --- | --- | +| `token_transfer` | ⚠️ **Open finding.** `initialize` has **no** `require_auth` call at all — [`lib.rs:18-26`](../contracts/token_transfer/src/lib.rs#L18-L26). Anyone who observes the deployed-but-uninitialized contract can call `initialize(attacker, any_token)` and become admin permanently; nothing in the function checks who is calling. Proven by the test suite itself: `test_initialize` ([`test.rs:70`](../contracts/token_transfer/src/test.rs#L70)) calls `client.initialize(...)` with **no `env.mock_all_auths()` anywhere in `setup()`**, and it succeeds — if any auth were required, this test would panic. | +| `group_treasury` | ⚠️ **Open finding**, same shape. `initialize` has no `require_auth` — [`lib.rs:33-49`](../contracts/group_treasury/src/lib.rs#L33-L49). `test_initialize` ([`test.rs:72`](../contracts/group_treasury/src/test.rs#L72)) also calls `client.initialize(...)` with no auth mocked anywhere in `setup()`, and succeeds. | +| `proposals` | ⚠️ **Partially mitigated, not closed.** `initialize` does call `admin.require_auth()` — [`lib.rs:45`](../contracts/proposals/src/lib.rs#L45) — but this only proves *the address named as admin authorized being admin*; it does not stop an attacker from calling `initialize(attacker_address)` and signing for `attacker_address` themselves. It stops a third party from being framed as admin without their consent; it does **not** stop an attacker from racing the legitimate deployer's own `initialize` call. The real mitigation for all three contracts is operational — collapsing deploy-then-initialize into the smallest possible window, ideally the same submitted transaction batch — not anything enforceable inside `initialize` itself, since the function cannot know who the "legitimate" deployer is before any state exists to record that. | + +**Reviewer guidance:** treat "front-running window between deploy and initialize" as a +deployment-process risk for all three contracts, not something a code change to `initialize` +alone can fully close. For `token_transfer` and `group_treasury` specifically, adding +*some* `require_auth` to `initialize` (even a deployer-controlled contract/address passed as a +constructor-style constraint) is still strictly better than none, since today literally no +signature of any kind is required to claim admin on either. + +--- + +## 3. Admin key powers + +What the admin address can do, per contract, once it holds that role: + +| Contract | Admin powers | Where | +| --- | --- | --- | +| `token_transfer` | `set_token_contract` (repoint which SEP-41 token this router transfers) ; `upgrade` (replace the contract's own Wasm via `env.deployer().update_current_contract_wasm`) | [`lib.rs:80-106`](../contracts/token_transfer/src/lib.rs#L80-L106) | +| `group_treasury` | `add_member` / `remove_member` (full control of who can vote) ; `withdraw` (move any amount of any token the treasury holds, directly, bypassing the entire member-vote system) | [`lib.rs:60-123`](../contracts/group_treasury/src/lib.rs#L60-L123), [`lib.rs:175-202`](../contracts/group_treasury/src/lib.rs#L175-L202) | +| `proposals` | Reserved but **unused**. The doc comment at [`lib.rs:38-40`](../contracts/proposals/src/lib.rs#L38-L40) states the admin slot exists for a future governance hook; no function in the current contract checks the stored admin against anything. | + +**Open finding, `group_treasury`:** the admin's direct `withdraw` is a complete bypass of the +member-voting system documented in +[`concepts-treasury-multisig-model.md`](concepts-treasury-multisig-model.md#two-independent-authorization-surfaces) +— a single admin key can move 100% of treasury funds with zero member approvals, in the same +contract whose entire other surface is built around requiring multiple members to agree. There +is no way to change or revoke the admin address after `initialize` (no `set_admin` function in +any of the three contracts), so a compromised or malicious admin key is a permanent, unilateral +risk for the lifetime of the deployed instance. + +**Upgrade risk (`token_transfer` only — the only contract with an `upgrade` function):** a +compromised admin key can replace the entire contract logic via `upgrade`, which is a strictly +larger blast radius than any individual function misuse. There is no timelock, multi-sig, or +second-party check on `upgrade` beyond the single `admin.require_auth()`. `group_treasury` and +`proposals` have no `upgrade` function at all, so this specific risk does not apply to them. + +--- + +## 4. Integer overflow on `i128` amounts + +No contract performs checked arithmetic (`checked_add`, `checked_mul`, etc.) anywhere — +confirmed by `grep -rn "checked_add\|checked_mul" contracts/*/src/*.rs` returning no matches. +The only protection against silent `i128` wraparound is a **workspace-level build setting**, +not contract code: + +```toml +# contracts/Cargo.toml +[profile.release] +overflow-checks = true +``` + +This is documented and deliberately justified in +[`concepts-resource-budget.md`](concepts-resource-budget.md#release-profile-settings-that-affect-size): +*"these contracts move funds; a silent `i128` wraparound in a balance is a critical bug. This +is a deliberate trade of size for safety."* With this flag on, an overflow traps (panics) at +the host level instead of wrapping silently. + +**Open finding:** this protection is a one-line Cargo profile setting that nothing in the +contract test suites exercises directly — `grep -rn "overflow" contracts/*/src/test.rs` returns +no matches in any of the three contracts. There is no test anywhere that constructs an amount +near `i128::MAX` and asserts the expected panic. A future change to `contracts/Cargo.toml` +(for example, to shrink Wasm size — see the same resource-budget document's own warning not to +do this) could silently remove this protection with no test failing to catch it. Any PR that +touches `[profile.release]` in `contracts/Cargo.toml` should be treated as security-relevant for +this reason alone, and a reviewer should not assume overflow safety is "tested" in the normal +sense — it is configured, and unverified by any test in this repo. + +--- + +## 5. Negative or zero amounts + +Every function that accepts an `i128` amount intended to be positive guards it with the same +`if amount <= 0 { panic!("amount must be positive"); }` pattern, which covers negative and zero +in one comparison: + +| Function | Guard | Zero tested? | Negative tested? | +| --- | --- | --- | --- | +| `token_transfer::transfer` | [`lib.rs:33-35`](../contracts/token_transfer/src/lib.rs#L33-L35) | ✅ `test_transfer_zero_panics` ([`test.rs:113`](../contracts/token_transfer/src/test.rs#L113)) | ⚠️ **Open finding** — not tested | +| `group_treasury::deposit` | [`lib.rs:151-153`](../contracts/group_treasury/src/lib.rs#L151-L153) | ✅ `test_deposit_zero_panics` ([`test.rs:175`](../contracts/group_treasury/src/test.rs#L175)) | ⚠️ **Open finding** — not tested | +| `group_treasury::withdraw` | [`lib.rs:176-178`](../contracts/group_treasury/src/lib.rs#L176-L178) | ⚠️ **Open finding** — no zero-amount test exists for `withdraw` specifically | ⚠️ **Open finding** — not tested | +| `group_treasury::propose_withdraw` | [`lib.rs:231-233`](../contracts/group_treasury/src/lib.rs#L231-L233) | ⚠️ **Open finding** — no zero-amount test exists | ⚠️ **Open finding** — not tested | +| `proposals::create_proposal` | [`lib.rs:69-71`](../contracts/proposals/src/lib.rs#L69-L71) | ⚠️ **Open finding** — no zero-amount test exists | ⚠️ **Open finding** — not tested | + +The guard condition (`<= 0`) is correct by inspection for both zero and negative values in every +one of these five call sites — this is not a correctness finding about the code. It *is* a test +coverage finding: only two of five guarded functions have any test proving the guard fires, and +**none** of the five has a test using an actual negative literal (every existing test uses +`0`). A negative amount passing one of these checks would be a critical bug (e.g., a negative +`withdraw` amount reducing `current - amount` by a negative number, i.e. *increasing* the stored +balance) and nothing in the current suite would catch a regression that weakened `<= 0` to +`== 0` by mistake. + +--- + +## 6. Threshold edge cases + +Scoped to `group_treasury`, the only contract with a fixed numeric approval `Threshold`. +(`proposals` uses simple majority of cast votes with no quorum and no threshold concept — see +[`concepts-proposal-lifecycle.md`](concepts-proposal-lifecycle.md#voting), so neither edge case +below applies to it.) + +| Edge case | Status | Evidence | +| --- | --- | --- | +| `threshold == 0` at `initialize` | ✅ Rejected | `panic!("threshold must be at least 1")` — [`lib.rs:37-39`](../contracts/group_treasury/src/lib.rs#L37-L39), proven by `test_initialize_zero_threshold_panics` ([`test.rs:361`](../contracts/group_treasury/src/test.rs#L361)) | +| `threshold` greater than member count | ⚠️ **Open finding.** Not rejected anywhere — `initialize` cannot check this in any case, since members are always `0` at init time (added afterward via `add_member`), and no later function re-validates `Threshold` against `Members.len()`. A treasury can be permanently initialized in a state where no proposal can ever reach `Threshold` approvals. Documented as a known limitation, not tested as a guarded case, in [`concepts-treasury-multisig-model.md`](concepts-treasury-multisig-model.md#known-limitations-verified-against-the-current-source-not-speculative) (see the "Fewer members than `Threshold`" row). There is no `set_threshold` function to recover from this once deployed. | +| Removing members below `Threshold` | ⚠️ **Open finding.** `remove_member` ([`lib.rs:89-123`](../contracts/group_treasury/src/lib.rs#L89-L123)) has no awareness of `Threshold` at all — it can remove members down to zero while `Threshold` stays fixed, with no warning, no block, and no test covering the scenario. Same source citation as above. | + +Both edge cases share one root cause: there is no function in `lib.rs` that ever changes +`Threshold` after `initialize`, and no function that validates it against the live member count +at any point afterward. Closing either finding requires either a `set_threshold` admin function +with its own validation, or a check inside `remove_member` that refuses to drop membership below +the stored threshold — neither exists today. + +--- + +## 7. Double voting + +| Contract | Guard | Test | +| --- | --- | --- | +| `group_treasury` (`approve_withdraw` / `reject_withdraw`) | `require_votable` checks `env.storage().instance().has(&DataKey::Vote(proposal_id, voter))` and panics `"already voted"` before recording a new vote — [`lib.rs:454-460`](../contracts/group_treasury/src/lib.rs#L454-L460) | ✅ `test_double_vote_panics` ([`test.rs:463`](../contracts/group_treasury/src/test.rs#L463)) and, specifically covering switching sides rather than repeating the same side, `test_approve_then_reject_same_member_panics` ([`test.rs:477`](../contracts/group_treasury/src/test.rs#L477)) — a member cannot flip from approve to reject either | +| `proposals` (`vote`) | Same pattern — `has(&vote_key)` check before recording — [`lib.rs:129-132`](../contracts/proposals/src/lib.rs#L129-L132) | ✅ `double_vote_panics` ([`test.rs:284`](../contracts/proposals/src/test.rs#L284)) | + +No open finding on this item for either contract — both the guard and the "can't switch sides" +variant are covered by named, specific tests rather than inferred from the happy path. + +One related, narrower gap worth a reviewer's attention even though it isn't "double voting" in +the counted sense: in `group_treasury`, a member removed after voting keeps their already-cast +vote counted in `approvals`/`rejections` — removal does not touch any `DataKey::Vote` or +`DataKey::Proposal` entry (documented in +[`concepts-treasury-multisig-model.md`](concepts-treasury-multisig-model.md#membership), "Member +removed during an active vote" row). This is a membership-consistency question, not a +double-voting one, and is called out here only so it isn't mistaken for covered by the tests +above. + +--- + +## 8. Storage archival + +Full coverage is in [`concepts-state-archival.md`](concepts-state-archival.md) — summarized +here for checklist purposes: + +| Contract | Status | +| --- | --- | +| `token_transfer` | ⚠️ **Open finding.** All storage is `instance()`; no `extend_ttl`/`.bump()` call exists anywhere in the contract. | +| `group_treasury` | ⚠️ **Open finding**, same shape, and higher-impact: every `DataKey` variant (including `Members` and every open `Proposal`) shares the one instance TTL, so the whole treasury becomes unusable at once if it archives. | +| `proposals` | ⚠️ **Open finding**, same shape. | + +No contract in this workspace extends any TTL. This is flagged once per contract here for +checklist completeness; see the linked document for the full per-key inventory, the risk +table, and the `stellar contract extend` / `stellar contract restore` operator commands. + +--- + +## Summary table + +| # | Item | `token_transfer` | `group_treasury` | `proposals` | +| --- | --- | --- | --- | --- | +| 1 | `require_auth` coverage | ✅ Pass | ✅ Pass (with the `propose_withdraw` caveat above) | ✅ Pass (two functions intentionally public) | +| 2 | Init front-running | ⚠️ Open — no auth at all | ⚠️ Open — no auth at all | ⚠️ Open — `require_auth` present but doesn't close the race | +| 2 | Re-initialization | ✅ Guarded & tested | ✅ Guarded & tested | ✅ Guarded, ⚠️ untested | +| 3 | Admin powers | Noted (upgrade = full logic replacement) | Noted (unilateral `withdraw` bypasses voting) | N/A — admin slot unused | +| 4 | Overflow | ⚠️ Mitigated by build flag only, untested | ⚠️ Same | ⚠️ Same | +| 5 | Zero/negative amounts | ⚠️ Zero tested, negative untested | ⚠️ Partial — see table | ⚠️ Untested | +| 6 | Threshold edge cases | N/A | ⚠️ Open — both scenarios unguarded | N/A — no threshold concept | +| 7 | Double voting | N/A | ✅ Pass | ✅ Pass | +| 8 | Storage archival | ⚠️ Open | ⚠️ Open | ⚠️ Open | + +--- + +## Using this checklist on a PR + +1. If the PR adds or changes a state-changing entry point, add a row for it to §1 and confirm + its auth proof follows the pattern in the callout at the top of this document — a + `mock_all_auths()`-only test is not sufficient evidence to mark it ✅. +2. If the PR touches `initialize` in any contract, re-read §2 — the front-running exposure is + about the *absence* of a check, which a diff-based review easily misses if it only looks at + what changed. +3. If the PR touches `contracts/Cargo.toml`'s `[profile.release]`, treat §4 as directly + relevant regardless of what the PR description says it's about. +4. Carry forward every ⚠️ **Open finding** above into the PR description rather than silently + re-approving it — this document records what is *currently* true, not what has been + accepted as permanently acceptable. From 8b4e555ad0c3fff9b5d3ac89f867cd25c43108d6 Mon Sep 17 00:00:00 2001 From: dnnyorji Date: Tue, 20 Oct 2026 02:15:34 +0100 Subject: [PATCH 4/4] docs(user-guide): add group treasury and proposals guide Explains membership and the approval threshold with a worked three-member example, that funds live in an on-chain contract rather than inside Clicked, why voting asks for a Freighter signature, and that proposal expiry/rejection never moves funds since nothing is reserved when a proposal is created. --- docs/user-guide/treasury.md | 124 ++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 docs/user-guide/treasury.md diff --git a/docs/user-guide/treasury.md b/docs/user-guide/treasury.md new file mode 100644 index 0000000..be01c72 --- /dev/null +++ b/docs/user-guide/treasury.md @@ -0,0 +1,124 @@ +# Group treasury and proposals + +How a group's shared treasury works in Clicked today: depositing funds, proposing a +withdrawal, approving or rejecting it, what happens once enough members agree, and what +happens if a proposal is rejected or simply runs out of time. + +This is written against the current **Treasury** page (`/app/treasury`) and its **Propose +Withdrawal** and proposal-voting controls. Clicked also has a separate **Governance +Proposals** page — that one covers unrelated, protocol-level community proposals (e.g. +"increase validator quorum"), not your group's own treasury. Nothing on that page affects your +group's funds; this guide only covers the Treasury page's proposals. + +--- + +## The short version + +A group treasury is a shared pool of funds that belongs to the group, not to any one person — +not even Clicked. Moving money out of it requires enough of the group's members to agree, the +same way a safe with multiple locks needs enough of the keyholders to show up. Clicked's job is +to show you the treasury's state and let you propose, vote, and (eventually) release funds — +the money itself sits in a smart contract on the Stellar network the whole time, not in a +Clicked-controlled account. + +## Membership and the approval threshold, in plain terms + +Every treasury has: + +- A **member list** — the specific people allowed to propose withdrawals and vote on them. + Being in the group conversation doesn't automatically make you a treasury member; membership + is managed separately by the treasury's admin. +- A **threshold** — the number of approval votes a withdrawal needs before it's allowed to go + through. This is a fixed number, not a percentage, and it doesn't automatically track the + member count if membership changes later (more on that below). + +### Worked example: three members, threshold of two + +Say a group treasury has three members — Alex, Bola, and Chidi — and a threshold of **2**. + +1. **Alex proposes a withdrawal** of 500 XLM to pay a vendor. Proposing something you believe + in counts as your own approval automatically — Alex's proposal starts at **1 approval out + of 2 needed**. +2. **Bola approves.** That's the second approval — **2 out of 2** — so the proposal is now + approved and ready to be carried out. Chidi's vote was never needed for this one; once a + proposal hits the threshold, it's approved regardless of how the remaining members would + have voted. +3. If instead **Chidi had rejected** the proposal before Bola voted, that single rejection + wouldn't kill it by itself — rejecting is a vote against, not a veto. A proposal is only + blocked once enough people reject it that reaching the threshold becomes mathematically + impossible. With 3 members and a threshold of 2, that point is reached at **2 rejections**: + once 2 of the 3 members have said no, there's no way for 2 approvals to still happen, so the + proposal is blocked. A single "no" vote, on its own, just lowers the odds — it doesn't end + the vote. + +**Why a threshold, not just "everyone agrees"?** Requiring unanimous agreement means one +unavailable or unresponsive member can freeze the treasury indefinitely. Requiring only one +person's approval means any single member could drain the treasury alone. A threshold in +between is the group's own choice of how much agreement is enough — set when the treasury was +created, not something you choose per withdrawal. + +**A subtlety worth knowing:** the threshold is a fixed number, set once. If members later leave +the group's treasury roster, the threshold doesn't shrink to match — a treasury with a +threshold of 2 still needs 2 approvals even if it's down to 2 members total, at which point +every remaining member must agree on everything. Ask whoever administers your treasury what the +current threshold and member count are if this matters to a decision you're making. + +## Funds live on-chain, not inside Clicked + +The treasury's balance isn't a number in a Clicked database that Clicked could edit or lose — +it's held by a smart contract on the Stellar network. Clicked's role is to show you that +balance, let members propose and vote, and (once a proposal is approved) trigger the on-chain +transfer. The money itself moves by the contract transferring tokens from its own on-chain +balance to the recipient, authorized by the vote outcome and the admin, not by anything +Clicked's servers decide on their own. + +**What this means if Clicked is ever down or unreachable:** your group's treasury balance is +not at risk — it continues to exist on the Stellar network exactly as it did before, independent +of whether Clicked's app or servers are running. What you'd lose access to is the *convenience* +of Clicked's interface for viewing the balance, proposing, and voting through chat. The funds +themselves aren't "inside" Clicked in any sense that an outage could threaten. + +## Why you're asked to sign with Freighter + +Freighter is the Stellar wallet extension Clicked uses to prove actions are really coming from +you, not just from whoever is logged into your Clicked account at the time. + +When you **approve or reject** a proposal, Clicked asks Freighter to sign a short message that +names the specific proposal and the specific choice you're making (approve or reject). You'll +see a Freighter popup asking you to confirm — that's you personally authorizing that exact +vote, in a way that can't be reused for a different proposal or a different vote. If you +cancel or close the Freighter popup, your vote is not recorded. + +This signature is separate from your Clicked login. Your login proves you're allowed to use +the app; your Freighter signature proves it was genuinely you, wallet in hand, who approved or +rejected that specific withdrawal — the same reason a bank asks for a card PIN even after +you've already shown ID. + +(Proposing a withdrawal and viewing the treasury don't currently prompt a Freighter signature +the way voting does — those actions go through your logged-in Clicked session. If that changes, +this guide will note it.) + +## Proposal expiry — what happens to the money + +When you propose a withdrawal, you also pick how long it stays open for voting (today: 24 +hours, 72 hours, or 7 days). This is strictly a voting deadline — it has nothing to do with the +funds being set aside or locked anywhere. + +**No money moves when a proposal is created, and none moves while it's pending.** Proposing a +withdrawal doesn't reserve, freeze, or earmark the amount out of the treasury's balance — the +treasury's full balance stays available the entire time a proposal is being voted on. This +matters for what happens at the end of a proposal's life: + +- **If it's approved** (reaches the threshold before expiring): the withdrawal can go ahead, + and the funds move only at that point — not earlier. +- **If it's rejected** (enough members vote no — see the worked example above): nothing + happens to the treasury's balance. The money was never moved in the first place, so there's + nothing to "return." The proposal simply ends without ever becoming a transfer. +- **If it expires** with neither outcome reached (not enough approvals, not enough rejections): + same as rejection, as far as the treasury's balance is concerned — nothing moves, and the + proposed withdrawal simply does not happen. The amount stays in the treasury, fully available + for a new proposal if the group still wants to make that payment. + +In short: a withdrawal proposal is a request and a vote, not a hold on the money. The treasury +only ever loses funds at the moment an approved withdrawal is actually carried out — rejection +and expiry both just mean that moment never arrives for that particular proposal.