External call in modifier (2)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L214 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L237

Vulnerability details

Impact

Modifier side-effects: Modifiers should only implement checks and not make state changes and external calls which violates the checks-effects-interactions pattern. An external call in modifier can lead to the reentrancy attack:

See Solidity Best Practices / Use modifiers only for checks https://consensys.net/blog/developers/solidity-best-practices-for-smart-contract-security/

Proof of Concept

LPFarming.sol use a modifer noContract with an external call https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L87

The modifier noContract is used in 4 functions: l.214 function deposit() l.237 function withdraw() l.332 function claim() (nonReentrant) l.347 function claimAll() (nonReentrant)

I think there is a risk for reentrancy attacks for the functions deposit and withdraw

Tools Used

VSC

Recommended Mitigation Steps

I would suggest to use modifier nonReentrant also for these functions to avoid a potential reentrancy attack.

[spaghettieth]

Openzeppelin's isContract doesn't even execute an external call.

[dmvt]

Invalid.