Platform: Code4rena
Start Date: 03/05/2022
Pot Size: $30,000 USDC
Total HM: 6
Participants: 93
Period: 3 days
Judge: gzeon
Id: 118
League: ETH
Rank: 27/93
Findings: 3
Award: $292.27
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: Kulk0
Also found by: 0x1f8b, 0xDjango, BowTiedWardens, Dinddle, broccolirob, defsec, dirk_y, hyh, rajatbeladiya, throttle, unforgiven
246.5367 USDC - $246.54
In ForgottenRunesWarriorsMinter.teamSummon() the owner can mint unrestricted amount of NFTs. This is more of a design issue than an actual bug in my opinion.
If the private keys were compromised during the launch the attacker could mint almost all of the NFTs. Normally I wouldn't say this is an issue but from your documentation, I understand that you are not planning to use a multi-sig wallet for the owner of the contracts. I definitely don't want to say that you are incompetent and you can't store your private keys safely but private keys are getting compromised very often in this space.
Manual Review
Limit how many NFTs can the owner mint. So even if the private keys were compromised the attacker couldn't destroy the entire set by minting thousands of the NFTs to himself making the entire set worth nothing.
I also think this will help with the trust of the protocol since the buyers will know exactly how many NFTs can the Dev Team mint for themselves.
#0 - kartoonjoy
2022-05-04T19:32:45Z
Per the warden's request, updated the title from 'The owner can mint amount of NFTs.' to 'The owner can mint all of the NFTs.'
#1 - cryppadotta
2022-05-05T01:47:23Z
This is true, but by design. It's a risk for minters, but it would be obvious, so we're economically disincentivized to do this. Acknowledged, but not changing it.
#2 - gzeoneth
2022-06-18T17:41:49Z
Sponsor acknowledged centralization risk in README.
#3 - dmitriia
2022-06-18T23:21:53Z
Centralization risk in general is one thing, the ability for unlimited mint, which is easily fixable, is another.
A kind of a boundary state here in my opinion, having 'acknowledged' and 'invalid' flags in the same time poses some contradiction.
#4 - gzeoneth
2022-06-19T15:53:51Z
Judging this as Med Risk since there are specified amounts of teamSummon in the doc
Forgotten Council DAO Creators Fund (teamSummon): ~333 Team & Partners (teamSummon): ~325 Community Honoraries and Contests (teamSummon): ~50
which is not enforced in the teamSummon function
🌟 Selected for report: defsec
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0x52, 0xDjango, 0xf15ers, 0xkatana, 0xliumin, AuditsAreUS, BowTiedWardens, CertoraInc, Cr4ckM3, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, Picodes, Ruhum, TerrierLover, TrungOre, VAD37, WatchPug, berndartmueller, broccolirob, catchup, cccz, cryptphi, csanuragjain, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, hubble, hyh, ilan, joestakey, kebabsec, kenta, kenzo, leastwood, m9800, marximimus, minhquanym, oyc_109, p4st13r4, pauliax, pedroais, peritoflores, plotchy, rajatbeladiya, reassor, rfa, robee, rotcivegaf, samruna, shenwilly, shung, simon135, sorrynotsorry, sseefried, teddav, throttle, tintin, unforgiven, z3s
30.2759 USDC - $30.28
Recommended steps:
Move numMinted += 1; Before the _safeMint()
See here: https://inspexco.medium.com/cross-contract-reentrancy-attack-402d27a02a15
Recommended Steps:
Remove the ^ from both contracts. This will lock the compiler version.
See here: https://swcregistry.io/docs/SWC-103
#0 - cryppadotta
2022-05-05T02:24:50Z
Sure - these are easy changes.
🌟 Selected for report: BowTiedWardens
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xProf, 0xc0ffEE, 0xf15ers, 0xkatana, 0xliumin, ACai, AlleyCat, CertoraInc, Cityscape, Cr4ckM3, DavidGialdi, Dinddle, FSchmoede, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, MiloTruck, Picodes, RoiEvenHaim, Tadashi, TerrierLover, TrungOre, VAD37, WatchPug, antonttc, catchup, defsec, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, ilan, joestakey, kebabsec, kenta, kenzo, marximimus, minhquanym, noobie, oyc_109, p4st13r4, pauliax, rajatbeladiya, reassor, rfa, robee, rotcivegaf, saian, samruna, shenwilly, shung, simon135, slywaters, sorrynotsorry, throttle, unforgiven, z3s
15.4505 USDC - $15.45
There is an unnecessary function that changes Weth address. Since weth address is just one and won't be changed you are much better of defining it as constant in the code. Or defining it as immutable and assigning it the address in the constructor. Both of these solutions will save a lot of gas.
In ForgottenRunesWarriorsMinter.bidSummon 5th require statement string is over 32 bytes. Make the string shorter to save gas. See here: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L140
In ForgottenRunesWarriorsMinter.publicSummon 4th require statement string is over 32 bytes. Make the string shorter to save gas. See here: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L210
In ForgottenRunesWarriorsMinter.refundAddress the function is defined as onlyOwner but also uses the nonReentrant modifier. Since only you can call this function the mutex is not necessary. See here: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L364