Self Audit Checklist
This self-audit checklist is meant to give you a framework to check for common issues, and address them. It is not an end-all-be-all, and is mainly useful for cleaning up your contracts before you get them professionally audited.
Note that some/most of the items mentioned on this page should be considered common practice in software development anyway, and not necessarily related to WAX smart contracts. However, they are still worth mentioning if you are planning on open sourcing and/or having your code audited, or even just writing secure contracts in general.
Avoid Excessive Commenting
It may be tempting to write comments all over your codebase to explain what's happening each step of the way. In reality, this makes it more difficult for people to read/review your code, and will likely just annoy whoever is doing the review. Take the following example.
There is no reason for this comment to exist, as all it does is explain what the code below is obviously doing.
Now look at the following example from one of the WaxFusion contracts.
If the comment wasn't there, you can still tell what the code is doing. However, without the context, someone attempting to review the contract would be thrown off as to why the hell this code exists in the first place. In this case, the comment is helpful.
TLDR: Don't avoid commenting altogether, but be mindful of whether or not a comment is necessary.
Use Descriptive Variable Names
This also ties into commenting. If you have a simple function like this one below...
It is very unclear what the check_time
function is actually doing. Is it supposed to return anything? Is it supposed to modify anything? What time is it checking exactly? Someone reviewing the code would have to go dig up the helper function to figure it out. A good principle to follow is, if you need a comment to explain what a function is for, then you need a better name for the function.
This could be refactored to be much more descriptive by changing the void
function to return a bool
instead, and using a better name.
Not only is the entire logic much more clear, but now you don't need an extra comment bloating up your code just to explain what a function is for.
Use Integers For Math, Not Decimals
This is especially true when dealing with financial calculations. double/float
types not only lose precision, but are not predictable and can therefore cause inconsistencies across different nodes. This behavior is not desirable in an application that requires predictable and exact results.
Take the following example, assuming both tokens have a precision of 8 decimals.
There are 1,000 WAX, and 2,000 WAXDAO in the pools. Which means the input amount of 500 WAX should receive 1,000 WAXDAO (this is just an example, the real formula would be more complex). It may be tempting to calculate the output like this...
However this is very bad practice and should be avoided. Instead, you can scale up/down using integers like this.
By scaling up first and then scaling back down, you avoid losing precision in intermediate steps, ensuring an accurate output.
I'm aware that this particular example would achieve the same result by just dividing 2,000 WAXDAO by 1,000 WAX and then multiplying by 10^8
. This would not be the case if you were dealing with smaller units, as you would lose precision after dividing. Rule of thumb is to always multiply first and then divide after.
Avoid Implicit Casting, And Ensure Cast Safety
If you don't know what implicit casting is, take a look at the following example.
Here you have a function that takes in 2 variables, which are both int64_t
types. When calling the function, we are passing in a uint64_t
and uint128_t
, which is not safe. The compiler will implicitly cast these variables to int64_t
, since we did not specify a type. We want to be explicitly casting when we pass these arguments, like this...
This is a good step in the right direction, but still not safe. The range of uint64_t
and uint128_t
can fall outside of the max possible values for int64_t
, resulting in undesired behavior like over/underflow. This is where developing your own safe casting logic can come into play, such as the following library which is a combination of my own code, and some code that was shared with me by others.
Notice that the specializations for the safe_cast
template perform checks to make sure that the numbers are safe before casting to int64_t
. You can utilize these templates like this.
This allows you to cast a larger type to a smaller type without risking overflow or underflow.
Check For Overflow/Underflow When Performing Math Operations
This also ties into the above snippet on ensuring type safety. But is more centered around arithmetic operations rather than simply casting. You can read more about best practices for these operations (and much more about proper coding standards) at the following URL.
The safecast.hpp
file shared above also has templates for addition, multiplication, subtraction and division that were implemented using these secure coding standards.
If you're unaware of how overflow/underflow work (or what these terms even mean), you have to understand the underlying binary system behind computer code. Ever heard the saying "everything is just 0s and 1s", but don't really know what that means?
Take a look at the following type.
This is an 8 bit integer, which means that the number itself is represented by a combination of 8 bits. The value of each bit can either be 0 or 1. For example...
8 bit unsigned integers can represent values between 0 and 255. So what happens when you try to do something like the following?
This should give you a value of 300, right? Wrong. It will not give you 300, and it will not throw an error either. What will happen here is, you will add 200 + 55 and get 255. Then, the 0s and 1s will keep rolling over, and your addition operation will start at 0 again and make its way to 44. This might not seem like a huge problem, now let's take the following example to demonstrate why you will lose all of your money and everyone else's, if you don't implement proper overflow checks.
You would think that the total_amount_requested
being 600 would prevent you from making this transfer, but it would not.
You would reach 255 twice after reaching the max uint8_t
value, and then have a final result of 90 (or 88?), which would pass your check. Not only that, but the set_user_balance
function would also result in the user having a positive balance at the end, because that operation will underflow.
At this point, you're hoping that your contract might have an insufficient balance, and hoping that you will be saved by an external contract that was built by someone who actually implemented proper over/underflow checks.
Do not make this mistake, you will lose all of your money and other people's money as well. The safecast.hpp
file I shared above has templates for basic math operations. You can extend that by adding more operations using secure coding standards, if needed.
Enable Compiler Warnings When Building Your Contracts
When compiling your contracts, you might typically use a command like this one.
While this gets the job done, it withholds some useful information. I mentioned earlier about things like implicit casting, which the compiler can notify you about if you've done that anywhere in your code. You can enable these warnings by adding some extra flags to your command, like this.
Create Proper Documentation
You have to keep in mind that not everyone is looking at your code from your perspective - they are not the ones who wrote the contract. Furthermore, even you will get confused once your contract grows in size and you don't remember what certain variables/functions were supposed to do.
In addition, even if you understand what the variables/functions are designed to do, you/others may lose track of how the overall system is supposed to work. What is the first thing a user has to do when they interact with the contract? What possible steps might come next? What types of users are there, does everyone have the same role or are some users treated differently than others?
These questions should all be answered in your documentation, which should be completed before submitting your code for review. Explaining the overarching scope of the contract will be appreciated by your code reviewer.
It will also be easier for them to catch potential vulnerabilities that they might not understand properly without the right documentation.
Graphs, charts etc are also very useful here. And every function/table/action etc should be described in detail somewhere, including:
What the function does
What inputs the function takes
What should be returned/modified by the function
Write Unit Tests For Every Function/Action
Yeah, I know... this is the part we all hate. But it's very important and should never be overlooked. Every time you create a new action/function on your contract, you should think of all possible scenarios for how that function might take place.
For example
This isn't the best example since you can just handle this by using the safecast::add
function, but regardless... What happens if you pass a double
to this function? What happens if both numbers are the max int value? You should be building unit tests to check for every single one of these scenarios, it will really help you dial in any potential issues and fix them before you deploy your contract.
I won't go too into detail about unit tests here, as I have a separate page dedicated to them.
Make Small Commits
This is one that I've been very guilty of myself. Once you have your initial code deployed, it's generally preferred if you separate your git commits so they change 1 or 2 things at a time.
Once people start reviewing your code, they can keep track of changes moving forward by looking at the commit history. If you rewrite 50% of the contract in 1 single pull request, it becomes virtually impossible for anyone to keep track, and basically requires an entire new review from scratch.
Your future self, other developers, and code reviewers will all thank you if you try to be a bit more mindful of how much work you're putting into a single pull request.
Do Not Hardcode Variables For Different Environments
This probably goes without saying, but ideally you want your compiled code hash to be the same regardless of what environment you deploy it in, and regardless of the current state of the chain/contract etc. I've been guilty of this myself. For example...
In order to deploy this on mainnet, you'd have to change the bool
to false, which would result in your testnet code having a different wasm hash than your mainnet code. This is not ideal, especially if you are having an audit done.
Your audit will be auditing 1 specific version of your contract code, which can be proven with a code hash. Having 2 different code hashes will complicate this process. Instead, you should just store this variable in a table and have an action for setting it.
The code above allows you to maintain the same code hash on any network, while also being able to adapt to different environments. If you're unsure how to interact with a singleton compared to a regular table, I suggest taking a look at the header file and the actions on the WaxFusion contracts.
Prevent Inline Action Exploits
This is a very tricky one to understand or explain properly. And even more difficult to design your logic around. Essentially, the order of actions within eosio/antelope transactions is somewhat complex, and if you don't plan your logic properly, your contract can be vulnerable to inline action injections.
A general rule of thumb is to never depend solely on reading data from other contracts during a transaction. For example, take a look at the following pseudo-code.
The code above will not work as expected, due to the order in which inline actions are executed. When you receive the notification about this eosio.token transfer, it's possible that the original action has not completed yet. As I said, this is complex to explain. But I'd recommend taking a look at cc32d9's handbook here:
And I'd also recommend reading about the vaults.sx exploit, which is linked in that handbook:
It's very hard to design something that requires reading tables from other contracts, but also is not subject to these vulnerabilities. If you don't fully understand the order of inline actions, my recommendation is this - worry less about the order, and more about making sure your contract is not reliant on things executing in some very specific order.
Your best bet is to keep track of state in your own contract as much as possible, and try to find ways to make transactions fail if a certain condition is not met - for example, sending tokens to a swap contract and specifying the minimum amount you expect to receive is a better solution than simply reading the table on that contract and hoping that the outcome will be what you expect.
There is not really a one-size-fits-all template for this, so if you have a specific case that you're unsure about - I recommend contacting me on Telegram @MikeD_Crypto and asking me to take a look over your code. This doesn't mean free audits of course, but I am happy to check out a certain function for you and let you know if I think it needs to be refactored.
DRY (Don't Repeat Yourself)
This is obviously a basic principle of coding and not necessarily related to WAX contracts. However, it's still worth mentioning.
If you have the same block of code repeated over and over in your contract, consider creating a helper function that performs all that logic, to make your code more readable and less redundant.
I won't go into detail here since this is such a basic concept, but please do read some articles and/or watch some videos on the topic of DRY code, if you are not familiar with it.
Readability Over Optimization
Yet again, another basic coding principle that any semi-intermediate developer should understand by now. But not all of us work with other people, so some experienced developers might still be unaware of this concept.
Sometimes you may be tempted to write some real "clever" code that uses bitwise operators or some other advanced, hard to read logic because you want to make it "more performant" or write it in a single line rather than 3 or 4 lines.
IMO, only optimize when you need to. Write 4 lines of code if it makes the code more readable and easy for someone to understand, rather than some complex single-line formula with modulo, bitwise and ternary operators combined into 1 cluster of unreadable logic.
Avoid Using Reserved Keywords
This is pretty self explanatory, but be careful of how you name your variables/functions. You don't want to be creating a variable called system
for example, to avoid any conflicts with existing variables/namespaces used in the system header files.
Last updated