External call in modifier (3)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L100 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L117 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L134

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

The modifier noContract in yVaultLPFarming.sol use an external call https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L56

The modifer is used in 4 functions: l.100 function deposit() l.117 function withdraw() l.134 function claim()

None if these 3 functions use the modifier nonReentrant (from OZ/.../ReentrancyGuardUpgradeable.sol) to protect against this attack (function claim() in LPFarming.sol use it)

Tools Used

VSC

Recommended Mitigation Steps

I would suggest to use modifier nonReentrant (from OZ) for these functions to avoid a potential reentrancy attack.

[spaghettieth]

Duplicate of #203

[dmvt]

This is just incorrect on every level. Invalid.