Forgotten Runes Warrior Guild contest - oyc_109's results

Unspecific Compiler Version Pragma

description

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

findings

IForgottenRunesWarriorsGuild.sol
5: pragma solidity ^0.8.6;

IForgottenRunesWarriorsMinter.sol
5: pragma solidity ^0.8.6;

interfaces/IWETH.sol
3: pragma solidity ^0.8.6;

ForgottenRunesWarriorsGuild.sol
1: pragma solidity ^0.8.0;

ForgottenRunesWarriorsMinter.sol
1: pragma solidity ^0.8.0;

missing checks for zero address

description

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places. Given that zero-address is used as an indicator for BNB, there is a greater risk of using it accidentally.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

findings

ForgottenRunesWarriorsGuild.sol
136: 
137:     function setMinter(address newMinter) public onlyOwner {
138:         minter = newMinter;
139:     }

ForgottenRunesWarriorsMinter.sol
530: 
531:     /**
532:      * @notice set the warriors token address
533:      */
534:     function setWarriorsAddress(
535:         IForgottenRunesWarriorsGuild _newWarriorsAddress
536:     ) public onlyOwner {
537:         warriors = _newWarriorsAddress;
538:     }
539: 
540:     /**
541:      * @notice set the weth token address
542:      */
543:     function setWethAddress(address _newWethAddress) public onlyOwner {
544:         weth = _newWethAddress;
545:     }

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

ForgottenRunesWarriorsGuild.sol
24: uint256 public numMinted = 0;

ForgottenRunesWarriorsMinter.sol
162: for (uint256 i = 0; i < numWarriors; i++) { 
220: for (uint256 i = 0; i < numWarriors; i++) {
259: for (uint256 i = 0; i < count; i++) {
355: for (uint256 i = startIdx; i < endIdx + 1; i++) {

Long Revert Strings

description

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

ForgottenRunesWarriorsGuild.sol
70: 'ERC721Metadata: URI query for nonexistent token'
116: 'ERC721Burnable: caller is not owner nor approved'

ForgottenRunesWarriorsMinter.sol
142: 'You can summon no more than 20 Warriors at a time'
148: 'Ether value sent is not sufficient'

use inline instead of function

description

the consequences of using extra functions:

• contract’s code size gets bigger
• the function calls consumes more gas than exectuing it as an inlined function (part of the code, without the function call)

findings

ForgottenRunesWarriorsGuild.sol

the functions setMinter and initialize are redundant
remove one of the functions to simply set the newMinter in a single function

redundant check

description

since the function already has the onlyOwner modifier, it is unnecessary to check if msg.sender is address(0)

findings

ForgottenRunesWarriorsGuild.sol
174: require(address(msg.sender) != address(0));

use constants to save gas

description

Constant variables are replaced at compile time by their values. By using constants for values that do not change, we can avoid fees of an SLOAD operation when using these values.

findings

ForgottenRunesWarriorsMinter.sol
56:     /// @notice The start price of the DA
57:     uint256 public startPrice = 2.5 ether;
58: 
59:     /// @notice The lowest price of the DA
60:     uint256 public lowestPrice = 0.6 ether;
61: 
62:     /// @notice The length of time for the price curve in the DA
63:     uint256 public daPriceCurveLength = 380 minutes;
64: 
65:     /// @notice The interval of time in which the price steps down
66:     uint256 public daDropInterval = 10 minutes;
67: 
68:     /// @notice The final price of the DA. Will be updated when DA is over and then used for subsequent phases
69:     uint256 public finalPrice = 2.5 ether;

90:     /// @notice The total number of tokens reserved for the DA phase
91:     uint256 public maxDaSupply = 8000;
92: 
93:     /// @notice Tracks the total count of NFTs sold (vs. freebies)
94:     uint256 public numSold;
95: 
96:     /// @notice Tracks the total count of NFTs for sale
97:     uint256 public maxForSale = 14190;
98: 
99:     /// @notice Tracks the total count of NFTs claimed for free
100:     uint256 public numClaimed;
101: 
102:     /// @notice Tracks the total count of NFTs that can be claimed
103:     /// @dev While we will have a merkle root set for this group, putting a hard cap helps limit the damage of any problems with an overly-generous merkle tree
104:     uint256 public maxForClaim = 1100;

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

ForgottenRunesWarriorsMinter.sol
162: for (uint256 i = 0; i < numWarriors; i++) {
220: for (uint256 i = 0; i < numWarriors; i++) {
259: for (uint256 i = 0; i < count; i++) {
355: for (uint256 i = startIdx; i < endIdx + 1; i++) {

require statements should be checked first

description

require statements can be placed earlier to reduce gas usage on revert ie move require to the top of the function if possible

findings

move the following require statement up above line 183 to save gas in the case where the require statement fails

ForgottenRunesWarriorsMinter.sol
187:         require(
188:             MerkleProof.verify(_merkleProof, mintlist1MerkleRoot, node) ||
189:                 MerkleProof.verify(_merkleProof, mintlist2MerkleRoot, node),
190:             'Invalid proof'
191:         );

do not cache variable used only once

description

for variables only used once, changing it to inline saves gas

findings

ForgottenRunesWarriorsMinter.sol
284:         uint256 dropPerStep = (startPrice - lowestPrice) /
285:             (daPriceCurveLength / daDropInterval);

287:         uint256 elapsed = block.timestamp - daStartTime;
288:         uint256 steps = elapsed / daDropInterval;

389:         uint256 totalCostOfMints = finalPrice * daNumMinted[minter];
390:         uint256 refundsPaidAlready = daAmountRefunded[minter];