Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[N12] Potential reentrancies #406

Open
cylon56 opened this issue Jun 15, 2022 · 1 comment
Open

[N12] Potential reentrancies #406

cylon56 opened this issue Jun 15, 2022 · 1 comment
Labels
openzeppelin PRs and issues related to OZ audit

Comments

@cylon56
Copy link

cylon56 commented Jun 15, 2022

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:

  • The invoke function of the Bulker contract can be re-entered.
  • The doTransferIn function of the Comet contract is often executed at the very beginning of executions, being it an anti-pattern to follow against reentrancy.

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

@kevincheng96 kevincheng96 added the openzeppelin PRs and issues related to OZ audit label Jun 15, 2022
@scott-silver
Copy link
Contributor

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openzeppelin PRs and issues related to OZ audit
Projects
None yet
Development

No branches or pull requests

3 participants