Platform: Code4rena
Start Date: 07/04/2022
Pot Size: $100,000 USDC
Total HM: 20
Participants: 62
Period: 7 days
Judge: LSDan
Total Solo HM: 11
Id: 107
League: ETH
Rank: 48/62
Findings: 1
Award: $151.35
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Dravee
Also found by: 0x1f8b, 0xDjango, 0xkatana, AuditsAreUS, Cityscape, Foundation, Funen, Hawkeye, IllIllI, JC, JMukesh, Jujic, Kthere, PPrieditis, Picodes, Ruhum, TerrierLover, TrungOre, WatchPug, berndartmueller, catchup, cccz, cmichel, delfin454000, dy, ellahi, hickuphh3, horsefacts, hubble, hyh, ilan, jayjonah8, kebabsec, kenta, minhquanym, pauliax, rayn, reassor, rfa, robee, samruna
151.3491 USDC - $151.35
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L675 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L560
In NFTVault.sol the borrow() function does not follow the Checks Effects Interactions pattern. There are important state updates that occur after an external call which happens in _openPosition(). Assuming that the nonReentrant modifier makes this ok is false due to the threat of cross function reentrancy. require checks should be done followed by state updates and then any external calls in accord with the Checks Effects Interactions pattern
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L675
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L560
https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html
Manual code review
The borrow() function should fully implement the Checks Effects Interactions pattern performing all external calls last and not making important state updates after external calls.
#0 - spaghettieth
2022-04-11T16:31:15Z
The example provided doesn't have any exploit vector and the code in question isn't vulnerable to cross function reentrancy. I'm not going to dispute this issue as the Checks Effects Interactions pattern allows for cleaner and safer code, but this issue should be severity 0 or at most 2.
#1 - spaghettieth
2022-04-14T14:25:20Z
Fixed in https://github.com/jpegd/core/pull/5
#2 - dmvt
2022-04-26T13:36:50Z
I'm going to leave this as part of the warden's QA report, but if the sponsor had not confirmed the suggested change I would have marked it invalid. The code does not have a reentrancy vulnerability and the warden has not actually described an exploit.
#3 - JeeberC4
2022-05-02T19:10:29Z
Generating QA Report as judge downgraded issue. Preserving original title: _borrow() has important state updates after external call