Forgotten Runes Warrior Guild contest - hansfriese's results

Low Risk Issues

1. The finalPrice would be wrong at the first time of Mintlist Phase if you don't update finalPrice right after the first Phase is ended in case the DA fails to sell out. ForgottenRunesWarriorsMinter.sol#L156-L160

Impact You are going to update the finalPrice manually before you refund when the DA fails to sell out, but the second phase will be started automatically right after the first phase is ended. So if you don't update the finalPrice on that timestamp, the finalPrice will be still 2.5 ETH(initial value) and it will revert when minters try to buy with the correct price. And if some minters(I think there will be no such a minter though) buy with 2.5 ETH then there is no method to refund because it doesn't save funds for each address from the second phase.

Proof of Concept The DA fails to sell out and the finalPrice isn't updated automatically. You don't update the finalPrice manually at the next timestamp. Then minters can't buy warriors with correct price until you set the finalPrice.

Recommended Mitigation Steps I see the duration between the first phase and the second one is 1 day and the price will be the lowestPrice before the first phase ends. (Actually within 380 mins with default settings) So you can modify to set the finalPrice when currentPrice = lowestPrice. You can add this code after #L160. if (currentPrice == lowestPrice && finalPrice != lowestPrice) { finalPrice = lowestPrice; } Then when you use default setting finalPrice will be set automatically in any cases. If you don't ensure the price will meet the lowestPrice during the first phase with different settings, I think it would be good to save the last price even though it cost more gas or you need to implement a logic to set the finalPrice automatically.

And I think it's related to gas optimizations but I want to explain here because I need to explain continuously. ForgottenRunesWarriorsMinter.sol#L145 With default settings, the price will be the lowestPrice within 380 mins and you don't need to calcualte currentPrice again and again once finalPrice = lowestPrice. So you can write like this instead of #L145. uint256 currentPrice = finalPrice == lowestPrice ? finalPrice : currentDaPrice();

Non-critical Issues

1. I know you will set correct start times for phases but I think it would be good to add one more require() as you added two already. ForgottenRunesWarriorsMinter.sol#487 require( newMintlistStartTime > newDaStartTime, 'Set mintlist after Da' );

2. NatSpec is incomplete @param is missing (You added @param for same calldata here: ForgottenRunesWarriorsMinter.sol#L169) ForgottenRunesWarriorsGuild.sol#L151 ForgottenRunesWarriorsGuild.sol#L156 ForgottenRunesWarriorsMinter.sol#L504 ForgottenRunesWarriorsMinter.sol#L512 ForgottenRunesWarriorsMinter.sol#L519 ForgottenRunesWarriorsMinter.sol#L526 ForgottenRunesWarriorsMinter.sol#L533 ForgottenRunesWarriorsMinter.sol#L542 ForgottenRunesWarriorsMinter.sol#L549 ForgottenRunesWarriorsMinter.sol#L556 ForgottenRunesWarriorsMinter.sol#L563 ForgottenRunesWarriorsMinter.sol#L570 ForgottenRunesWarriorsMinter.sol#L578 ForgottenRunesWarriorsMinter.sol#L585 ForgottenRunesWarriorsMinter.sol#L592 ForgottenRunesWarriorsMinter.sol#L599

3. It would be good to use (uint256).max instead of 64 fs. ForgottenRunesWarriorsMinter.sol#L18 ForgottenRunesWarriorsMinter.sol#L23 ForgottenRunesWarriorsMinter.sol#L27 ForgottenRunesWarriorsMinter.sol#L31 ForgottenRunesWarriorsMinter.sol#L35

1. You can modify daMinters not to save duplicate addresses in order to save gas. ForgottenRunesWarriorsMinter.sol#L151 There is a mapping that counts the number of minted warriors on that address already so you can add a new address only by checking this mapping. You can write instead of #L151. if (daNumMinted[msg.sender] == 0) { daMinters.push(msg.sender); } Then this array won't save duplicated addresses and it will be better when you refund later also.

2. 0 is less efficient than != 0 for unsigned integers 1). ForgottenRunesWarriorsMinter.sol#L141 numWarriors > 0 && numWarriors <= 20, 2). ForgottenRunesWarriorsMinter.sol#L211 numWarriors > 0 && numWarriors <= 20, 3). ForgottenRunesWarriorsMinter.sol#L378 if (owed > 0) {

3. ++i costs less gas compared to i++ or i += 1 1). ForgottenRunesWarriorsGuild.sol#104 numMinted += 1; 2). ForgottenRunesWarriorsMinter.sol#193 numSold += 1; 3). ForgottenRunesWarriorsMinter.sol#248 numClaimed += 1; 4). ForgottenRunesWarriorsMinter.sol#162 for (uint256 i = 0; i < numWarriors; i++) { 5). ForgottenRunesWarriorsMinter.sol#220 for (uint256 i = 0; i < numWarriors; i++) { 6). ForgottenRunesWarriorsMinter.sol#259 for (uint256 i = 0; i < count; i++) { 7). ForgottenRunesWarriorsMinter.sol#355 for (uint256 i = startIdx; i < endIdx + 1; i++) {