3.39 System Pitfalls

So far we have discussed security pitfalls and best practices focused on the Solidity language, the underlying EVM, the different token standards and so on... Now we are going to level up and discuss similar pitfalls and best practices, but focusing at the application level. These are software engineering best practices that have been developed and refined over decades, that apply specifically to smart contact applications as well.

These application level aspects are arguably more important to discuss from a smart contract security auditing perspective because they can't be generalized across smart contact applications like we have done with Solidity or EVM level concepts. Because of that, there is a lack of tooling support for security pitfalls and best practices at this level, thus there is a greater dependency on manual analysis when it comes to security auditing. When that is insufficient (or incorrectly done) it has led to massive exploits that have resulted in losses of many millions of dollars.

System Specification

With that context and motivation, let's talk about system specification. The design of any system or application starts with what is known as requirements gathering where such requirements are determined based on the target application category, the target market and the target users. Once those requirements are determined, they are translated (or coded) into a very detailed specification.

This specification is required to describe in great detail how the different components of the system need to behave to achieve the design requirements and it's not just the "how" aspect, but also the "why" aspect: why is something being designed and specified the way it is being done. Without such a detailed specification, a system implementation will not have a baseline to be evaluated against the requirements that we have collected earlier.

This is something critical for determining if the system behaves correctly, if the functions actually meet certain requirements that were designed (that were collected earlier). So to summarize, the design of a system begins with requirements, these requirements are translated into a very detailed specification which in future once we have an implementation allows us to evaluate, if the implementation actually meets the requirements system documentation.

System Documentation

System documentation is another critical component from a software engineering best practice. This is something that is often confused with specification. Remember that specification deals with design and requirements of the system whereas documentation deals with the actual implementation. The documentation describes what the different system components do to achieve the specification goals and how they do that. This has to cover various aspects related to the assets managed by that system, the actors within the context of that system and the various actions that these actors perform. It should also address the security specific aspects of the trust model and the threat model that are relevant to the system.

So to summarize, in the design flow we start with the requirements that helps us create the specification which in turn helps us execute the implementation of that system. This implementation should be accompanied by extensive documentation that helps one evaluate it against the specification for correctness across various attributes.

So with that high level view of system design let's now start discussing security aspects related to various application logic related constructs and concepts.

Comments

Code comments can be considered as part of documentation that is in line with the code. We should ensure that the code is well commented with the correct level of details and relevant information both with NatSpec and inline comments. This will help improve readability, maintainability and also auditability because comments can help document not only the functionality, but also the rationale behind it and any assumptions made, all of which can be analyzed while manually reviewing the code.

• The comments should accurately reflect what the corresponding code does.

• Discrepancies between code and comments should be addressed any to do's indicated by comments should also be addressed.

• Commented code and stale comments should also be removed

These are all the various aspects related to comments that need to be kept in mind while developing code or manually reviewing it.

Function Parameters

The first one is function parameters. From a security perspective one should ensure that proper input validation has been performed for all function parameters. This is especially true if the visibility of such functions is public or external, because in these cases users who may potentially be untrusted can control the values that are assigned to these parameters and such tainted values can affect the control and data flow of the function and any logic thereafter.

The best practice here is to make sure that there are valid sanity and threshold checks performed on these parameters, depending on what types they are. For example, if they are of the address type, then a zero address validation should be performed because otherwise it could lead to exceptions during runtime or it could lead to tokens being burnt or access control being denied as we have discussed so far.

The risk that we are trying to address here is from incorrect or invalid values being assigned to function parameters either accidentally or maliciously by users interacting with these functions.

Function Arguments

The arguments that are passed to functions, the call sites, that correspond to the function parameters are also something that need to be evaluated from a security perspective. At a high level, the arguments that are used at the call sites (the callers' arguments) should match the parameters that are required by the functions (or the callees).

This matching should happen both in terms of their validity as well as their order, or in other words: the arguments at the call sites should be valid in that smart contract applications context to what the function parameters expect. The order of such arguments should match the order of the function parameters as expected. These are the best practices that need to be followed when it comes to function arguments and the corresponding function parameters.

Function Visibility

We have discussed function visibility several times. This is something that is specific to the Solidity language that has four visibility specifiers. The order from maximum visibility to minimum visibility starts with public, external, internal then finally private.

From a security perspective, to follow the principle of least privilege is critical to make sure that the strictest visibility is applied on the various functions. The reason is that, if a function is accidentally made external or public (when it should actually be internal or private) because of some critical functionality that should not be exposed to external, then this mistake can be exploited by users some of whom may be untrusted to invoke functionality that they are not supposed to have access to. This again is very relevant here because of the byzantine threat model.

Function Modifiers

Function modifiers are another interesting aspect of smart contracts written in Solidity. They are critical from a security perspective because modifiers are used to implement access control within the smart contracts and they're also used for different types of data validation in accounting and other application specific contents.

Things to be kept in mind when analyzing modifiers is: to determine if any specific modifier is missing for the functions being analyzed, to check if they have been applied incorrectly on functions that either don't require these modifiers or that require these modifiers also.

If there are multiple modifiers used on a function, we have discussed how the ordering of the modifiers affects the logic implemented. Modifiers affect both control and data flow because from a control flow perspective, they could implement authorization checks that could revert if those checks fail, and therefore affect the control flow. They could also do different types of validation of the data that is being passed to the modifiers, in which case they do affect the data flow as well. The best practice with function modifiers is to ensure that correct modifiers have been used on the correct functions and in the correct order.

Function Returns

Smart contracts typically have multiple functions defined within them, and calls to such functions execute the logic within those functions, then return control back to the call sites. In many of these cases, the functions also return a value along with the control flow. Such return values should be analyzed to make sure that the correct values are being returned. This is being done along all the paths within that function.

Another aspect to be checked is to ensure that for functions returning values, their call sites do indeed use those return values appropriately and do not ignore them. This is critical not only for the data flow aspect of the application logic context, but also from a security perspective. This is critical because this is the way that error conditions being returned by those function calls are caught and handled appropriately. Ignoring these could result in undefined behavior in the best cases and in the worst cases could result in serious vulnerabilities.

Function Timeliness

By timeliness we mean: when can these functions be called? Externally accessible functions (those with the external or public visibility) may be called at any time by users interacting with those smart contracts. On the flip side, they may never be called.

The reason for this again is it could be accidental or it could be malicious, so it's not safe to assume that functions will be called in a very timely manner at specific system phases that make sense from the application logic context. Therefore, the implementation of functions within a contract should be very robust to track system state transitions, determine what state the system is currently in and in this state, which functions are expected (or make sense) to be called.

For example, in the context of Proxy-based upgradable contracts where initialization functions are required to be used instead of constructors, such functions are meant to be called atomically along with contract deployment during construction to prevent anyone else from initializing those contracts with arbitrary values. Such initialization functions are not meant (or allowed) to be called after deployment.

Function Repetitiveness

Function repetitiveness is an aspect that refers to the number of times a function may be called. Again with public or external functions in a contract, they may be called any number of times by users. So it is not safe to assume that they will be called at all, called only once or a specific number of times as it makes sense to the application logic context.

The function implementation and any state transitions happening within that function should not be making any assumptions on the number of times a particular function is called. They should be robust enough to track, prevent or ignore arbitrary repetitive invocations of functions or account for them in an idempotent way.

Again, taking the example of Proxy-based upgradable contracts, initialization functions are meant to be called only once, which is why one of the security best practices is to use the initializer modifier from that OpenZeppelin library that we discussed earlier.

Function order

Along with timeliness and repetitiveness, the ordering of functions also matter. This refers to which function is called and when. public/external functions can be triggered by users in any order, so state transitions happening within those functions should not be making any assumptions on the order in which these functions are being called just because it makes sense from that application's context.

The implementation should be robust enough to handle an arbitrary order of functions being called. This may again happen accidentally by users interacting with that application or it may be triggered maliciously. Again, taking the example of Proxy-based upgradable contracts and their requirement of initialization functions: such initialization functions are meant to be called before any other contract functions can be called, that ordering is critical because initialization functions initialize state variables. Allowing any other contact function, that requires those state variables to be initialized, to be called would not make sense and could lead to vulnerabilities. So function ordering is something that needs to be paid attention to from a security perspective.

Function Inputs

Function inputs determine what data functions work with in the context of those particular function calls. public and external functions again can be called with any arbitrary input, so it is not safe for functions to make assumptions on the validity of the arguments that are being supplied to it. Without complete and proper validation on these inputs (these could be zero address checks, bound checks, sanity or threshold checks depending on the type of those arguments) we can't assume that these function inputs will comply with any assumptions being made about them in the function code.

Conditionals

Conditionals are used to affect the control flow aspects of the function implementation. Functions are rarely straight line code: they have different control flow constructs such as if, else, for, while, do, break, continue and return, within the Solidity smart contracts that are used to implement complex control flow to reflect the different conditions that these functions need to work with.

Such conditionals have different predicates within them for the various checks that need to be enforced. Predicates involve the use of simple or complex expressions. These expressions involve operands or variables that are used along with operators. All these aspects of conditionals need to be checked to make sure that they enforce the control flow as anticipated by the developers. A common error is the use of the logical or (||) operator instead of the logical and (&&) operator within conditionals. These have caused serious security issues where they were being used to check for access control decisions. In such cases the authorization checks would pass, if only one of the expressions in the predicate were true instead of requiring all of them to be true.