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 immediately reject undelegation when the delegated account is closed; operations now return a clear "Delegated account is already closed" error.

• Tests

  • Added end-to-end test and 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 that verifies undelegation fails when the delegated account is closed.

Changes

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

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 accurately describes the main change: adding a check to prevent undelegation of closed delegated accounts.
Description check	✅ Passed	The PR description covers Problem, Solution, and Deploy Notes sections, though it omits some template sections like Status/Type/Core Change table and Before & After screenshots.
Linked Issues check	✅ Passed	The PR implements all coding requirements from issue #82: adds early guard to reject undelegation on closed accounts, introduces DelegateAccountClosed error, and includes regression test.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issue #82: error variant addition, undelegation guard, and corresponding test.
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 681bf71 and d583b3c.

📒 Files selected for processing (1)

• src/error.rs

---

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.