clober-dex / coupon-finance

Coupon Finance Solidity Contracts
Other
1 stars 0 forks source link

Refactor: Remove unnecessary `nonReentrant` #67

Closed detectivekim closed 1 year ago

detectivekim commented 1 year ago

Description

The current nonReentrant modifier is being used in BorrowController.sol, DepositController.sol, and OdosRepayAdapter.sol. However, the reentrant attack can occur when using payable(to).call{value: amount} or invoking functions with malicious ERC20 tokens.

Assuming the absence of malicious ERC20 tokens, the _burnAllSubstitute function, which contains the logic for payable(to).call{value: amount}, is safe from nonReentrant attacks as it always occurs as the final step.

I believe that unnecessary use of nonReentrant increases gas costs, so it is appropriate to remove it. Could vulnerabilities arise from removing the nonReentrant from the Controllers? @trust1995 @HickupHH3

detectivekim commented 1 year ago

Could you guys @trust1995 @HickupHH3 please check if it's safe to remove the nonReentrant?

HickupHH3 commented 1 year ago

currently too early for me to say, am still digesting the codebase. can better advise once i have a clearer picture.