ReEntrancy with multiple coordinating malicious cTokens

[coburncoburn]

The borrowFresh and redeemFresh functions transfer tokens out of the protocol before updating internal accounting. This is a reentrancy vulnerability for tokens that define a malicious transfer function. The cToken contract has a reentrancy guard for this purpose. However, if multiple malicious tokens coordinate, the reentrancy guards on individual tokens are not sufficient to prevent the comptroller from reading stale liquidity data when checking these transfers.

Compound governance has been diligent in not listing malicious tokens, but the contracts should be fixed for the future.

The fix is to simply move the doTransferOut call to after internal accounting in the affected functions.

https://github.com/compound-finance/compound-protocol/blob/master/contracts/CToken.sol#L786 https://github.com/compound-finance/compound-protocol/blob/master/contracts/CToken.sol#L694

This flaw was discovered by DeFiPie, an independent deployment of the Compound Protocol.

[llama]

Interesting vulnerability. borrowFresh is only called by borrowInternal, which has the nonReentrant modifier. So how was this possible? Was DeFiPie using a version of CToken.sol without that modifier?

[coburncoburn]

Interesting vulnerability. borrowFresh is only called by borrowInternal, which has the nonReentrant modifier. So how was this possible? Was DeFiPie using a version of CToken.sol without that modifier?

Please refer to DeFiPie post mortem write up for more details https://medium.com/defipie/hacking-investigation-85e07454f1c9

[llama]

@coburncoburn I read the post mortem thoroughly. And I can't easily see if nonReentrant was present or not in the contracts that DefiPie was using, because they were never uploaded to etherscan.

nonReentrant has been present in borrowInternal since the initial commit 2 years ago. However, it was not present in Compound v1. Perhaps DeFiPie had deployed Compound V1? The only problem with that theory is that the screenshot in the post mortem clearly shows code from Compound V2 (this repo).

[coburncoburn]

@llama There is one main subtlety I think you are missing: no individual cToken is ever reentered. cToken A calls into Token B which calls cTokenB which calls token C which calls cToken C which finally calls into a real token with actual value.

[llama]

Ahh of course, thanks @coburncoburn, that explains it. As you said, Compound has the protection of not allowing arbitrary coin listings. Additional assurance will be gained by moving doTransferOut after internal accounting updates (and ensuring doTransferIn happens only before internal accounting updates).

I have an additional proposal, which is to add nonReentrant to every somethingAllowed function in the Comptroller. I believe that would also have prevented this, and I can't find a case where we'd need to call multiple of those in one transaction.

[coburncoburn]

Ahh of course, thanks @coburncoburn, that explains it. As you said, Compound has the protection of not allowing arbitrary coin listings. Additional assurance will be gained by moving doTransferOut after internal accounting updates (and ensuring doTransferIn happens only before internal accounting updates).

I have an additional proposal, which is to add nonReentrant to every somethingAllowed function in the Comptroller. I believe that would also have prevented this, and I can't find a case where we'd need to call multiple of those in one transaction.

adding nonReentrant to the somethingAllowed sounds interesting, but this approach would not work without some additional thinking. somethingAllowed does in fact return before the reentrancy, so it is not actually being reentered.

[coburncoburn]

https://twitter.com/CreamdotFinance/status/1432249771750686721 this flaw has been exploited in a second Compound fork

[coburncoburn]

#153 has been started to address this issue

[wenzhenxiang]

now the code logic is after the transaction, and then update state the logic of borrow, repay, deposit, redeem needs to be changed to

1.Validation 2.UpdateState 3.Transfer 4.emit event