Forgotten Runes Warrior Guild contest - 0xkatana's results

[L-01] Require statements can be bypassed

There are two require statements that can be bypassed in setPhaseTimes. The setter functions setDaStartTime, setMintlistStartTime, setPublicStartTime and setClaimsStartTime can be used directly to bypass these require checks. Bypassing these require statements can break assumptions and alter how currentDaPrice calculates price. The owner could set bad start time values accidentally or on purpose.

Proof of concept

The require statements that can be bypassed https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L488-L495

Recommended Mitigation Steps

Add stricter controls in the setter functions to prevent the start time value assumptions from breaking.

[L-02] Missing zero check

If the daPriceCurveLength state variable is set to zero, currentDaPrice will revert because the dropPerStep calculation will be dividing by zero. A check to prevent this value from holding a zero value should exist in setDaPriceCurveLength

setLowestPrice would be a good place to add a zero check to prevent the price from going to zero

Proof of concept

The zero check should be added to setDaPriceCurveLength https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L564

Otherwise the result is a divide by zero error uint256 dropPerStep = (startPrice - lowestPrice) / (daPriceCurveLength / daDropInterval);

dropPerStep = (startPrice - lowestPrice) / (0 / daDropInterval);

dropPerStep = (startPrice - lowestPrice) / 0;

Recommended Mitigation Steps

Add this line to setDaPriceCurveLength

require(_newTime != 0);

Add this line to setLowestPrice

require(_newPrice != 0);

[G-01] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

There was one place where an int is initialized to zero https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L24

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-02] Redundant empty string initialization

Solidity does not recognize null as a value, so a string variable is initialized to an empty string. Setting a string variable to empty string is redundant and can waste gas.

There was one place where an int is initialized to zero https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L36

Recommended Mitigation Steps

Remove the redundant empty string initialization string public METADATA_PROVENANCE_HASH; instead of string public METADATA_PROVENANCE_HASH = '';

[G-03] Non-public variables save gas

The MAX_WARRIORS constant variable is public, but changing the visibility of this variable to private or internal can save gas.

Recommended Mitigation Steps

Declare the MAX_WARRIORS variable private or internal

uint256 private constant MAX_WARRIORS = 16000;

[G-04] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L70 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L116

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-05] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

There are many examples of this in for loops https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L162 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L220 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L259 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L355

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-06] Use type(uint256).max

The value 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff is used for five variables in ForgottenRunesWarriorsMinter but using type(uint256).max uses less gas than this constant value. Source https://forum.openzeppelin.com/t/using-the-maximum-integer-in-solidity/3000/13

There are five examples of this where changes can save gas https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L18 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L23 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L27 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L31 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L35

Recommended Mitigation Steps

Use type(uint256).max

[G-07] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Locations where this was found include https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L141 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L211 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L378

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-08] Split up require statements instead of &&

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

One example is

require(numWarriors > 0 && numWarriors <= 20);

This can be improved to

require(numWarriors > 0);
require(numWarriors <= 20);

Several places had require statements with many logical "and"s. Instead, split into two to save gas https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L141 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L211

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

[G-09] Add payable to functions that won't receive ETH

Identifying a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas. This is especially relevant because withdrawAll functions exist to withdraw any ETH accidentally sent.

There are many functions that have the onlyOwner modifier in the contracts. Some examples are https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L257 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L352 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L364 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L427 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L434 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L441 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L448 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L455 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L462

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-10] using strings unnecessary

The line using Strings for uint256; is unnecessary because ForgottenRunesWarriorsGuild inherits ERC721 which already has this line.

The unnecessary line https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L18

Recommended Mitigation Steps

Removing lines of code can save gas

[G-11] Unnecessary require statements

There are two require statements that can be easily bypassed. The comment before these require statements says "they're just guardrails of the typical case". Because these require statements can be bypassed by using the setters directly, they should be removed for gas savings because they don't provide any protection.

The unnecessary lines https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L488-L495

Recommended Mitigation Steps

Remove require statements to save gas

[G-12] Unnecessary variable

There is one unnecessary variable in currentDaPrice that can be removed by combining two lines of calculations

Combine the two lines to remove one unnecessary variable https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L284 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L289

The two lines are

uint256 dropPerStep = (startPrice - lowestPrice) /
(daPriceCurveLength / daDropInterval);

uint256 stepDeduction = steps * dropPerStep;

The two lines can be combined to

uint256 stepDeduction = steps * (startPrice - lowestPrice) / (daPriceCurveLength / daDropInterval);

Recommended Mitigation Steps

Remove variable to save gas