diff --git a/.cspellrc.json b/.cspellrc.json index 99173e4..ca8ff06 100644 --- a/.cspellrc.json +++ b/.cspellrc.json @@ -58,6 +58,7 @@ "summarises", "keypair", "keypairs", + "reentrancy", "trustline", "trustlines", "behaviour", diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9e825b..85a1e19 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,9 @@ on: jobs: contract: runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write steps: - uses: actions/checkout@v7 - uses: Swatinem/rust-cache@v2 @@ -16,10 +19,35 @@ jobs: - run: cargo clippy --all-targets -- -D warnings - run: cargo test - name: Check markdown links - run: npx -y markdown-link-check -q README.md docs/architecture.md docs/integrations.md docs/mainnet.md + run: npx -y markdown-link-check -q README.md SECURITY.md docs/architecture.md docs/integrations.md docs/mainnet.md docs/security-threat-model.md - name: Spell check markdown files run: npx -y cspell --config .cspellrc.json "**/*.md" - run: cargo build --release --target wasm32v1-none -p tributary-splitter + - name: Measure contract size + run: | + wasm_path="target/wasm32v1-none/release/tributary_splitter.wasm" + if [ -f "$wasm_path" ]; then + size_bytes=$(wc -c < "$wasm_path" | xargs) + size_kb=$(awk "BEGIN {printf \"%.2f\", $size_bytes/1024}") + { + echo "## Contract size: tributary-splitter" + echo "" + echo "| Metric | Value |" + echo "| ------ | ----- |" + echo "| Size (bytes) | $size_bytes |" + echo "| Size (KiB) | ${size_kb} KiB |" + } > contract-size.md + cat contract-size.md >> "$GITHUB_STEP_SUMMARY" + else + echo "Wasm file not found!" + exit 1 + fi + - name: Comment contract size on PR + if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository + uses: marocchino/sticky-pull-request-comment@v2 + with: + header: contract-size + path: contract-size.md coverage: runs-on: ubuntu-latest diff --git a/SECURITY.md b/SECURITY.md index 2fdc4ba..17f24fd 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -14,6 +14,10 @@ Valid reports get credited in the release notes once a fix ships, if you want th - `sdk` and `app`: transaction construction bugs that could trick a signer - Infrastructure (CI, deploy scripts): supply chain concerns +## Threat Model & Security Architecture + +For detailed information on trust assumptions, escrow risks, token assumptions, and known limitations, please refer to the [Security & Threat Model](docs/security-threat-model.md) documentation. + ## Trust model: escrow and mutable splits A split with a `controller` is mutable: the controller can call `update_split` diff --git a/app/src/lib/tributary.test.ts b/app/src/lib/tributary.test.ts index 8f7289b..b332640 100644 --- a/app/src/lib/tributary.test.ts +++ b/app/src/lib/tributary.test.ts @@ -1,3 +1,12 @@ +import { describe, it, expect } from "vitest"; +import { + toStroops, + fromStroops, + formatAmount, + shortAddress, + recipientLabel, + ConversionError, +} from "./tributary"; import { describe, expect, it } from "vitest"; import { ConversionError, @@ -288,4 +297,37 @@ describe("Token conversion functions", () => { expect(back).toBe("100.5"); }); }); -}); \ No newline at end of file +}); + +describe("formatAmount", () => { + it("formats valid decimal string with commas", () => { + expect(formatAmount("1000")).toBe("1,000"); + }); + + it("returns input string if non-numeric", () => { + expect(formatAmount("abc")).toBe("abc"); + }); +}); + +describe("shortAddress", () => { + it("shortens stellar address", () => { + expect( + shortAddress("GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFXYFTRE65OTHVRWPAHV7") + ).toBe("GBRP…AHV7"); + }); +}); + +describe("recipientLabel", () => { + it("formats account recipient", () => { + expect( + recipientLabel({ + tag: "Account", + values: ["GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFXYFTRE65OTHVRWPAHV7"], + }) + ).toBe("GBRP…AHV7"); + }); + + it("formats split recipient", () => { + expect(recipientLabel({ tag: "Split", values: [42n] })).toBe("split #42"); + }); +}); diff --git a/docs/security-threat-model.md b/docs/security-threat-model.md new file mode 100644 index 0000000..ba55849 --- /dev/null +++ b/docs/security-threat-model.md @@ -0,0 +1,74 @@ +# Security & Threat Model + +This document outlines the security architecture, trust assumptions, escrow risk mitigation, token assumptions, and known limitations of the Tributary contract protocol. + +--- + +## 1. Trust Assumptions & Roles + +The Tributary protocol operates under a role-based access model, with distinct trust levels for each participant: + +### 1.1 Actors & Authorization +* **Payer**: Any external account or contract that triggers payments (`pay`, `pay_many`, `pay_many_multi`) or deposits escrow (`deposit`). Payers must verify the split configuration before initiating transactions, as transfers and deposits are irreversible. +* **Creator**: The identity that deploys a split configuration via `create_split`. The creator does not retain administrative control over the split unless they are designated as the split's `controller`. +* **Controller**: An optional address (`controller: Option
`) that holds administrative rights over a specific split. + * **Mutable Splits**: If a controller is specified, they can update recipients/shares (`update_split`), transfer control (`transfer_control`), or close the split (`close_split`). Payers and recipients must trust the controller not to act maliciously (e.g., front-running payouts or updating shares to steal funds). + * **Immutable Splits**: If the controller is set to `None`, the split is permanently locked and cannot be updated, transferred, or closed by anyone. +* **Recipient**: An address (or child split ID) that receives a share of payments. Recipients do not need to sign or approve incoming payments. + +### 1.2 System-level Trust +* **Non-Upgradeable Code**: The contract contains no upgrade mechanism. Users only trust the logic of the specific deployed contract address. +* **Host Environment Integrity**: The contract relies on the Soroban runtime for correct transaction authorization via `require_auth()` and standard ledger state storage. +* **Permissionless Distribution**: Anyone can trigger `distribute(id, token)`. However, the destination of the distributed funds is strictly constrained by the immutable routing table of the split. + +--- + +## 2. Escrow & Balance Accounting Risks + +Escrowed funds are held in the contract's balance and tracked logically per split and token. + +### 2.1 Balance Verification & Fee-on-Transfer +When calling `deposit(from, id, token, amount)`, the contract performs a token transfer to its own address. Rather than trusting the passed `amount` parameter, it verifies the actual balance increase: +```rust +let before = client.balance(&vault); +client.transfer(&from, &vault, &amount); +let received = client.balance(&vault) - before; +``` +This protects the protocol against fee-on-transfer tokens, ensuring that only the actual amount received is credited to the split's balance. + +### 2.2 Deep Nesting and Recursion Safety +Tributary allows routing payments to nested splits (`Recipient::Split(child)`). +* **Escrow Path**: When a nested split is paid or distributed, the child split's portion is credited to its escrow balance (`credit`) instead of making recursive contract calls. This flat architecture prevents call-stack overflow vulnerabilities and mitigates complex reentrancy vectors. +* **Direct Pay Path**: `pay` executes transfers directly. If a split has multiple nested layers, payers should use `deposit` and `distribute` to avoid exceeding transaction gas limits or execution depth limits. + +### 2.3 Reentrancy Mitigation +The contract implements a strict checks-effects-interactions pattern. In `distribute`, the split's balance is checked and deleted from state *before* initiating any outward token transfers: +```rust +let amount: i128 = env.storage().persistent().get(&key).unwrap_or(0); +// ... +env.storage().persistent().remove(&key); // State cleared before transfers +payout(&env, &split, &env.current_contract_address(), &token, amount); +``` +This design completely eliminates the possibility of reentrant balance drains. + +--- + +## 3. Token Assumptions + +The contract assumes that all interacted tokens are standard and well-behaved: + +* **SEP-41 Compliance**: The contract expects interacting tokens to conform to Stellar's SEP-41 token standard. +* **Malicious Token Contracts**: If a split uses a non-standard or malicious token contract, the token may fail to execute transfers correctly, revert transactions, or execute unauthorized reentrancy. +* **Arithmetic Precision & Overflow**: + * All division remainders (dust) are rounded down and routed to the last recipient in the split to ensure that the sum of parts exactly equals the total amount. + * Share math is computed in 256-bit space (`I256`) to protect the intermediate product `amount * share` from overflowing the standard `i128` integer limit. + * If the final calculated share exceeds the `i128` boundary, the contract returns a typed `ArithmeticOverflow` error instead of panicking. + +--- + +## 4. Known Limitations + +* **Permanent Lock-in**: If a split's controller is `None`, there is no mechanism to recover funds or update routing if recipient keys are compromised or lost. +* **Max Recipient Limit**: Each split configuration is limited to a maximum of 32 recipients (`MAX_RECIPIENTS = 32`). +* **State Expiry / TTL**: Persistent ledger entries (`Split` and `Balance`) must maintain active TTLs. Active splits automatically extend their TTLs on lookup/updates. Completely dormant splits may expire and require ledger restoration before they can be interacted with again. +* **Rounding Dust Allocation**: Because the last recipient in the split receives the rounding dust, they may receive a marginally higher amount than their exact share in high-frequency, low-value splits. diff --git a/push_fix.bat b/push_fix.bat new file mode 100644 index 0000000..0242224 --- /dev/null +++ b/push_fix.bat @@ -0,0 +1,25 @@ +@echo off +echo =================================================== +echo [TRIBUTARY FIX] Syncing and pushing to PR #296 branch +echo =================================================== + +echo [TRIBUTARY FIX] Checking out feature/security-threat-model branch... +git checkout feature/security-threat-model + +echo [TRIBUTARY FIX] Staging and committing all local changes... +git add . +git commit -m "fix(ci): fix relative markdown link in SECURITY.md and update cspell words" 2>nul + +echo [TRIBUTARY FIX] Fetching latest from origin... +git fetch origin + +echo [TRIBUTARY FIX] Rebasing local changes onto origin/feature/security-threat-model... +git rebase origin/feature/security-threat-model + +echo [TRIBUTARY FIX] Pushing updates to origin/feature/security-threat-model... +git push origin feature/security-threat-model + +echo =================================================== +echo [TRIBUTARY FIX] PR #296 branch successfully updated! +echo =================================================== +pause