Auditing is a craft: one has to face the code, stare hard at it, and dig into its entrails hoping to come out rewarded. Of course there’s vulnerability databases one must check, and common patterns to be wary of, but ultimately it boils down to a bare-handed fight. No wonder why automated tools, if ever improving, are extremely limited in finding code issues! We should cherish those occasions when we are able to capture in words what goes on in an auditor’s mind when they scan a piece of code and see something that “smells fishy”. Here is such an attempt, it will hopefully provide a glimpse into the auditing mindset.
In our audits, we see many kinds of variables used in many different ways. We also sometimes see single variables used in different ways. Think a variable tracking some kind of time that’s also used for access control, or a variable encoding an amount that is also used to indicate some kind of validity. When we see this, it smells fishy.
Let’s use an example to make this clearer (please take the following snippets as pseudocode, as I’m leaving out things to keep it simple). Say a contract tracks users, and some delays associated to them for some purpose we don’t care about, like so:
address[] users;
mapping(address => uint256) userDelays;
Users can be registered, assigning them a delay:
function registerUser(address _user, uint256 _delay) {
users.push(_user);
userDelays[_user] = _delay;
}
The delay can later be modified, in particular decreased with a function such as:
function decreaseDelay(address _user, uint256 _decrease) {
userDelays[_user].sub(_decrease);
}
(Note that it’s using SafeMath to make sure the delays don’t go below zero! 
)
We now want users to perform an action in the system, but we want to make sure that only registered users can do so. We promptly realize we are storing our users in an array, which would be cumbersome to traverse each time we perform the action… So why not use the delay! We have a mapping to track delays, so if we only modified the registerUser function to allow only positive values, we can encode registered users as those that have a non-zero associated delay! The modified function now looks like this:
function registerUser(address _user, uint256 _delay) {
require(_delay > 0);
users.push(_user);
userDelays[_user] = _delay;
}
Voilà! We can now control user access by checking their delays:
function performAction() {
require(userDelays[msg.sender] != 0);
...
}
So, what’s the catch? Well, the problem here is that the performAction function thinks of userDelays as a variable encoding a permission. The decreaseDelay function, however, still thinks as userDelays as delays, and is happy to allow them to be zero, since there was nothing wrong with having zero delays in the first place. But now, if we decrease a user’s delay to zero using this function, they won’t be able to call performAction, as if they hadn’t been registered.
You might think that the problem here is the decreaseDelay function allowing a zero delay, but I insist: there was nothing wrong with zero delays! It was only when we burdened the _userDelays variable with the extra permission encoding that a zero delay became problematic. I dubbed this Semantic Overloading: if you come across variables used with more than one meaning, watch out! There might be functions that think of these variables in terms of only one of their meanings, messing things up.
Just to be sure, this pattern is not necessarily bad practice. It can be an efficient way to encode things, and Solidity’s zero-valued defaults make its use even more tempting! Zero is off, anything other than zero is on… But it does make an auditor’s nose tingle!