Skip to content

[ADHOC] Add campaign finalization gate for withdrawals and unified withdrawal flow#520

Merged
mmackz merged 41 commits intofeature/timebased-incentivesfrom
matthew/cancelable-by-budget
Feb 27, 2026
Merged

[ADHOC] Add campaign finalization gate for withdrawals and unified withdrawal flow#520
mmackz merged 41 commits intofeature/timebased-incentivesfrom
matthew/cancelable-by-budget

Conversation

@mmackz
Copy link
Contributor

@mmackz mmackz commented Feb 24, 2026

Overview

Introduces a campaign finalization mechanism that gates all withdrawal operations (both budget-funded and direct-funded) until the operator explicitly marks a campaign as complete. This ensures withdrawals only occur after final merkle roots are published and claim windows have been properly established.

Problem Addressed

Previously, withdrawals could occur immediately after campaign end time without waiting for final state publication. This created a race condition where:

  • Operators might still be publishing final merkle roots
  • The claim expiry window hadn't been definitively established
  • Withdrawals and claims could interfere with each other

Key Changes

Campaign State Management

  • Adds finalized boolean flag to track completion status
  • New setFinalized() function allows manager to mark campaign as complete (only after endTime)
  • Finalization can occur during root updates via new finalize flag in RootUpdate struct

Unified Withdrawal Architecture

  • Replaces separate withdrawUndistributed() and withdrawToBudget() with single withdraw() function in manager
  • Automatically routes to appropriate destination based on funding model:
    • Budget-funded campaigns: routes through budget clawback for accounting
    • Direct-funded campaigns: transfers directly to creator
  • Both paths now require finalization before execution

Authorization Improvements

  • cancelCampaign() now supports multi-party authorization:
    • Manager owner (always allowed)
    • Budget-authorized users (for budget-funded campaigns)
    • Campaign creator (for direct-funded campaigns)
  • withdraw() uses same authorization pattern
  • Removes OnlyCreator error in favor of unified NotAuthorized pattern

Enhanced View Functions

  • New getWithdrawable() view in manager provides unified interface
  • Updated to return 0 if campaign not finalized (even if endTime passed)
  • Accounts for both still-owed amounts and claim expiry windows

Clawback Protection

  • Clawback operations now also require finalization
  • Prevents budget clawback before final state is published
  • Maintains protection of unclaimed funds during active claim window

Testing Updates

  • Updates all root update calls to include finalize parameter
  • Adds comprehensive finalization test suite covering:
    • Single and batch finalization
    • Double-finalization protection
    • Emergency root updates after finalization
    • Withdrawal blocking without finalization
    • Authorization checks for both funding models
  • Refactors withdrawal tests to use unified manager interface

Summary by CodeRabbit

Release Notes

  • New Features

    • Campaigns now require explicit finalization before withdrawals are permitted
    • New function to check withdrawable amounts per campaign
    • Root updates can trigger campaign finalization automatically
  • Refactor

    • Redesigned withdrawal mechanism with stricter finalization gates
    • Enhanced authorization controls for campaign management operations

@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2026

⚠️ No Changeset found

Latest commit: 7a0b055

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "@boostxyz/test" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@github-actions github-actions bot added the EVM label Feb 24, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 24, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch matthew/cancelable-by-budget

Comment @coderabbitai help to get the list of available commands and usage tips.

@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against ff11438

1 similar comment
@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against ff11438

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol (1)

437-451: ⚠️ Potential issue | 🟡 Minor

Batch finalize=true for a pre-endTime campaign reverts the entire transaction.

When finalize is true, setFinalized() on the campaign will revert with CampaignNotEnded if block.timestamp < endTime. In updateRootsBatch, this means a single entry with finalize=true targeting an active campaign will revert the entire batch, even if other entries are valid root-only updates.

This could be an operational footgun for the operator. Consider either:

  • Documenting this clearly so operators never include finalize=true for active campaigns in a batch, or
  • Wrapping the setFinalized() call in a try/catch (or adding a timestamp check in the manager) so a bad finalize flag doesn't poison the batch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol` around lines
437 - 451, The batch finalize issue: calling
TimeBasedIncentiveCampaign(campaign).setFinalized() will revert if
block.timestamp < endTime, which can abort updateRootsBatch; fix by guarding the
finalize call — in updateRoot (and the loop in updateRootsBatch) check the
campaign's endTime via TimeBasedIncentiveCampaign(campaign).endTime() (and still
check finalized()) and only call setFinalized() when block.timestamp >= endTime,
or alternatively make the external call in a try/catch and ignore
CampaignNotEnded reverts so a bad finalize flag doesn't poison the entire batch;
update references: updateRoot, updateRootsBatch, setFinalized, finalized,
endTime, TimeBasedIncentiveCampaign.
🧹 Nitpick comments (2)
packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol (1)

3249-3258: Consider adding a test for finalize=true before endTime.

The current finalization tests always warp past endTime before finalizing. There's no test verifying that updateRoot(..., true) reverts with CampaignNotEnded when the campaign is still active. This is an important boundary condition, especially for the batch-update scenario where a premature finalize flag could revert the entire batch.

🧪 Suggested test
function test_Finalization_RevertBeforeEndTime() public {
    (uint256 campaignId, TimeBasedIncentiveCampaign campaign) = _createCampaignWithRoot();

    // Campaign hasn't ended yet — finalize should revert
    vm.expectRevert(TimeBasedIncentiveCampaign.CampaignNotEnded.selector);
    manager.updateRoot(campaignId, keccak256("root"), 1 ether, true);

    assertFalse(campaign.finalized(), "Should not be finalized");
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol` around lines
3249 - 3258, Add a new unit test that ensures calling manager.updateRoot(...,
true) before the campaign end time reverts with
TimeBasedIncentiveCampaign.CampaignNotEnded.selector: use
_createCampaignWithRoot() to get (campaignId, campaign), call
vm.expectRevert(TimeBasedIncentiveCampaign.CampaignNotEnded.selector) then
invoke manager.updateRoot(campaignId, keccak256("root"), 1 ether, true), and
finally assert that campaign.finalized() is still false; name the test e.g.
test_Finalization_RevertBeforeEndTime to mirror existing tests.
packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol (1)

170-174: setFinalized() lacks idempotency guard — benign but worth noting.

setFinalized() can be called multiple times successfully (it just sets true again). The Manager already guards against double-emission of the CampaignFinalized event by checking !finalized() before calling, so this is safe in practice. However, a require(!finalized) check here would make the campaign contract defensively self-consistent.

🛡️ Optional: add idempotency guard
 function setFinalized() external onlyTimeBasedIncentiveManager {
     if (block.timestamp < endTime) revert CampaignNotEnded();
+    if (finalized) revert CampaignNotFinalized(); // or a dedicated AlreadyFinalized error
     finalized = true;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol` around lines
170 - 174, Add an idempotency guard to setFinalized so repeated calls revert
instead of silently re-setting state: inside function setFinalized (protected by
onlyTimeBasedIncentiveManager) check that finalized is false (e.g.,
require(!finalized) or revert with a new error like AlreadyFinalized()) before
checking endTime and setting finalized = true; this makes the contract
self-consistent and avoids redundant calls even if the manager already prevents
double-emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol`:
- Around line 437-451: The batch finalize issue: calling
TimeBasedIncentiveCampaign(campaign).setFinalized() will revert if
block.timestamp < endTime, which can abort updateRootsBatch; fix by guarding the
finalize call — in updateRoot (and the loop in updateRootsBatch) check the
campaign's endTime via TimeBasedIncentiveCampaign(campaign).endTime() (and still
check finalized()) and only call setFinalized() when block.timestamp >= endTime,
or alternatively make the external call in a try/catch and ignore
CampaignNotEnded reverts so a bad finalize flag doesn't poison the entire batch;
update references: updateRoot, updateRootsBatch, setFinalized, finalized,
endTime, TimeBasedIncentiveCampaign.

---

Nitpick comments:
In `@packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol`:
- Around line 170-174: Add an idempotency guard to setFinalized so repeated
calls revert instead of silently re-setting state: inside function setFinalized
(protected by onlyTimeBasedIncentiveManager) check that finalized is false
(e.g., require(!finalized) or revert with a new error like AlreadyFinalized())
before checking endTime and setting finalized = true; this makes the contract
self-consistent and avoids redundant calls even if the manager already prevents
double-emission.

In `@packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol`:
- Around line 3249-3258: Add a new unit test that ensures calling
manager.updateRoot(..., true) before the campaign end time reverts with
TimeBasedIncentiveCampaign.CampaignNotEnded.selector: use
_createCampaignWithRoot() to get (campaignId, campaign), call
vm.expectRevert(TimeBasedIncentiveCampaign.CampaignNotEnded.selector) then
invoke manager.updateRoot(campaignId, keccak256("root"), 1 ether, true), and
finally assert that campaign.finalized() is still false; name the test e.g.
test_Finalization_RevertBeforeEndTime to mirror existing tests.

ℹ️ Review info

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 83ad947 and ff11438.

📒 Files selected for processing (3)
  • packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol
  • packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol
  • packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol
📜 Review details
🔇 Additional comments (13)
packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol (4)

52-54: LGTM — finalized state variable and gating logic are clean.

The finalized boolean properly gates withdrawals and clawbacks, and integrates well with the existing campaign lifecycle.


223-237: Unified withdrawTo is well-structured.

The function correctly gates on finalization and end time, computes the withdrawable amount using _stillOwed(), and transfers to the specified destination. The balance-based accounting ensures safety without needing reentrancy guards for standard ERC20 tokens.


245-269: Clawback finalization gate is correct.

Adding the !finalized check at Line 253 properly prevents clawbacks before finalization, consistent with the withdrawal gating logic.


272-278: getWithdrawable finalization gate is consistent.

Returns 0 when not finalized or before endTime, aligning with the withdrawal preconditions in withdrawTo.

packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol (5)

99-101: LGTM — Generalized Withdrawn event.

Clean rename from WithdrawnToBudget to Withdrawn with the added destination parameter to support both budget-funded and direct-funded paths.


499-520: Expanded cancelCampaign ACL looks correct.

The three-tier authorization (owner → budget-authorized → creator) properly covers both funding models and the emergency admin case.


533-537: Owner cannot call withdraw() — intentional asymmetry with cancelCampaign?

Unlike cancelCampaign (which allows the owner), withdraw() restricts to budget-authorized users or the creator only. This means the protocol owner cannot force-withdraw funds. If this is intentional (the owner shouldn't unilaterally move funds), this is a good security property. Just confirming this is by design rather than an omission.


558-565: LGTM — getWithdrawable view delegates cleanly.

Simple delegation to the campaign contract with an existence check. The campaign's getWithdrawable() already handles finalization and endTime gating.


522-556: The encoding convention is properly aligned. The budget-funded withdrawal flow correctly encodes and decodes the amount across all layers:

  1. TimeBasedIncentiveManager.withdraw() passes abi.encode(withdrawable) as clawbackData
  2. ABudget.clawbackFromTarget() wraps this in a ClawbackPayload struct with the encoded uint256 as the .data field, then encodes the entire struct
  3. TimeBasedIncentiveCampaign.clawback() decodes the struct and then decodes .data as uint256

This matching convention is verified by the existing test coverage, including tests that successfully perform the clawback operation. There is no encoding mismatch or risk of silent failure.

packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol (4)

3245-3367: Comprehensive finalization test suite — good coverage.

The tests cover single/batch finalization, double-finalization protection, emergency root updates after finalization, withdrawal gating, and the cancel→finalize→withdraw flow. Well structured.


3150-3243: Cancellation ACL tests are thorough.

Covers budget-authorized user, budget owner, direct-funded creator, and unauthorized caller paths. Good.


2247-2258: Clawback revert error changed from CampaignNotEnded to CampaignNotFinalized — correct.

Since clawback() now checks !finalized before block.timestamp <= endTime, the first revert hit for a non-finalized, non-ended campaign is CampaignNotFinalized. The test expectation at Line 2256 is properly updated.


2053-2085: Direct-funded withdrawal test validates the new unified flow end-to-end.

Creates a direct campaign, finalizes via root update, and verifies funds reach the creator. Good integration test of the withdrawTo path.

@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against 3b8746e

@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against 5abd7e4

@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against 8ef7f7e

1 similar comment
@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against 8ef7f7e

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol (2)

173-184: Updated CampaignCreated event assertion in test_CreateCampaign_EmitsCampaignCreated.

The 60 days literal at Line 183 corresponds to the default claimExpiryDuration. Consider using a named constant or manager.claimExpiryDuration() to avoid fragility if the default ever changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol` around lines 173
- 184, The test is asserting the CampaignCreated event using a hardcoded "60
days" for claim expiry; replace this fragile literal with the actual default
value by referencing the manager's claimExpiryDuration (e.g., use
manager.claimExpiryDuration()) or a named constant so the assertion stays
correct if the default changes; update the assertion in
test_CreateCampaign_EmitsCampaignCreated where
TimeBasedIncentiveManager.CampaignCreated is emitted to use
manager.claimExpiryDuration() (or the named constant) instead of 60 days.

3163-3256: Cancellation ACL tests cover all key scenarios.

Tests confirm budget-authorized users, budget admins, and direct-funded creators can cancel, while unauthorized addresses are rejected.

Minor nit: test_CancelCampaign_ByBudgetOwner (Line 3186) tests a budget-authorized user (budgetAdmin), not the budget owner per se (which is address(this)). The naming is slightly misleading but the test logic is valid and valuable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol` around lines
3163 - 3256, The test named test_CancelCampaign_ByBudgetOwner is misleading
because it actually grants authorization to budgetAdmin via budget.setAuthorized
and asserts an authorized (not owner) account can cancel; rename this function
(e.g., test_CancelCampaign_ByBudgetAuthorizedUser or
test_CancelCampaign_ByBudgetAdminAuthorized) and update any references to
test_CancelCampaign_ByBudgetOwner so the name reflects that the case exercises a
budget-authorized address (look for the function symbol
test_CancelCampaign_ByBudgetOwner and the budget.setAuthorized call to locate
the code to change).
packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol (1)

170-174: setFinalized is idempotent — consider a short-circuit to save gas on repeated calls.

If the manager's guard (!finalized()) ever fails to prevent a redundant call, setFinalized will still write true to an already-true slot, paying a SSTORE cost for no state change. A trivial early return would make this robust regardless of caller logic.

♻️ Optional: add early return
 function setFinalized() external onlyTimeBasedIncentiveManager {
     if (block.timestamp < endTime) revert CampaignNotEnded();
+    if (finalized) return;
     finalized = true;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol` around lines
170 - 174, The setFinalized function currently always writes finalized = true
even if already true; add a short-circuit check at the start of setFinalized
(referencing function setFinalized and state variable finalized) to return
immediately when finalized is already true (while keeping the existing
onlyTimeBasedIncentiveManager guard and the endTime check), which prevents
unnecessary SSTORE costs on repeated calls.
packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol (1)

492-496: Batch finalization: one un-finalizable campaign reverts the entire batch.

If any entry in the batch has finalize=true but the corresponding campaign hasn't ended, the whole batch reverts. Operators batching updates for mixed active/ended campaigns must ensure finalize=false for any campaign where block.timestamp < endTime.

This is consistent with the single-update behavior and acceptable, but documenting this constraint in the NatDoc for updateRootsBatch would help operators avoid runtime surprises.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol` around lines
492 - 496, The batch finalization is atomic: inside updateRootsBatch, any
element with updates[i].finalize=true that calls
TimeBasedIncentiveCampaign(campaign).setFinalized() will revert the entire batch
if that campaign hasn't ended (i.e., block.timestamp < endTime), so add a
NatSpec note to updateRootsBatch documenting this constraint — explain that
batch processing is atomic, that a single un-finalizable campaign (finalize=true
while campaign not ended) will revert the whole batch, and instruct operators to
set finalize=false for any active campaigns when composing batched updates;
reference updateRootsBatch, updates[i].finalize, TimeBasedIncentiveCampaign, and
setFinalized in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol`:
- Around line 170-174: The setFinalized function currently always writes
finalized = true even if already true; add a short-circuit check at the start of
setFinalized (referencing function setFinalized and state variable finalized) to
return immediately when finalized is already true (while keeping the existing
onlyTimeBasedIncentiveManager guard and the endTime check), which prevents
unnecessary SSTORE costs on repeated calls.

In `@packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol`:
- Around line 492-496: The batch finalization is atomic: inside
updateRootsBatch, any element with updates[i].finalize=true that calls
TimeBasedIncentiveCampaign(campaign).setFinalized() will revert the entire batch
if that campaign hasn't ended (i.e., block.timestamp < endTime), so add a
NatSpec note to updateRootsBatch documenting this constraint — explain that
batch processing is atomic, that a single un-finalizable campaign (finalize=true
while campaign not ended) will revert the whole batch, and instruct operators to
set finalize=false for any active campaigns when composing batched updates;
reference updateRootsBatch, updates[i].finalize, TimeBasedIncentiveCampaign, and
setFinalized in the comment.

In `@packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol`:
- Around line 173-184: The test is asserting the CampaignCreated event using a
hardcoded "60 days" for claim expiry; replace this fragile literal with the
actual default value by referencing the manager's claimExpiryDuration (e.g., use
manager.claimExpiryDuration()) or a named constant so the assertion stays
correct if the default changes; update the assertion in
test_CreateCampaign_EmitsCampaignCreated where
TimeBasedIncentiveManager.CampaignCreated is emitted to use
manager.claimExpiryDuration() (or the named constant) instead of 60 days.
- Around line 3163-3256: The test named test_CancelCampaign_ByBudgetOwner is
misleading because it actually grants authorization to budgetAdmin via
budget.setAuthorized and asserts an authorized (not owner) account can cancel;
rename this function (e.g., test_CancelCampaign_ByBudgetAuthorizedUser or
test_CancelCampaign_ByBudgetAdminAuthorized) and update any references to
test_CancelCampaign_ByBudgetOwner so the name reflects that the case exercises a
budget-authorized address (look for the function symbol
test_CancelCampaign_ByBudgetOwner and the budget.setAuthorized call to locate
the code to change).

ℹ️ Review info

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 83ad947 and 8ef7f7e.

📒 Files selected for processing (3)
  • packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol
  • packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol
  • packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol
📜 Review details
⏰ 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: Use Case Tests / Use Case Tests
  • GitHub Check: Verify / Verify
🔇 Additional comments (16)
packages/evm/contracts/timebased/TimeBasedIncentiveCampaign.sol (4)

52-53: LGTM — clean additions for finalization state and error.

The finalized flag and CampaignNotFinalized error are well-placed and logically consistent with the gating requirements across withdrawTo, clawback, and getWithdrawable.

Also applies to: 112-113


226-237: Redundant CampaignNotEnded check after finalization gate.

setFinalized() already enforces block.timestamp >= endTime at finalization time, and endTime can only decrease (via setEndTime). So if finalized == true, block.timestamp > endTime is guaranteed. The Line 228 check is defense-in-depth, which is reasonable for a smart contract — just noting it's logically redundant.


245-269: Finalization gate correctly added to clawback.

The CampaignNotFinalized check at Line 253 properly prevents premature clawback before the operator has published the final reward state, ensuring user funds are protected.


272-278: getWithdrawable correctly returns 0 when not finalized.

This prevents off-chain consumers from observing a misleading withdrawable amount before the campaign is finalized.

packages/evm/contracts/timebased/TimeBasedIncentiveManager.sol (7)

21-26: LGTM — RootUpdate struct extended with finalize flag.

Clean addition enabling atomic root-update-and-finalize in both single and batch operations.


461-475: Finalization logic in updateRoot is well-guarded against double-emission.

The !TimeBasedIncentiveCampaign(campaign).finalized() check before calling setFinalized() correctly prevents redundant events. Note that if finalize=true is passed for a campaign that hasn't ended yet, the entire updateRoot call reverts (because setFinalized() checks block.timestamp < endTime). This means an operator cannot update the root and request finalization in one call for an active campaign — they must use finalize=false until after endTime. This is correct behavior but worth noting for the operator integration.


526-543: Expanded cancellation ACL looks correct.

Owner can always cancel. Budget-funded campaigns allow any budget-authorized user. Direct-funded campaigns restrict to the creator only. The nested if structure is clear and avoids unnecessary isAuthorized calls for the owner path.


565-573: Budget-funded withdrawal path: getWithdrawable() and clawback compute the same available — safe but tightly coupled.

getWithdrawable() returns balance - owed and the clawback path then requests exactly that amount. Inside clawback, the same available is recomputed and checked against the requested amount. Since this is atomic within a single transaction, the values are guaranteed to match. The approach is correct.


101-103: Event renames and additions are clean.

Withdrawn unifies the old WithdrawnToBudget for both funding models. CampaignFinalized provides a clear on-chain signal for indexers.

Also applies to: 113-115


294-305: CampaignCreated event emission updated consistently in both createCampaign and createCampaignDirect.

Both paths now emit budget (or address(0) for direct), rewardToken, endTime, and claimExpiryDuration. The event signature has 3 indexed fields (creator, budget, rewardToken) which is the Solidity maximum for non-anonymous events.

Also applies to: 381-392


546-580: withdraw() does not grant owner access — intentional by design, confirmed by explicit function documentation.

The code correctly implements a security boundary where cancelCampaign() allows owner override (line 533) but withdraw() restricts access to budget-authorized users (budget-funded) or the campaign creator (direct-funded). The inline comments explicitly differentiate the two functions, confirming this was intentional. No action needed.

packages/evm/test/timebased/TimeBasedIncentiveManager.t.sol (5)

1949-1951: Tests properly gate all withdrawal/clawback paths behind finalization.

All pre-existing withdrawal and clawback tests have been updated to include a manager.updateRoot(..., true) call after warping past endTime. This ensures the finalization prerequisite is met consistently across the test suite.

Also applies to: 1974-1974, 2001-2003, 2059-2059, 2085-2085, 2116-2118, 2146-2147, 2188-2188, 2207-2211, 2274-2274, 2308-2308, 2417-2419, 2547-2549, 2595-2597, 2678-2678, 2848-2848, 2866-2866, 2895-2895, 2917-2920, 2970-2972


3258-3324: Finalization test suite is comprehensive and well-structured.

Good coverage of:

  • Basic finalization via updateRoot with finalize=true
  • Revert when attempting finalization before endTime
  • Event emission and no-double-emit behavior
  • Root updates remaining functional after finalization

The test_Finalization_NoDoubleEmit test (Lines 3292–3310) correctly uses CampaignFinalized.selector (available since Solidity 0.8.15) and properly verifies that repeated finalize=true calls don't re-emit the event.


3326-3346: Good batch finalization test with mixed finalize flags.

test_Finalization_BatchFinalize confirms that only campaigns with finalize=true get finalized, leaving others unaffected. This is important for operators who may batch updates for campaigns in different lifecycle stages.


3348-3394: Critical flow test: cancel → still requires explicit finalization before withdrawal.

test_Finalization_CancelDoesNotFinalize and test_Finalization_WithdrawToBudgetBlockedWithoutFinalize together verify that cancellation alone doesn't unlock funds — the operator must still publish a final root. This is a key safety property of the design.


2828-2855: getWithdrawable finalization-awareness test is thorough.

The test correctly verifies that getWithdrawable returns 0 before finalization (even after endTime), and returns the correct balance - stillOwed amount after finalization. The two-phase assertion (Lines 2843-2854) is a clean way to validate the gating behavior.

@jonathandiep
Copy link
Contributor

Warnings
⚠️

By using the [ADHOC] title prefix, you are bypassing best practice protections.

⚠️

Are you sure you want to be submitting a change without including a changeset? If you're just changing docs or tests, you probably don't need to. See the publishing section of the README for more info.

Generated by 🚫 dangerJS against 7a0b055

@mmackz mmackz merged commit bf6d296 into feature/timebased-incentives Feb 27, 2026
8 of 9 checks passed
@mmackz mmackz deleted the matthew/cancelable-by-budget branch February 27, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants