Compound’s reentrancy protection is broken.
On August 31st, C.R.E.A.M. had a roughly 20 million dollar reentrancy hack. The C.R.E.A.M. codebase was forked from Compound. The same flaw that enabled the C.R.E.A.M. hack is still present in the Compound codebase. While the Compound team does not judge there to be an immediate threat to Compound’s assets, this is something we should fix for the future.
Disclosure Timeline
On August 31st, I examined the C.R.E.A.M. hack right after it happened. I realized that the same code vulnerability that enabled the attack was present in Compound. I emailed security@compound.com that same day, but have never received a reply.
On Sept 1, since I had not heard back from the email, I Discord PM’d two people associated with the Compound project and shared the vulnerability with them.
I also messaged the C.R.E.A.M. team that day with a simpler fix than the one they had proposed in their post mortem. C.R.E.A.M. committed my simpler fix the day after they received the message, instead of their fix.
On Nov 6th, I’m posting this here on the forums so that we can begin the process of fixing this in Compound.
What is a reentrancy attack?
The very first big hack on an Ethereum smart contract was a reentrancy attack. Reentrancy attacks are THE classic Ethereum smart contract vulnerability. Consider a sample bank smart contract with this withdrawn method:
withdraw(address user, uint256 amount){
uint256 oldBalance = balances[user];
// Check
require(oldBalance >= amount);
// Calculate
uint256 newBalance = oldBalance - amount;
// Transfer funds
coin.transfer(user, amount);
// Store
balances[user] = newBalance;
}
If an attacker was able to run their own code during the coin transfer in this withdraw, then the attacker could withdraw more than they had deposited.
How? The attacker starts with $1,000 deposited, and requests a withdrawal of $1,000. The withdrawal function reads in the user’s balance, checks that the user has funds, and sets a temporary variable newBalance to $0. This “newBalance” variable is not a write to storage, rather it is only in memory during this execution of the withdraw function.
When the $1,000 is transferred to the attacker, the attacker gets to run code at this point in the middle of the withdraw function. Immediately, the attacker requests another withdrawal for $1,000. Because the storage keeping track of the user’s balance has not yet been updated, the withdrawal functions loads in the user’s balance as still $1,000 and permits another withdrawal to begin.
After the second transfer in the second withdraw is complete, then the newBalance in each running copy of the withdrawal function are stored. In each case, the newBalance being stored would be zero. The attacker has doubled their money, and could repeat the attack.
Compound Borrow Function
A core part of the Compound protocol is that a user can never borrow a greater amount than the user’s collateral permits. Before making a loan, the Compound codebase scans through each lending pool in the system to verify how much the user has borrowed from each and lent to each.
The Compound borrow function looks like this (focusing just on the user balance and the transfer):
function borrowFresh(address payable borrower, uint borrowAmount) internal returns (uint) {
// ... snip ...
vars.accountBorrows = borrowBalanceStoredInternal(borrower);
vars.accountBorrowsNew = add_(vars.accountBorrows, borrowAmount);
// ... snip ...
doTransferOut(borrower, borrowAmount);
/* We write the previously calculated values into storage */
accountBorrows[borrower].principal = vars.accountBorrowsNew;
// ... snip ...
}
Compound looks exactly like a textbook reentrancy vulnerability example, with the user borrows storage write after the external call.
Reentrancy Defenses
There are three common reentrancy defenses that smart contracts use.
1. External function calls last
If we jump back to our example, but move just one line of code, we can prevent a reentrancy attack:
withdraw(address user, uint256 amount){
// Check
uint256 oldBalance = balances[user];
require(oldBalance > amount);
// Calculate
uint256 newBalance = oldBalance - amount;
// Store
balances[user] = newBalance;
// Transfer funds
coin.transfer(user, amount); // MOVED DOWN TO END
}
If the call to an external function is after everything else in the function, then the attacker gains no advantage by calling the code from inside the transfer. The new balance has already been stored. This simple method of preventing attacks was learned painfully in the early days of Ethereum, and this defense is in the official Solidity documentation.
This defense is broken in Compound.
2. NonReentrant locks
Sometimes you need to make multiple external calls, or there are unavoidable calculations that must be done after external calls. In such a case, the usual solution is to use a single contract storage slot as a lock. Whenever any state changing function in the contract is called, the lock storage is checked to see if other code is executing. If it is locked, the function will revert. If it is unlocked, then the function will write a locked value to the storage and continue normal execution. After running the code, the last step in the function will unlock the lock. By doing so, an attacker cannot executed any code in the same contract at the same time.
NonReentrant locks are incredibly common and are essentially standard for smart contracts these days. It’s a simple tool that prevents a tremendous amount of mischief.
The Compound lending pool contracts have reentrancy locks on their contract functions. How did C.R.E.A.M. get hacked when this protection is here?
The Compound lending system is made up of multiple contracts with one “CToken” pool contract for each asset supported. So Compound has a lending contract for ETH, as it does for USDC, as it does for WBTC, etc. Each one of these individual lending contracts has its own separate lock for protecting that individual contract against reentrancy.
However, the system as a whole is not protected because while an attacker cannot be inside the same contract twice, they can be inside different lending pool contracts simultaneously.
In each round of the C.R.E.A.M. attack, the attacker put up one and a half million dollars of collateral, then borrowed AMP. During the borrow function for in the AMP pool contract, the AMP coin called the attackers code, allowing the attacker to start a second borrow of ETH. Because the AMP borrow had not written any record of the AMP borrow to storage yet, when the ETH borrow in the ETH pool contract checked with all pool contracts to see how much the attacker had already borrowed, it saw that the attacker had no debts and lots of collateral, and thus allowed a duplicate ETH borrow.
With the ability to get two borrows for a single set of collateral, it was game over.
This defense is broken in Compound.
3. Only using trusted external coins
Lastly, a common method is to have a whitelist of trusted coins or contracts that your own contract is allowed to interact with. As long as these external contract are not attacking you themselves, and not calling out to external contracts themselves (in methods you use) this can be safe.
But it’s very easy to get wrong. Either by forgetting to check somewhere inside your code that a coin is in your list before calling it, or by adding a trusted coin without checking it for reentrancy possibilities. It’s also possible that a coin that was safe before could potentially be upgraded later to unsafe behavior.
This is the only defense Compound currently has.
Should Compound fix this?
At the moment Compound depends on the correct behavior of all listed tokens, now and in the future. That scares me. 
The C.R.E.A.M. hack happened with a token that had no intentions of being evil. It was just used by the attacker in a way that was not anticipated. It’s not just the intentions of the listed tokens that counts. A coin that is good now could also be upgraded in the future to have exploitable behavior.
In looking at the discussion on these forums around the last batch of coins that were successfuly accepted for listing on Compound, I did not see discussion verifying that these coins would not allow reentrancy attacks. This indicates the possibility that Compound could have listed a vulnerable coin, and that this bug could have resulted in a loss.
I think the fix is small, and then we have multiple layers of defense against reentrancy attacks, now and into the future.
In my opinion, you keep systems secure by aggressively moving the fix the slightest sign of vulnerability. This issue is more than just a trace of a vulnerability - a near identical codebase has been exploited through it.
Proposed steps to fixing this
-
Someone should be designated to be in charge of the fix for this.
-
C.R.E.A.M.'s GitHub commit makes a good start for the fix. In addition, we should run the automated tool
siltheragainst the Compound codebase and look for any other state writes after external calls. -
We should also examine fixing the reentrancy locks so they work globally. If this results in too much complexity, then it could be skipped.
-
Once there is have a proposed fix for this, it should be audited before going live. If Compound has an upcoming super-audit, it would work to fold this into that audit. Otherwise, it could be a good trial smaller engagement for testing out working with a first tier auditor. The timing on getting this in is not critical, and it should 100% be audited before going live.
-
The new code should be used for new CTokens launches, and we should upgrade any CTokens that are for coins with upgradeable contracts.