From e4812a1cbe573eb5f7ea80cd2008c3cc576a36c1 Mon Sep 17 00:00:00 2001 From: licette32 Date: Mon, 30 Mar 2026 09:07:54 -0300 Subject: [PATCH] chore(soroban): feature address-validation --- README.md | 3 + src/lib.rs | 36 +++++++- tests/create_vault.rs | 196 ++++++++++++++++++++++++++++++++++++++++++ vesting.md | 89 ++++++++----------- 4 files changed, 271 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 72e375c..c543586 100644 --- a/README.md +++ b/README.md @@ -761,6 +761,9 @@ During `create_vault`, the contract enforces: - `start_timestamp` must not be in the past - `end_timestamp` must be strictly greater than `start_timestamp` - `end_timestamp - start_timestamp` must not exceed `MAX_VAULT_DURATION` +- `success_destination` must differ from `failure_destination` (returns `Error::SameDestination`, code `#10`); equal destinations make the success/failure outcome financially indistinguishable, removing the accountability incentive of the vault +- `creator` must differ from `success_destination` and `failure_destination` (returns `Error::InvalidAddress`, code `#11`); a creator that is also a destination could trivially recover funds regardless of milestone outcome +- `verifier` (when `Some`) must differ from `creator` (returns `Error::InvalidAddress`, code `#11`); a verifier equal to the creator provides no independent validation All validations occur before event emission or state mutation, ensuring invalid vaults cannot be created. diff --git a/src/lib.rs b/src/lib.rs index 70ae578..e3335a9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,6 +35,11 @@ pub enum Error { InvalidTimestamps = 8, /// Vault duration (end − start) exceeds MAX_VAULT_DURATION. DurationTooLong = 9, + /// success_destination and failure_destination must be different addresses. + SameDestination = 10, + /// An address parameter is an obviously invalid placeholder: creator must differ from + /// success_destination and failure_destination, and verifier (when Some) must differ from creator. + InvalidAddress = 11, } // --------------------------------------------------------------------------- @@ -108,8 +113,17 @@ impl DisciplrVault { /// Create a new productivity vault. Transfers USDC from creator to contract. /// /// # Validation Rules - /// - `amount` must be positive; otherwise returns `Error::InvalidAmount`. + /// - `amount` must be within `[MIN_AMOUNT, MAX_AMOUNT]`; otherwise returns `Error::InvalidAmount`. /// - `start_timestamp` must be strictly less than `end_timestamp`; otherwise returns `Error::InvalidTimestamps`. + /// - `end_timestamp - start_timestamp` must not exceed `MAX_VAULT_DURATION`; otherwise returns `Error::DurationTooLong`. + /// - `success_destination` must differ from `failure_destination`; otherwise returns `Error::SameDestination`. + /// Allowing equal destinations would make the success/failure outcome indistinguishable to the + /// creator, removing the accountability incentive that is the core purpose of the vault. + /// - `creator` must differ from `success_destination` and `failure_destination`; otherwise returns + /// `Error::InvalidAddress`. A creator that is also a destination could trivially recover funds + /// regardless of milestone outcome, defeating the vault's accountability mechanism. + /// - `verifier` (when `Some`) must differ from `creator`; otherwise returns `Error::InvalidAddress`. + /// A verifier equal to the creator provides no independent validation. /// /// # Prerequisites /// Creator must have sufficient USDC balance and authorize the transaction. @@ -150,6 +164,26 @@ impl DisciplrVault { return Err(Error::DurationTooLong); } + // Validate destinations are distinct (Issue #124) + if success_destination == failure_destination { + return Err(Error::SameDestination); + } + + // Validate address roles are distinct (Issue #125) + // Creator must not be a destination: would allow trivial fund recovery regardless of outcome. + if creator == success_destination { + return Err(Error::InvalidAddress); + } + if creator == failure_destination { + return Err(Error::InvalidAddress); + } + // Verifier must not be the creator: would provide no independent validation. + if let Some(ref v) = verifier { + if *v == creator { + return Err(Error::InvalidAddress); + } + } + // Pull USDC from creator into this contract. let token_client = token::Client::new(&env, &usdc_token); token_client.transfer(&creator, &env.current_contract_address(), &amount); diff --git a/tests/create_vault.rs b/tests/create_vault.rs index 545c8d2..f31b0e3 100644 --- a/tests/create_vault.rs +++ b/tests/create_vault.rs @@ -270,6 +270,202 @@ fn test_get_vault_state_never_created_id_returns_none() { assert!(client.get_vault_state(&42u32).is_none()); } +// --------------------------------------------------------------------------- +// Issue #124: success_destination vs failure_destination equality +// --------------------------------------------------------------------------- + +/// Verifica que create_vault rechaza cuando success_destination == failure_destination. +/// Implicación UX: si ambas direcciones fueran iguales, el resultado de éxito y fracaso +/// sería indistinguible para el creador, eliminando el incentivo de cumplir el milestone. +#[test] +#[should_panic(expected = "Error(Contract, #10)")] +fn test_same_destination_rejected() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + // Misma dirección para success y failure + let same_dest = Address::generate(&env); + + client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &None, + &same_dest, + &same_dest, + ); +} + +/// Verifica que create_vault acepta cuando success_destination != failure_destination. +/// Caso explícito del constraint introducido en el issue #124. +#[test] +fn test_different_destinations_accepted() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + let success = Address::generate(&env); + let failure = Address::generate(&env); + + // Las dos direcciones son distintas: debe crearse sin error + let vault_id = client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &None, + &success, + &failure, + ); + + assert_eq!(vault_id, 0u32); + + let vault = client.get_vault_state(&vault_id).unwrap(); + assert_ne!(vault.success_destination, vault.failure_destination); +} + +// --------------------------------------------------------------------------- +// Issue #125: Zero-address / dummy address validation +// --------------------------------------------------------------------------- + +/// Verifica que create_vault rechaza cuando creator == success_destination. +/// Un creador que es también el destino de éxito puede recuperar fondos +/// trivialmente sin importar si completó el milestone. +#[test] +#[should_panic(expected = "Error(Contract, #11)")] +fn test_creator_as_success_destination_rejected() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + let failure = Address::generate(&env); + + client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &None, + &creator, // success_destination == creator + &failure, + ); +} + +/// Verifica que create_vault rechaza cuando creator == failure_destination. +/// Un creador que es también el destino de fracaso recupera fondos +/// sin consecuencia por no completar el milestone. +#[test] +#[should_panic(expected = "Error(Contract, #11)")] +fn test_creator_as_failure_destination_rejected() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + let success = Address::generate(&env); + + client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &None, + &success, + &creator, // failure_destination == creator + ); +} + +/// Verifica que create_vault rechaza cuando verifier == creator. +/// Un verificador igual al creador no provee validación independiente, +/// permitiendo al creador auto-validar y liberar fondos sin control externo. +#[test] +#[should_panic(expected = "Error(Contract, #11)")] +fn test_verifier_same_as_creator_rejected() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + let success = Address::generate(&env); + let failure = Address::generate(&env); + + client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &Some(creator.clone()), // verifier == creator + &success, + &failure, + ); +} + +/// Verifica que create_vault acepta cuando creator, verifier, success y failure +/// son todos direcciones distintas generadas independientemente. +#[test] +fn test_all_distinct_addresses_accepted() { + let (env, client, usdc, usdc_asset) = setup(); + + let creator = Address::generate(&env); + let verifier = Address::generate(&env); + let success = Address::generate(&env); + let failure = Address::generate(&env); + let now = 1_725_000_000u64; + env.ledger().set_timestamp(now); + + usdc_asset.mint(&creator, &MIN_AMOUNT); + + let vault_id = client.create_vault( + &usdc, + &creator, + &MIN_AMOUNT, + &now, + &(now + 86_400), + &BytesN::from_array(&env, &[0u8; 32]), + &Some(verifier.clone()), + &success, + &failure, + ); + + assert_eq!(vault_id, 0u32); + + let vault = client.get_vault_state(&vault_id).unwrap(); + assert_ne!(vault.creator, vault.success_destination); + assert_ne!(vault.creator, vault.failure_destination); + assert_ne!(vault.success_destination, vault.failure_destination); + assert_eq!(vault.verifier, Some(verifier)); +} + #[test] fn test_get_vault_state_cancelled_vault_remains_readable() { let (env, client, usdc, usdc_asset) = setup(); diff --git a/vesting.md b/vesting.md index d3479e5..1f54af3 100644 --- a/vesting.md +++ b/vesting.md @@ -95,7 +95,7 @@ pub fn create_vault( - `start_timestamp`: When vault becomes active (unix seconds) - `end_timestamp`: Deadline for milestone validation (unix seconds) - `milestone_hash`: SHA-256 hash of milestone document -- `verifier`: Optional verifier address (None = anyone can validate) +- `verifier`: Optional verifier address (None = creator validates) - `success_destination`: Address to receive funds on success - `failure_destination`: Address to receive funds on failure @@ -103,7 +103,12 @@ pub fn create_vault( **Requirements:** - Caller must authorize the transaction (`creator.require_auth()`) -- `end_timestamp` must be greater than `start_timestamp` +- `amount` must be within `[MIN_AMOUNT, MAX_AMOUNT]`; otherwise returns `Error::InvalidAmount` +- `start_timestamp` must be strictly less than `end_timestamp`; otherwise returns `Error::InvalidTimestamps` +- `end_timestamp - start_timestamp` must not exceed `MAX_VAULT_DURATION`; otherwise returns `Error::DurationTooLong` +- `success_destination` must differ from `failure_destination`; otherwise returns `Error::SameDestination` (error code `#10`). Equal destinations make the success/failure outcome financially indistinguishable, removing the accountability incentive of the vault. +- `creator` must differ from `success_destination` and `failure_destination`; otherwise returns `Error::InvalidAddress` (error code `#11`). A creator that is also a destination could trivially recover funds regardless of milestone outcome, defeating the vault's accountability mechanism. +- `verifier` (when `Some`) must differ from `creator`; otherwise returns `Error::InvalidAddress` (error code `#11`). A verifier equal to the creator provides no independent validation. - USDC transfer must be approved by creator before calling **Emits:** [`vault_created`](#vault_created) event @@ -123,9 +128,9 @@ pub fn validate_milestone(env: Env, vault_id: u32) -> bool **Returns:** `bool` - True if validation successful -**Requirements (TODO):** +**Requirements:** - Vault must exist and be in `Active` status -- Caller must be the designated verifier (if set) +- Caller must be the designated verifier (if set), or creator (if verifier is None) - Current timestamp must be before `end_timestamp` **Emits:** [`milestone_validated`](#milestone_validated) event @@ -134,20 +139,21 @@ pub fn validate_milestone(env: Env, vault_id: u32) -> bool ### `release_funds` -Releases locked funds to the success destination (typically after validation). +Releases locked funds to the success destination (after validation or deadline). ```rust -pub fn release_funds(env: Env, vault_id: u32) -> bool +pub fn release_funds(env: Env, vault_id: u32, usdc_token: Address) -> bool ``` **Parameters:** - `vault_id`: ID of the vault to release funds from +- `usdc_token`: Address of the USDC token contract **Returns:** `bool` - True if release successful -**Requirements (TODO):** +**Requirements:** - Vault status must be `Active` -- Caller must be authorized (verifier or contract logic) +- Milestone must be validated OR current time must be past `end_timestamp` - Transfers USDC to `success_destination` - Sets status to `Completed` @@ -158,17 +164,19 @@ pub fn release_funds(env: Env, vault_id: u32) -> bool Redirects funds to the failure destination when milestone is not completed by deadline. ```rust -pub fn redirect_funds(env: Env, vault_id: u32) -> bool +pub fn redirect_funds(env: Env, vault_id: u32, usdc_token: Address) -> bool ``` **Parameters:** - `vault_id`: ID of the vault to redirect funds from +- `usdc_token`: Address of the USDC token contract **Returns:** `bool` - True if redirect successful -**Requirements (TODO):** +**Requirements:** - Vault status must be `Active` - Current timestamp must be past `end_timestamp` +- Milestone must NOT have been validated - Transfers USDC to `failure_destination` - Sets status to `Failed` @@ -179,15 +187,16 @@ pub fn redirect_funds(env: Env, vault_id: u32) -> bool Allows the creator to cancel the vault and retrieve locked funds. ```rust -pub fn cancel_vault(env: Env, vault_id: u32) -> bool +pub fn cancel_vault(env: Env, vault_id: u32, usdc_token: Address) -> bool ``` **Parameters:** - `vault_id`: ID of the vault to cancel +- `usdc_token`: Address of the USDC token contract **Returns:** `bool` - True if cancellation successful -**Requirements (TODO):** +**Requirements:** - Caller must be the vault creator - Vault status must be `Active` - Returns USDC to creator @@ -297,24 +306,20 @@ Emitted when a milestone is successfully validated. ## Security and Trust Model -<<<<<<< HEAD This section outlines the security properties, trust assumptions, and known limitations of the Disciplr Vault contract to assist auditors and users. ### Trust Model -1. **Absolute Verifier Power**: If a `verifier` is designated, they hold absolute power over the milestone validation process. The contract cannot verify off-chain project completion; it relies entirely on the `verifier`'s signature or authorization. -2. **Creator Authority**: The `creator` is the only address authorized to `create_vault` or `cancel_vault`. They must authorize the initial USDC funding. +1. **Verifier Trust (Critical)**: When a `verifier` is designated (via `Some(Address)`), that address has **absolute power** to validate the milestone and cause funds to be released to the `success_destination` before the deadline. If the verifier is compromised or malicious, they can release funds prematurely or to a non-compliant recipient. +2. **Creator Authority**: The `creator` is the only address authorized to `create_vault` or `cancel_vault`. They must authorize the initial USDC funding. If no `verifier` is set (`None`), only the `creator` can validate the milestone. 3. **No Administrative Overrides**: There is no "admin" or "owner" role with the power to sweep funds or override the vault logic. Funds can only flow to the predefined `success_destination`, `failure_destination`, or back to the `creator` on cancellation. +4. **Immutable Destinations**: Once a vault is created, the `success_destination` and `failure_destination` are immutable. This prevents redirection of funds after the vault is funded. -### External Dependencies - -1. **USDC Token Contract**: The contract interacts with an external USDC token address (Stellar Asset Contract). The security of the vault depends on the integrity and availability of this external contract. -2. **Ledger Reliability**: The contract relies on the Stellar network's ledger timestamp for all timing constraints (`start_timestamp`, `end_timestamp`). - -### Assumptions +### Security Assumptions -1. **Immutable Destinations**: The `success_destination` and `failure_destination` are fixed at vault creation and cannot be changed. -2. **USDC Compliance**: It is assumed the provided `usdc_token` address follows the standard Soroban/Stellar token interface. +1. **Stellar Ledger Integrity**: We assume the underlying Stellar blockchain and Soroban runtime correctly enforce authorization (`require_auth`) and maintain state integrity. +2. **Ledger Timestamp**: The contract relies on `env.ledger().timestamp()` for all time-based logic (start/end windows). We assume ledger timestamps are reasonably accurate and monotonic as per Stellar network consensus. +3. **Token Contract Behavior**: The contract interacts with a USDC token contract (standard Soroban token interface). We assume the token contract is honest and follows the expected transfer behavior. ### Known Limitations & Security Notes @@ -322,6 +327,8 @@ This section outlines the security properties, trust assumptions, and known limi 2. **Checks-Effects-Interactions (CEI)**: In `release_funds`, `redirect_funds`, and `cancel_vault`, the USDC transfer is initiated *before* the internal status is updated to `Completed`, `Failed`, or `Cancelled`. While Soroban's atomicity safeguards against most reentrancy/partial-success risks, this is a deviation from the strict CEI pattern. 3. **Lack of Emergency Stops**: There is currently no circuit breaker or emergency pause mechanism. 4. **Precision**: All amounts are handled as `i128` in stroops (7 decimals for USDC); users must ensure they provide correct decimal-adjusted amounts. +5. **Equal Destinations Rejected (Issue #124)**: `success_destination` and `failure_destination` must be different addresses. If they were equal, the outcome of the vault (success vs. failure) would be financially indistinguishable for the creator: funds would arrive at the same address regardless of whether the milestone was completed. This eliminates the accountability incentive that is the core purpose of the vault, and could be used to disguise a vault with no real consequence for non-completion. The contract enforces this at creation time with `Error::SameDestination` (code `#10`). +6. **Invalid Address Roles Rejected (Issue #125)**: The contract rejects configurations where address roles overlap in ways that defeat the vault's accountability mechanism. Specifically: (a) `creator == success_destination` — the creator would trivially recover funds on success regardless of milestone completion; (b) `creator == failure_destination` — the creator would recover funds on failure with no consequence for non-completion; (c) `verifier == creator` — the creator would be validating their own milestone, providing no independent oversight. All three cases return `Error::InvalidAddress` (code `#11`). Note: the Soroban SDK does not expose a way to detect the Stellar zero-address (`GAAA...WHF`) at the contract level in `no_std` environments; these role-overlap checks are the detectable "obviously invalid placeholder" validations available at contract level. ### Recommendations for Production @@ -331,35 +338,7 @@ This section outlines the security properties, trust assumptions, and known limi 4. **Upgradeability**: Consider proxy pattern for contract upgrades 5. **Comprehensive Tests**: Achieve 95%+ test coverage 6. **External Audits**: Have security experts review before mainnet deployment -======= -This section outlines the security assumptions, trust model, and known limitations of the Disciplr Vault contract. It is intended for auditors, developers, and users to understand the risks and guarantees provided by the system. - -### Trust Model - -1. **Verifier Trust (Critical)**: When a `verifier` is designated (via `Some(Address)`), that address has **absolute power** to validate the milestone and cause funds to be released to the `success_destination` before the deadline. If the verifier is compromised or malicious, they can release funds prematurely or to a non-compliant recipient. -2. **Creator Power**: If no `verifier` is set (`None`), only the `creator` can validate the milestone. Additionally, the `creator` can cancel the vault at any time to reclaim funds, assuming the vault is still `Active`. -3. **Immutable Destinations**: Once a vault is created, the `success_destination` and `failure_destination` are immutable. This prevents redirection of funds after the vault is funded, assuming the core contract logic remains secure. - -### Security Assumptions - -1. **Stellar Ledger Integrity**: We assume the underlying Stellar blockchain and Soroban runtime correctly enforce authorization (`require_auth`) and maintain state integrity. -2. **Ledger Timestamp**: The contract relies on `env.ledger().timestamp()` for all time-based logic (start/end windows). We assume ledger timestamps are reasonably accurate and monotonic as per Stellar network consensus. -3. **Token Contract Behavior**: The contract interacts with a USDC token contract (standard Soroban token interface). We assume the token contract is honest and follows the expected transfer behavior. - -### Known Limitations & Risks - -1. **USDC Token Address Consistency**: The `usdc_token` address is **not stored** in the vault data. Instead, it is passed as an argument to methods like `release_funds`, `redirect_funds`, and `cancel_vault`. - > [!WARNING] - > There is a risk that a caller provides a different token address than the one used during vault creation. Users should verify the token contract used in interactions matches the intended asset. -2. **CEI Pattern Violations**: Some methods perform token transfers **before** updating the internal vault status. While Soroban's atomicity mitigates some traditional reentrancy risks, following the Checks-Effects-Interactions (CEI) pattern more strictly is a recommended enhancement for future versions. -3. **No Administrative Overrides**: There is no "admin" or "owner" role with the power to rescue funds from a stalled vault (e.g., if a verifier loses their key and the deadline is far in the future). Funds are strictly bound by the `end_timestamp` and authorization rules. -4. **Lack of Reentrancy Guards**: The contract does not currently implement explicit reentrancy guards, relying instead on the synchronous and atomic nature of Soroban contract calls. - -### Recommendations for Integration - -- **Off-chain Verification**: The `milestone_hash` should represent a clear, legally or technically binding document that both creator and verifier agree upon. -- **Multisig Verifiers**: For high-value vaults, we highly recommend using a multisig address (G-address or contract-based account) as the `verifier`. ->>>>>>> c6890851d8814bd858b9c8b6f3777c8363ab4c49 +7. **Multisig Verifiers**: For high-value vaults, use a multisig address as the `verifier` --- @@ -489,6 +468,8 @@ Expected output should include tests for: - Fund release and redirect logic - Vault cancellation - State retrieval +- Equal destination rejection (Issue #124) +- Invalid address role rejection (Issue #125) --- @@ -498,6 +479,8 @@ Expected output should include tests for: disciplr-contracts/ ├── src/ │ └── lib.rs # DisciplrVault contract implementation +├── tests/ +│ └── create_vault.rs # Integration tests for create_vault ├── Cargo.toml # Project dependencies ├── README.md # Project overview └── vesting.md # This documentation @@ -518,3 +501,5 @@ disciplr-contracts/ | Version | Changes | |---------|---------| | 0.1.0 | Initial release with basic vault structure, stubbed implementations | +| 0.2.0 | Issue #124: reject equal success/failure destinations (`SameDestination` error) | +| 0.3.0 | Issue #125: reject invalid address role overlaps (`InvalidAddress` error) |