fix: prevent undelegation of closed delegated accounts

[JkrishnaD]

Problem

• Undelegation could be invoked on a delegated account that was already closed, allowing state cleanup and fee handling to proceed without a valid root account.
• This could lead to invalid undelegation flows and inconsistent state during delegation cleanup.

Solution

• Added an early guard in process_undelegate to reject undelegation if the delegated account is already closed (zero lamports / empty data).
• Introduced a dedicated error to explicitly signal attempts to undelegate closed accounts.
• Added a regression test to ensure undelegation fails deterministically when the delegated account is closed.

Deploy Notes

• No new dependencies
• No migrations required

Closes #82

Summary by CodeRabbit

• Bug Fixes

  • Added validation to reject undelegation when the delegate account is closed and now returns a clear "Delegated account is already closed" error.

• Tests

  • Added end-to-end test coverage and a test harness to verify undelegation fails for closed delegate accounts.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a new DelegateAccountClosed error variant, an early pre-check in the undelegate processor to reject closed delegated accounts, and a test verifying undelegation fails when the delegated account is closed.

Changes

Cohort / File(s)	Summary
Error definition src/error.rs	Adds DelegateAccountClosed = 41 to DlpError with #[error("Delegated account is already closed")].
Processor validation src/processor/fast/undelegate.rs	Adds an early pre-check in process_undelegate to detect closed delegated accounts (lamports == 0 and empty data) and return DelegateAccountClosed before other validations.
Test coverage tests/test_undelegate_close_account.rs	New async test test_undelegate_close_account and helper setup_program_test_env that build a ProgramTest environment that includes a closed delegated PDA and assert the closed-delegate error on undelegate.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• GabrielePicco

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely describes the main change: preventing undelegation of closed delegated accounts, which matches the core objective.
Description check	✅ Passed	The PR description covers Problem, Solution, and Deploy Notes sections from the template, addressing the core issue and solution comprehensively.
Linked Issues check	✅ Passed	All coding requirements from issue #82 are met: added guard to prevent undelegation on closed accounts, introduced DelegateAccountClosed error, and included regression test.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #82: error variant addition, undelegation guard, and test coverage for closed account prevention.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c893f7 and 681bf71.

📒 Files selected for processing (3)

• src/error.rs
• src/processor/fast/undelegate.rs
• tests/test_undelegate_close_account.rs

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-15T11:45:25.802Z

Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.

Applied to files:

• tests/test_undelegate_close_account.rs

🧬 Code graph analysis (1)

tests/test_undelegate_close_account.rs (2)

src/pda.rs (4)

• delegation_metadata_pda_from_delegated_account (106-112)
• delegation_record_pda_from_delegated_account (98-104)
• fees_vault_pda (149-151)
• validator_fees_vault_pda_from_validator (153-159)

tests/fixtures/accounts.rs (2)

• get_delegation_metadata_data (99-105)
• get_delegation_record_data (54-56)

🔇 Additional comments (4)

src/error.rs (1)

90-91: Error variant is correctly implemented.

The new DelegateAccountClosed error variant is well-defined with a unique discriminant (41) and integrates properly with the existing error handling infrastructure. The functionality is sound.

Note: A past review comment suggested renaming to DelegatedAccountClosed for consistency with error messages at lines 16 and 18 which use "Delegated account". This is a cosmetic consistency issue and doesn't affect functionality.

src/processor/fast/undelegate.rs (2)

71-71: Documentation accurately reflects the new behavior.

The updated comment correctly documents the early-stop condition when the delegated account is closed, maintaining clarity in the function's step-by-step flow.

---

91-95: Early guard is correctly implemented and well-positioned.

The check correctly identifies closed accounts using the AND condition (lamports == 0 && data_is_empty), which properly defines a fully closed Solana account. The guard is positioned early in the function to short-circuit processing before expensive validations, preventing invalid state cleanup and fee handling on accounts that are already closed.

The logic correctly handles edge cases:

• Accounts with zero lamports but retained data won't trigger this guard (will fail in subsequent validations)
• Accounts with data but non-zero lamports will proceed to the fast-path at line 166 (as intended)

tests/test_undelegate_close_account.rs (1)

44-137: Test setup is comprehensive and accurately simulates the scenario.

The helper function correctly constructs a test environment that replicates the bug condition:

• A delegated account that is closed (zero lamports, empty data) while still owned by the delegation program
• Valid delegation metadata and record PDAs that reference this closed account
• All necessary fee vaults and the owner program

This setup effectively tests the early guard's ability to detect and reject undelegation attempts on closed accounts before expensive validations or state cleanup.

---

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.}

[JkrishnaD]

@GabrielePicco check this out

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2c0490 and e25f8c2.

📒 Files selected for processing (3)

• src/error.rs
• src/processor/fast/undelegate.rs
• tests/test_undelegate_close_account.rs

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_undelegate_close_account.rs (2)

src/pda.rs (4)

• delegation_metadata_pda_from_delegated_account (106-112)
• delegation_record_pda_from_delegated_account (98-104)
• fees_vault_pda (149-151)
• validator_fees_vault_pda_from_validator (153-159)

tests/fixtures/accounts.rs (2)

• get_delegation_metadata_data (99-105)
• get_delegation_record_data (54-56)

🔇 Additional comments (4)

src/processor/fast/undelegate.rs (2)

71-71: LGTM! Documentation clearly describes the new early-exit behavior.

The step documentation accurately reflects the new pre-check that rejects closed accounts before proceeding with undelegation.

---

91-95: LGTM! Pre-check correctly identifies and rejects closed accounts.

The guard properly detects closed delegated accounts (zero lamports AND empty data) and returns early with a specific error. This prevents invalid cleanup/fee handling downstream, as stated in the PR objectives.

The check is positioned before other validations, which is appropriate because:

1. Account state (lamports, data) is publicly readable on Solana, so checking before ownership validation does not leak sensitive information.
2. Early exit avoids potential arithmetic errors or state inconsistencies in subsequent logic when operating on a closed account.
3. This is distinct from the fast-path at line 166, which handles accounts with empty data but non-zero lamports.

src/error.rs (1)

90-91: LGTM! Error variant is well-defined and follows existing patterns.

The DelegateAccountClosed variant:

• Uses the next sequential discriminant (41) after InvalidDelegationRecordData (40)
• Has a clear, descriptive error message that matches its usage in the undelegate processor
• Is correctly positioned before InfallibleError (100)
• Will be automatically handled by the From<DlpError> implementations

tests/test_undelegate_close_account.rs (1)

46-139: LGTM! Test environment setup is comprehensive and correctly models the edge case.

The setup properly creates the scenario the PR addresses:

• Closed delegated account (0 lamports, empty data) that is still owned by dlp::id() (lines 66-68)
• Valid delegation_record and delegation_metadata PDAs
• All required accounts (fee vaults, owner program) properly initialized

This edge case (dlp-owned but closed account) is precisely what the new guard in process_undelegate is designed to catch.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

tests/test_undelegate_close_account.rs (1)

40-42: Weak assertion: verify the specific error code 41.

The test only checks that an error occurred but doesn't verify that the specific error is DelegateAccountClosed (error code 41). The transaction could fail for other reasons (e.g., invalid account, missing signer), and this test would incorrectly pass. Additionally, the debug println! should be removed.

🔎 Recommended fix to assert the specific error and remove debug output

     let res = banks.process_transaction(tx).await;
-    println!("{:?}", res);
-    assert!(res.is_err(), "Delegate account is already closed");
+    
+    let err = res.expect_err("Expected transaction to fail for closed account");
+    let transaction_error = err.unwrap();
+    match transaction_error {
+        solana_sdk::transaction::TransactionError::InstructionError(_, instruction_error) => {
+            match instruction_error {
+                solana_program::instruction::InstructionError::Custom(code) => {
+                    assert_eq!(
+                        code, 41,
+                        "Expected DelegateAccountClosed error (code 41), got: {}",
+                        code
+                    );
+                }
+                _ => panic!("Expected Custom error, got: {:?}", instruction_error),
+            }
+        }
+        _ => panic!("Expected InstructionError, got: {:?}", transaction_error),
+    }

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e25f8c2 and 5c893f7.

📒 Files selected for processing (3)

• src/error.rs
• src/processor/fast/undelegate.rs
• tests/test_undelegate_close_account.rs

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-15T11:45:25.802Z

Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.

Applied to files:

• tests/test_undelegate_close_account.rs

🧬 Code graph analysis (1)

tests/test_undelegate_close_account.rs (2)

src/pda.rs (4)

• delegation_metadata_pda_from_delegated_account (106-112)
• delegation_record_pda_from_delegated_account (98-104)
• fees_vault_pda (149-151)
• validator_fees_vault_pda_from_validator (153-159)

tests/fixtures/accounts.rs (2)

• get_delegation_metadata_data (99-105)
• get_delegation_record_data (54-56)

🔇 Additional comments (3)

src/processor/fast/undelegate.rs (2)

91-95: LGTM! Early guard correctly rejects closed accounts.

The check correctly identifies a fully closed account (zero lamports AND empty data) and fails fast before expensive validations. The AND condition is appropriate: accounts with empty data but some lamports are handled by the fast-path at line 166, while this early exit prevents processing accounts that have been garbage-collected.

---

71-71: Documentation accurately reflects the new early-exit behavior.

tests/test_undelegate_close_account.rs (1)

45-138: Test setup is comprehensive and correct.

The setup properly configures a closed delegated account (0 lamports, empty data) along with all required PDAs and vaults. This correctly exercises the early-exit path in the undelegate processor.