Our company talks a lot about code quality. While some of the projects chosen for code audits may not be that familiar to our readers, I'm sure many of them use finance apps. Our readers may not be using this particular application, but the point of the article is that developing finance apps without code quality control poses a significant risk to all users.
BTCPay Server is a free, open-source Bitcoin payment processor which allows users to accept Bitcoin without fees or intermediaries.
BTCPay Server is an actively developing application; its source code is available on GitHub.
Checking the project with the PVS-Studio static analyzer was easy – BTCPay Server is written in C# and can be opened in Visual Studio. PVS-Studio has a plugin for this IDE that makes it possible to run the analysis and view the results directly in Visual Studio.
The article describes 10 of the most curious code fragments where PVS-Studio found errors.
Example 1
V3022 Expression 'request.PaymentTolerance < 0 && request.PaymentTolerance > 100' is always false. Probably the '||' operator should be used here. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 241
private IActionResult Validate(StoreBaseData request)
{
....
if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100)
ModelState.AddModelError(nameof(request.PaymentTolerance),
"PaymentTolerance can only be between 0 and 100 percent");
....
}Many people have faced transaction cancellations due to incorrect data input in financial apps. Sometimes apps just notify users of incorrect data, but sometimes it's all about a silly bug in the code.
So, in this code fragment, the '&&' and '||' operators are mixed up. The code was originally supposed to check for a value between 0 and 100, but the user would not get a warning.
As a result, an incorrect value will traverse through the code and can lead to an error in the application's logic.
Example 2
V3001 There are identical sub-expressions 'e.Name == InvoiceEvent.FailedToConfirm' to the left and to the right of the '||' operator. BTCPayServer\HostedServices\BitpayIPNSender.cs 264
public Task StartAsync(CancellationToken cancellationToken)
{
....
if (invoice.FullNotifications)
{
if (e.Name == InvoiceEvent.Expired ||
e.Name == InvoiceEvent.PaidInFull ||
e.Name == InvoiceEvent.FailedToConfirm || // <=
e.Name == InvoiceEvent.MarkedInvalid ||
e.Name == InvoiceEvent.MarkedCompleted ||
e.Name == InvoiceEvent.FailedToConfirm || // <=
e.Name == InvoiceEvent.Completed ||
e.Name == InvoiceEvent.ExpiredPaidPartial
)
{
await Notify(invoice, e, false, sendMail);
sendMail = false;
}
}
if (e.Name == InvoiceEvent.Confirmed)
{
await Notify(invoice, e, false, sendMail);
sendMail = false;
}
....
}Identical comparisons are a consequence of copy-paste. As a result, the comparison has two identical constants.
We can explain this code fragment in two ways:
I believe the second point is the case.
Let's take a look at the constants:
public class InvoiceEvent : IHasInvoiceId
{
public const string Created = "invoice_created";
public const string ReceivedPayment = "invoice_receivedPayment";
public const string PaymentSettled = "invoice_paymentSettled";
public const string MarkedCompleted = "invoice_markedComplete";
public const string MarkedInvalid = "invoice_markedInvalid";
public const string Expired = "invoice_expired";
public const string ExpiredPaidPartial = "invoice_expiredPaidPartial";
public const string PaidInFull = "invoice_paidInFull";
public const string PaidAfterExpiration = "invoice_paidAfterExpiration";
public const string FailedToConfirm = "invoice_failedToConfirm";
public const string Confirmed = "invoice_confirmed";
public const string Completed = "invoice_completed";
....
}All constants can be divided into 3 groups:
Then we can assume that the PaidAfterExpiration constant should be instead of one of the copies because the repeated code and the unused constant are similar to entries from the payment completion group.
Example 3
V3061 Parameter 'storeId' is always rewritten in method body before being used. BTCPayServer\Controllers\UIStoresController.cs 890
[HttpPost("{storeId}/tokens/create")]
public async Task<IActionResult> CreateToken(string storeId, ....)
{
if (!ModelState.IsValid)
{
return View(nameof(CreateToken), model);
}
model.Label = model.Label ?? String.Empty;
var userId = GetUserId();
if (userId == null)
return Challenge(AuthenticationSchemes.Cookie);
storeId = model.StoreId; // <=
....
}The value of the storeid parameter is rewritten before being used. In most cases, this indicates an error because the function code no longer matches its interface.
Example 4
V3095 The 'request' object was used before it was verified against null. Check lines: 355, 364. BTCPayServer\Controllers\GreenField\GreenfieldPullPaymentController.cs 355
public async Task<IActionResult> CreatePayoutThroughStore(
string storeId, CreatePayoutThroughStoreRequest request)
{
if (request.Approved is true)
{
if (!(await _authorizationService.AuthorizeAsync(....)).Succeeded)
{
return this.CreateAPIPermissionError(....);
}
}
if (request is null ||
!PaymentMethodId.TryParse(request?.PaymentMethod, ....))
{
ModelState.AddModelError(nameof(request.PaymentMethod),
"Invalid payment method");
return this.CreateValidationError(ModelState);
}
....
}The request variable is accessed without checking first, and then it is checked. Something is certainly wrong with this code.
Example 5
V3008 The 'model.StoreName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 166, 165. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 166
private void ToModel(StoreBaseData restModel, StoreData model, ....)
{
var blob = model.GetStoreBlob();
model.StoreName = restModel.Name;
model.StoreName = restModel.Name;
model.StoreWebsite = restModel.Website;
model.SpeedPolicy = restModel.SpeedPolicy;
model.SetDefaultPaymentId(defaultPaymentMethod);
....
}The analyzer detected two identical assignments. There is obvious copy-paste programming, like in the second example. This code fragment may contain an error.
Example 6
V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 59, 64. BTCPayServer\Hosting\BTCpayMiddleware.cs 59
public async Task Invoke(HttpContext httpContext)
{
CultureInfo.CurrentCulture = CultureInfo.InvariantCulture;
CultureInfo.CurrentUICulture = CultureInfo.InvariantCulture;
try
{
var bitpayAuth = GetBitpayAuth(httpContext, out bool isBitpayAuth);
var isBitpayAPI = IsBitpayAPI(httpContext, isBitpayAuth);
if (isBitpayAPI && httpContext.Request.Method == "OPTIONS")
{
httpContext.Response.StatusCode = 200;
httpContext.Response.SetHeader("Access-Control-Allow-Origin", "*");
if (httpContext.Request.Headers.ContainsKey("...."))
{
httpContext.Response.SetHeader("Access-Control-Allow-Headers",
httpContext.Request.Headers["...."].FirstOrDefault());
}
return; // We bypass MVC completely
}
httpContext.SetIsBitpayAPI(isBitpayAPI);
if (isBitpayAPI) // <=
{
httpContext.Response.SetHeader("Access-Control-Allow-Origin", "*");
httpContext.SetBitpayAuth(bitpayAuth);
}
if (isBitpayAPI) // <=
{
await _Next(httpContext);
return;
}
....
}
....
}The isBitpayAPI variable is checked twice in a row. The variable is not changed, while the blocks of conditional statements differ greatly.
We may assume that the variable's state should be updated in the first condition block as follows:
isBitpayAPI = IsBitpayAPI(httpContext, isBitpayAuth);Example 7
V3125 The 'request' object was used after it was verified against null. Check lines: 136, 130. BTCPayServer\Controllers\GreenField\GreenfieldLightningNodeApiController.cs 136
public virtual async Task<IActionResult> OpenChannel(....)
{
var lightningClient = await GetLightningClient(cryptoCode, true);
if (request?.NodeURI is null)
{
ModelState.AddModelError(nameof(request.NodeURI),
"A valid node info was not provided to open a channel with");
}
if (request.ChannelAmount == null)
{
ModelState.AddModelError(nameof(request.ChannelAmount), "....");
}
....
}This code snippet is similar to the fourth example: the request variable is sometimes verified against null, and sometimes it is not. However, in this case, the error is different.
The project's code contains similar code fragments that have extra code between the given conditions:
if (!ModelState.IsValid)
{
return this.CreateValidationError(ModelState);
}So, there are many indirect checks throughout the code, but none in the above code fragment.
Example 8
V3168 Awaiting on expression with potential null value can lead to NullReferenceException. BTCPayServer\HostedServices\InvoiceWatcher.cs 383
private async Task<bool> UpdateConfirmationCount(InvoiceEntity invoice)
{
....
var transactionResult = await _explorerClientProvider.GetExplorerClient(
payment.GetCryptoCode())?.GetTransactionAsync(....);
....
}The '?' operator suggests the return of a null value. In this case, await null will result in a NullReferenceException.
Example 9
V3022 Expression 'items == null' is always false. BTCPayServer\Services\Invoices\InvoiceRepository.cs 427
public async Task MassArchive(string[] invoiceIds, bool archive = true)
{
using var context = _applicationDbContextFactory.CreateContext();
var items = context.Invoices.Where(a => invoiceIds.Contains(a.Id));
if (items == null)
{
return;
}
foreach (InvoiceData invoice in items)
{
invoice.Archived = archive;
}
await context.SaveChangesAsync();
}The Where extension method returns an empty collection (if it does not contain any elements that meet the condition) instead of null. Although the method is self-written in this case, its implementation is still related to the standard method:
public static IQueryable<TEntity> Where<TEntity>(....) where TEntity : class
{
return System.Linq.Queryable.Where(obj, predicate);
}It's possible that the developers didn't intend to call the save changes method, but the method is called because of the incorrect comparison.
Example 10
V3108 It is not recommended to return 'null' from 'ToString()' method. BTCPayServer\Controllers\UILNURLController.cs 364
public class LightningAddressSettings
{
....
public override string ToString()
{
return null;
}
....
}This may not be a bug, but returning a null value from the overridden ToString method is definitely not a good idea, and Microsoft does not recommend it either. There are many cases in which this method is called, sometimes even in unexpected ways, which can cause possible exceptions in the application.
In total, the analyzer issued 239 warnings, and that's quite enough for such a small project. BTCPay Server still has some bugs to explore. However, the 10 warnings described would be enough to reduce the credibility of an application that processes financial data.
It seems that static analysis in general, and even some free static analysis tools, have never been introduced into the project development process. We would be glad if BTCPay Server developers would consider our solution for the code quality control of their project. We are willing to provide the PVS-Studio trial license.
If you want to avoid repeating the same mistakes, download PVS-Studio. It supports the analysis of C, C++, C#, and Java code.