compound-finance / comet

An efficient money market protocol for Ethereum and compatible chains (aka Compound III, Compound v3).
Other
241 stars 141 forks source link

[N12] Potential reentrancies #406

Open cylon56 opened 2 years ago

cylon56 commented 2 years ago

In the codebase, we found two places where reentrancy can occur. However, those do not pose any security issue or concern but awareness should be raised:

To improve clarity, consider either reducing the attack surface by making those functions non-reentrant or clearly stating this risk in the docstrings.

scott-silver commented 2 years ago

@cylon56 for doTransferIn, you'd want to reverse the normal logic of checks-effects-interactions, right?

for example, in supplyBase, we start by doing the transfer in of base tokens from the from address. this transfer is the opportunity for a malicious token to attempt to take some action/alter some state in a way that we do not intend.

after the transfer, we accrue, update balances, etc, doing all the required checks along the way and reverting if we discover that we're in some unexpected/undesirable state.

this ordering is intentional. if we did it the opposite way, then it would allow the transferIn to take a reentrant action after all of our other checks had run. this seems like it would have a greater potential of allowing a malicious token to take an unexpected action, no?