Self-audit, a new bug, and call for community auditing of BlockParty
Back in June, Josef Jelacic decided to use BlockParty at his UX & Adoption conference in Prague.
BlockParty has been my personal pet project over the last two years and I was always the one to deploy and administer the entire process. Josef used BlockParty not only to host his events (pre-conf dinner and spare slots for the unconf event) but he actually deployed the whole smart contract and its frontend all by himself.
Hats off to Josef for his bravery but this reminded me of the importance of security. Even though the deposit is small, it still sucks for losing money for bugs.
Doing a self-audit
So I decided to audit my own code for a sanity check. You can find the full report of my self-audit here.
To begin with, I followed the “How to prepare for a security audit” guide by Trail of Bits and added all sorts of tools (such as linting and code coverage) to make the environment as clean as possible.
It felt good to see apassing green sign and over 98% coverage, though it later came back to bite me :-(
The next thing I did was to describe the behavior of the smart contract as much as possible. I not only commented all my smart contract source code but also used tools like surya and solgraph to visualise my code.
$ surya describe contracts/Conference.sol
+ Conference (Destructible, GroupAdmin)
- [Pub] <fallback>
- [Ext] registerWithEncryption ($)
- [Ext] register ($)
- [Int] registerInternal
- [Ext] withdraw
- [Pub] totalBalance
- [Pub] isRegistered
- [Pub] isAttended
- [Pub] isPaid
- [Pub] payout
- [Ext] payback
- [Ext] cancel
- [Ext] clear
- [Ext] setLimitOfParticipants
- [Ext] changeName
- [Ext] attend
Another important thing was to write down past vulnerabilities. I wrote down the detail of when, by who and how they were discovered.
Next, I copied my contracts and pasted them into various automated smart contract security analysis sites.
I’ve tried the following three sites and here were the results.
Oyente
The latest solc supported version is 0.4.17 which was quite old hence I could not test at all.
Securify.ch
No error was reported. This may be because I used the site in the past and fixed the warnings at that time.
SmartDeck
No critical errors were reported, but there were lots of useful warnings. The full report is here. I fixed a few warnings but not all of them. For example, this line was marked for “Reentrancy”, but I did not think adding an Ethereum address into an array could cause any re-entrancy issues.
Discovering a new bug.
A new bug was reported by a user of Mythril, a security analysis tool for Ethereum smart contracts which I’d only briefly dabbled with.
When I ran Mythril (0.18.6 at that time), there were lots of duplicate errors against unrecognised functions (eg: _function_0x05f203d9) which made it hard to point to the exact location of the vulnerabilities (the full output is here), so I went to the mythril gitter channel and asked for feedback on my log.
Mythril reported a transaction order dependence issue, but it was decided that the issue does not pose a risk considering the business logic (and this was confirmed by Mythril engineers).
==== Transaction order dependence ====
Type: Warning
Contract: Unknown
Function name: withdraw()
PC address: 5265
A possible transaction order independence vulnerability exists in function withdraw(). The value or direction of the call statement is determined from a tainted storage location
--------------------
In file: contracts/Conference.sol:142participant.addr.transfer(payoutAmount)--------------------
10 days after I posted my self-audit report to the Mythril gitter channel, I was contacted by a user called nemozqqz regarding a typo in one of the functions.
“Array index bug in revoke function of GroupAdmin #175”
Yes, it was clearly a typo! The bug was introduced back in March when I was enhancing the grant feature to allow deleting multiple admins.
Looking back, it was a bad commit because I did not write an additional test to cover the feature.
Interestingly, the test coverage tool was showing amber at a line below the vulnerable code (line 36), but not at the typo line itself (line 35).
Again, this is completely a logical error so none of the tools I tried were able to detect it. One defensive way to code is to avoid using meaningless variables like i and j (as suggested in the comment by vietlq) but I wonder if there are better idioms to make it more static analyzer (or coverage tool) friendly.
The bug was fixed in this PR, but ENS Hackathon is currently using BlockParty with this vulnerability as it was discovered after the contract was deployed. This bug is not critical enough to cancel the entire operation but I need to remember the fact that I will have problem revoking the admin role once I grant it to someone.
Call for community auditing of BlockParty
As you just saw, some of the critical bugs tend to be very simple and you don’t need deep knowledge of Solidity nor the internals of the EVM to figure them out. If more people look into the code base, there is a better chance the bugs will get spotted beyond going through these static analysis tools.
There are lots of Ethereum related community events (EthBerlin, Web3 summit, DevCon4) and I am keen to host more pre/post/sub events using BlockParty.
If you have a basic understanding of Solidity programming, I would appreciate if you could spend thirty minutes going through the code and let me know if you find anything suspicious.
The best way to report any bugs will be to contact me privately via gitter channel (I am @makoto ) or twitter (@makoto_inoue)to assess the impact , then raise a github issue publicly (this is exactly how nemozqqz escalated. Thank you so much!).
Special thanks to Bernhard Mueller , Ramesh Nair and Jeff Lau for reviewing this post
