M-M02: fix(contracts): require zero allocs on close

[nksazonov]

Description

In cooperative path in closeChannel, the contract validates the submitted CLOSE state, computes transition effects, applies them, and then zeroes meta.lockedFunds. However, the engine does not enforce that the final allocations are actually paid out before the channel is deleted. _calculateCloseEffects only checks that userAllocation + nodeAllocation <= lockedFunds, that the resulting finalLockedFunds is non-negative, and that it is large enough for the special nodeAllocation handling.
In ChannelHub, cooperative close path does not automatically pay out the final allocations shown in the CLOSE state. The user only receives funds if userFundsDelta < 0. Consequently, if the signed CLOSE state keeps the same net flows as the previous state, both deltas can be zero even though the state still shows positive allocations. In that case, no funds are transferred during close, but _applyEffects still sets meta.lockedFunds to zero. The channel is removed, while the tokens remain stuck in the contract.

Summary by CodeRabbit

• Bug Fixes

  • Enhanced validation for channel closure states to ensure both user and node allocations are properly zeroed during closure, preventing inconsistent fund handling.

• Documentation

  • Updated channel closure specification to clarify that final payouts use net flow deltas for complete fund withdrawal, with channel status transitioning to CLOSED.

• Tests

  • Added validation tests for closure intent handling with incorrect allocation scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request refactors channel closure validation and fund handling in the ChannelEngine and ChannelHub contracts. It replaces the previous allocation sum validation with a strict requirement that both user and node allocations must be zero during channel closure, and removes special-case fund-pushing logic from the state transition handler to rely on standard delta processing.

Changes

Cohort / File(s)	Summary
Smart Contract Logic contracts/src/ChannelEngine.sol, contracts/src/ChannelHub.sol	Changed CLOSE validation in ChannelEngine to enforce userAllocation == 0 and nodeAllocation == 0 instead of checking allocation sum bounds. Removed special-case handling in ChannelHub that directly pushed node allocation, now relying on existing net-flow delta processing.
Tests contracts/test/ChannelEngine/ChannelEngine_validateTransition.t.sol	Added internal helper method for constructing operating contexts and introduced two new test cases validating that ChannelEngine correctly rejects CLOSE intents when user or node allocations are non-zero.
Documentation protocol-description.md	Updated channel closure specification to clarify that CLOSE state must have both allocations at zero, with fund payouts handled through negative net flow deltas rather than direct allocation pushing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• YNU-714: feat(contracts-new): add non-initial channel creation #510: Modifies ChannelHub._applyTransitionEffects' CLOSE path handling to rely on delta processing instead of special-case fund pushes, directly overlapping with this PR's removal of allocation-pushing logic.
• YNU-797: feat(contracts/ChannelHub): add NodeBalanceUpdated event #621: Changes node balance updates and emissions in _applyTransitionEffects code paths that interact with the CLOSE handling modified in this PR.

Suggested reviewers

• dimast-x
• ihsraham
• philanton

Poem

🐰 When channels close, no tricks remain,
Just zeros where allocs did reign—
Delta flows now do the deed,
No special cases that we need!
Clean and pure, the closure's right, ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(contracts): require zero allocs on close' directly and specifically describes the main change: enforcing zero allocations in the CLOSE state to prevent funds from getting stuck in the contract.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/audit-m-m02

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

contracts/test/ChannelEngine/ChannelEngine_validateTransition.t.sol (1)

51-61: Good complementary test for node allocation.

This correctly validates that even when userAllocation == 0 passes the first check, a non-zero nodeAllocation still triggers the revert. The net flow setup (allocsSum == netFlowsSum) ensures the universal validation passes, isolating the CLOSE-specific check.

Consider adding a happy-path test that validates a successful CLOSE with zero allocations and appropriate negative deltas, ensuring the full transition completes without revert:

💡 Optional: Add happy-path test

function test_close_success_zeroAllocations() public {
    ChannelEngine.TransitionContext memory ctx = _operatingCtx(1000);

    // User withdraws all funds: userAllocation -> 0, userNetFlow -> 0
    // userNfDelta = 0 - 1000 = -1000 (push 1000 to user)
    State memory candidate =
        TestUtils.nextState(ctx.prevState, StateIntent.CLOSE, [uint256(0), uint256(0)], [int256(0), int256(0)]);

    ChannelEngine.TransitionEffects memory effects = ChannelEngine.validateTransition(ctx, candidate);
    
    assertEq(effects.userFundsDelta, -1000);
    assertEq(effects.nodeFundsDelta, 0);
    assertTrue(effects.closeChannel);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@contracts/test/ChannelEngine/ChannelEngine_validateTransition.t.sol` around
lines 51 - 61, Add a happy-path unit test named e.g.
test_close_success_zeroAllocations that uses TransitionContext ctx =
_operatingCtx(1000), builds a candidate State via TestUtils.nextState with
StateIntent.CLOSE and zero allocations/deltas (both user and node zero), calls
ChannelEngine.validateTransition(ctx, candidate) to get
ChannelEngine.TransitionEffects, and asserts effects.userFundsDelta equals
-1000, effects.nodeFundsDelta equals 0, and effects.closeChannel is true;
reference the existing helpers TransitionContext, TestUtils.nextState,
ChannelEngine.validateTransition, and the TransitionEffects fields to locate
where to add the test.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@contracts/test/ChannelEngine/ChannelEngine_validateTransition.t.sol`:
- Around line 51-61: Add a happy-path unit test named e.g.
test_close_success_zeroAllocations that uses TransitionContext ctx =
_operatingCtx(1000), builds a candidate State via TestUtils.nextState with
StateIntent.CLOSE and zero allocations/deltas (both user and node zero), calls
ChannelEngine.validateTransition(ctx, candidate) to get
ChannelEngine.TransitionEffects, and asserts effects.userFundsDelta equals
-1000, effects.nodeFundsDelta equals 0, and effects.closeChannel is true;
reference the existing helpers TransitionContext, TestUtils.nextState,
ChannelEngine.validateTransition, and the TransitionEffects fields to locate
where to add the test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9eef6887-e8d3-4958-9d6c-acccc081e0d2

📥 Commits

Reviewing files that changed from the base of the PR and between 8151e30 and 4558e39.

📒 Files selected for processing (4)

• contracts/src/ChannelEngine.sol
• contracts/src/ChannelHub.sol
• contracts/test/ChannelEngine/ChannelEngine_validateTransition.t.sol
• protocol-description.md

💤 Files with no reviewable changes (1)

• contracts/src/ChannelHub.sol