feat: rent recipient in forward_token

[Frizellle]

No description provided.

[julien51]

thanks @Frizellle !

[Copilot]

Pull request overview

This PR adds support for specifying a custom rent recipient in the forward_token function. When forwarding tokens, the rent from the closed token account can now be sent to a specified account instead of always going to the sender.

Changes:

• Modified forward_token to accept an optional rent recipient via remaining accounts
• Added validation to ensure rent recipient account is writable
• Added comprehensive test coverage for the new rent recipient functionality

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
packages/solana-vm/programs/relay-forwarder/src/lib.rs	Implements rent recipient logic with validation and new error type
packages/solana-vm/tests/relay-forwarder.ts	Adds test case verifying rent recipient receives lamports from closed account

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Syntax error: missing semicolon and method call. The line should be split into two statements: closing the parenthesis for CpiContext::new_with_signer with ); on line 89, then calling close_account(close_account_cpi_ctx)?; on line 90.

Suggested change

	)
	);
	close_account(close_account_cpi_ctx)?;

[Copilot]

Variable name DepositEvent uses PascalCase which is typically reserved for classes/types in TypeScript. Consider renaming to depositEvent to follow conventional camelCase naming for variables.

Suggested change

	const DepositEvent = events.find((event) => event.name === "depositEvent");
	assert.exists(DepositEvent);
	assert.equal(DepositEvent?.data.amount.toNumber(), depositAmount);
	assert.equal(
	DepositEvent?.data.depositor.toBase58(),
	depositor.publicKey.toBase58()
	);
	assert.equal(DepositEvent?.data.id.toString(), id.toString());
	assert.equal(DepositEvent?.data.token.toBase58(), mint.toBase58());
	const depositEvent = events.find((event) => event.name === "depositEvent");
	assert.exists(depositEvent);
	assert.equal(depositEvent?.data.amount.toNumber(), depositAmount);
	assert.equal(
	depositEvent?.data.depositor.toBase58(),
	depositor.publicKey.toBase58()
	);
	assert.equal(depositEvent?.data.id.toString(), id.toString());
	assert.equal(depositEvent?.data.token.toBase58(), mint.toBase58());

[julien51]

Security Audit Report

I've completed a thorough security review and found several concerning issues:

🔴 CRITICAL: Rent Theft via Front-Running

The Issue:
The PR allows the sender to redirect rent refunds to ANY writable account via remaining_accounts[0]. Combined with the lack of access control on forward_token, this creates a rent theft vulnerability.

Attack Scenario:

1. Alice creates forwarder_token_account (pays ~0.002 SOL rent)
2. Alice deposits 1M tokens to forwarder
3. Bob monitors the mempool
4. Bob front-runs with forward_token, specifying HIS account as rent recipient
5. Bob steals Alice's rent refund (~0.002 SOL)
6. Alice's tokens are forwarded but she loses the rent

Code Location: packages/solana-vm/programs/relay-forwarder/src/lib.rs:71-82

let rent_fee_payer = ctx.remaining_accounts.get(0);

let destination_info: AccountInfo<'info> = match rent_fee_payer {
    Some(account) => {
        require!(account.is_writable, ForwarderError::RentRecipientMustBeWritable);
        account.clone()  // ⚠️ No validation of ownership or authority
    }
    None => ctx.accounts.sender.to_account_info(),
};

---

🟡 HIGH: Missing Rent Recipient Validation

The Issue:
The code only checks is_writable but performs NO validation on:

• Account ownership
• Account type (could be PDA, token account, program account, etc.)
• Relationship to the sender

Risks:

• Rent could be sent to:
  • A PDA that can't access the funds
  • A program-owned account
  • An attacker's account (as shown above)

Recommendation:
Add validation that the rent recipient is either:

1. The sender (signer)
2. A system account owned by the sender
3. Or require a signature from the rent recipient

---

🟡 MEDIUM: No Access Control on forward_token

Pre-existing Issue (made worse by PR):
Anyone can call forward_token - there's no verification that the sender has any relationship to the tokens being forwarded or the account being closed.

Before PR: Rent goes to the caller (potentially unfair but deterministic)
After PR: Caller can redirect rent anywhere (enables theft)

Attack Variations:

1. Griefing: Attacker calls forward_token with invalid rent recipient to cause failures
2. MEV Extraction: Bots compete to claim rent refunds from public forwarder accounts
3. Rent Denial: Legitimate user loses rent to faster attacker

---

🟢 LOW: Missing Test Coverage

The test only covers the happy path. Missing security-critical tests for:

• Multiple concurrent callers (race conditions)
• Invalid/malicious rent recipients
• Unauthorized senders
• Rent recipient = closed account (edge case)

---

Recommended Fixes

Option 1: Require Rent Recipient to be Sender (Conservative)

let destination_info: AccountInfo<'info> = match rent_fee_payer {
    Some(account) => {
        require!(account.is_writable, ForwarderError::RentRecipientMustBeWritable);
        require!(
            account.key() == ctx.accounts.sender.key(), 
            ForwarderError::RentRecipientMustBeSender
        );
        account.clone()
    }
    None => ctx.accounts.sender.to_account_info(),
};

Option 2: Require Rent Recipient Signature (More Flexible)

let destination_info: AccountInfo<'info> = match rent_fee_payer {
    Some(account) => {
        require!(account.is_writable, ForwarderError::RentRecipientMustBeWritable);
        require!(account.is_signer, ForwarderError::RentRecipientMustSign);
        account.clone()
    }
    None => ctx.accounts.sender.to_account_info(),
};

Option 3: Add Access Control to forward_token

Track who created the forwarder_token_account and only allow them to call forward_token:

// Add to ForwardToken struct:
pub struct ForwardToken<'info> {
    // ... existing fields ...
    
    /// The original creator/funder of the forwarder token account
    #[account(
        constraint = creator.key() == forwarder_token_account.get_creator() 
            @ ForwarderError::UnauthorizedSender
    )]
    pub creator: Signer<'info>,
}

(Note: This requires tracking the creator, which isn't currently stored)

---

Summary

VERDICT: ❌ DO NOT MERGE without fixes

The PR introduces a critical rent theft vulnerability by allowing arbitrary rent redirection without authorization checks. The most practical fix is Option 1 - require the rent recipient to be the sender. This maintains the intended functionality (allowing senders to specify where their rent goes) while preventing theft.

Impact Assessment:

• Severity: High (financial loss via front-running)
• Likelihood: High (easy to exploit, profitable for MEV bots)
• Affected Value: ~0.002 SOL per forwarder token account (small per-attack but scales with volume)

Questions for Clarification:

1. Who should rightfully receive the rent? The person who created the account, the person who calls forward_token, or should it be configurable?
2. Is forward_token meant to be permissionless? If not, what authorization model should exist?
3. What's the use case for redirecting rent to a different account? This will inform the appropriate security model.