How Decentralised is bZx?
Some alarming conclusions about a protocol that has over $15m USD locked up.
|Kerman Kohli||Feb 17, 2020|
Hello everyone! This post is slightly different since it covers technical analysis rather than the usual opinionated analysis.
I had initially posted this tweet after the bZx arb event to gauge how much interest would there be from the community to understand at a more granular level the technical architecture of DeFi protocols:
While it didn't hit my goal of 300, it was sufficient evidence for me to write this post and test waters. I want to make it clear that I'll be reviewing many other DeFi protocols in the future but just choosing bZx as it's the most relevant at this point in time.
So here's their Github page where their open source code lives: https://github.com/bZxNetwork/bZx-monorepo
As per bZX, they've had the ZKLabs audit their contracts: https://github.com/mattdf/audits/blob/master/bZx/bzx-audit.pdf
No major vulnerabilities were found and all code appears to be safe on the surface.
Upon initial impressions, it seems that the system uses a proxy implementation architecture. For those of you that aren't super technical, a proxy basically is where things are stored, but an implementation actually decides what's stored. In bZx's case they've got their BZXProxy.sol that holds all state (balances, loan details etc) and BZX.sol that determines the logic for what happens if you get a loan, trade etc. Think of the proxy as the house that keeps the valuables, and the humans inside the house deciding what happens.
As you can see from the diagram above, the owner of ProxySettings.sol is a very valuable key as it can:
Roll a completely different bZx implementation
Change the logic of any function
Set which oracle to use
Pause the contracts themselves'
TLDR: they own the protocol fully.
So who is the owner of this exactly? I had to search quite hard to find their contract addresses (bit of a red flag as this should be super easy to find for anyone) but eventually found them here at: https://github.com/bZxNetwork/bZx-monorepo/tree/master/packages/bzx.js/src/contracts/mainnet
Etherscan link of BZXProxy here: https://etherscan.io/address/0x1Cf226E9413AddaF22412A2E182F9C0dE44AF002#code
The current owner of the contracts is: https://etherscan.io/address/0xbb536eb24fb89b544d4bd9e9f1f34d9fd902bb96
So who is 0xbb5..b96 you ask? Well it's another contract called "Timelock" which ensures there's a delay before enforcing any changes at the protocol level. But let’s dig even deeper, who are the admins that can act on behalf of Timelock?
To my surprise, it's literally only one key that can make changes! https://etherscan.io/address/0xadff3ada12ed0f8a87e31e5a04dfd2ee054e1118
Derived directly from: https://etherscan.io/address/0xbb536eb24fb89b544d4bd9e9f1f34d9fd902bb96#readContract
Best practises when dealing with high value keys is to use a hardware wallet which never exposes the private key in plain-text to begin with. Furthermore, any admin owners should have multiple keys to authorise changes. The concerning thing here is that 0xadf..118 has made over 5,786 transactions! Most of those transactions have been to execute upgrades.
Why is that concerning? Because it's made a lot of transactions, but also regularly - all within very short time periods. This is a speculative assumption but I'm pretty convinced this private key is not held via a hardware wallet but rather executed from a computer signing transactions automatically.
We could give benefit of the doubt to the bZx team here that they're using a cold storage solution, however it raises a key question that: how does anyone in the community audit their changes if they're proposing at least 40-50 transactions within a very short period of time? Each change transaction provides the following details:
Here's a random transaction I picked out from that list. Unfortunately we can't really figure out what this change does since the
data field is pure gibberish and no easy way to decode this. I'm not blaming bZx here since it's a problem with forwarding calls in Solidity. To better demonstrate the problem I've got another handy little diagram for you all:
Over here we have our caller, who decides to call a target with a value and data. So in this case we have 0xadf...118 (the admin of TimeLock which is the admin of Proxy) calling TimeLock with 0 ETH but with some data (0xgibberish). The problem here is that data (0xgibberish) is in turn calling another contract who is then becoming the caller for the target of more data! Essentially you have a nesting problem where the actual execution is hidden by the layer of nesting.
Explained in pictures again below:
This becomes problematic as the first call's data is actually a chain of many calls that are extremely time consuming to debug and hard to actually follow even if someone knew that a change was occurring! Lack of upgrade transparency makes it hard to know what's a real update or a malicious update. The prime culprit of such an attack in my opinion is poor architecture and upgradeability planning.
To put all of the above in very simple english: bZx's admin is actually just a regular private key with a timelock that makes transactions on a regular basis in batches of 20-30 where each transaction is difficult to audit the changes actually being made to the underlying protocol.
While digging through bZx's code, I wanted to uncover how well they've written test cases for their contracts and the kind of edge cases they' accounted for. Scarily enough, their testing is very out of date and there is very strong evidence to suggest the team does not use test driven development to write their smart contracts! All their tests are over a year old (on master and development branches).
Test driven development is essentially the art of writing tests before you write your code. This ensures that you can get guarantees about certain components of your system as you build it, but also that you've checked that the code behaves exactly as you expect it to. For writing smart contracts, strong test cases are an absolute minimum and should not be avoided. It also highlights an important point that all of bZx's code is YOLO code that's manually tested. Audits are the only backbone that give me slight confidence I should use their platform.
Let's dig in a bit further into
The order taking contract is a fairly logic heavy one that should have extensive tests written considering the original source is over 700 lines long (https://github.com/bZxNetwork/bZx-monorepo/blob/development/packages/contracts/contracts/shared/OrderTakingFunctions.sol)
Once again I was let down to find that the test cases are over a year old and only include 6 test cases! What are those test cases? They're all positive test cases such as:
it("there should be the full amount to cancel"
it("should allow to cancel 'trader' on-chain order loan"
and 4 other positive test case scenarios. This is not what unit tests should be looking out for, they need to test the negative where each function is tested with multiple inputs to guard against negative scenarios.
So if I was to summarise bZx, the way they develop software is essentially by writing a bunch of Solidity code, writing 5-10 test cases for the entire contract (the usual norm is to have at least 5 test cases per method) and then just ship. My guess was pretty on-point as their last commit to master (https://github.com/bZxNetwork/bZx-monorepo/commit/f3416c25dab93778657e00034f134a4b2da988d2) shows exactly that. In fact, in this commit, they've edited quite a lot of Solidity code and there's no test cases that have been modified either.
I admire a team that's worked really hard to ship something of value to the ecosystem, but in it's current form bZx makes me very nervous about how they're securing their contracts from an opsec perspective, transparency of upgrades and robustness of test cases.
My initial goal was to dig deeper into the protocol's code itself but I didn't think that the opsec aspect of their code would raise such a big red flag. I'm also quite frankly, surprised that no one in the ecosystem has discovered this earlier. Admin keys are important to upgrade, but the use of admin privileges should be done very carefully.
Their lack of proper testing is extremely scary as what they're writing is essentially YOLO code that has guarantees that it works, but not guarantees that it can stand not-expected inputs. Furthermore, they've only had an initial audit for their code but nothing for the subsequent upgrades that don't have unit tests to rely on either!
While DeFi is still young and growing, it's important that we as a community self regulate to ensure that users who aren't as technically savvy are made of the risks and teams can receive help from the community to fix potential issues.
Have any questions? Reach out to me on Twitter @kermankohli
Personal side note to the bZx team: I’m more than happy to help and offer my input on how you can improve your code to address some of the problems listed above. Please don’t hesitate to reach out :)