External call in modifier (1)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L628 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L799 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L879 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L919

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

NFTVault.sol use the modifier validNFTIndex with an external call https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L120

modifer validNFTIndex is used in multiple functions: l.321 function setNFTType l.347 function setPendingNFTValueETH l.360 function finalizePendingNFTValueETH l.628 function showPosition l.675 function borrow (nonReentrant) l.756 function repay (nonReentrant) l.799 function closePosition l.830 function liquidate (nonReentrant) l.879 function repurchase l.919 function claimExpiredInsuranceNFT

3 functions are protected by modifier nonReentrant (from OZ/.../ReentrancyGuardUpgradeable.sol)

However there are functions can modify state but not protected by modifier nonReentrant: l.628 function showPosition l.799 function closePosition l.879 function repurchase l.919 function claimExpiredInsuranceNFT

I think there is a risk for reentrancy attacks for these functions.

Tools Used

VSC

Recommended Mitigation Steps

I would suggest to use modifier nonReentrant in these 4 functions to avoid a potential reentrancy attack.

[spaghettieth]

This is not an issue as the modifier is executed before any of the function's code, which makes a reentrancy attack using the modifier impossible. The modifier also only calls the ERC721 view function ownerOf, which doesn't give control to end users.

[dmvt]

Though against best practices, in this case violating the practice has functional reasoning behind it and does not introduce any reentrancy risk. Invalid.