From d1f36056590bd1b9b78d425423026bb172892864 Mon Sep 17 00:00:00 2001 From: D'Angelo Rodriguez <70290504+dangelo352@users.noreply.github.com> Date: Mon, 22 Jun 2026 00:31:44 -0400 Subject: [PATCH] fix: enforce terminal state before token transfers --- USDC_INTEGRATION.md | 3 + docs/SECURITY_REENTRANCY.md | 18 ++++++ src/lib.rs | 115 ++++++++++++++++++++++++++++++++--- tests/proptest_timestamps.rs | 1 - 4 files changed, 127 insertions(+), 10 deletions(-) create mode 100644 docs/SECURITY_REENTRANCY.md diff --git a/USDC_INTEGRATION.md b/USDC_INTEGRATION.md index 938032a..d98b412 100644 --- a/USDC_INTEGRATION.md +++ b/USDC_INTEGRATION.md @@ -107,6 +107,8 @@ The contract cannot guarantee: - The `usdc_token` address is supplied at call time and is not pinned inside vault state. - The integrated token contract is therefore part of the effective security boundary. +- Settlement paths use checks-effects-interactions ordering so terminal status is + persisted before the token transfer call. - Production review should include both the Disciplr contract and the chosen Stellar USDC asset configuration. ## Related Documentation @@ -114,6 +116,7 @@ The contract cannot guarantee: - [README.md](README.md) - [vesting.md](vesting.md) - [src/lib.rs](src/lib.rs) +- [docs/SECURITY_REENTRANCY.md](docs/SECURITY_REENTRANCY.md) ## Primary References diff --git a/docs/SECURITY_REENTRANCY.md b/docs/SECURITY_REENTRANCY.md new file mode 100644 index 0000000..d7be176 --- /dev/null +++ b/docs/SECURITY_REENTRANCY.md @@ -0,0 +1,18 @@ +# Reentrancy and Terminal State Ordering + +Disciplr vault settlement functions follow checks-effects-interactions ordering. +Each fund-moving terminal path writes its final vault status before invoking the +external token contract: + +- `release_funds` writes `VaultStatus::Completed` before transferring to `success_destination`. +- `redirect_funds` writes `VaultStatus::Failed` before transferring to `failure_destination`. +- `cancel_vault` writes `VaultStatus::Cancelled` before transferring back to `creator`. + +This matters because `usdc_token` is supplied as a contract address at call time. +If a malicious or non-standard token implementation attempted to re-enter the +vault during transfer, the nested call would observe a non-`Active` vault and fail +with `Error::VaultNotActive`. + +The regression tests cover the terminal-state matrix by asserting that, after a +successful release, redirect, or cancel call, every second terminal path is +rejected and no second balance delta is paid. diff --git a/src/lib.rs b/src/lib.rs index 9893393..35a4eb5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -254,6 +254,9 @@ impl DisciplrVault { // ----------------------------------------------------------------------- /// Release vault funds to `success_destination`. + /// + /// Applies checks-effects-interactions ordering: the vault is marked + /// `Completed` before the external token transfer is invoked. pub fn release_funds(env: Env, vault_id: u32, usdc_token: Address) -> Result { let vault_key = DataKey::Vault(vault_id); let mut vault: ProductivityVault = env @@ -277,6 +280,9 @@ impl DisciplrVault { return Err(Error::NotAuthorized); } + vault.status = VaultStatus::Completed; + env.storage().instance().set(&vault_key, &vault); + let token_client = token::Client::new(&env, &usdc_token); token_client.transfer( &env.current_contract_address(), @@ -284,9 +290,6 @@ impl DisciplrVault { &vault.amount, ); - vault.status = VaultStatus::Completed; - env.storage().instance().set(&vault_key, &vault); - env.events().publish( (Symbol::new(&env, "funds_released"), vault_id), vault.amount, @@ -299,6 +302,9 @@ impl DisciplrVault { // ----------------------------------------------------------------------- /// Redirect funds to `failure_destination` (e.g. after deadline without validation). + /// + /// Applies checks-effects-interactions ordering: the vault is marked + /// `Failed` before the external token transfer is invoked. pub fn redirect_funds(env: Env, vault_id: u32, usdc_token: Address) -> Result { let vault_key = DataKey::Vault(vault_id); let mut vault: ProductivityVault = env @@ -320,6 +326,9 @@ impl DisciplrVault { return Err(Error::NotAuthorized); } + vault.status = VaultStatus::Failed; + env.storage().instance().set(&vault_key, &vault); + let token_client = token::Client::new(&env, &usdc_token); token_client.transfer( &env.current_contract_address(), @@ -327,9 +336,6 @@ impl DisciplrVault { &vault.amount, ); - vault.status = VaultStatus::Failed; - env.storage().instance().set(&vault_key, &vault); - env.events().publish( (Symbol::new(&env, "funds_redirected"), vault_id), vault.amount, @@ -342,6 +348,9 @@ impl DisciplrVault { // ----------------------------------------------------------------------- /// Cancel vault and return funds to creator. + /// + /// Applies checks-effects-interactions ordering: the vault is marked + /// `Cancelled` before the external token transfer is invoked. pub fn cancel_vault(env: Env, vault_id: u32, usdc_token: Address) -> Result { let vault_key = DataKey::Vault(vault_id); let mut vault: ProductivityVault = env @@ -356,6 +365,9 @@ impl DisciplrVault { return Err(Error::VaultNotActive); } + vault.status = VaultStatus::Cancelled; + env.storage().instance().set(&vault_key, &vault); + let token_client = token::Client::new(&env, &usdc_token); token_client.transfer( &env.current_contract_address(), @@ -363,9 +375,6 @@ impl DisciplrVault { &vault.amount, ); - vault.status = VaultStatus::Cancelled; - env.storage().instance().set(&vault_key, &vault); - env.events() .publish((Symbol::new(&env, "vault_cancelled"), vault_id), ()); Ok(true) @@ -982,6 +991,94 @@ mod tests { assert_eq!(vault.status, VaultStatus::Cancelled); } + #[test] + fn release_sets_terminal_status_before_any_second_terminal_path() { + let setup = TestSetup::new(); + let client = setup.client(); + + setup.env.ledger().set_timestamp(setup.start_timestamp); + let vault_id = setup.create_default_vault(); + setup.env.ledger().set_timestamp(setup.end_timestamp + 1); + + let usdc = setup.usdc_client(); + let success_before = usdc.balance(&setup.success_dest); + let failure_before = usdc.balance(&setup.failure_dest); + + assert!(client.release_funds(&vault_id, &setup.usdc_token)); + assert_eq!( + client.get_vault_state(&vault_id).unwrap().status, + VaultStatus::Completed + ); + + assert!(client + .try_redirect_funds(&vault_id, &setup.usdc_token) + .is_err()); + assert_eq!( + usdc.balance(&setup.success_dest) - success_before, + setup.amount + ); + assert_eq!(usdc.balance(&setup.failure_dest), failure_before); + } + + #[test] + fn redirect_sets_terminal_status_before_any_second_terminal_path() { + let setup = TestSetup::new(); + let client = setup.client(); + + setup.env.ledger().set_timestamp(setup.start_timestamp); + let vault_id = setup.create_default_vault(); + setup.env.ledger().set_timestamp(setup.end_timestamp + 1); + + let usdc = setup.usdc_client(); + let success_before = usdc.balance(&setup.success_dest); + let failure_before = usdc.balance(&setup.failure_dest); + + assert!(client.redirect_funds(&vault_id, &setup.usdc_token)); + assert_eq!( + client.get_vault_state(&vault_id).unwrap().status, + VaultStatus::Failed + ); + + assert!(client + .try_release_funds(&vault_id, &setup.usdc_token) + .is_err()); + assert_eq!(usdc.balance(&setup.success_dest), success_before); + assert_eq!( + usdc.balance(&setup.failure_dest) - failure_before, + setup.amount + ); + } + + #[test] + fn cancel_sets_terminal_status_before_any_second_terminal_path() { + let setup = TestSetup::new(); + let client = setup.client(); + + setup.env.ledger().set_timestamp(setup.start_timestamp); + let vault_id = setup.create_default_vault(); + + let usdc = setup.usdc_client(); + let creator_before = usdc.balance(&setup.creator); + let success_before = usdc.balance(&setup.success_dest); + let failure_before = usdc.balance(&setup.failure_dest); + + assert!(client.cancel_vault(&vault_id, &setup.usdc_token)); + assert_eq!( + client.get_vault_state(&vault_id).unwrap().status, + VaultStatus::Cancelled + ); + + assert!(client + .try_release_funds(&vault_id, &setup.usdc_token) + .is_err()); + assert!(client + .try_redirect_funds(&vault_id, &setup.usdc_token) + .is_err()); + assert_eq!(usdc.balance(&setup.creator) - creator_before, setup.amount); + assert_eq!(usdc.balance(&setup.success_dest), success_before); + assert_eq!(usdc.balance(&setup.failure_dest), failure_before); + } + // ----------------------------------------------------------------------- // More upstream tests migrated // ----------------------------------------------------------------------- diff --git a/tests/proptest_timestamps.rs b/tests/proptest_timestamps.rs index d8e37e8..bd89ed3 100644 --- a/tests/proptest_timestamps.rs +++ b/tests/proptest_timestamps.rs @@ -279,7 +279,6 @@ fn edge_start_eq_now_succeeds() { assert_eq!(vault.end_timestamp, end); } - #[test] fn edge_start_eq_end_rejected() { let (env, client, usdc, usdc_asset) = setup();