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: 22/93
Findings: 3
Award: $348.30
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: throttle
Also found by: 0xDjango, BowTiedWardens, WatchPug, defsec, dipp, fatherOfBlocks, gzeon, hake, reassor, shung, unforgiven
296.7571 USDC - $296.76
Reversible DOS in bidSummon() function. Although it is reversible, the unintended causes can take some time troubleshooting, and it may adversely effect the auction phases.
setDaDropInterval(0) results in division by zero in currentDaPrice(), disabling bidSummon(),startPrice < lowestPrice results in underflow in currentDaPrice(), disabling bidSummon(),daPriceCurveLength < daDropInterval results in division by zero in currentDaPrice(), disabling bidSummon().None.
Consider including proper require checks in these setter function, or ideally removing these setter functions by making these variables constants.
#0 - cryppadotta
2022-05-06T15:51:24Z
All good points and something to be careful of
#1 - gzeoneth
2022-06-18T18:48:20Z
Duplicate of #27
🌟 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
35.025 USDC - $35.02
I suggest making these functions external.
I suggest removing virtual modifier from these functions.
^0.8.0) is used instead of a locked pragmaI suggest locking both of the contracts' versions to 0.8.6 (latest compiler version defined in Hardhat configuration file of the project).
_msgSender() function is unnecessarily used instead of msg.sender_msgSender() can be safely replaced by msg.sender in these instances.
ForgottenRunesWarriorsGuild:101: _isApprovedOrOwner(_msgSender(), tokenId), ForgottenRunesWarriorsGuild:115: require(_msgSender() == minter, 'Not a minter');
I suggest making ForgottenRunesWarriorsGuild.mint() function capable of minting multiple NFTs by taking a uint256 amount argument. This way, the following external call loops can be replaced with a single call, greatly optimizing the minting cost.
ForgottenRunesWarriorsGuildinitialize() simply calls setMinter() without doing anything else. I suggest removing initialize() and making setMinter() an external function.
NatSpec comment for ForgottenRunesWarriorsGuild.claimlistMinted (L87) says “Tracks the total count of NFTs claimed by a given address”. It should be “Tracks if a given address minted in the claimlist”.
ForgottenRunesWarriorsGuild.METADATA_PROVENANCE_HASH (L36) is a mutable variable but named with all capital letters. That is against the official style guide. I suggest using mixedCase style or making the variable immutable or constant.
I suggest emitting events in these functions.
Some variables are initialized by their maximum value (i.e.: 0xfff…fff). These can instead be written as type(uint256).max to make the code more readable.
Almost all the variables in the contract are mutable. This increases gas cost when these variables are read, increases centralization, and requires making many assumptions when auditing.
I suggest making these variables constant or immutable, and removing respective setter functions.
ForgottenRunesWarriorsMinter.wethForgottenRunesWarriorsMinter.warriorsForgottenRunesWarriorsMinter.maxDaSupplyForgottenRunesWarriorsMinter.maxForSaleForgottenRunesWarriorsMinter.maxForClaimChanging the following variables during auction phase can especially have adverse effects. I strongly suggest making these variables constant or immutable, and removing respective setter functions.
ForgottenRunesWarriorsMinter.startPriceForgottenRunesWarriorsMinter.lowestPriceForgottenRunesWarriorsMinter.daPriceCurveLengthForgottenRunesWarriorsMinter.daDropIntervalForgottenRunesWarriorsMinter requires having many assumptions when auditingChanging start times of minting phases can also have adverse effects. I strongly suggest making these variables constant or immutable, and removing respective setter functions. However, ForgottenRunes team have explicitly stated that they want to be able to pause the minting in case of a DOS on their frontend. Therefore I suggest removing all setter functions for start time variables, and utilize a dynamic method similar to the example below.
lastPauseTime state variable that is updated with block.timestamp any time pause() function is called,daStartTime state variable that is incremented with block.timestamp - lastPauseTime any time unpause() is called,daStartTime is set to block.timestamp, and pause() function is called,mintlistStartTime variable is replaced by daStartTime + 1 days so it updates dynamically,publicStartTime variable is replaced by daStartTime + 2 days so it updates dynamically,claimlistStartTime variable is replaced by daStartTime + 3 days so it updates dynamically,selfRefundsStartTime variable is replaced by daStartTime + 4 days, so it updates dynamically.Then, the team can start the auction by calling unpause(). And whenever the contract needs to be paused, all start time variables would dynamically increase based on the pause duration. This would prevent any potential adverse effect that can result from invalid inputs from the team.
finalPrice needs to be manually set if all auction supply is not soldfinalPrice can be algorithmically defined to reduce centralization of the contract. In the current implementation, bidSummon() function sets the finalPrice only when the whole auction supply is sold. Team states that this is to prevent setting finalPrice at every mint. However, due to step reduction of the price, finalPrice would only need to be set daPriceCurveLength / daDropInterval times at most, which is 38 based on the default values. So I suggest replacing the if statement in the bidSummon() function with the following. Then the setter function of finalPrice can be removed from the codebase.
if (currentPrice < finalPrice) { finalPrice = currentPrice; }
ForgottenRunesWarriorsGuildBoth the METADATA_PROVENANCE_HASH and baseTokenURI are mutable. This defeats the purpose of having a provenance hash for verifying the integrity of the off-chain metadata. At minimum, I suggest making METADATA_PROVENANCE_HASH immutable or constant. Ideally, I suggest making both of these variables immutable or constant, and using an ipfs:// URI for the base URI. An alternative would be to just renounce the contract ownership after ensuring collection has no issues after the reveal.
I suggest replacing address(VARIABLE) in the following instances with just VARIABLE.
ForgottenRunesWarriorsGuild.sol:174: require(address(msg.sender) != address(0)); ForgottenRunesWarriorsMinter.sol:258: require(address(recipient) != address(0), 'address req'); ForgottenRunesWarriorsMinter.sol:609: require(address(vault) != address(0), 'no vault'); ForgottenRunesWarriorsMinter.sol:617: require(address(vault) != address(0), 'no vault');
🌟 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
16.5224 USDC - $16.52
It costs extra gas to initialize variables to their default values. I suggest not initializing these variables.
In ForgottenRunesWarriorsGuild.mint(), first, local variable tokenId is initialized with numMinted (L102), then, numMinted is incremented by one (L104). I suggest removing incrementation line, and replacing initialization line with uint256 tokenId = numMinted++;. This will increment numMinted by one and assign the value before incrementation to tokenId.
In ForgottenRunesWarriorsMinter.claimSummon(), first, numClaimed < maxForClaim is checked (L234), then numClaimed is incremented by one (L248). These can be combined in the same line, by changing L248 to require(numCLaimed++ < maxForClaim, 'No more claims');.
In ForgottenRunesWarriorsMinter.bidSummon(), all the bidder addresses are pushed to a storage array. This significantly increases the minting cost. Especially, since it is trivial to do off-chain indexing of the wallet addresses that participate in the auction.
I suggest removing daMinters array and numDaMinters() function from the code, and indexing auction participants off chain. And for issueRefunds(), the daMinters range arguments can be replaced by an address array argument.
numSold + numWarriors calculation twiceIt is more efficient to store the result of numSold + numWarriors in a local variable, then use that local variable, instead of doing the same calculation more than once. This is especially the case since numSold is a storage variable hence is costly to read.
i++ is used for incrementation in for loopsUsing ++i instead of i++ is cheaper in for loops. I suggest replacing i++ with ++i in the instances below.
ForgottenRunesWarriorsMinter.sol:162: for (uint256 i = 0; i < numWarriors; i++) { ForgottenRunesWarriorsMinter.sol:220: for (uint256 i = 0; i < numWarriors; i++) { ForgottenRunesWarriorsMinter.sol:259: for (uint256 i = 0; i < count; i++) { ForgottenRunesWarriorsMinter.sol:355: for (uint256 i = startIdx; i < endIdx + 1; i++) {
msg.sender != address(0)It does not make sense to check if the message sender is not the zero address. Even if it did, msg.sender is already checked to be the owner through onlyOwner modifier. So there is no point in checking this again. Following lines can be safely removed.
ForgottenRunesWarriorsGuild.sol:174: require(address(msg.sender) != address(0)); ForgottenRunesWarriorsMinter.sol:628: require(address(msg.sender) != address(0));
numSold < maxForSalerequire(numSold < maxDaSupply, 'Auction sold out'); and require(numSold < maxForSale, 'Sold out'); statements can be removed from the functions below. Because for all the cases below, there exists a succeeding require check, that fails if these checks also fail.
When a variable is a boolean, there is no need to explicitly compare it with a boolean literal. I suggest replacing VARIABLE == false checks below by !VARIABLE.
ForgottenRunesWarriorsMinter.sol:182: require(mintlistMinted[msg.sender] == false, 'Already minted'); ForgottenRunesWarriorsMinter.sol:238: require(claimlistMinted[msg.sender] == false, 'Already claimed');
In ForgottenRunesWarriorsMinter, user information related to dutch auction is stored in three separate mappings: daAmountPaid, daAmountRefunded, and daNumMinted. Among these, daAmountPaid and daNumMinted are written in bidSummon(), and all of them are read in refundOwed(). Therefore having these three mappings in a single thightly packed struct would save significant gas during bidding and refunding. I suggest packing these variables like in the example below.
struct DaMinterInfo { uint96 amountPaid; uint96 amountRefunded; uint32 numMinted; } mapping(address => DaMinterInfo) public daMinterInfo;
This would also require some changes to the code where these variables are used, and could require usage of a library like OpenZeppelin’s SafeCast.sol. But I believe it is worth to make this effort for the gas benefit.
At the end of ForgottenRunesWarriorsMinter.currentDaPrice(), there are following checks.
ForgottenRunesWarriorsMinter.sol:291: // don't go negative in the next step ForgottenRunesWarriorsMinter.sol:292: if (stepDeduction > startPrice) { ForgottenRunesWarriorsMinter.sol:293: return lowestPrice; ForgottenRunesWarriorsMinter.sol:294: } ForgottenRunesWarriorsMinter.sol:295: uint256 currentPrice = startPrice - stepDeduction; ForgottenRunesWarriorsMinter.sol:296: return currentPrice > lowestPrice ? currentPrice : lowestPrice;
However, all of these lines can be safely removed and replaced by the following.
return startPrice - stepDeduction;
This is because all of the checks above are guaranteed due to the if statement in L279. After the if statement, elapsed local variable is guaranteed to be less than daPriceCurveLength. Which in turn guarantees that startPrice - stepDeduction >= lowestPrice.