Fix reentrancy vulnerability#128
Conversation
WalkthroughA reentrancy guard is integrated into the PaymentStream contract, systematically wrapping all externally accessible, state-mutating functions to prevent reentrancy attacks. The contract's logic is refactored to follow the Checks-Effects-Interactions (CEI) pattern. Comprehensive malicious contract-based tests are introduced to verify the effectiveness of the reentrancy protection, and a new coding rules document for Cairo projects is added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PaymentStream
participant ReentrancyGuard
participant ERC20Token
User->>PaymentStream: withdraw/withdraw_max/cancel/transfer_stream/collect_protocol_fee
PaymentStream->>ReentrancyGuard: start_guard()
alt Guard not active
PaymentStream->>PaymentStream: update state (Checks, Effects)
PaymentStream->>ERC20Token: external call (Interactions)
PaymentStream->>ReentrancyGuard: end_guard()
PaymentStream-->>User: return result
else Guard active
PaymentStream-->>User: revert with reentrancy error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for the good work, @wheval. When do you think the PR will be ready? |
The PR is ready, sorry for the delay! |
Utilitycoder
left a comment
Utilitycoder
left a comment
There was a problem hiding this comment.
Good job. Thanks for taking time to work on the issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_reentrancy_protection.cairo (1)
186-339: Consider if separate attacker contracts are necessaryThe MaliciousWithdrawAttacker, MaliciousCancelAttacker, and MaliciousTransferAttacker contracts appear redundant since the MaliciousERC20 already implements these attack vectors. Consider removing these unless they're intended for testing direct attacks vs token-transfer-triggered attacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.cursor/rules/cairo-coder.mdc(1 hunks)src/payment_stream.cairo(12 hunks)tests/test_reentrancy_protection.cairo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (9)
.cursor/rules/cairo-coder.mdc (1)
69-126: Well-structured development workflow guidelinesThe development workflow section provides excellent guidance on coding standards, testing practices, and effective use of the Cairo Coder MCP tool. The emphasis on atomic queries and providing specific context aligns with best practices for AI-assisted development.
src/payment_stream.cairo (5)
8-8: ReentrancyGuard component properly integratedThe ReentrancyGuard component from OpenZeppelin is correctly integrated with all necessary elements: import, component declaration, internal implementation, storage substorage, and event handling.
Also applies to: 36-38, 49-49, 69-70, 102-102
404-419: Correct refactoring of fee collection to avoid nested guardsThe separation of
collect_protocol_fee(with guard) and_collect_protocol_fee_internal(without guard) properly prevents nested reentrancy guards when called from other protected functions like_withdraw.Also applies to: 732-732
817-834: Withdraw functions properly protected with CEI patternBoth
withdrawandwithdraw_maxare correctly wrapped with reentrancy guards. The internal_withdrawfunction follows the Checks-Effects-Interactions pattern by updating all state (lines 711-726) before making external calls (lines 732-733).
836-870: transfer_stream correctly implements reentrancy protectionThe function is properly wrapped with reentrancy guards and follows the CEI pattern. All state updates and the NFT transfer occur within the protected section.
972-1101: Cancel function properly implements reentrancy protection with CEI patternThe function correctly wraps all operations with reentrancy guards and meticulously follows the CEI pattern:
- All state updates (lines 1025-1059) including status change, balance updates, and NFT burn
- External calls (lines 1071-1072, 1088) only after state is finalized
This is the most complex function and it's implemented securely.
tests/test_reentrancy_protection.cairo (3)
31-184: Well-designed malicious ERC20 for reentrancy testingThe MaliciousERC20 contract effectively simulates reentrancy attacks with:
- Multiple attack modes for different PaymentStream functions
- Attack count limitation to prevent infinite loops
- Realistic attack trigger during token transfers
This provides comprehensive test coverage for the reentrancy guards.
425-561: Comprehensive reentrancy attack test coverageThe test cases effectively verify reentrancy protection for all critical functions:
- Proper setup with malicious tokens configured for each attack type
- Realistic scenarios with time progression for withdrawals
- Correct assertion of reentrancy guard errors
These tests confirm the reentrancy protection is working as intended.
606-662: Good coverage of normal operations post-protectionThis test effectively verifies that the reentrancy protection doesn't interfere with normal operations. It tests withdraw, transfer_stream, and cancel functions with non-malicious tokens, ensuring the guards only activate during actual attacks.
|
Hello ser @Utilitycoder, I just noticed that I accidentally committed a Cursor MCP config file in this PR. While I used Cursor to assist during development, I wrote and reviewed the logic myself — the tool was more of a support than a driver. I’m going to clean that up and also fix a test that isn’t passing properly. I’ll open a follow-up PR shortly to address both. Just wanted to flag this in case the config file gave the wrong impression. Appreciate your understanding! |
2 participants
Closes #127
Summary by CodeRabbit
New Features
Tests