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.

//check that the quantity received is positive
check( quantity.amount > 0, "quantity must be positive" );

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.

/**
 *  for unit testing
 *  since the eosio contract in our local environment doesnt automatically adjust our wax 
 *  balance when we stake CPU, I just added a require_recipient in the delegatebw action.
 *  then we get a notification requesting the payment within the same transaction
 *  and inline transfer wax to eosio, mimicing the behaviour of the real system contract
 */
void polcontract::receive_system_request(const name& payer, const asset& wax_amount){
    if(!DEBUG) return;

    if(payer == _self){
        transfer_tokens( "eosio"_n, wax_amount, WAX_CONTRACT, "stake" );
    }
}

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...

//determine whether or not the deadline has passed
check_time( current_time );

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.

check( is_deadline_passed( current_time ), "the deadline has not passed yet" );

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.

//calculate output of WAXDAO
asset input_amount = asset( 50000000000, WAX_SYMBOL ); //500 WAX
asset wax_pool = asset( 100000000000, WAX_SYMBOL ); //1000 WAX
asset waxdao_pool = asset( 200000000000, WAXDAO_SYMBOL ); //2000 WAXDAO

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...

double wax_double = double( wax_pool.amount );
double waxdao_double = double( waxdao_pool.amount );
double waxdao_price = waxdao_double / wax_double;
double output_amount = waxdao_price * double( input_amount.amount );

However this is very bad practice and should be avoided. Instead, you can scale up/down using integers like this.

uint128_t output_amount = uint128_t( input_amount.amount) * uint128_t( waxdao_pool.amount ) / uint128_t( wax_pool.amount );

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.

int64_t calculate_something( const int64_t& a, const int64_t& b ){
    return a * b + 100;
}

uint64_t x = 12;
uint128_t y = 14;

int64_t outcome = calculate_something( x, y );

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...

int64_t outcome = calculate_something ( int64_t(x), int64_t(y) );

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.

int64_t outcome = calculate_something ( safecast::safe_cast<int64_t>(x), safecast::safe_cast<int64_t>(y) );

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.

uint8_t //unsigned 8 bit integer

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...

00000001 //the number 1
00000000 //the number 0
01010101 //the number 85

8 bit unsigned integers can represent values between 0 and 255. So what happens when you try to do something like the following?

uint8_t a = 200;
uint8_t b = 100;
uint8_t sum = a + b;

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.

ACTION withdraw( name user, vector<uint8_t> amounts_to_withdraw ){
    uint8_t user_balance = 250;
    uint8_t total_amount_requested = 0;
    
    //let's assume amounts_to_withdraw contains 3 values:
    //100, 200, and 300
    
    for( uint8_t amt : amounts_to_withdraw ){
        total_amount_requested += amt;
    }
    
    check( user_balance >= amounts_to_withdraw, "you are overdrawn" );
    transfer_to_user( user, total_amount_requested );
    set_user_balance( user_balance - total_amount_requested );
}

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.

eosio-cpp mycontract.cpp -o mycontract.wasm --abigen

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.

eosio-cpp -I -Wall -Wextra -Wconversion -Wpedantic mycontract.cpp -o mycontract.wasm --abigen

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

void do_something( int a, int b ){
    return a + b;
}

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.

Unit Tests

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...

bool TESTNET = true;
int64_t MINIMUM_WAX_TO_STAKE = TESTNET ? 100 : 10000;

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.

struct [[eosio::table, eosio::contract(CONTRACT_NAME)]] config {
	int64_t 	minimum_wax_to_stake;

	EOSLIB_SERIALIZE( config, (minimum_wax_to_stake) )
};
using config_singleton = eosio::singleton<"config"_n, config>;

ACTION mycontract::setminimum( const int64_t& minimum_wax_to_stake ){
	require_auth( _self );
	
	config c = config_s.get();
	c.minimum_wax_to_stake = minimum_wax_to_stake;
	config_s.set(c, _self);	
}

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.

[[eosio::on_notify("eosio.token::transfer")]] void receive_token_transfer(name from, name to, eosio::asset quantity, std::string memo);

void receive_token_transfer(name from, name to, eosio::asset quantity, std::string memo){
    //check our balance on eosio.token contract
    
    //perform some logic and make a transfer
    
    //check our balance again on eosio.token contract and compare it against
    //our previous balance
}

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.

PreviousRenouncing Contract OwnershipNextUnit Tests

Last updated