diff --git a/contracts/stream_contract/README.md b/contracts/stream_contract/README.md index fbc13242..c3f05421 100644 --- a/contracts/stream_contract/README.md +++ b/contracts/stream_contract/README.md @@ -103,6 +103,10 @@ Error codes from `src/errors.rs`: | 9 | `InvalidDuration` | Duration is zero | | 10 | `InvalidTokenAddress` | Token address is not a token contract | | 11 | `InvalidRate` | `amount / duration` rounds to zero | +| 12 | `StreamPaused` | Operation requires a stream that is not paused | +| 13 | `StreamNotPaused` | `resume_stream` called on a stream that is not paused | +| 14 | `StreamAlreadyPaused` | `pause_stream` called on an already-paused stream | +| 15 | `ArithmeticOverflow` | An amount or timestamp calculation left its type's range | ## Test Snapshots diff --git a/contracts/stream_contract/src/errors.rs b/contracts/stream_contract/src/errors.rs index 596d68d3..0afa21c9 100644 --- a/contracts/stream_contract/src/errors.rs +++ b/contracts/stream_contract/src/errors.rs @@ -35,4 +35,9 @@ pub enum StreamError { StreamNotPaused = 13, /// `pause_stream` was called on a stream that is already paused. StreamAlreadyPaused = 14, + /// An amount or timestamp calculation exceeded the range of its type. + /// + /// Returned instead of letting `overflow-checks` panic and abort the whole + /// transaction, so callers get a typed failure they can handle. + ArithmeticOverflow = 15, } diff --git a/contracts/stream_contract/src/lib.rs b/contracts/stream_contract/src/lib.rs index e4451bdd..6bf2a293 100644 --- a/contracts/stream_contract/src/lib.rs +++ b/contracts/stream_contract/src/lib.rs @@ -205,6 +205,7 @@ impl StreamContract { /// - `InvalidDuration` — `duration` is 0. /// - `InvalidRate` — `net_amount / duration` rounds to zero. /// - `InvalidTokenAddress` — `token_address` is not a token contract. + /// - `ArithmeticOverflow` — the protocol fee calculation overflows `i128`. pub fn create_stream( env: Env, sender: Address, @@ -232,7 +233,7 @@ impl StreamContract { token_client.transfer(&sender, &contract_address, &amount); // Deduct protocol fee; returns net amount (== amount when no fee config). - let net_amount = Self::collect_fee(&env, &token_address, amount, stream_id); + let net_amount = Self::collect_fee(&env, &token_address, amount, stream_id)?; let rate_per_second = net_amount / (duration as i128); // Reject streams where integer division rounds the rate to zero. @@ -338,7 +339,8 @@ impl StreamContract { for input in streams.iter() { let stream_id = next_stream_id(&env); let start_time = env.ledger().timestamp(); - let net_amount = Self::collect_fee(&env, &input.token_address, input.amount, stream_id); + let net_amount = + Self::collect_fee(&env, &input.token_address, input.amount, stream_id)?; let cliff_time = input.cliff_duration.map(|duration| start_time + duration); let stream = Stream { sender: sender.clone(), @@ -395,7 +397,7 @@ impl StreamContract { let start_time = env.ledger().timestamp(); let contract_address = env.current_contract_address(); token::Client::new(&env, &token_address).transfer(&sender, &contract_address, &amount); - let net_amount = Self::collect_fee(&env, &token_address, amount, stream_id); + let net_amount = Self::collect_fee(&env, &token_address, amount, stream_id)?; let rate_per_second = net_amount / duration as i128; if rate_per_second == 0 { return Err(StreamError::InvalidRate); @@ -494,6 +496,8 @@ impl StreamContract { /// - `StreamNotFound` — no stream exists with `stream_id`. /// - `Unauthorized` — caller is not the stream's sender. /// - `StreamInactive` — stream has been cancelled or fully withdrawn. + /// - `ArithmeticOverflow` — the fee calculation, the new deposited total, + /// or the projected end time overflows. pub fn top_up_stream( env: Env, sender: Address, @@ -518,12 +522,15 @@ impl StreamContract { token_client.transfer(&sender, &contract_address, &amount); // Collect protocol fee and get net amount - let net_amount = Self::collect_fee(&env, &stream.token_address, amount, stream_id); + let net_amount = Self::collect_fee(&env, &stream.token_address, amount, stream_id)?; // Update stream state. `last_update_time` is intentionally left untouched: // it is the accrual anchor for `calculate_claimable`, and advancing it to // `now` would discard any already-vested, unwithdrawn tokens. - stream.deposited_amount += net_amount; + stream.deposited_amount = stream + .deposited_amount + .checked_add(net_amount) + .ok_or(StreamError::ArithmeticOverflow)?; let now = env.ledger().timestamp(); let claimable = Self::calculate_claimable(&stream, now); @@ -531,7 +538,7 @@ impl StreamContract { .deposited_amount .saturating_sub(stream.withdrawn_amount) .saturating_sub(claimable); - let new_end_time = now + (remaining / stream.rate_per_second) as u64; + let new_end_time = Self::project_end_time(now, remaining, stream.rate_per_second)?; save_stream(&env, stream_id, &stream); @@ -619,6 +626,27 @@ impl StreamContract { Ok(()) } + /// Project the timestamp at which `remaining` tokens finish draining at + /// `rate_per_second`, starting from `now`. + /// + /// Both steps are checked. A balance large enough to drain for more than + /// `u64::MAX` seconds, or a projection that runs past the end of the u64 + /// timestamp range, returns `ArithmeticOverflow`. The plain + /// `now + (remaining / rate) as u64` this replaces silently truncated the + /// quotient and then panicked on the addition under `overflow-checks`, + /// aborting an otherwise valid top-up or resume. + fn project_end_time( + now: u64, + remaining: i128, + rate_per_second: i128, + ) -> Result { + let seconds_remaining = u64::try_from(remaining / rate_per_second) + .map_err(|_| StreamError::ArithmeticOverflow)?; + + now.checked_add(seconds_remaining) + .ok_or(StreamError::ArithmeticOverflow) + } + /// Validate that a stream is active. /// /// # Errors @@ -643,9 +671,13 @@ impl StreamContract { recipient: &Address, amount: i128, now: u64, - ) { - // Effects: update stream state - stream.withdrawn_amount += amount; + ) -> Result<(), StreamError> { + // Effects: update stream state. The checked add runs before any state + // mutation or transfer, so an overflow leaves the stream untouched. + stream.withdrawn_amount = stream + .withdrawn_amount + .checked_add(amount) + .ok_or(StreamError::ArithmeticOverflow)?; stream.last_update_time = now; if stream.withdrawn_amount >= stream.deposited_amount { @@ -659,6 +691,8 @@ impl StreamContract { // Interaction: transfer tokens only after state is committed to storage let token_client = token::Client::new(env, &stream.token_address); token_client.transfer(&env.current_contract_address(), recipient, &amount); + + Ok(()) } /// Withdraw all currently claimable tokens from a stream. @@ -672,6 +706,7 @@ impl StreamContract { /// - `Unauthorized` — caller is not the stream's recipient. /// - `StreamInactive` — stream is already inactive. /// - `InvalidAmount` — no claimable balance (fully withdrawn already). + /// - `ArithmeticOverflow` — the new withdrawn total overflows `i128`. pub fn withdraw(env: Env, recipient: Address, stream_id: u64) -> Result { recipient.require_auth(); @@ -696,7 +731,7 @@ impl StreamContract { } // Apply withdrawal: updates state, persists to storage, then transfers (CEI) - Self::apply_withdrawal(&env, &mut stream, stream_id, &recipient, claimable, now); + Self::apply_withdrawal(&env, &mut stream, stream_id, &recipient, claimable, now)?; let completed = stream.status == StreamStatus::Completed; @@ -853,6 +888,7 @@ impl StreamContract { /// - `StreamNotFound` — no stream exists with `stream_id`. /// - `Unauthorized` — caller is not the stream's sender. /// - `StreamNotPaused` — stream is active but not currently paused. + /// - `ArithmeticOverflow` — the projected end time overflows `u64`. pub fn resume_stream(env: Env, sender: Address, stream_id: u64) -> Result { sender.require_auth(); @@ -881,7 +917,7 @@ impl StreamContract { .saturating_sub(stream.withdrawn_amount) .saturating_sub(claimable_at_resume); // rate_per_second is guaranteed >= 1 due to create_stream's InvalidRate guard - let new_end_time = now + (remaining / stream.rate_per_second) as u64; + let new_end_time = Self::project_end_time(now, remaining, stream.rate_per_second)?; stream.paused = false; stream.paused_at = None; @@ -938,10 +974,20 @@ impl StreamContract { /// If no protocol config exists or the fee rate is 0, returns `amount` unchanged. /// If fee calculation truncates to 0, no transfer/event occurs and `amount` is unchanged. /// Time complexity: O(1). - fn collect_fee(env: &Env, token_address: &Address, amount: i128, stream_id: u64) -> i128 { + fn collect_fee( + env: &Env, + token_address: &Address, + amount: i128, + stream_id: u64, + ) -> Result { match try_load_config(env) { Some(cfg) if cfg.fee_rate_bps > 0 => { - let fee = amount * (cfg.fee_rate_bps as i128) / 10_000; + // `amount` is caller-supplied and can reach i128::MAX, so the + // bps multiplication is the first thing that would overflow. + let fee = amount + .checked_mul(cfg.fee_rate_bps as i128) + .ok_or(StreamError::ArithmeticOverflow)? + / 10_000; if fee > 0 { let token_client = token::Client::new(env, token_address); token_client.transfer(&env.current_contract_address(), &cfg.treasury, &fee); @@ -955,9 +1001,11 @@ impl StreamContract { }, ); } - amount - fee + // `fee_rate_bps` is capped at MAX_FEE_RATE_BPS (10%), so `fee` + // is always well below `amount` and this cannot underflow. + Ok(amount - fee) } - _ => amount, + _ => Ok(amount), } } } diff --git a/contracts/stream_contract/src/test.rs b/contracts/stream_contract/src/test.rs index fdad22b2..b4e3d541 100644 --- a/contracts/stream_contract/src/test.rs +++ b/contracts/stream_contract/src/test.rs @@ -2874,3 +2874,208 @@ fn test_stream_created_event_field_names_match_decoder_expectations() { "stream_created event fields drifted from soroban-event-worker.ts's decodeMap expectations" ); } + +// ─── #1297 Overflow regressions for the #1224 unchecked arithmetic sites ────── +// +// Issue #1224 ("Functional Edge Case #22") identified five call sites that used +// plain `+=` / `*` / `+` while the rest of the file uses checked or saturating +// arithmetic. `overflow-checks` is on for both the release profile the WASM +// ships with and the dev profile these tests run under, so an overflow at any +// of them panicked and aborted the whole transaction instead of returning a +// `StreamError`. The tests below pin each site at its boundary and assert the +// typed `ArithmeticOverflow` error. +// +// 1. `collect_fee` — `amount * fee_rate_bps` +// 2. `top_up_stream` — `deposited_amount +=` +// 3. `apply_withdrawal` — `withdrawn_amount +=` +// 4. `top_up_stream` — `now + (remaining / rate) as u64` +// 5. `resume_stream` — `now + (remaining / rate) as u64` + +/// Overwrites a stream record in place. +/// +/// Reaching an i128 boundary through the public API alone would take an +/// impractical number of calls, so these tests park the stream one step below +/// the ceiling and then drive the real entrypoint across it. Same technique as +/// `test_claimable_max_i128_rate_overflow` and +/// `test_calculate_claimable_underflow_returns_zero` above. +fn force_stream(env: &Env, client: &StreamContractClient<'_>, stream_id: u64, stream: &Stream) { + env.as_contract(&client.address, || { + env.storage() + .persistent() + .set(&types::DataKey::Stream(stream_id), stream); + }); +} + +/// Site 1 — `collect_fee`: `amount * (cfg.fee_rate_bps as i128)`. +/// +/// At the maximum fee rate the multiplication overflows for any amount above +/// `i128::MAX / 1_000`, so `i128::MAX` is well past the boundary. +#[test] +fn test_create_stream_rejects_fee_multiplication_overflow() { + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + let recipient = Address::generate(&env); + mint(&env, &token, &sender, i128::MAX); + + let client = create_contract(&env); + client.initialize( + &Address::generate(&env), + &Address::generate(&env), + &MAX_FEE_RATE_BPS, + ); + + assert_eq!( + client.try_create_stream(&sender, &recipient, &token, &i128::MAX, &1_000), + Err(Ok(StreamError::ArithmeticOverflow)) + ); +} + +/// Site 2 — `top_up_stream`: `stream.deposited_amount += net_amount`. +/// +/// The stream is parked one unit below `i128::MAX`, so any positive top-up +/// pushes the deposited total out of range. +#[test] +fn test_top_up_rejects_deposited_amount_overflow() { + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + mint(&env, &token, &sender, 20_000); + + let client = create_contract(&env); + let id = client.create_stream(&sender, &Address::generate(&env), &token, &10_000, &100); + + let mut stream = client.get_stream(&id).unwrap(); + stream.deposited_amount = i128::MAX - 1; + force_stream(&env, &client, id, &stream); + + assert_eq!( + client.try_top_up_stream(&sender, &id, &5_000), + Err(Ok(StreamError::ArithmeticOverflow)) + ); +} + +/// Site 3 — `apply_withdrawal`: `stream.withdrawn_amount += amount`. +/// +/// Exercised at the boundary rather than past it. `calculate_claimable` clamps +/// its result to `deposited_amount - withdrawn_amount`, which makes +/// `withdrawn_amount + claimable <= deposited_amount <= i128::MAX` an invariant +/// of every reachable call, so no input can push this site over. The test pins +/// the exact state where the sum lands on `i128::MAX`: the checked add must +/// succeed and the withdrawal must complete, so a future change to that clamp +/// which does let this site overflow surfaces here as a test failure instead of +/// as an aborted transaction in production. +#[test] +fn test_withdraw_at_i128_max_withdrawn_boundary_does_not_overflow() { + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + let recipient = Address::generate(&env); + mint(&env, &token, &sender, 20_000); + + let client = create_contract(&env); + let id = client.create_stream(&sender, &recipient, &token, &10_000, &100); + + // 1 000 units still claimable, and withdrawn + claimable lands exactly on + // i128::MAX. The huge rate makes `streamed` exceed `remaining`, so the + // clamp rather than the elapsed time decides the amount. + let mut stream = client.get_stream(&id).unwrap(); + stream.deposited_amount = i128::MAX; + stream.withdrawn_amount = i128::MAX - 1_000; + stream.rate_per_second = i128::MAX; + force_stream(&env, &client, id, &stream); + + env.ledger().with_mut(|l| l.timestamp += 10); + + assert_eq!(client.try_withdraw(&recipient, &id), Ok(Ok(1_000))); + + let settled = client.get_stream(&id).unwrap(); + assert_eq!(settled.withdrawn_amount, i128::MAX); + assert!(!settled.is_active); + assert_eq!(settled.status, StreamStatus::Completed); +} + +/// Remaining balance whose drain time cannot be represented as a `u64`. +/// +/// `Q = 3 * 2^64 - 101`. At one unit per second the stream needs `Q` seconds to +/// drain, which is past `u64::MAX`. The pre-fix code truncated that quotient +/// with `as u64`, giving `2^64 - 101`, then panicked on `now + (2^64 - 101)` +/// for any `now > 100`. The fixed code rejects the quotient before it is ever +/// truncated. +const END_TIME_OVERFLOW_REMAINING: i128 = 3 * (1_i128 << 64) - 101; + +/// Ledger timestamp for the two end-time tests. Any value above 100 makes the +/// pre-fix truncated addition overflow. +const END_TIME_OVERFLOW_NOW: u64 = 1_000; + +/// Site 4 — `top_up_stream`: `now + (remaining / rate_per_second) as u64`. +#[test] +fn test_top_up_rejects_end_time_projection_overflow() { + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + mint(&env, &token, &sender, 20_000); + + let client = create_contract(&env); + let id = client.create_stream(&sender, &Address::generate(&env), &token, &10_000, &100); + + env.ledger() + .with_mut(|l| l.timestamp = END_TIME_OVERFLOW_NOW); + + // A 10-unit top-up brings the deposited balance to exactly Q. Anchoring + // last_update_time at `now` keeps the claimable amount at 0, so the whole + // balance counts as remaining. + let mut stream = client.get_stream(&id).unwrap(); + stream.deposited_amount = END_TIME_OVERFLOW_REMAINING - 10; + stream.withdrawn_amount = 0; + stream.rate_per_second = 1; + stream.last_update_time = END_TIME_OVERFLOW_NOW; + force_stream(&env, &client, id, &stream); + + assert_eq!( + client.try_top_up_stream(&sender, &id, &10), + Err(Ok(StreamError::ArithmeticOverflow)) + ); +} + +/// Site 5 — `resume_stream`: `now + (remaining / rate_per_second) as u64`. +#[test] +fn test_resume_rejects_end_time_projection_overflow() { + let env = Env::default(); + env.mock_all_auths(); + + let (token, _) = create_token(&env); + let sender = Address::generate(&env); + mint(&env, &token, &sender, 20_000); + + let client = create_contract(&env); + let id = client.create_stream(&sender, &Address::generate(&env), &token, &10_000, &100); + + env.ledger() + .with_mut(|l| l.timestamp = END_TIME_OVERFLOW_NOW); + + // Paused with paused_at == last_update_time, so nothing accrued while + // paused and the full balance is still remaining at resume. + let mut stream = client.get_stream(&id).unwrap(); + stream.deposited_amount = END_TIME_OVERFLOW_REMAINING; + stream.withdrawn_amount = 0; + stream.rate_per_second = 1; + stream.last_update_time = 500; + stream.paused = true; + stream.paused_at = Some(500); + stream.status = StreamStatus::Paused; + force_stream(&env, &client, id, &stream); + + assert_eq!( + client.try_resume_stream(&sender, &id), + Err(Ok(StreamError::ArithmeticOverflow)) + ); +}