test(contracts): add overflow regression coverage

[zainabwahab-eth]

Closes #1297
Closes #1224

Summary

Adds boundary-value regression tests for all five unchecked arithmetic sites identified by the audit, plus the checked-arithmetic fix they assert against.

Why both are in one PR. #1297 asks for tests that assert a typed Err at each site. That typed error did not exist on main: #1224 ("Functional Edge Case #22") is open with no PR, StreamError had no overflow variant, and all five sites still used plain += / * / +. overflow-checks is on in both the release profile the WASM ships with and the dev profile cargo test uses, so an overflow at any of them aborted the whole transaction instead of returning an error. Tests written to #1297's spec cannot pass without #1224, and weakening them to #[should_panic] would defeat the point of the issue, so the fix lands here too.

The five sites

Issue #1224 cites lib.rs:309, 317, 425, 647, 707; the file has grown since, so these are the current locations on main:

#	Site	Was	Now
1	collect_fee	amount * (cfg.fee_rate_bps as i128)	checked_mul
2	top_up_stream	stream.deposited_amount += net_amount	checked_add
3	apply_withdrawal	stream.withdrawn_amount += amount	checked_add
4	top_up_stream	now + (remaining / rate) as u64	project_end_time
5	resume_stream	now + (remaining / rate) as u64	project_end_time

Production changes

• errors.rs: new ArithmeticOverflow = 15. No new error type was invented for the tests; this is [Audit] Unchecked arithmetic in fee/top-up/withdrawal paths panics instead of returning a typed error #1224's canonical overflow error and every test uses it.
• collect_fee now returns Result<i128, StreamError>; the ? is propagated at its four call sites (create_stream, batch_create_streams, create_stream_with_cliff, top_up_stream). The amount - fee subtraction is left as-is: fee_rate_bps is capped at MAX_FEE_RATE_BPS (10%), so fee is always well below amount.
• apply_withdrawal now returns Result<(), StreamError>. The checked add is the first statement, so an overflow returns before any state mutation, storage write or transfer — the CEI ordering is unchanged.
• New private helper project_end_time(now, remaining, rate_per_second). The old expression truncated the quotient with as u64 and then panicked on the addition; the helper rejects a quotient that does not fit in u64 via u64::try_from and uses checked_add for the timestamp.
• Entrypoint doc comments list ArithmeticOverflow under # Errors.

Tests

Test	Site	Boundary
test_create_stream_rejects_fee_multiplication_overflow	1	amount = i128::MAX at MAX_FEE_RATE_BPS
test_top_up_rejects_deposited_amount_overflow	2	deposited_amount = i128::MAX - 1
test_withdraw_at_i128_max_withdrawn_boundary_does_not_overflow	3	withdrawn + claimable == i128::MAX
test_top_up_rejects_end_time_projection_overflow	4	drain time > u64::MAX seconds
test_resume_rejects_end_time_projection_overflow	5	drain time > u64::MAX seconds

The boundary values differ per site, matching what each operation actually needs to overflow: i128::MAX for the fee multiplication and the deposited total, and a remaining balance of 3 * 2^64 - 101 at one unit per second for the two u64 timestamp projections. That constant is chosen so the pre-fix as u64 truncation yields 2^64 - 101, which then overflows now + … for any now > 100.

Tests follow the existing conventions: client.try_* asserted against Err(Ok(StreamError::…)), and a force_stream helper that parks a stream one step below the ceiling via env.as_contract + persistent storage — the same technique test_claimable_max_i128_rate_overflow and test_calculate_claimable_underflow_returns_zero already use.

One honest deviation on site 3

apply_withdrawal's withdrawn_amount += cannot overflow. calculate_claimable clamps its result to deposited_amount - withdrawn_amount, which makes withdrawn + claimable <= deposited <= i128::MAX an invariant of every reachable call. Forced state does not open a path either: remaining only reaches i128::MAX when withdrawn is negative, and then the sum is bounded from above anyway. The audit flagged the site as unchecked, which is correct, but not as reachable.

Rather than fabricate an Err for it, its test pins the exact state where the sum lands on i128::MAX and asserts the withdrawal succeeds and completes the stream. If a future change to that clamp does let this site overflow, the test fails. The site is still converted to checked_add, since #1224 names it.

So: four of the five tests fail against the unfixed code, one passes either way and guards the invariant. Stated plainly rather than claimed as five.

Testing

All commands run in a rust:1.85 container against contracts/ (the local Windows toolchain cannot build — Application Control blocks cargo from executing build scripts, os error 4551).

cargo fmt --all -- --check              -> clean
cargo clippy --all-targets -- -D warnings -> clean, no warnings
cargo build --target wasm32-unknown-unknown --release
                                        -> Finished; stream_contract.wasm = 41,055 bytes (budget 200,000)
cargo test                              -> 123 passed; 0 failed; 0 ignored
cargo test overflow                     -> 8 passed; 0 failed  (5 new + 3 pre-existing)

Baseline for comparison: cargo test on this branch with only the production fix and no new tests was 118 passed, 0 failed, so the fix breaks nothing and the five new tests take it to 123.

Proof the tests catch the bug

With contracts/stream_contract/src/lib.rs reverted to main and the new tests left in place:

test test::test_create_stream_rejects_fee_multiplication_overflow ... FAILED
test test::test_top_up_rejects_deposited_amount_overflow ... FAILED
test test::test_top_up_rejects_end_time_projection_overflow ... FAILED
test test::test_resume_rejects_end_time_projection_overflow ... FAILED
test test::test_withdraw_at_i128_max_withdrawn_boundary_does_not_overflow ... ok
test test::test_claimable_max_i128_rate_overflow ... ok
test test::test_fuzz_large_amount_no_overflow ... ok
test test::test_fuzz_claimable_overflow_and_cancel_invariants ... ok

test result: FAILED. 4 passed; 4 failed

The failure mode is exactly what the audit describes — the overflow aborts the transaction instead of returning an error:

assertion `left == right` failed
  left: Err(Err(Abort))
 right: Err(Ok(ArithmeticOverflow))

Scope

Production contract logic was changed — this PR implements #1224. That is a deliberate deviation from #1297 being test-only, because #1297's acceptance criteria are unreachable without it.

Changes are confined to contracts/stream_contract/: errors.rs (+5), lib.rs (+78/-15), test.rs (+205), README.md (+4). No other audit issue was fixed.

The README edit fills in the StreamError code table, which stopped at 11 and was already missing 12–14; adding only row 15 would have left a visible gap in a table this PR has to touch anyway.

Pre-existing issue found, not fixed

transfer_recipient at lib.rs:456 has a sixth unchecked stream.withdrawn_amount += settled_amount of the same class. It is not among #1224's five — it arrived later, in PR #1343. Like site 3 it is bounded by the calculate_claimable clamp and cannot overflow, so it is reported here rather than changed.

Acceptance criteria

• All five unchecked arithmetic sites have boundary tests
• Tests use per-site boundary values (i128::MAX for the amount sites, a > u64::MAX drain time for the timestamp sites) rather than one constant everywhere
• Tests assert a typed Err (ArithmeticOverflow), not a panic — with the documented exception of site 3, which asserts the boundary succeeds because overflow there is unreachable
• Four of five fail against the unfixed implementation; verified above
• Existing overflow/fuzz tests still pass — test_claimable_max_i128_rate_overflow, test_fuzz_large_amount_no_overflow, test_fuzz_claimable_overflow_and_cancel_invariants all green and unmodified
• Contract test suite passes (123/123)
• cargo fmt, cargo clippy -D warnings, and the release WASM build all pass

🤖 Generated with Claude Code