[BOOST-6102] Streaming Incentives Campaign Creation & Configuration #511
Conversation
|
|
1 similar comment
|
mmackz
changed the title
|
|
4 similar comments
|
|
|
|
mmackz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/evm/contracts/streaming/StreamingManager.sol`:
- Around line 121-164: The createCampaign() function performs external calls
(LibClone.clone -> budget.disburse() and StreamingCampaign.initialize()) before
updating state (campaignCount and campaigns), so add a reentrancy protection:
either apply a nonReentrant modifier to createCampaign() or reorder to follow
checks-effects-interactions (increment campaignCount and set
campaigns[campaignId] before making external calls), ensuring you reference the
same symbols (createCampaign, campaignCount, campaigns, budget.disburse,
StreamingCampaign.initialize, LibClone.clone) when implementing the change.
🧹 Nitpick comments (4)
packages/evm/contracts/streaming/StreamingManager.sol (1)
6-6: Unused import:SafeTransferLib.
SafeTransferLibis imported but not used in this contract. The contract relies onABudget.disburse()for all token transfers.🧹 Proposed fix
-import {SafeTransferLib} from "@solady/utils/SafeTransferLib.sol";Also remove the
usingdeclaration at line 15:- using SafeTransferLib for address;packages/evm/test/streaming/StreamingManager.t.sol (2)
162-163: Clarifyvm.expectEmitusage for unpredictable addresses.Using
vm.expectEmit(true, true, true, false)correctly skips non-indexed data verification since the campaign address is unknown. However, the subsequent emit statement still provides all values includingaddress(0)for the campaign address, which could be misleading to readers.Consider adding a comment clarifying that the non-indexed data check is disabled, so the placeholder values are ignored.
294-308: Test relies on specific internal budget behavior.The test comment explains that with 1000 ether requested, the fee (100 ether) succeeds first, depleting the budget, and then the net transfer (900 ether) fails. This assumes a specific execution order and partial success behavior in
ABudget.disburse. If the budget implementation changes to check total availability upfront, this test may behave differently.Consider adding an explicit comment or a more isolated test that doesn't depend on partial disburse success.
packages/evm/contracts/streaming/StreamingCampaign.sol (1)
100-104:onlyStreamingManagermodifier is defined but currently unused.This modifier is scaffolding for future functions (e.g.,
setMerkleRoot,claim). Consider adding a comment indicating its intended future use, or defer adding it until the functions that require it are implemented to avoid dead code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/evm/contracts/streaming/StreamingCampaign.solpackages/evm/contracts/streaming/StreamingManager.solpackages/evm/test/streaming/StreamingManager.t.sol
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mmackz
Repo: boostxyz/boost-protocol PR: 479
File: packages/sdk/src/Incentives/ERC20VariableCriteriaIncentiveV2.ts:285-292
Timestamp: 2025-09-22T23:21:17.136Z
Learning: In the boost-protocol repository, security improvements like targetContract filtering should be kept separate from feature-focused PRs and handled in dedicated security improvement PRs.
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Verify / Verify
🔇 Additional comments (12)
packages/evm/contracts/streaming/StreamingManager.sol (4)
114-114: Consider allowingstartTime == block.timestampfor immediate campaign starts.The current check
startTime < block.timestampallowsstartTime == block.timestamp, which is correct for same-block campaign creation. However, this edge case isn't explicitly tested.
100-176: LGTM on core createCampaign logic.The validation sequence is comprehensive: authorization check, parameter validation, fee calculation, disburse operations with failure checks, and proper initialization. The use of pre-increment for
campaignIdensures IDs start at 1, which is a good practice for distinguishing unset values.
187-192: LGTM on setProtocolFee.Proper owner-only access control, bounds validation, and event emission with old/new values for auditability.
196-201: LGTM on setProtocolFeeReceiver.Proper owner-only access control, zero-address validation, and event emission.
packages/evm/test/streaming/StreamingManager.t.sol (4)
25-54: LGTM on test setup.The setup properly initializes all components with appropriate authorization roles. Good practice using constants for addresses and fee values.
196-236: Good edge case coverage for 100% fee scenario.This test validates an important edge case where
netAmount = 0and the campaign is initialized with zero rewards. The assertions verify both token balances and campaign state correctly.
120-152: Good zero-fee edge case coverage.Properly tests that no fee is taken when
protocolFee = 0and the campaign receives the full amount.
365-392: LGTM on event verification test.Good use of
vm.recordLogs()and manual log parsing to verifyCampaignInitializedevent emission with correct indexed parameters.packages/evm/contracts/streaming/StreamingCampaign.sol (4)
56-58: LGTM on implementation protection.Correctly uses
_disableInitializers()in the constructor to prevent the implementation contract from being initialized directly.
69-98: Initialize is unguarded but safe in current usage pattern.The
initialize()function lacks explicit access control (e.g.,onlyStreamingManager), relying instead on theinitializermodifier to prevent re-initialization. This is safe becauseStreamingManager.createCampaign()atomically clones and initializes in the same transaction, leaving no window for front-running.However, for defense-in-depth and to make the intended usage pattern explicit, consider adding the
onlyStreamingManagercheck. This would require the manager address to be passed as an immutable clone argument or checked viamsg.sender == tx.origin-style patterns, which adds complexity.Given the current atomic pattern is safe and the
onlyStreamingManagermodifier exists for future claim functions, the current implementation is acceptable.
34-38: Scaffolding for merkle claims noted.The
merkleRootandclaimedmapping are defined but have no setters or claim logic. Per the PR objectives, this is intentional scaffolding for future merkle claim processing in subsequent epics.
1-105: Overall: Well-structured cloneable campaign contract.The contract follows the minimal proxy pattern correctly with proper use of Solady's
Initializable. State variables are appropriately typed (usinguint64for timestamps to save storage), and the event provides good observability for off-chain indexing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mmackz
changed the title
|
2 similar comments
|
|
There was a problem hiding this comment.
Approval ✅
I've completed the review and confirm that all issues have been addressed:
✅ Previously Flagged Issue - RESOLVED
The reentrancy vulnerability in createCampaign() has been properly fixed. State updates (campaignCount and campaigns mapping) now occur before external calls to budget.disburse() and StreamingCampaign.initialize(), following the checks-effects-interactions pattern.
✅ Code Review Summary
StreamingManager.sol:
- Proper input validation (zero address checks, fee limits, timing validation)
- State management follows best practices
- Clean event emission
- Owner-only setters properly protected
StreamingCampaign.sol:
- Correct use of Initializable pattern from Solady
- Implementation contract properly disabled via constructor
- Access control scaffolding in place for future functionality
- Clean state management
Tests:
- 30 tests passing with comprehensive coverage
- Edge cases handled (0% fee, 100% fee, authorization, validation)
No security concerns or code quality issues found. The PR is ready to merge! 🚀
|
2 similar comments
|
|
|
mmackz
deleted the
matthew/boost-6102-epic-1-campaign-creation-configuration
branch
2 participants
Summary
Implements Epic 1 of Streaming Incentives - the foundational contracts for campaign creation and configuration.
Linear Tickets:
Contracts Added
StreamingManager.sol- Factory contract that deploys campaigns, handles protocol fees, maintains campaign registry. UUPS upgradeable.StreamingCampaign.sol- Per-campaign contract (clone) that holds reward tokens and will process merkle claimsFeatures
createCampaign()- Deploys campaign clone, disburses funds from budget, handles fee calculationinitialize()- Sets up campaign state (manager, budget, creator, config hash, reward token, timing)setProtocolFee()- Owner-only setter with validation and eventsetProtocolFeeReceiver()- Owner-only setter with validation and eventUUPS Upgradeability (StreamingManager)
Initializable,UUPSUpgradeable,Ownableinitialize(owner, campaignImpl, protocolFee, protocolFeeReceiver)withinitializermodifier_authorizeUpgrade(address)withonlyOwnerfor upgrade authorizationsetCampaignImplementation(address)- allows updating campaign template without redeploying manager__padding+__gap[50]) for future upgrade safetyTest Coverage
Design Decisions
Test plan
forge test --match-contract StreamingManagerTest)Generated with Claude Code