Platform: Code4rena
Start Date: 28/11/2022
Pot Size: $192,500 USDC
Total HM: 33
Participants: 106
Period: 11 days
Judge: LSDan
Total Solo HM: 15
Id: 186
League: ETH
Rank: 34/106
Findings: 4
Award: $764.31
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0xNazgul, Atarpara, Awesome, Aymen0909, BClabs, Kong, ali_shehab, bullseye, chaduke, csanuragjain, datapunk, fatherOfBlocks, hansfriese, kaliberpoziomka8552, nicobevi, pashov, pzeus, shark, unforgiven, web3er, xiaoming90
44.934 USDC - $44.93
THe removeFeeder method is lacking onlyRole(*DEFAULT_ADMIN_ROLE*) modifier, even though the NatSpec states “Allows owner to remove feeder”. Due to this, now everyone can remove a feeder anytime. Different types of attack can be executed, one of which is the following:
setPriceremoveFeeder transactionThe likelihood of this happening is high because attacks on missing access control are very easy to execute. The impact is also high because an attacker can force the protocol to operate with a stale price or even to not have any price updaters apart from the owner of the protocol. Since both likelihood and impact are high this should be a High severity vulnerability.
Add onlyRole(DEFAULT_ADMIN_ROLE) modifier to the removeFeeder method
#0 - JeffCX
2022-12-18T03:48:08Z
Valid finding.
#1 - JeffCX
2022-12-18T04:05:13Z
Duplicate of https://github.com/code-423n4/2022-11-paraspace-findings/issues/149 since it has sponsor confirmed tag
#2 - JeffCX
2022-12-18T04:08:09Z
#3 - c4-judge
2022-12-20T16:59:19Z
dmvt marked the issue as duplicate of #31
#4 - c4-judge
2023-01-23T15:59:49Z
dmvt marked the issue as satisfactory
609.8302 USDC - $609.83
Currently the NFTFloorOracle allows for an asset to be paused and the setPrice functionality has whenNotPaused(_asset) modifier. This modifier is missing in the getPrice functionality, so even if an asset was paused any contract calling getPrice will get the stale price successfully.
The impact of this issue is High because protocol will be running with a stale price, but the likelihood is Medium because it needs an asset to be paused. This results in Medium severity.
Add whenNotPaused(_asset) modifier to getPrice
#0 - JeffCX
2022-12-18T03:46:22Z
The code checks the expirationPeriod for price though.
require( (block.number - updatedAt) <= config.expirationPeriod, "NFTOracle: asset price expired" );
#1 - c4-judge
2022-12-20T17:41:55Z
dmvt marked the issue as duplicate of #490
#2 - c4-judge
2023-01-23T20:51:43Z
dmvt marked the issue as partial-50
🌟 Selected for report: IllIllI
Also found by: 0x4non, 0x52, 0xAgro, 0xNazgul, 0xSmartContract, 0xackermann, 9svR6w, Awesome, Aymen0909, B2, BRONZEDISC, Bnke0x0, Deekshith99, Deivitto, Diana, Dravee, HE1M, Jeiwan, Kaiziron, KingNFT, Lambda, Mukund, PaludoX0, RaymondFam, Rolezn, Sathish9098, Secureverse, SmartSek, __141345__, ahmedov, ayeslick, brgltd, cccz, ch0bu, chrisdior4, cryptonue, cryptostellar5, csanuragjain, datapunk, delfin454000, erictee, gz627, gzeon, helios, i_got_hacked, ignacio, imare, jadezti, jayphbee, joestakey, kankodu, ksk2345, ladboy233, martin, nadin, nicobevi, oyc_109, pashov, pavankv, pedr02b2, pzeus, rbserver, ronnyx2017, rvierdiiev, shark, unforgiven, xiaoming90, yjrwkk
103.9175 USDC - $103.92
The addFeeder functionality in NFTFloorOracle pushes an address to the feeders array. Since a check for a maximum array size is missing, if too many feeders are added this can result in impossibility for a price update, since the setPrice functionality calls _combine which iterates over the feeders array. If the array is too big this iteration might cost too much, or even more than the max block gas limit, so price will be stale, until an admin calls removeFeeder which can take some time, plus if the attacker wants it to take more time he can do block stuffing attack on the remove transaction as well.
The impact of this issue is High because protocol will be running with a stale price, but the likelihood is low since it requires the owner to be compromised/malicious or to not be careful with adding too many feeders. This results in Medium severity.
Add a max array size and check if there are max number of feeders already when calling addFeeder
#0 - JeffCX
2022-12-18T03:52:34Z
Unlikely to happen but yes, there is no upper bound for the feed array.
#1 - JeffCX
2022-12-18T04:31:46Z
#2 - c4-judge
2023-01-09T22:46:27Z
dmvt changed the severity to QA (Quality Assurance)
#3 - c4-judge
2023-01-25T16:45:51Z
dmvt marked the issue as grade-b