fix: repair Rust contract merge corruption, fix vesting/auth bugs#215
Merged
Merged
Conversation
CI has been red on main: the test build failed on a duplicate formatUsd export, eslint-plugin-security was incompatible with ESLint 10 (breaking lint and the pre-commit hook for every contributor), and Next 16's Turbopack default conflicts with Sentry's webpack config. Several files also had unresolved merge conflicts silently merged in — duplicate declarations, dead code after early returns, and in two cases an entire abandoned feature branch pasted on top of the real (already-split) page implementation. Restored the intended page.tsx/dashboard.tsx and page.tsx/create-form.tsx server/client splits, finished the create-form.tsx split that had been left as a placeholder stub, and removed five orphaned files that never compiled and were never imported anywhere. tsc --noEmit was fully masked by `ignoreBuildErrors: true`; removing it turned up real bugs now fixed: Sentry's beforeSend was silently crashing (targeted the pre-v8 breadcrumb shape, so wallet addresses were never actually being redacted), fee estimation read a `.cost` field the current SDK type doesn't have, and a stream card had a <div> nested inside a <p> causing a hydration warning. Removing ignoreBuildErrors also means the build now fails loudly on this class of bug instead of merging silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The streaming contract has had the same "both sides of a merge conflict kept" corruption as the frontend, recurring across at least 5 separate merges — deep enough that Contract CI and the security scan have likely never actually passed on main. Restored a single, correct version of initialize/pause/unpause/withdraw/transfer_stream/bump_stream/create_stream and the delegate functions (including a missing DataKey::Delegate variant), plus the equivalent duplicate/orphaned test functions and wrong assertions in the test files. Two real security bugs surfaced during the repair, both fixed: - migrate() fetched the admin from storage but never checked their authorization, so anyone could call it to silently un-pause the contract, bypassing the admin-only pause/incident-response mechanism. - top_up()'s vesting bookkeeping was inconsistent with what withdraw/ cancel actually use (unlocked_amount): funds added via top_up sat invisible until end_time instead of streaming out progressively, and a second top-up could retroactively re-apply the new rate across the entire elapsed time, overstating vesting. Fixed by re-anchoring the schedule off unlocked_amount on every top-up, with a regression test reproducing the original bug. Also: unchecked += replaced with checked_add on withdrawn_amount, remove_from_index now extends TTL like its push_to_index counterpart, two false positives fixed in the repo's own Soroban security-pattern script (name() flagged as an unauthenticated write), and .lintstagedrc's "cargo fmt --check" fixed to "rustfmt --check" — the former silently failed on every staged .rs file since cargo fmt doesn't accept file-path arguments, so the pre-commit hook never actually checked Rust formatting. Ran cargo fmt across the crate accordingly. Dropped the create-form UI fields for stream metadata (name/category/ memo) — they built a local object that was never actually sent in the create_stream call, so they silently discarded whatever a user typed. Verified: cargo test (121/121), cargo clippy --all-targets -D warnings, cargo audit, cargo build --target wasm32v1-none --release, and the frontend build/test suite all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🔐 Contract Security Scancargo auditSoroban pattern check |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The streaming contract has had the same "both sides of a merge conflict kept" corruption pattern as the frontend (fixed in a separate commit), recurring across at least 5 separate merges going back weeks — deep enough that
Contract CIand theSecurity Scanworkflow have almost certainly never actually passed onmain.initialize,pause/unpause,withdraw,transfer_stream,bump_stream,create_stream, and the delegate functions (including aDataKey::Delegateenum variant that was referenced everywhere but never actually declared), plus the equivalent duplicate/orphaned test functions and wrong assertions in the test files.migrate()fetched the admin from storage but never checked their authorization — anyone could call it to silently un-pause the contract, bypassing the admin-only pause/incident-response mechanism.top_up()'s vesting bookkeeping was inconsistent with whatwithdraw/cancelactually use (unlocked_amount): funds added viatop_upsat invisible untilend_timeinstead of streaming out progressively, and a second top-up could retroactively re-apply the new rate across the entire elapsed time, overstating vesting. Fixed by re-anchoring the schedule offunlocked_amounton every top-up, with a regression test reproducing the original bug.+=replaced withchecked_addonwithdrawn_amount;remove_from_indexnow extends TTL like itspush_to_indexcounterpart.name()flagged as an unauthenticated write)..lintstagedrc'scargo fmt --checkfixed torustfmt --check— the former silently failed on every staged.rsfile sincecargo fmtdoesn't accept file-path arguments, meaning the pre-commit hook never actually checked Rust formatting. Rancargo fmtacross the crate accordingly.create_streamcall, so they silently discarded whatever a user typed.Test plan
cargo test— 121/121 passingcargo clippy --all-targets -- -D warnings— cleancargo audit— no vulnerabilitiescargo build --target wasm32v1-none --release— succeedsnode scripts/soroban-security-check.mjs— no HIGH severity findingsnpm test/npm run build— unaffected, still passing🤖 Generated with Claude Code