feat(contracts): guard payment streams against reentrancy - #565
feat(contracts): guard payment streams against reentrancy#565numdinkushi wants to merge 1 commit into
Conversation
Add shared state locks, TTL handling, and malicious callback coverage for payment stream mutations while pinning compatible contract dependencies.
|
@numdinkushi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds a shared Soroban reentrancy guard, integrates it into NFT and payment stream contracts, extends NFT stream storage TTLs, adds callback rejection tests, updates workspace wiring, and performs distributor compatibility cleanup. ChangesContract safety and workspace integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StreamContract
participant TokenContract
participant ReentrancyGuard
StreamContract->>ReentrancyGuard: acquire lock
StreamContract->>TokenContract: invoke token transfer
TokenContract->>StreamContract: invoke callback
StreamContract->>ReentrancyGuard: acquire existing lock
ReentrancyGuard-->>StreamContract: reject acquisition
StreamContract-->>TokenContract: return ReentrantCall
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/libs/common/src/lib.rs (1)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for the public guard API.
Document
ReentrancyGuardandReentrancyGuard::acquire, including that the guard releases the instance lock when dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/libs/common/src/lib.rs` around lines 7 - 12, Add rustdoc comments for the public ReentrancyGuard struct and its acquire method. Document the guard’s purpose, acquisition behavior, and that dropping the returned guard releases the instance lock.contracts/nft-stream/src/lib.rs (1)
150-197: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMove the escrow transfer after stream persistence (checks-effects-interactions).
token_client.transfer(...)at lines 164-165 runs before the stream counter, ownership record, andStreamare persisted (lines 167-197). The reentrancy guard already blocks any reentrant call into a guarded entrypoint during this window, so this isn't currently exploitable, but it diverges frompayment-stream's safer pattern (itscreate_streamtransfers tokens only after all state is written). Moving the transfer to the end hardens against any future path that bypasses the guard (e.g., a new unguarded mutator) and keeps both contracts consistent.♻️ Suggested reordering
- let token_client = token::Client::new(&env, &token); - token_client.transfer(&sender, &env.current_contract_address(), &total_amount); - let stream_id: u64 = env .storage() .instance() .get(&DataKey::StreamCounter) .unwrap_or(0); @@ env.storage() .persistent() .set(&DataKey::Stream(new_stream_id), &stream); Self::extend_persistent_ttl(&env, &DataKey::Stream(new_stream_id)); Self::extend_instance_ttl(&env); + + let token_client = token::Client::new(&env, &token); + token_client.transfer(&sender, &env.current_contract_address(), &total_amount);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/nft-stream/src/lib.rs` around lines 150 - 197, Move the token_client.transfer call in the stream-creation function to after the stream counter, ownership record, Stream persistence, and TTL updates complete. Keep all validation and state construction unchanged, and retain the transfer using the existing token_client and amount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@contracts/libs/common/src/lib.rs`:
- Around line 7-12: Add rustdoc comments for the public ReentrancyGuard struct
and its acquire method. Document the guard’s purpose, acquisition behavior, and
that dropping the returned guard releases the instance lock.
In `@contracts/nft-stream/src/lib.rs`:
- Around line 150-197: Move the token_client.transfer call in the
stream-creation function to after the stream counter, ownership record, Stream
persistence, and TTL updates complete. Keep all validation and state
construction unchanged, and retain the transfer using the existing token_client
and amount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dcd4627-9e2a-4cba-9ce5-b46ba9fb0e17
⛔ Files ignored due to path filters (1)
contracts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.gitignorecontracts/Cargo.tomlcontracts/distributor/src/lib.rscontracts/libs/common/Cargo.tomlcontracts/libs/common/src/lib.rscontracts/libs/common/test_snapshots/tests/rejects_nested_acquisition.1.jsoncontracts/libs/common/test_snapshots/tests/releases_lock_when_scope_ends.1.jsoncontracts/nft-stream/Cargo.tomlcontracts/nft-stream/src/lib.rscontracts/nft-stream/test_snapshots/tests/reentrant_token_callback_is_rejected.1.jsoncontracts/payment-stream/Cargo.tomlcontracts/payment-stream/src/lib.rscontracts/payment-stream/src/test.rscontracts/payment-stream/test_snapshots/test/test/test_reentrant_token_callback_is_rejected.1.json
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
1 similar comment
|
dont forget to offramp using https://stellar.fundable.finance/offramp its fast, free and p2p rates |
2 participants
Summary
Closes #514.
Adds reentrancy protection across all state-mutating payment and NFT stream contract functions.
Changes
ReentrantCallcontract errors.Testing
wasm32-unknown-unknown.Summary by CodeRabbit
New Features
Bug Fixes
Tests