From a6723821ab230490e09fdfc3dc389f594aa9daad Mon Sep 17 00:00:00 2001 From: karanjadavi Date: Sun, 27 Sep 2026 15:29:05 +0300 Subject: [PATCH 1/2] vesting_escrow: reject backward-moving ledger timestamps (#1615) Adds a monotonic-timestamp guard (require_monotonic_timestamp) to claim() and clawback(), independent of the existing same-ledger replay check. Rejects a fund-moving call if the ledger timestamp is behind the last observed value, as defense-in-depth against a manipulated or anomalous timestamp reaching vesting-calculation logic. Adds claim_rejects_timestamp_regression and clawback_rejects_timestamp_regression tests. Documents the mechanism in contracts/vesting_escrow/README.md. --- contracts/vesting_escrow/README.md | 21 ++++ contracts/vesting_escrow/src/lib.rs | 24 +++++ contracts/vesting_escrow/src/test.rs | 137 ++++++++++++++++++++++----- 3 files changed, 158 insertions(+), 24 deletions(-) diff --git a/contracts/vesting_escrow/README.md b/contracts/vesting_escrow/README.md index 319bd8f8..c6ceed25 100644 --- a/contracts/vesting_escrow/README.md +++ b/contracts/vesting_escrow/README.md @@ -99,6 +99,27 @@ The contract includes comprehensive replay-attack tests: --- +## Timestamp Manipulation Resistance (Issue #1615) + +The vesting schedule relies on `e.ledger().timestamp()` for cliff and linear-vesting calculations. Stellar consensus enforces a monotonically increasing ledger close time across the network, but this contract adds a defense-in-depth check rather than trusting that guarantee unconditionally. + +### Protection Mechanism +- A `LastObservedTimestamp` key stores the highest ledger timestamp seen by any fund-moving call (`claim()`, `clawback()`). +- Before computing a vested amount for either call, `require_monotonic_timestamp` compares the current `e.ledger().timestamp()` against the stored value and returns `TimestampRegression` if it has moved backward. +- On success, the stored value is advanced to the new timestamp (only ever forward). + +### Anomaly Scenario Prevented +If a manipulated or anomalous ledger timestamp were ever observed moving backward relative to a prior fund-moving call, the contract now rejects the call outright instead of silently recomputing a smaller (or otherwise incorrect) vested amount from it. This check is independent of, and in addition to, the existing `LedgerReplayDetected` same-ledger check (Issue #1600), which only guards duplicate calls within a single ledger sequence and does not by itself detect a backward-moving timestamp across different ledgers. + +### Scope +- `preview_vested_amount` and `get_vesting_snapshot` are read-only views and are intentionally left unguarded: they cannot move funds, and `preview_vested_amount` is designed to accept an arbitrary timestamp for forward-looking previews. +- `partial_clawback` and `extend_vesting` do not read `e.ledger().timestamp()` for their accounting and are unaffected. + +### Test Coverage +- **claim_rejects_timestamp_regression**: A legitimate claim succeeds; a subsequent claim at an anomalous backward timestamp is rejected with `TimestampRegression`; a further claim at a timestamp forward of the last observed value succeeds again. +- **clawback_rejects_timestamp_regression**: A clawback attempted after a backward-moving timestamp is rejected the same way, and the grant remains active. + +--- ## Test Coverage for Edge Cases (Issue #1595) The contract includes comprehensive edge-case tests to verify correct behavior under boundary conditions: diff --git a/contracts/vesting_escrow/src/lib.rs b/contracts/vesting_escrow/src/lib.rs index c0173998..5d67b20c 100644 --- a/contracts/vesting_escrow/src/lib.rs +++ b/contracts/vesting_escrow/src/lib.rs @@ -51,6 +51,8 @@ pub enum ContractError { NoPendingAdminTransfer = 21, /// Proposed admin does not match the caller. NotProposedAdmin = 22, + /// Ledger timestamp observed by a fund-moving call is behind the last recorded value. + TimestampRegression = 23, } // ── Events ──────────────────────────────────────────────────────────────────── @@ -219,6 +221,8 @@ pub enum DataKey { PendingAdmin, /// State version for migration tracking. StateVersion, + /// Last ledger timestamp observed before a fund-moving call (monotonicity guard). + LastObservedTimestamp, } const PERSISTENT_TTL_THRESHOLD: u32 = 20_000; @@ -562,6 +566,7 @@ impl VestingContract { config.beneficiary.require_auth(); Self::require_unique_ledger(&e, &DataKey::LastClaimLedger)?; + Self::require_monotonic_timestamp(&e)?; let vested = Self::calc_vested(&e, &config); let claimable = Self::calc_claimable(vested, config.claimed_amount); @@ -612,6 +617,7 @@ impl VestingContract { if !config.is_active { return Err(ContractError::AlreadyRevoked); } + Self::require_monotonic_timestamp(&e)?; let vested = Self::calc_vested(&e, &config); let vested_floor = Self::max_i128(vested, config.claimed_amount); @@ -1125,6 +1131,24 @@ impl VestingContract { Ok(()) } + fn require_monotonic_timestamp(e: &Env) -> Result { + let now = e.ledger().timestamp(); + let last: u64 = e + .storage() + .instance() + .get(&DataKey::LastObservedTimestamp) + .unwrap_or(0); + if now < last { + return Err(ContractError::TimestampRegression); + } + if now > last { + e.storage() + .instance() + .set(&DataKey::LastObservedTimestamp, &now); + } + Ok(now) + } + fn require_not_paused(env: &Env) -> Result<(), ContractError> { let paused: bool = env .storage() diff --git a/contracts/vesting_escrow/src/test.rs b/contracts/vesting_escrow/src/test.rs index e37ce334..c4ab67d2 100644 --- a/contracts/vesting_escrow/src/test.rs +++ b/contracts/vesting_escrow/src/test.rs @@ -7,7 +7,7 @@ use soroban_sdk::{ token, }; -// ── Shared test helpers ─────────────────────────────────────────────────────── +// ── Shared test helpers ─────────────────────────────────────────────────────── fn setup() -> ( Env, @@ -192,7 +192,7 @@ fn test_vesting_flow() { assert_eq!(token_client.balance(&contract_id), 0); } -// ── ISSUE #904 test ──────────────────────────────────────────────────────────── +// ── ISSUE #904 test ──────────────────────────────────────────────────────────── #[test] fn extend_vesting_overflow_returns_error() { @@ -212,7 +212,7 @@ fn extend_vesting_overflow_returns_error() { assert_eq!(result, Err(Ok(ContractError::DurationOverflow))); } -// ── ISSUE #905 test ──────────────────────────────────────────────────────────── +// ── ISSUE #905 test ──────────────────────────────────────────────────────────── #[test] fn partial_clawback_amount_exceeds_total_returns_invariant_violation() { @@ -233,7 +233,7 @@ fn partial_clawback_amount_exceeds_total_returns_invariant_violation() { assert_eq!(result, Err(Ok(ContractError::ClawbackBelowClaimed))); } -// ── ISSUE #906 test ──────────────────────────────────────────────────────────── +// ── ISSUE #906 test ──────────────────────────────────────────────────────────── #[test] fn transfer_beneficiary_to_same_address_returns_error() { @@ -253,7 +253,7 @@ fn transfer_beneficiary_to_same_address_returns_error() { assert_eq!(result, Err(Ok(ContractError::SameBeneficiary))); } -// ── ISSUE #910 tests ────────────────────────────────────────────────────────── +// ── ISSUE #910 tests ────────────────────────────────────────────────────────── #[test] fn initialize_with_zero_start_time_returns_error() { @@ -292,7 +292,7 @@ fn initialize_with_nonzero_start_time_succeeds() { assert!(result.is_ok()); } -// ── ISSUE #908 tests ────────────────────────────────────────────────────────── +// ── ISSUE #908 tests ────────────────────────────────────────────────────────── #[test] fn initialize_with_zero_duration_returns_error() { @@ -312,7 +312,7 @@ fn initialize_with_zero_duration_returns_error() { assert_eq!(result, Err(Ok(ContractError::ZeroDuration))); } -// ── ISSUE #909 tests ────────────────────────────────────────────────────────── +// ── ISSUE #909 tests ────────────────────────────────────────────────────────── #[test] fn cliff_equals_duration_vests_all_at_single_instant() { @@ -349,7 +349,7 @@ fn cliff_equals_duration_vests_all_at_single_instant() { assert_eq!(client.get_vested_amount(), amount); } -// ── ISSUE #907 tests ────────────────────────────────────────────────────────── +// ── ISSUE #907 tests ────────────────────────────────────────────────────────── #[test] fn clawback_event_includes_admin_address() { @@ -386,7 +386,7 @@ fn clawback_event_includes_admin_address() { assert_eq!(config.clawback_admin, clawback_admin); } -// ── EDGE-CASE TESTS FOR ISSUE #1595 ─────────────────────────────────────────── +// ── EDGE-CASE TESTS FOR ISSUE #1595 ─────────────────────────────────────────── #[test] fn zero_cliff_immediate_vesting() { @@ -559,7 +559,7 @@ fn boundary_timestamp_large_start_time() { assert_eq!(client.get_vested_amount(), amount, "should fully vest even with large timestamp"); } -// ── REPLAY-ATTACK PROTECTION TESTS (Issue #1600) ──────────────────────────── +// ── REPLAY-ATTACK PROTECTION TESTS (Issue #1600) ──────────────────────────── #[test] fn same_ledger_claim_replay_detected() { @@ -586,7 +586,7 @@ fn same_ledger_claim_replay_detected() { // First claim in ledger sequence N should succeed client.claim(); let config_after_first = client.get_config(); - assert!(config_after_first.total_claimed > 0, "first claim should succeed"); + assert!(config_after_first.claimed_amount > 0, "first claim should succeed"); // Attempting second claim in the SAME ledger sequence should fail with LedgerReplayDetected // This prevents an attacker from repeatedly calling claim() in the same ledger @@ -596,7 +596,7 @@ fn same_ledger_claim_replay_detected() { // Verify state hasn't changed from failed replay attempt let config_after_replay = client.get_config(); - assert_eq!(config_after_first.total_claimed, config_after_replay.total_claimed, + assert_eq!(config_after_first.claimed_amount, config_after_replay.claimed_amount, "failed replay attempt should not change claimed amount"); } @@ -621,16 +621,16 @@ fn claim_allowed_in_different_ledgers() { // First claim at ledger 100 e.ledger().set_timestamp(start_time + 200); - e.ledger().set_sequence(100); + e.ledger().set_sequence_number(100); client.claim(); - let claimed_at_ledger_100 = client.get_config().total_claimed; + let claimed_at_ledger_100 = client.get_config().claimed_amount; - // Advance to ledger 101 and claim again — this should succeed + // Advance to ledger 101 and claim again — this should succeed // (different ledger sequence means not a replay) e.ledger().set_timestamp(start_time + 400); - e.ledger().set_sequence(101); + e.ledger().set_sequence_number(101); client.claim(); - let claimed_at_ledger_101 = client.get_config().total_claimed; + let claimed_at_ledger_101 = client.get_config().claimed_amount; assert!(claimed_at_ledger_101 > claimed_at_ledger_100, "claim in different ledger should succeed and increase claimed amount"); @@ -642,7 +642,7 @@ fn same_ledger_clawback_replay_detected() { let start_time = 1_000u64; e.ledger().set_timestamp(start_time); - e.ledger().set_sequence(100); + e.ledger().set_sequence_number(100); client.initialize( &funder, @@ -677,7 +677,7 @@ fn partial_clawback_replay_protection() { let start_time = 1_000u64; e.ledger().set_timestamp(start_time); - e.ledger().set_sequence(100); + e.ledger().set_sequence_number(100); client.initialize( &funder, @@ -713,7 +713,7 @@ fn cross_ledger_replay_test_with_realistic_sequence() { e.ledger().set_timestamp(start_time); // Simulate realistic ledger sequences (Stellar creates ledgers ~every 5 seconds) - e.ledger().set_sequence(50_000_000); // Mainnet-realistic sequence + e.ledger().set_sequence_number(50_000_000); // Mainnet-realistic sequence client.initialize( &funder, @@ -733,22 +733,22 @@ fn cross_ledger_replay_test_with_realistic_sequence() { let contract_id = e.register(VestingContract, ()); let first_balance = token_client.balance(&beneficiary); - // Try to claim again in same ledger — should fail + // Try to claim again in same ledger — should fail let result = client.try_claim(); assert_eq!(result, Err(Ok(ContractError::LedgerReplayDetected))); // Advance ~12 seconds (realistic ledger interval) e.ledger().set_timestamp(start_time + 112); - e.ledger().set_sequence(50_000_012); + e.ledger().set_sequence_number(50_000_012); - // Claim in new ledger — should succeed + // Claim in new ledger — should succeed client.claim(); let second_balance = token_client.balance(&beneficiary); assert!(second_balance > first_balance, "claim in later ledger should succeed and increase balance"); } -// ── PROPERTY-BASED TESTS FOR CLIFF LOGIC ────────────────────────────────────── +// ── PROPERTY-BASED TESTS FOR CLIFF LOGIC ────────────────────────────────────── #[cfg(test)] mod cliff_properties { @@ -1000,3 +1000,92 @@ mod cliff_properties { } } } + + +// -- TIMESTAMP MANIPULATION RESISTANCE TESTS (Issue #1615) -------------------- + +#[test] +fn claim_rejects_timestamp_regression() { + let (e, funder, beneficiary, clawback_admin, admin, token_contract, _, _, client) = setup(); + + let start_time = 1_000u64; + e.ledger().set_timestamp(start_time); + e.ledger().set_sequence_number(100); + + client.initialize( + &funder, + &beneficiary, + &token_contract, + &start_time, + &100u64, + &1000u64, + &10_000i128, + &clawback_admin, + &admin, + ); + + // Legitimate claim at a forward-moving timestamp. + e.ledger().set_timestamp(start_time + 200); + e.ledger().set_sequence_number(101); + client.claim(); + let claimed_before = client.get_config().claimed_amount; + assert!(claimed_before > 0, "first claim should succeed and record a vested amount"); + + // Simulate an anomalous/manipulated ledger timestamp moving backward on a + // later ledger. This must be rejected rather than silently recomputing a + // smaller (or stale) vested amount. + e.ledger().set_timestamp(start_time + 50); + e.ledger().set_sequence_number(102); + let result = client.try_claim(); + assert_eq!(result, Err(Ok(ContractError::TimestampRegression)), + "a backward-moving ledger timestamp must be rejected"); + + // State must be unchanged after the rejected call. + let claimed_after = client.get_config().claimed_amount; + assert_eq!(claimed_before, claimed_after, + "rejected timestamp-regression claim must not alter claimed amount"); + + // A subsequent claim at a timestamp that is forward of the last observed + // value (not just forward of the manipulated one) must still succeed. + e.ledger().set_timestamp(start_time + 400); + e.ledger().set_sequence_number(103); + client.claim(); + let claimed_final = client.get_config().claimed_amount; + assert!(claimed_final > claimed_before, + "claim should succeed again once the timestamp legitimately moves forward"); +} + +#[test] +fn clawback_rejects_timestamp_regression() { + let (e, funder, beneficiary, clawback_admin, admin, token_contract, _, _, client) = setup(); + + let start_time = 1_000u64; + e.ledger().set_timestamp(start_time); + e.ledger().set_sequence_number(200); + + client.initialize( + &funder, + &beneficiary, + &token_contract, + &start_time, + &100u64, + &1000u64, + &10_000i128, + &clawback_admin, + &admin, + ); + + e.ledger().set_timestamp(start_time + 200); + e.ledger().set_sequence_number(201); + client.claim(); + + // Move the clock backward before the clawback call. + e.ledger().set_timestamp(start_time + 50); + e.ledger().set_sequence_number(202); + let result = client.try_clawback(); + assert_eq!(result, Err(Ok(ContractError::TimestampRegression)), + "clawback must also reject a backward-moving ledger timestamp"); + + let config = client.get_config(); + assert!(config.is_active, "rejected clawback must not deactivate the grant"); +} \ No newline at end of file From 9094e4667817b74300c3165064f790c7e0f138bc Mon Sep 17 00:00:00 2001 From: karanjadavi Date: Sun, 27 Sep 2026 15:41:49 +0300 Subject: [PATCH 2/2] docs(milestone_escrow): note timestamp-regression review for #1615 --- contracts/milestone_escrow/README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/milestone_escrow/README.md b/contracts/milestone_escrow/README.md index 7fb67013..e7857faa 100644 --- a/contracts/milestone_escrow/README.md +++ b/contracts/milestone_escrow/README.md @@ -144,3 +144,7 @@ soroban contract invoke \ - **`vesting_escrow`**: Provides time-based vesting, whereas `milestone_escrow` provides performance-based verification. - **`bulk_payment`**: Can trigger batch escrow creation for project teams. - **`orgusd`**: Supported as escrow token asset. + +## Timestamp-Manipulation Review (Issue #1615) + +`created_at` is recorded via `e.ledger().timestamp()` at escrow creation for informational/audit purposes only. Milestone release is triggered by verification calls, not by comparing timestamps, so this contract is not exposed to the validator-timestamp regression risk addressed in `vesting_escrow`. No code changes required here.