Platform: Code4rena
Start Date: 21/06/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 96
Period: 3 days
Judge: HardlyDifficult
Total Solo HM: 5
Id: 140
League: ETH
Rank: 55/96
Findings: 2
Award: $45.51
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xkatana, BowTiedWardens, Chom, ElKu, Funen, GalloDaSballo, JC, JMukesh, JohnSmith, Lambda, Limbooo, MadWookie, MiloTruck, Nethermind, Noah3o6, Nyamcil, Picodes, PwnedNoMore, Randyyy, RoiEvenHaim, SmartSek, StErMi, Tadashi, TerrierLover, TomJ, Tomio, Treasure-Seeker, UnusualTurtle, Varun_Verma, Wayne, Waze, _Adam, apostle0x01, asutorufos, berndartmueller, c3phas, catchup, cccz, cloudjunky, codexploder, cryptphi, defsec, delfin454000, dipp, ellahi, exd0tpy, fatherOfBlocks, hansfriese, hyh, joestakey, kebabsec, kenta, masterchief, minhquanym, naps62, oyc_109, pashov, peritoflores, reassor, rfa, robee, sach1r0, saian, sashik_eth, shenwilly, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, ych18, zuhaibmohd, zzzitron
28.2787 USDC - $28.28
#1 unmatch between comment and function (should be implementer_role) Impact Causing confuse to user and developer.
Proof of Concept https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L156
Tool Used Manual review
Recommendation Mitigation Steps change from FEE_ROLE to IMPLEMENTER_ROLE
#2 Missing param natspec _totalSupply Impact _buyPrimaryCurve, _buySecondaryCurve, _sellPrimaryCurve and _sellSecondaryCurve function have natspec comment which is missing the _totalSupply function parameter. Issues with comments are low risk based on Code4rena risk categories.
Proof of Concept https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L275
Tool Used Manual review
Recommendation Mitigation Steps Add natspec comments include _totalSupply parameter in function that mantion.
#3 Use call instead transfer Impact usage of send() or transfer() would cause an out of gas error.
Proof of Concept https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L80
Tool Used Manual review
Recommendation Mitigation Steps use .call() because .transfer() fowards 2300 gas whereas .call() forwards all / set gas.
#4 Missing state admin Impact the constructor initialize _admin but the state admin was missing, and caused of it made constructor doesn't work properly.
Proof of Concept https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/AccessControlMechanism.sol#L19
Tool Used Manual review
Recommendation Mitigation Steps i suggest to add state admin, so that the constructor can work to initialize param admin. and make it immutable
#5 constructor cant initialize the state Impact the constructor initialize _vaultImplementation, _feeTo, and _basketImplementation but the states was missing, and caused of it made constructor doesn't work properly.
Proof of Concept https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L23
Tool Used Manual review
Recommendation Mitigation Steps i suggest to add state _vaultImplementation, _feeTo, and _basketImplementation, so that the constructor can work to initialize _vaultImplementation, _feeTo, and _basketImplementation. and make it immutable
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 8olidity, ACai, BowTiedWardens, Chandr, Chom, ElKu, Fitraldys, Funen, IgnacioB, JC, Lambda, Limbooo, MiloTruck, Noah3o6, Nyamcil, Picodes, Randyyy, SmartSek, StErMi, TerrierLover, TomJ, Tomio, UnusualTurtle, Waze, _Adam, ajtra, c3phas, cRat1st0s, catchup, codexploder, cryptphi, defsec, delfin454000, ellahi, exd0tpy, fatherOfBlocks, hansfriese, joestakey, kebabsec, kenta, m_Rassska, minhquanym, oyc_109, pashov, reassor, rfa, robee, sach1r0, saian, sashik_eth, simon135, slywaters, ych18, ynnad, zuhaibmohd
17.2338 USDC - $17.23
#1 Reduce string
reduce string size of error message to bytes32 can reduce the gas fee. reduce these if possible.
#2 Use calldata instead memory
In the external functions where the function argument is read-only, the function() has an inputed parameter that using memory, if this function didnt change the parameter, its cheaper to use calldata then memory. so we suggest to change it.
#3 Use !=0 instead of >0
for unsigned integer, >0 is less efficient then !=0, so use !=0 instead of >0. apply to others.
#4 Looping (aritmathic)
default uint is 0 so remove unnecassary explicit can reduce gas. caching the array length (_tokens.length) can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity. pre increment e.g ++i more cheaper gas than post increment e.g i++. i suggest to use pre increment.
#5 Use storage instead memory
Use storage instead of memory to reduce the gas fee. I suggest change from
TwavObservation memory _twavObservationCurrent = twavObservations[(_index)]; TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
to
TwavObservation storage _twavObservationCurrent = twavObservations[(_index)]; TwavObservation storage _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
#6 Change visibility to private or internal
change visibility from public to private or internal can save gas. so i recommend to change it.
#0 - mundhrakeshav
2022-06-26T07:32:45Z
#3 , #8, #9, #15