Blockchain’s Bug Tax: The Neo and NBitcoin Mistakes a Linter Spotted | HackerNoon

Blockchain development is a high-stakes game where code quality really matters. A single undetected bug can lead to major and sometimes irreversible financial losses. Should we really gamble on skipping a static analyzer check? Let’s put it to the test by diving into the code of the Neo and NBitcoin projects.

Introduction

Since I brought up the unique nature of blockchain development, let’s delve into what that specifically entails.

First, many blockchain projects handle digital assets with real-world value: tokens, cryptocurrencies, NFTs, access rights, and others. A code error might not just cause a program malfunction but lead directly to users’ financial losses.

Second, fixing code in a blockchain project after release may be challenging. In decentralized networks, every node must agree to accept the changes. As for smart contracts, post-release fixes are often outright impossible. Often, it’s also impossible to change data once it’s recorded on the blockchain, including incorrect data resulting from a code error. These limitations are the price paid for near-perfect data integrity.

Technically, each node stores a full or partial copy of the blockchain to achieve this. The network uses specific algorithms to get all nodes to agree on a single, valid version of the blockchain. If a node blockchain version conflicts with the accepted one, it automatically synchronizes with the network version. So, any unauthorized changes to the blockchain state are rejected by the network.

It’s not a full list, but it sufficiently illustrates a potentially immense cost of a code error in a blockchain project.

This article reviews examples of both evident and potential errors detected by PVS-Studio static analyzer in two open-source C# projects.

• Neo is a full-featured, community-driven blockchain platform.
• NBitcoin is a .NET library for Bitcoin-related operations.

About PVS-Studio

PVS-Studio is a static code analyzer that automatically detects potential errors and security vulnerabilities in the source code. As of this writing, PVS-Studio supports analyzing C#, C, C++, and Java code.

PVS-Studio integrates with various development tools, including IDEs, build systems, CI services, and other code quality tools like SonarQube.

To analyze Neo and NBitcoin, we used a basic approach—a check via an IDE plugin (in this case, Visual Studio). This plugin allows quick analysis of solutions, projects, or individual files directly from the IDE.

It also provides a simple and convenient interface for reviewing analysis results.

Now that we have a general understanding of the analyzer, let’s review the detected issues.

Potential issues in the Neo code

We’ll start with the Neo blockchain, version 3.8.2 (the latest at the time of writing).

Suspiciously redundant expression

internal static CommandStringToken Parse(...., 
                                         ref int index,
                                         ....)
{
  ....
  var ret = new CommandStringToken(....);
  index += end - index;
  return ret;
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3107 Identical expression ‘index’ to the left and to the right of compound assignment. CommandStringToken.cs 80

Note this strange expression: index += end – index. It’s equivalent to index = index – index + end. The subtraction result is always 0. So, the entire expression evaluates to the value of the end variable. This makes the subtraction redundant. This could indicate a bug if, for example, a different value was intended to be subtracted instead of index.

Incorrect format

public override string ToString()
{
  var sb = new StringBuilder();

  sb.AppendFormat("{1:X04} {2,-10}{3}{4}", 
                  Position, 
                  OpCode, 
                  DecodeOperand());

  return sb.ToString();
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3025 [CWE-685] Incorrect format. A different number of format items is expected while calling ‘AppendFormat’ function. Format items not used: {3}, {4}. Arguments not used: 1st. VMInstruction.cs 105

Calling the overridden ToString method inevitably causes an exception. This is due to an incorrect sb.AppendFormat call containing two mistakes.

• The number of arguments to insert is less than the number of placeholders in the format string, which causes an exception.
• Even if we fix the first issue by matching the number of arguments and placeholders, the call still throws an exception. It’s because the placeholder indexing starts at 0, not 1. This means a 5th argument is required for the placeholder with the index 4, which is absent.

Operator precedence confusion

public override int Size =>   base.Size
                            + ChangeViewMessages?.Values.GetVarSize() ?? 0
                            + 1 + PrepareRequestMessage?.Size ?? 0
                            + PreparationHash?.Size ?? 0
                            + PreparationMessages?.Values.GetVarSize() ?? 0
                            + CommitMessages?.Values.GetVarSize() ?? 0;

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3123 [CWE-783] Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. RecoveryMessage.cs 35

The analyzer issued several V3123 warnings for this code, but we’ll break down only one for brevity. The ?? operator has lower precedence than the + operator. However, the formatting of this expression suggests the developer expected the opposite.

Does the order of operations matter here? To answer, let’s look at the example of an addition sub-expression if ChangeViewMessages is null:

base.Size + ChangeViewMessages?.Values.GetVarSize() ?? 0

Enter fullscreen mode Exit fullscreen mode

Regardless of the base.Size value, the sub-expression result is always 0 because adding base.Size to null results in null.

If we place ChangeViewMessages?.Values.GetVarSize() ?? 0 in parentheses, changing the operation order, the result becomes base.Size.

Potential null dereference

Issue 1

public OracleNeoFSProtocol(Wallet wallet, ECPoint[] oracles)
{
  byte[] key = oracles.Select(p => wallet.GetAccount(p))
                      .Where(p => p is not null && p.HasKey && !p.Lock)
                      .FirstOrDefault()
                      .GetKey()
                      .PrivateKey;

  privateKey = key.LoadPrivateKey();
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3146 [CWE-476] Possible null dereference. The ‘FirstOrDefault’ can return default null value. OracleNeoFSProtocol.cs 37

This code risks a typical null dereference by immediately using the value returned by FirstOfDefault on a reference-type collection. If the collection is empty, this method returns null, leading to an exception.

Issue 2

public bool ValidatorsChanged
{
  get
  {
    ....
    TrimmedBlock currentBlock = NativeContract.Ledger.GetTrimmedBlock(....);

    TrimmedBlock previousBlock = 
      NativeContract.Ledger.GetTrimmedBlock(...., currentBlock.Header  // <=
  .PrevHash);                                                  

    return    currentBlock.Header.NextConsensus
           != previousBlock.Header.NextConsensus;  // <=
  }
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warnings:

V3080 [CWE-476] Possible null dereference. Consider inspecting ‘currentBlock’. ConsensusContext.cs 89

V3080 [CWE-476] Possible null dereference. Consider inspecting ‘previousBlock’. ConsensusContext.cs 90

The analyzer warns twice about potential null dereferences, pointing to the currentBlock and previousBlock variables. Why is dereferencing these variables dangerous? Let’s look at their source—the GetTrimmedBlock method:

public TrimmedBlock GetTrimmedBlock(IReadOnlyStore snapshot, UInt256 hash)
{
  if (snapshot is null)
    throw new ArgumentNullException(nameof(snapshot));

  var key = CreateStorageKey(Prefix_Block, hash);

  if (snapshot.TryGet(key, out var item))
    return item.Value.AsSerializable<TrimmedBlock>();

  return null;
}

Enter fullscreen mode Exit fullscreen mode

This method can indeed return null. It’s possible that this might only occur under specific circumstances. Only the code author can confirm this for sure. We can estimate the probability by checking how often the GetTrimmedBlock return value is checked for null elsewhere. I found that the return value is checked in 70% of calls, indicating a significant risk of an exception.

Issue 3

private void OnTimer(Timer timer)
{
  ....
  if (   timer.Height != context.Block.Index     // <=
      || timer.ViewNumber != context.ViewNumber)
  {
    return;
  }

  if (   context.Block != null                 // <=
      && context.TransactionHashes?.Length > context.Transactions?
    .Count)
   {
     ....
   }     
}

The analyzer warning: V3095 [CWE-476] The ‘context.Block’ object was used before it was verified against null. Check lines: 173, 191. ConsensusService.cs 173

Another warning about a potential null dereference. The context.Block property is used without the check first, but is checked for null later. At best, this is a redundant check. However, it’s possible that the code containing the dereference was added later than the base logic with the check. In this case, the developer might not have noticed that context.Block could be null.

Incorrect loop

[RpcMethodWithParams]
protected internal virtual JToken GetCandidates()
{
  ....
  foreach (var item in resultstack)
  {
    var value = (Array)item;
    foreach (Struct ele in value)
    {
      var publickey = ele[0].GetSpan().ToHexString();
      json["publickey"] = publickey;
      json["votes"] = ele[1].GetInteger().ToString();

      json["active"] = validators.ToByteArray()
                                 .ToHexString()
                                 .Contains(publickey);
      jArray.Add(json);
      json = new();
    }

    return jArray;          // <=
  }
  ....
}

The analyzer warning: V3020 [CWE-670] An unconditional ‘return’ within a loop. RpcServer.Blockchain.cs 380

Note the method return statement inside the loop body. This loop lacks any conditions or continue statements to alter its flow. As a result, it always exits after the first iteration, which almost certainly is a critical error. The developer probably made a typo—the intention was likely to return the value after the loop.

Typo in the Equals method implementation

public bool Equals(Nep11BalanceKey other)
{
  if (other is null) return false;

  if (ReferenceEquals(this, other)) return true;

  return    UserScriptHash.Equals(other.UserScriptHash) 
         && AssetScriptHash.Equals(AssetScriptHash)     // <=
         && Token.Equals(other.Token);
}

The analyzer warning: V3062 An object ‘AssetScriptHash’ is used as an argument to its own method. Consider checking the first actual argument of the ‘Equals’ method. Nep11BalanceKey.cs 57

A simple but very subtle error. In this Equals implementation, theAssetScriptHash field of the parent object is compared to itself. Clearly, the intention was to compare it with the value of the same field of the other object.

Interestingly enough, the analyzer found the exact same error in the equivalent code of another class:

public bool Equals(Nep17BalanceKey other)
{
  if (other is null) return false;
  if (ReferenceEquals(this, other)) return true;
  return    UserScriptHash.Equals(other.UserScriptHash) 
         && AssetScriptHash.Equals(AssetScriptHash);
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3062 An object ‘AssetScriptHash’ is used as an argument to its own method. Consider checking the first actual argument of the ‘Equals’ method. Nep17BalanceKey.cs 50

Potential issues in NBitcoin

Now let’s move on to potential issues in NBitcoin version 9.0.1.

Error due to careless copy paste

bool SameSigHash(uint a, uint b)
{
  if (a == b)
    return true;

  if (GetTransaction() is not IHasForkId)
    return false;

  a = ((uint)a & ~(0x40u));
  b = ((uint)a & ~(0x40u)); // <=
  return a == b;
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and ‘b’ variable should be used instead of ‘a’ PSBTInput.cs 948

At the end of the method, a and b variables are assigned the result of the same expression. While this could theoretically be intentional, the following comparison makes that unlikely. It looks like the line for ‘b’ was copied from ‘a’ but the author forgot to edit it.

Infinite recursion

public static Message ReadNext(Socket socket, 
                               Network network, 
                               uint version, 
                               CancellationToken cancellationToken, 
                               out PerformanceCounter counter)
{
  return ReadNext(socket, 
                  network, 
                  version, 
                  cancellationToken, 
                  out counter);
}

Enter fullscreen mode Exit fullscreen mode

Here is the analyzer warning:

V3110 [CWE-674] Possible infinite recursion inside ‘ReadNext’ method. Message.cs 167

Calling this method overload causes StackOverflowException, as the only expression in its body is a recursive call to itself.

Identical switch blocks

public override string ToString()
{
  switch (this.Tag)
  {
    case Tags.BoolAnd:
      return "BoolAnd";
    case Tags.BoolOr:
      return "BoolAnd";   // <=
    case Tags.Add:
      return "Add";
    case Tags.Equal:
      return "Equal";
    case Tags.EqualVerify:
      return "EqualVerify";
    ....
  }
  throw new Exception("Unreachable");
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3139 Two or more case-branches perform the same actions. ScriptToken.cs 169

The ToString method implementation contains a switch statement where one case block duplicates another. Given the explicit pattern implying each case block should be unique, we can confidently say this is a typo.

Overloading Equals method without overloading GetHashCode

public class MoneyBag : IMoney, 
                        IEnumerable<IMoney>, 
                        IEquatable<MoneyBag>
{
  ....
  public bool Equals(MoneyBag other)
  {
    return Equals(other as IMoney);
  }

  public bool Equals(IMoney other)
  {
    if (other is null)
      return false;
    var m = new MoneyBag(other);
    return m._bag.SequenceEqual(_bag);
  }
  ....
}

Enter fullscreen mode Exit fullscreen mode

The analyzer warning: V3126 Type ‘MoneyBag’ implementing IEquatable interface does not override ‘GetHashCode’ method. Money.cs 78

The analyzer detected a class implementing the IEquatable<T> interface but not overriding the GetHashCode method. This can lead to issues, as some methods, like those in the Linq library, start by checking hash codes. They only use the Equals method if the hash codes match. If the hash codes differ, the objects are considered unequal.

Conclusion

Our check with PVS-Studio static analyzer uncovered several issues, even in the high-stakes code of blockchain projects. Some of these could be quite serious, leading to unexpected exceptions, incorrect equality checks, and infinite recursion.

This article covered most obvious problems, while many warnings requiring deeper code understanding remained behind the scenes.

If you want to try PVS-Studio on your project, you’re welcome to get a trial license on the official website. This documentation section provides activation instructions and links to other sections to help you get started with the analyzer.

Thanks for your attention and see you in future articles!