Audits and ERC20 tokens
Why ERC20 is a checkbook
Recently I did a code review for an ERC20 token implementation called
Hodl DAO. It was mostly an exercise of good code hygiene and quality, but being an ERC20 token contract, it had some flaws that might be generalizable to other ERC20 tokens.
The code review itself was pretty straightforward — first step is always to ensure the code deployed matches the code presented for review. In this case, I had notified the author of a discrepancy between the code in Github and the code as verified by etherscan.io, so the code had been updated to the proper version by the time I started my official review. The code in both locations ultimately had a few discrepancies, but after changing some whitespace options (
diff -Bw) the code matched up pretty well, outside of some slight differences in comments.
Following verification that what I was reviewing was actually what was deployed (within reason), I broke my review down into a method-by-method analysis. As you might have guessed from my previous series on writing a smart contract, I’m a big fan of unit testing. My code reviews generally follow the same procedure: review units of code (generally functions), and then review how they fit together.
The majority of issues surfaced were minor. Things like always returning
true from a method, unnecessary
if/else statements around simple arithmetic (don’t need an
if/else to decide not to add
0, for example).
However, as I was going through, I noticed there’s an oddity around how ERC20 token contracts handle the
transferFrom paradigm. It’s worth noting that there’s already a known issue around how this is handled: a race condition attack under which someone can extract more tokens from someone who has approved them to spend a certain amount and then later changes their mind.
In a similar vein, there are generally no protections or checks around how much a user is approving another user to spend. In fact, many ERC20 implementations place no bounds on the
approve at all! You can approve someone to spend more tokens than you own, or approve multiple people to spend all of your tokens, each.
In this way, an ERC20 token is a lot like a checking account.
approve functions like a checkbook: you write a check to a person telling them they can take a certain amount of your money; it’s on them when to spend it, and it’s on you to make sure you don’t overdraw you account. There are no checks in place to make sure the total of your approvals never exceed your total balance.
In other words, it’s entirely possible to write a check you can’t pay for.
This leads to some interesting (if relatively minor) potential attack vectors.
Say Alice has 100 tokens. She approves Bob to spend 50 of them. Bob never gets around to spending them, so she later approves Charles to spend all 100 of them.
For those who can add, that means she’s written 150 tokens’ worth of checks, but only has 100 tokens in her account. Should both Bob and Charles attempt to call
transferFrom with the amounts they have been approved, one will fail to get the tokens they think they have access to.
The consequence of this is not only that Bob or Charles won’t get paid — it’s that they can get paid later, if they simply wait for Alice to obtain more tokens. Should Alice obtain another 50 tokens, either Bob or Charles can immediately call
transferFrom and take those tokens from Alice.
Even worse, say Alice has 100 tokens. She intends to pay Bob all 100 tokens, and approves no one else so she doesn’t run into issues with “overdrawing” her account.
However, she accidentally approves 1000 tokens. Bob, seeing she only has 100 tokens, transfers all 100 of them from her. Alice, thinking everything is complete (and lacking feedback that there are an additional 900 tokens outstanding on the approval), proceeds to buy more tokens so she can pay other people. Bob simply has to wait for her to “refill” her account before calling
transferFrom again to drain off any new tokens she buys.
ERC20 tokens should be an improvement on checking accounts, by all means. Unlike a checking account, you do have the means of seeing every “check” (approval) you’ve written, before they’ve been cashed and show up on your statement. However, most interfaces for viewing ERC20 tokens don’t even show who has been approved to spend your tokens… which actually makes them worse than the current status quo of banking.
A simple improvement on the ERC20 contract would be to simply keep track of the total approvals for each tokenholder, and further ensure that the sum never exceeds the user’s balance. This takes the burden of remembering every approval off the user and moves it to the contract. This could also be extended to prevent users from transferring tokens away if they have approvals for those tokens already in place.
The counterpoint to these improvements is that it does make the contract less flexible and more complex. There are a few use cases where approving more tokens than one holds is desirable, and where being able to transfer approved tokens would be helpful — e.g. a contract was approved to spend those tokens, but is broken and cannot.
As far as potential issues go, this one is fairly minor, provided people are diligent in their usage of
approve. It is, however, an opportunity for improvement in the experience of using
approve and tracking one’s approvals. Proper emission of events on
approve can be used to keep track of past approvals, though can be slow for token contracts with long histories and hard to correlate with the matching