Invoice Payment Hash Inconsistency in CheckPendingInvoices #5

New Issue

Closed

opened 2025-05-26 16:52:21 +00:00 by saulteafarmer · 1 comment

saulteafarmer commented 2025-05-26 16:52:21 +00:00

Owner

Copy Link

There's a critical bug in the CheckPendingInvoices() method that causes payment verification to fail due to inconsistent payment hash handling between invoice creation and payment status checking. This results in successfully paid invoices being treated as unpaid, causing players to lose their purchases.

Root Cause Analysis

Inconsistent Hash Case Handling

The issue stems from redundant and inconsistent case conversion of payment hashes throughout the payment verification process:

1. Invoice Creation - Hash Stored as Lowercase (Lines ~580, ~630)

var pendingInvoice = new PendingInvoice
{
    RHash = invoiceResponse.PaymentHash.ToLower(), // ← Stored as lowercase
    Player = player,
    // ...
};
pendingInvoices.Add(pendingInvoice);

2. CheckPendingInvoices - Redundant Conversion (Lines ~450-470)

private void CheckPendingInvoices()
{
    foreach (var invoice in pendingInvoices.ToList())
    {
        string localPaymentHash = invoice.RHash.ToLower(); // ❌ REDUNDANT: Already lowercase
        
        CheckInvoicePaid(localPaymentHash, isPaid =>
        {
            // Payment checking logic uses localPaymentHash
        });
    }
}

3. Retry Counter Logic - Mixed Hash Usage (Lines ~475-545)

// SUCCESS PATH - Uses localPaymentHash
if (isPaid)
{
    // ...
    retryCounts.Remove(invoice.RHash); // ❌ BUG: Uses invoice.RHash (lowercase)
}
else
{
    // RETRY PATH - Inconsistent hash usage
    if (!retryCounts.ContainsKey(localPaymentHash)) // ← Uses localPaymentHash
    {
        retryCounts[localPaymentHash] = 0;
        Puts($"Initialized retry count for paymentHash: {localPaymentHash}");
    }

    retryCounts[localPaymentHash]++; // ← Uses localPaymentHash
    
    if (retryCounts[localPaymentHash] >= maxRetries)
    {
        // TIMEOUT PATH - Mixed usage
        int finalRetryCount = retryCounts[localPaymentHash]; // ← localPaymentHash
        retryCounts.Remove(localPaymentHash);               // ← localPaymentHash
        
        // But logging uses invoice.RHash
        Puts($"Invoice {localPaymentHash} expired after {finalRetryCount} retries.");
    }
}

The Core Problem

Since invoice.RHash is already lowercase from creation, calling .ToLower() again creates no difference between localPaymentHash and invoice.RHash. However, the inconsistent usage of these variables throughout the method creates potential for bugs and makes the code fragile.

Specific Code Issues

Issue 1: Redundant Conversion

// invoice.RHash is already lowercase, so this is redundant
string localPaymentHash = invoice.RHash.ToLower();

Issue 2: Inconsistent Variable Usage

// Sometimes uses localPaymentHash
retryCounts[localPaymentHash]++;

// Sometimes uses invoice.RHash  
retryCounts.Remove(invoice.RHash);

Issue 3: Potential Dictionary Key Mismatch

If there were ever a case where the hash formats differed, the retry counter logic would break:

// Increment with one key format
retryCounts[localPaymentHash]++;

// Remove with different key format  
retryCounts.Remove(invoice.RHash); // Could be different key!

Impact

• ❌ Payment verification failures - Inconsistent hash handling could cause lookup failures
• ❌ Players don't receive purchased items/VIP status - Paid invoices treated as unpaid
• ❌ Currency gets incorrectly refunded - Players lose money due to false timeouts
• ❌ False timeout errors - Successful payments marked as expired
• ❌ Inconsistent logging - Transaction records use mixed hash formats
• ❌ Retry counter corruption - Dictionary operations on mismatched keys

Steps to Reproduce

Potential Scenario (if hash formats ever differed):

1. Player uses /buyblood 10 command
2. Invoice created with RHash = paymentHash.ToLower()
3. Player pays the Lightning invoice successfully
4. CheckPendingInvoices() creates localPaymentHash = invoice.RHash.ToLower()
5. Payment verification fails due to inconsistent hash usage
6. Invoice eventually times out and player gets refunded instead of receiving items

Proposed Fix

Solution: Consistent Hash Handling

Eliminate redundant conversions and use consistent variable names:

private void CheckPendingInvoices()
{
    foreach (var invoice in pendingInvoices.ToList())
    {
        // ✅ Use consistent hash format throughout - no redundant conversion
        string paymentHash = invoice.RHash; // Already lowercase from creation
        
        CheckInvoicePaid(paymentHash, isPaid =>
        {
            if (isPaid)
            {
                pendingInvoices.Remove(invoice);

                switch (invoice.Type)
                {
                    case PurchaseType.Currency:
                        RewardPlayer(invoice.Player, invoice.Amount);
                        break;
                    case PurchaseType.Vip:
                        GrantVip(invoice.Player);
                        break;
                    case PurchaseType.SendBitcoin:
                        invoice.Player.Reply(Lang("CurrencySentSuccess", invoice.Player.Id, invoice.Amount, currencyName));
                        break;
                }

                // Log successful payment
                if (invoice.Type == PurchaseType.SendBitcoin)
                {
                    var logEntry = new SellInvoiceLogEntry
                    {
                        SteamID = GetPlayerId(invoice.Player),
                        LightningAddress = ExtractLightningAddress(invoice.Memo),
                        Success = true,
                        SatsAmount = invoice.Amount,
                        PaymentHash = paymentHash, // ✅ Consistent hash usage
                        CurrencyReturned = false,
                        Timestamp = DateTime.UtcNow,
                        RetryCount = retryCounts.ContainsKey(paymentHash) ? retryCounts[paymentHash] : 0
                    };
                    LogSellTransaction(logEntry);
                }
                else
                {
                    var logEntry = CreateBuyInvoiceLogEntry(
                        player: invoice.Player,
                        invoiceID: paymentHash, // ✅ Consistent hash usage
                        isPaid: true,
                        amount: invoice.Type == PurchaseType.SendBitcoin ? invoice.Amount : invoice.Amount * pricePerCurrencyUnit,
                        type: invoice.Type,
                        retryCount: retryCounts.ContainsKey(paymentHash) ? retryCounts[paymentHash] : 0
                    );
                    LogBuyInvoice(logEntry);
                }

                // ✅ Consistent cleanup
                retryCounts.Remove(paymentHash);
                Puts($"Invoice {paymentHash} marked as paid.");
            }
            else
            {
                // ✅ Use consistent hash for retry counting
                if (!retryCounts.ContainsKey(paymentHash))
                {
                    retryCounts[paymentHash] = 0;
                    Puts($"Initialized retry count for paymentHash: {paymentHash}");
                }

                retryCounts[paymentHash]++;
                Puts($"Retry count for paymentHash {paymentHash}: {retryCounts[paymentHash]} of {maxRetries}");

                if (retryCounts[paymentHash] >= maxRetries)
                {
                    pendingInvoices.Remove(invoice);
                    int finalRetryCount = retryCounts[paymentHash];
                    retryCounts.Remove(paymentHash); // ✅ Consistent cleanup

                    PrintWarning($"Invoice for player {GetPlayerId(invoice.Player)} expired (amount: {invoice.Amount} sats).");
                    invoice.Player.Reply(Lang("InvoiceExpired", invoice.Player.Id, invoice.Amount));

                    // Handle expiry logic with consistent hash usage
                    if (invoice.Type == PurchaseType.SendBitcoin)
                    {
                        var basePlayer = invoice.Player.Object as BasePlayer;
                        if (basePlayer != null)
                        {
                            ReturnCurrency(basePlayer, invoice.Amount / satsPerCurrencyUnit);
                        }

                        var failedLogEntry = new SellInvoiceLogEntry
                        {
                            SteamID = GetPlayerId(invoice.Player),
                            LightningAddress = ExtractLightningAddress(invoice.Memo),
                            Success = false,
                            SatsAmount = invoice.Amount,
                            PaymentHash = paymentHash, // ✅ Consistent hash usage
                            CurrencyReturned = true,
                            Timestamp = DateTime.UtcNow,
                            RetryCount = finalRetryCount
                        };
                        LogSellTransaction(failedLogEntry);
                    }
                    else
                    {
                        var failedLogEntry = CreateBuyInvoiceLogEntry(
                            player: invoice.Player,
                            invoiceID: paymentHash, // ✅ Consistent hash usage
                            isPaid: false,
                            amount: invoice.Type == PurchaseType.SendBitcoin ? invoice.Amount : invoice.Amount * pricePerCurrencyUnit,
                            type: invoice.Type,
                            retryCount: finalRetryCount
                        );
                        LogBuyInvoice(failedLogEntry);
                    }

                    Puts($"Invoice {paymentHash} expired after {finalRetryCount} retries.");
                }
            }
        });
    }
}

Additional Improvements

Ensure Consistent Hash Storage

Make sure all hash storage is consistently lowercase:

// In all invoice creation methods
var pendingInvoice = new PendingInvoice
{
    RHash = invoiceResponse.PaymentHash?.ToLower() ?? string.Empty, // ✅ Ensure lowercase
    // ...
};

Add Hash Validation

private bool IsValidPaymentHash(string hash)
{
    return !string.IsNullOrEmpty(hash) && hash.All(c => char.IsLetterOrDigit(c)) && hash == hash.ToLower();
}

Environment

• Lightning Network: Payment hashes are case-sensitive identifiers
• Dictionary Operations: Key matching must be exact
• Async Processing: Hash consistency critical across async operations

Severity

🔴 Critical - Players lose real money when payments are incorrectly processed

Additional Notes

This bug represents a fundamental data consistency issue that could cause the plugin's core payment processing to fail unpredictably. The fix ensures that:

• Payment hashes are consistently formatted throughout the codebase
• Dictionary operations use consistent keys
• Logging and audit trails maintain data integrity
• Players receive their purchases when payments are successful

There's a **critical bug** in the `CheckPendingInvoices()` method that causes payment verification to fail due to **inconsistent payment hash handling** between invoice creation and payment status checking. This results in successfully paid invoices being treated as unpaid, causing players to lose their purchases. ## Root Cause Analysis ### **Inconsistent Hash Case Handling** The issue stems from **redundant and inconsistent** case conversion of payment hashes throughout the payment verification process: #### **1. Invoice Creation - Hash Stored as Lowercase** (Lines ~580, ~630) ```csharp var pendingInvoice = new PendingInvoice { RHash = invoiceResponse.PaymentHash.ToLower(), // ← Stored as lowercase Player = player, // ... }; pendingInvoices.Add(pendingInvoice); ``` #### **2. CheckPendingInvoices - Redundant Conversion** (Lines ~450-470) ```csharp private void CheckPendingInvoices() { foreach (var invoice in pendingInvoices.ToList()) { string localPaymentHash = invoice.RHash.ToLower(); // ❌ REDUNDANT: Already lowercase CheckInvoicePaid(localPaymentHash, isPaid => { // Payment checking logic uses localPaymentHash }); } } ``` #### **3. Retry Counter Logic - Mixed Hash Usage** (Lines ~475-545) ```csharp // SUCCESS PATH - Uses localPaymentHash if (isPaid) { // ... retryCounts.Remove(invoice.RHash); // ❌ BUG: Uses invoice.RHash (lowercase) } else { // RETRY PATH - Inconsistent hash usage if (!retryCounts.ContainsKey(localPaymentHash)) // ← Uses localPaymentHash { retryCounts[localPaymentHash] = 0; Puts($"Initialized retry count for paymentHash: {localPaymentHash}"); } retryCounts[localPaymentHash]++; // ← Uses localPaymentHash if (retryCounts[localPaymentHash] >= maxRetries) { // TIMEOUT PATH - Mixed usage int finalRetryCount = retryCounts[localPaymentHash]; // ← localPaymentHash retryCounts.Remove(localPaymentHash); // ← localPaymentHash // But logging uses invoice.RHash Puts($"Invoice {localPaymentHash} expired after {finalRetryCount} retries."); } } ``` ### **The Core Problem** Since `invoice.RHash` is **already lowercase** from creation, calling `.ToLower()` again creates no difference between `localPaymentHash` and `invoice.RHash`. However, the **inconsistent usage** of these variables throughout the method creates potential for bugs and makes the code fragile. ## Specific Code Issues ### **Issue 1: Redundant Conversion** ```csharp // invoice.RHash is already lowercase, so this is redundant string localPaymentHash = invoice.RHash.ToLower(); ``` ### **Issue 2: Inconsistent Variable Usage** ```csharp // Sometimes uses localPaymentHash retryCounts[localPaymentHash]++; // Sometimes uses invoice.RHash retryCounts.Remove(invoice.RHash); ``` ### **Issue 3: Potential Dictionary Key Mismatch** If there were ever a case where the hash formats differed, the retry counter logic would break: ```csharp // Increment with one key format retryCounts[localPaymentHash]++; // Remove with different key format retryCounts.Remove(invoice.RHash); // Could be different key! ``` ## Impact - ❌ **Payment verification failures** - Inconsistent hash handling could cause lookup failures - ❌ **Players don't receive purchased items/VIP status** - Paid invoices treated as unpaid - ❌ **Currency gets incorrectly refunded** - Players lose money due to false timeouts - ❌ **False timeout errors** - Successful payments marked as expired - ❌ **Inconsistent logging** - Transaction records use mixed hash formats - ❌ **Retry counter corruption** - Dictionary operations on mismatched keys ## Steps to Reproduce **Potential Scenario** (if hash formats ever differed): 1. Player uses `/buyblood 10` command 2. Invoice created with `RHash = paymentHash.ToLower()` 3. Player pays the Lightning invoice successfully 4. `CheckPendingInvoices()` creates `localPaymentHash = invoice.RHash.ToLower()` 5. Payment verification fails due to inconsistent hash usage 6. Invoice eventually times out and player gets refunded instead of receiving items ## Proposed Fix ### **Solution: Consistent Hash Handling** Eliminate redundant conversions and use consistent variable names: ```csharp private void CheckPendingInvoices() { foreach (var invoice in pendingInvoices.ToList()) { // ✅ Use consistent hash format throughout - no redundant conversion string paymentHash = invoice.RHash; // Already lowercase from creation CheckInvoicePaid(paymentHash, isPaid => { if (isPaid) { pendingInvoices.Remove(invoice); switch (invoice.Type) { case PurchaseType.Currency: RewardPlayer(invoice.Player, invoice.Amount); break; case PurchaseType.Vip: GrantVip(invoice.Player); break; case PurchaseType.SendBitcoin: invoice.Player.Reply(Lang("CurrencySentSuccess", invoice.Player.Id, invoice.Amount, currencyName)); break; } // Log successful payment if (invoice.Type == PurchaseType.SendBitcoin) { var logEntry = new SellInvoiceLogEntry { SteamID = GetPlayerId(invoice.Player), LightningAddress = ExtractLightningAddress(invoice.Memo), Success = true, SatsAmount = invoice.Amount, PaymentHash = paymentHash, // ✅ Consistent hash usage CurrencyReturned = false, Timestamp = DateTime.UtcNow, RetryCount = retryCounts.ContainsKey(paymentHash) ? retryCounts[paymentHash] : 0 }; LogSellTransaction(logEntry); } else { var logEntry = CreateBuyInvoiceLogEntry( player: invoice.Player, invoiceID: paymentHash, // ✅ Consistent hash usage isPaid: true, amount: invoice.Type == PurchaseType.SendBitcoin ? invoice.Amount : invoice.Amount * pricePerCurrencyUnit, type: invoice.Type, retryCount: retryCounts.ContainsKey(paymentHash) ? retryCounts[paymentHash] : 0 ); LogBuyInvoice(logEntry); } // ✅ Consistent cleanup retryCounts.Remove(paymentHash); Puts($"Invoice {paymentHash} marked as paid."); } else { // ✅ Use consistent hash for retry counting if (!retryCounts.ContainsKey(paymentHash)) { retryCounts[paymentHash] = 0; Puts($"Initialized retry count for paymentHash: {paymentHash}"); } retryCounts[paymentHash]++; Puts($"Retry count for paymentHash {paymentHash}: {retryCounts[paymentHash]} of {maxRetries}"); if (retryCounts[paymentHash] >= maxRetries) { pendingInvoices.Remove(invoice); int finalRetryCount = retryCounts[paymentHash]; retryCounts.Remove(paymentHash); // ✅ Consistent cleanup PrintWarning($"Invoice for player {GetPlayerId(invoice.Player)} expired (amount: {invoice.Amount} sats)."); invoice.Player.Reply(Lang("InvoiceExpired", invoice.Player.Id, invoice.Amount)); // Handle expiry logic with consistent hash usage if (invoice.Type == PurchaseType.SendBitcoin) { var basePlayer = invoice.Player.Object as BasePlayer; if (basePlayer != null) { ReturnCurrency(basePlayer, invoice.Amount / satsPerCurrencyUnit); } var failedLogEntry = new SellInvoiceLogEntry { SteamID = GetPlayerId(invoice.Player), LightningAddress = ExtractLightningAddress(invoice.Memo), Success = false, SatsAmount = invoice.Amount, PaymentHash = paymentHash, // ✅ Consistent hash usage CurrencyReturned = true, Timestamp = DateTime.UtcNow, RetryCount = finalRetryCount }; LogSellTransaction(failedLogEntry); } else { var failedLogEntry = CreateBuyInvoiceLogEntry( player: invoice.Player, invoiceID: paymentHash, // ✅ Consistent hash usage isPaid: false, amount: invoice.Type == PurchaseType.SendBitcoin ? invoice.Amount : invoice.Amount * pricePerCurrencyUnit, type: invoice.Type, retryCount: finalRetryCount ); LogBuyInvoice(failedLogEntry); } Puts($"Invoice {paymentHash} expired after {finalRetryCount} retries."); } } }); } } ``` ## Additional Improvements ### **Ensure Consistent Hash Storage** Make sure all hash storage is consistently lowercase: ```csharp // In all invoice creation methods var pendingInvoice = new PendingInvoice { RHash = invoiceResponse.PaymentHash?.ToLower() ?? string.Empty, // ✅ Ensure lowercase // ... }; ``` ### **Add Hash Validation** ```csharp private bool IsValidPaymentHash(string hash) { return !string.IsNullOrEmpty(hash) && hash.All(c => char.IsLetterOrDigit(c)) && hash == hash.ToLower(); } ``` ## Environment - **Lightning Network**: Payment hashes are case-sensitive identifiers - **Dictionary Operations**: Key matching must be exact - **Async Processing**: Hash consistency critical across async operations ## Severity 🔴 **Critical** - Players lose real money when payments are incorrectly processed ## Additional Notes This bug represents a fundamental data consistency issue that could cause the plugin's core payment processing to fail unpredictably. The fix ensures that: - Payment hashes are consistently formatted throughout the codebase - Dictionary operations use consistent keys - Logging and audit trails maintain data integrity - Players receive their purchases when payments are successful

saulteafarmer commented 2025-06-20 18:48:32 +00:00

Author

Owner

Copy Link

Fixed on 0.4.0

Fixed on 0.4.0

saulteafarmer closed this issue 2025-06-20 18:48:32 +00:00

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

0.4.0

0.3.0

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

saulteafarmer

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rustysats/orangemart.cs#5

No description provided.