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: 16/93
Findings: 4
Award: $611.56
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: throttle
Also found by: 0xDjango, BowTiedWardens, WatchPug, defsec, dipp, fatherOfBlocks, gzeon, hake, reassor, shung, unforgiven
Unbounded and under-constrained variables.
dsStartTime | daPriceCurveLength | daDropIntervalThe team can change the above variables during sale. It will either increase or decrease the price of an NFT. Or it can make currentDaPrice() revert.
uint256 dropPerStep = (startPrice - lowestPrice) / (daPriceCurveLength / daDropInterval); uint256 elapsed = block.timestamp - daStartTime; uint256 steps = elapsed / daDropInterval; uint256 stepDeduction = steps * dropPerStep; // don't go negative in the next step if (stepDeduction > startPrice) { return lowestPrice; } uint256 currentPrice = startPrice - stepDeduction;
dsStartTime | mintlistStartTime | publicStartTime | claimsStartTime selfRefundsStartTimeThe team can change the above variables. It can result in the wrong sale phases order. For example, the public sale can end up being before every other phase due to accidentally setting it to 0.
Manual review
Possible mitigation:
MAX_SUPPLY in the NFT smart contract.#0 - wagmiwiz
2022-05-05T09:58:10Z
This is true but is a low operational risk and can be undone.
#1 - gzeoneth
2022-06-18T18:37:58Z
Decided to consolidate all issues regarding missing validation of the listed variables here.
🌟 Selected for report: Kulk0
Also found by: 0x1f8b, 0xDjango, BowTiedWardens, Dinddle, broccolirob, defsec, dirk_y, hyh, rajatbeladiya, throttle, unforgiven
246.5367 USDC - $246.54
Team minting supply is unbounded
function teamSummon(address recipient, uint256 count) external onlyOwner { require(address(recipient) != address(0), 'address req'); for (uint256 i = 0; i < count; i++) { _mint(recipient); } }
Manual review
Possible mitigation:
#0 - wagmiwiz
2022-05-05T09:41:41Z
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.
#1 - KenzoAgada
2022-06-06T05:43:10Z
Duplicate of #104.
🌟 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
52.771 USDC - $52.77
Check if address/numbers are not 0
Add input validation
numMinted is never decreased in burn function.
Decrease numMinted
The above function might be harmful if called accidentally (very unlikely).
Remove function
It's more readable to use type(uint256).max for max uint value
Consider type(uint256).max
Consider removing that line
According to documentation teamSummon starts at the same time as bidSummon:
/* Timeline: bidSummon : |------------| mintlistSummon : |------------|-------------------------------------| publicSummon : |------------|------------------------| claimSummon : |------------|-----------| teamSummon : |---------------------------------------------------------------|
But in fact it is time-unbounded:
function teamSummon(address recipient, uint256 count) external onlyOwner { require(address(recipient) != address(0), 'address req'); for (uint256 i = 0; i < count; i++) { _mint(recipient); } }
Consider fixing either code or documentation
This:
for (uint256 i = startIdx; i < endIdx + 1; i++) { _refundAddress(daMinters[i]); }
can be simplified to
for (uint256 i = startIdx; i <= endIdx; i++) { _refundAddress(daMinters[i]); }
This one is just a personal style preference.
4.6.0 OpenZeppelin version is available
Bump packages accordingly
The above function is not needed. First of all, initialize name is misleading because it can be called multiple times. Additionally, setMinter() is public function
Remove function
The above function is not needed. It doesn't make sense to have a burn function.
Remove function
🌟 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.491 USDC - $15.49
In the following code there is a nft minting in a for loop. This is very costly. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L162-L164
Use gas efficient ERC721 implementation from here: https://www.erc721a.org/
Minting multiple NFTs to the same wallet is nearly the same price as minting a single NFT. This would save money for every multi-mint.
Consider using optimized ERC721
Consider removing that line
This:
for (uint256 i = startIdx; i < endIdx + 1; i++) { _refundAddress(daMinters[i]); }
can be optimized to
for (uint256 i = startIdx; i <= endIdx;) { _refundAddress(daMinters[i]); unchecked { ++i }; }
Consider optimizing all for loop this way
Above function is not needed. First of all initialize name is misleading because it can be called multiple times. Additionally, setMinter() is public function. Removing can save some deployment gas and EVM function selection gas.
Remove function
Above function is not needed. It doesn't make sense to have a burn function. Removing can save some deployment gas and EVM function selection gas.
Remove function