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: 76/96
Findings: 1
Award: $28.28
🌟 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
https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L113 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L25 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVaultFactory.sol#L132
Detailed description of the impact of this finding.
There are two possible issues here.
The address feeTo is set directly in the constructor, but I noticed that there is no further check to see if this address is a 0 address, and that feeTo will be set to a 0 address if the constructor is not passed a parameter to the function, and that it takes time to change the feeTo address, and after initiating a proposal, the address can only be changed when the delay has ended If a malicious user calls this function, the full amount of Ether in the Factory contract will be sent to the 0x000 black hole address! This means that the money will be locked up in the black hole address forever.
Another problem is that
The current FEE_ROLE privileged account calls the proposeNewAdminFeeAddress() function to initiate the proposal to change the pendingFeeTo address and feeToUpdateTime, because the 0x000 address is not checked and if ADMIN does not notice anything different, once pendingFeeTo is set to 0x000 and a malicious attacker listens to the contract, calls updateNewAdminFeeAddress() to set the feeTo to 0x000 as soon as the current time exceeds the previously set feeToUpdateTime, and immediately afterwards calls withdrawAdminFee() function, at which point the full amount of Ether in the Factory contract will be sent to the 0x000 black hole address! This means that the funds will be locked in the black hole address forever.
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
Add access control to the withdrawAdminFee function and check that feeTo is not a Zero Address
#0 - GalloDaSballo
2022-06-26T22:08:36Z
Address 0 check, historically low severity, being suggested as High Severity finding
#1 - HardlyDifficult
2022-07-03T12:34:24Z
Good report. I agree that checking for != address(0) is a worthwhile addition.
Validating the input field is non-zero is a nice to have. It's effectively not much different than making a simple typo when inserting the expected address - which we cannot guard against in the contract. Lowing to Low risk and converting this into a QA report for the warden.