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

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. Neo is a full-featured, community-driven blockchain platform. Neo NBitcoin is a .NET library for Bitcoin-related operations. NBitcoin 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 PVS-Studio integrates with various development tools, including IDEs, build systems, CI services, and other code quality tools like SonarQube. integrates 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). 3.8.2 Suspiciously redundant expression internal static CommandStringToken Parse(...., ref int index, ....) { .... var ret = new CommandStringToken(....); index += end - index; return ret; } 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 V3107 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. index += end – index index = index – index + end 0 end 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(); } 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 V3025 VMInstruction.cs 105 Calling the overridden ToString method inevitably causes an exception. This is due to an incorrect sb.AppendFormat call containing two mistakes. ToString sb.AppendFormat 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. 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; 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 V3123 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: ChangeViewMessages null base.Size + ChangeViewMessages?.Values.GetVarSize() ?? 0 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. base.Size base.Size null null If we place ChangeViewMessages?.Values.GetVarSize() ?? 0 in parentheses, changing the operation order, the result becomes base.Size. ChangeViewMessages?.Values.GetVarSize() ?? 0 base.Size Potential null dereference Issue 1 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(); } 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 V3146 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. null FirstOfDefault null Issue 2 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; // <= } } 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 ConsensusContext.cs 89 V3080 [CWE-476] Possible null dereference. Consider inspecting 'previousBlock'. ConsensusContext.cs 90 V3080 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: null currentBlock previousBlock GetTrimmedBlock 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 (); return null; } 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 (); 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. null GetTrimmedBlock null Issue 3 Issue 3 private void OnTimer(Timer timer) { .... if ( timer.Height != context.Block.Index // context.Transactions? .Count) { .... } } private void OnTimer(Timer timer) { .... if ( timer.Height != context.Block.Index // 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 V3095 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. context.Block null context.Block 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; // <= } .... } [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 V3020 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. continue 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); } 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 V3062 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. Equals AssetScriptHash other 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); } 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 V3062 Nep17BalanceKey.cs 50 Potential issues in NBitcoin Now let's move on to potential issues in NBitcoin version 9.0.1. 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; } 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 V3127 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. a b 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); } 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 V3110 Message.cs 167 Calling this method overload causes StackOverflowException, as the only expression in its body is a recursive call to itself. StackOverflowException 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"); } 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 V3139 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. ToString switch case case Overloading Equals method without overloading GetHashCode public class MoneyBag : IMoney, IEnumerable , IEquatable { .... 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); } .... } public class MoneyBag : IMoney, IEnumerable , IEquatable { .... 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 V3126 Money.cs 78 The analyzer detected a class implementing the IEquatable 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. IEquatable GetHashCode Linq Equals 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. website documentation section Thanks for your attention and see you in future articles!