Forgotten Runes Warrior Guild contest - BowTiedWardens's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L520

Vulnerability details

Issue: withdrawAll and withdraw have no safeguards in place to restrict their calling to after users have been refunded. If these are called before users are refunded, they can make the refund mechanism underfunded.

Consequence: Users may not receive their owed refunds

Proof of Concept

• Auction finishes with the contract holding 1000 ETH and 200 ETH of refunds owed
• Through mistake, malice, or owner account compromise, the owner calls withdrawAll or withdraw(900e18)
• Contract balance now less than the 200 ETH required to refund users
• Manual intervention by the team will be required to refund the users

Mitigations

Mitigation: Calculate the amount owed to the team. The remainder can be assumed to be for processing refunds. Add checks to withdraw and withdrawAll which will not allow withdrawal of more than the team's owed amount.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L505 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L513

Vulnerability details

Issue: Merkle roots may be updated arbitrarily at any time by the owner.

Consequence: A malicious owner or owner account compromise can update the merkle tree arbitrarily to include or exclude users from the mintlist.

Mitigations

Mitigation: Add a state lock to only allow the merkle roots to be set once.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L441

Vulnerability details

Issue: There is no check in setDaStartTime against setting a time in the past or far in the future. A malicious owner or owner account compromise can arbitrarily terminate the Dutch auction by setting setDaStartTime back in the past or prevent the Dutch auction from starting by setting setDaStartTime to the far future.

Consequence:

• The Dutch auction can be terminated prematurely (setDaStartTime in the past).
• The Dutch auction can be prevented from starting (setDaStartTime in the far future).

Mitigations

Suggestion 1: Remove the setDaStartTime function and set the daStartTime parameter in the constructor. Suggestion 2: Add the following check when setting daStartTime: require(_newTime >= block.timestamp)

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L257

Vulnerability details

Issue: There is no quantity control on the teamSummon function.

Consequence: A malicious owner or owner account compromise can mint for free up to the max supply enforced by the Guild contract itself, bypassing the auction and sale phases.

Mitigations

Mitigation: Add a constant for maxTeamSupply. Add a counter for teamMinted. Add a check along the lines of require(teamMinted + count < maxTeamSupply) in teamSummon.

Table of Contents:

• [L-01] Missing events for critical parameters that change the state
• [N-01] Related data should be grouped in a struct
• [N-02] The pragmas used are not the same everywhere and are not fixed

[L-01] Missing events for critical parameters that change the state

None of these setter functions emit events to record changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary:

 - daStartTime = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#442) 
 - mintlistStartTime = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#449) 
 - publicStartTime = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#456) 
 - claimsStartTime = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#463) 
 - selfRefundsStartTime = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#470) 
 - startPrice = _newPrice (contracts/ForgottenRunesWarriorsMinter.sol#551) 
 - lowestPrice = _newPrice (contracts/ForgottenRunesWarriorsMinter.sol#558) 
 - daPriceCurveLength = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#565) 
 - daDropInterval = _newTime (contracts/ForgottenRunesWarriorsMinter.sol#572) 
 - finalPrice = _newPrice (contracts/ForgottenRunesWarriorsMinter.sol#580) 

[N-01] Related data should be grouped in a struct

The "Given address in the DA" mappings should be grouped in a struct.

From:

File: ForgottenRunesWarriorsMinter.sol
75:     /// @notice Tracks the total amount paid by a given address in the DA
76:     mapping(address => uint256) public daAmountPaid;
77: 
78:     /// @notice Tracks the total amount refunded to a given address for the DA
79:     mapping(address => uint256) public daAmountRefunded;
80: 
81:     /// @notice Tracks the total count of NFTs minted by a given address in the DA
82:     mapping(address => uint256) public daNumMinted;

To

    struct DaInfo {
        uint256 daAmountPaid;   
        uint256 daAmountRefunded;   
        uint256 daNumMinted;   
    }

    mapping(address => DaInfo) public daInfo;

It would be less error-prone and more readable.

[N-02] The pragmas used are not the same everywhere and are not fixed

contracts/ForgottenRunesWarriorsGuild.sol:
  1: pragma solidity ^0.8.0;

contracts/ForgottenRunesWarriorsMinter.sol:
  1: pragma solidity ^0.8.0;

contracts/interfaces/IForgottenRunesWarriorsGuild.sol:
  5: pragma solidity ^0.8.6;

contracts/interfaces/IForgottenRunesWarriorsMinter.sol:
  5: pragma solidity ^0.8.6;

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

Table of Contents:

• Caching storage values in memory
• Unchecking arithmetics operations that can't underflow/overflow
• Unnecessary initialize() function
• ForgottenRunesWarriorsGuild.forwardERC20s() and ForgottenRunesWarriorsMinter.forwardERC20s(): Unnecessary require statements
• ForgottenRunesWarriorsMinter: bidSummon()and publicSummon(): Unnecessary require statement
• Boolean comparisons
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• ForgottenRunesWarriorsMinter.currentDaPrice(): > should be >=
• Splitting require() statements that use && saves gas
• ++i costs less gas compared to i++ or i += 1
• Increments can be unchecked
• Public functions to external
• No need to explicitly initialize variables with default values
• Upgrade pragma to at least 0.8.4
• Use msg.sender instead of OpenZeppelin's _msgSender() when meta-transactions capabilities aren't used
• Use Custom Errors instead of Revert Strings to save Gas

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/ForgottenRunesWarriorsGuild.sol:
  100:         require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');  //@audit gas: numMinted SLOAD 1 (should declare tokenId earlier and use it instead)
  102:         uint256 tokenId = numMinted; //@audit gas: numMinted SLOAD 2 (should be declared earlier)
  104:         numMinted += 1;  //@audit gas: numMinted SLOAD 3 (should be numMinted = tokenId + 1)

contracts/ForgottenRunesWarriorsMinter.sol:
  136:         require(numSold < maxDaSupply, 'Auction sold out'); //@audit gas: numSold SLOAD 1, maxDaSupply SLOAD 1
  137:         require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining'); //@audit gas: numSold SLOAD 2, maxDaSupply SLOAD 2
  154:         numSold += numWarriors; //@audit gas: numSold SLOAD 3 (equivalent to numSold = numSold + numWarriors)
  156:         if (numSold == maxDaSupply) { //@audit gas: numSold SLOAD 4, maxDaSupply SLOAD 3
  177:         require(numSold < maxForSale, 'Sold out'); //@audit gas: numSold SLOAD 1
  193:         numSold += 1; //@audit gas: numSold SLOAD 2 (equivalent to numSold = numSold + 1)
  207:         require(numSold < maxForSale, 'Sold out'); //@audit gas: numSold SLOAD 1, maxDaSupply SLOAD 1
  208:         require(numSold + numWarriors <= maxForSale, 'Not enough remaining'); //@audit gas: numSold SLOAD 2, maxDaSupply SLOAD 2
  219:         numSold += numWarriors; //@audit gas: numSold SLOAD 3 (equivalent to numSold = numSold + numWarriors)
  234:         require(numClaimed < maxForClaim, 'No more claims'); //@audit gas: numSold SLOAD 1
  248:         numClaimed += 1; //@audit gas: numSold SLOAD 2 (equivalent to numSold = numSold + 1)
  279:         if (block.timestamp >= daStartTime + daPriceCurveLength) {//@audit gas: daStartTime SLOAD 1, daPriceCurveLength SLOAD 1
  284:         uint256 dropPerStep = (startPrice - lowestPrice) / //@audit gas: startPrice SLOAD 1, lowestPrice SLOAD 1
  285:             (daPriceCurveLength / daDropInterval); //@audit gas: daPriceCurveLength SLOAD 2, daDropInterval SLOAD 1
  287:         uint256 elapsed = block.timestamp - daStartTime;//@audit gas: daStartTime SLOAD 2
  288:         uint256 steps = elapsed / daDropInterval; //@audit gas: daDropInterval SLOAD 2
  292:         if (stepDeduction > startPrice) { //@audit gas: startPrice SLOAD 2
  293:             return lowestPrice; //@audit gas: lowestPrice SLOAD 2
  295:         uint256 currentPrice = startPrice - stepDeduction; //@audit gas: startPrice SLOAD 3
  296:         return currentPrice > lowestPrice ? currentPrice : lowestPrice;  //@audit gas: lowestPrice SLOAD 2 & 3
  401:             IWETH(weth).deposit{value: amount}(); //@audit gas: weth SLOAD 1
  402:             IERC20(weth).transfer(to, amount); //@audit gas: weth SLOAD 2
  609:         require(address(vault) != address(0), 'no vault'); //@audit gas: vault SLOAD 1
  610:         require(payable(vault).send(_amount)); //@audit gas: vault SLOAD 2
  617:         require(address(vault) != address(0), 'no vault'); //@audit gas: vault SLOAD 1
  618:         require(payable(vault).send(address(this).balance)); //@audit gas: vault SLOAD 2

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping L295 with an unchecked block (see @audit):

File: ForgottenRunesWarriorsMinter.sol
291:         // don't go negative in the next step
292:         if (stepDeduction > startPrice) {
293:             return lowestPrice;
294:         }
295:         uint256 currentPrice = startPrice - stepDeduction; //@audit gas: should be unchecked due to L292-L293

Unnecessary initialize() function

The initialize() function isn't an initializer. It just calls setMinter(), which has the same visibility and authorization level as initialize():

File: ForgottenRunesWarriorsGuild.sol
52:     function initialize(address newMinter) public onlyOwner { 
53:         setMinter(newMinter);
54:     }
...
137:     function setMinter(address newMinter) public onlyOwner {
138:         minter = newMinter;
139:     }

It could even be called repeatedly.

As the initialize() function is not needed, I suggest deleting it and directly calling setMinter() to "Conveniently initialize the contract".

ForgottenRunesWarriorsGuild.forwardERC20s() and ForgottenRunesWarriorsMinter.forwardERC20s(): Unnecessary require statements

Here, as the onlyOwner modifier is applied, the address(0) checks are not needed here:

contracts/ForgottenRunesWarriorsGuild.sol:
  173      function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
  174:         require(address(msg.sender) != address(0)); //@audit gas: there's the onlyOwner modifier: msg.sender can't be address(0)
  175          token.transfer(msg.sender, amount);
  176      }

contracts/ForgottenRunesWarriorsMinter.sol:
  627      function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
  628:         require(address(msg.sender) != address(0)); //@audit gas: there's the onlyOwner modifier: msg.sender can't be address(0)
  629          token.transfer(msg.sender, amount);
  630      }

I suggest removing these checks.

ForgottenRunesWarriorsMinter: bidSummon()and publicSummon(): Unnecessary require statement

The code is as such:

File: ForgottenRunesWarriorsMinter.sol
130:     function bidSummon(uint256 numWarriors)
131:         external
132:         payable
133:         nonReentrant
134:         whenNotPaused
135:     {
136:         require(numSold < maxDaSupply, 'Auction sold out');
137:         require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
138:         require(daStarted(), 'Auction not started');
139:         require(!mintlistStarted(), 'Auction phase over');
140:         require(
141:             numWarriors > 0 && numWarriors <= 20,
142:             'You can summon no more than 20 Warriors at a time'
143:         );
...
201:     function publicSummon(uint256 numWarriors)
202:         external
203:         payable
204:         nonReentrant
205:         whenNotPaused
206:     {
207:         require(numSold < maxForSale, 'Sold out');
208:         require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
209:         require(publicStarted(), 'Public sale not started');
210:         require(
211:             numWarriors > 0 && numWarriors <= 20,
212:             'You can summon no more than 20 Warriors at a time'
213:         );

Logically speaking, numSold + numWarriors <= maxForSale could only reach the edge-case if numWarriors == 0, but that's prevented with the condition that follows in both functions: numWarriors > 0 && numWarriors <= 20. Meaning that, with numSold + numWarriors <= maxForSale and numWarriors > 0, we don't need to check if numSold < maxForSale as it just can't happen.

I suggest removing the 2 require(numSold < maxDaSupply) checks L136 and L207.

Furthermore, notice that 'Not enough remaining' and 'Sold out' kinda mean the same thing, so the additionnal require statement might not be justified.

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(!directValue) instead of if(directValue == false) here:

ForgottenRunesWarriorsMinter.sol:182:        require(mintlistMinted[msg.sender] == false, 'Already minted');
ForgottenRunesWarriorsMinter.sol:238:        require(claimlistMinted[msg.sender] == false, 'Already claimed');

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

ForgottenRunesWarriorsMinter.sol:141:            numWarriors > 0 && numWarriors <= 20,
ForgottenRunesWarriorsMinter.sol:211:            numWarriors > 0 && numWarriors <= 20,

Also, please enable the Optimizer.

ForgottenRunesWarriorsMinter.currentDaPrice(): > should be >=

The return statement is as follows:

ForgottenRunesWarriorsMinter.sol:296:        return currentPrice > lowestPrice ? currentPrice : lowestPrice;

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)
Furthermore, lowestPrice is read from storage while currentPrice is read from memory.

Therefore, it's possible to always save 3 gas and sometimes further save 1 SLOAD (when currentPrice == lowestPrice) by replacing the code to:

ForgottenRunesWarriorsMinter.sol:296:        return currentPrice >= lowestPrice ? currentPrice : lowestPrice;

Splitting require() statements that use && saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:

contracts/ForgottenRunesWarriorsMinter.sol:
  140          require(
  141:             numWarriors > 0 && numWarriors <= 20,
  142              'You can summon no more than 20 Warriors at a time'
  143          );

  210          require(
  211:             numWarriors > 0 && numWarriors <= 20,
  212              'You can summon no more than 20 Warriors at a time'
  213          );

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

ForgottenRunesWarriorsGuild.sol:104:        numMinted += 1;
ForgottenRunesWarriorsMinter.sol:162:        for (uint256 i = 0; i < numWarriors; i++) {
ForgottenRunesWarriorsMinter.sol:193:        numSold += 1;
ForgottenRunesWarriorsMinter.sol:220:        for (uint256 i = 0; i < numWarriors; i++) {
ForgottenRunesWarriorsMinter.sol:248:        numClaimed += 1;
ForgottenRunesWarriorsMinter.sol:259:        for (uint256 i = 0; i < count; i++) {
ForgottenRunesWarriorsMinter.sol:355:        for (uint256 i = startIdx; i < endIdx + 1; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

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++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

Public functions to external

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

initialize(address) should be declared external:
 - ForgottenRunesWarriorsGuild.initialize(address) (contracts/ForgottenRunesWarriorsGuild.sol#52-54)
exists(uint256) should be declared external:
 - ForgottenRunesWarriorsGuild.exists(uint256) (contracts/ForgottenRunesWarriorsGuild.sol#85-87)
setProvenanceHash(string) should be declared external:
 - ForgottenRunesWarriorsGuild.setProvenanceHash(string) (contracts/ForgottenRunesWarriorsGuild.sol#145-147)
withdrawAll() should be declared external:
 - ForgottenRunesWarriorsGuild.withdrawAll() (contracts/ForgottenRunesWarriorsGuild.sol#163-165)
forwardERC20s(IERC20,uint256) should be declared external:
 - ForgottenRunesWarriorsGuild.forwardERC20s(IERC20,uint256) (contracts/ForgottenRunesWarriorsGuild.sol#173-176)
numDaMinters() should be declared external:
 - ForgottenRunesWarriorsMinter.numDaMinters() (contracts/ForgottenRunesWarriorsMinter.sol#337-339)
issueRefunds(uint256,uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.issueRefunds(uint256,uint256) (contracts/ForgottenRunesWarriorsMinter.sol#350-358)
refundAddress(address) should be declared external:
 - ForgottenRunesWarriorsMinter.refundAddress(address) (contracts/ForgottenRunesWarriorsMinter.sol#364-366)
selfRefund() should be declared external:
 - ForgottenRunesWarriorsMinter.selfRefund() (contracts/ForgottenRunesWarriorsMinter.sol#371-374)
pause() should be declared external:
 - ForgottenRunesWarriorsMinter.pause() (contracts/ForgottenRunesWarriorsMinter.sol#427-429)
unpause() should be declared external:
 - ForgottenRunesWarriorsMinter.unpause() (contracts/ForgottenRunesWarriorsMinter.sol#434-436)
setSelfRefundsStartTime(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setSelfRefundsStartTime(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#469-471)
setPhaseTimes(uint256,uint256,uint256,uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setPhaseTimes(uint256,uint256,uint256,uint256) (contracts/ForgottenRunesWarriorsMinter.sol#480-500)
setMintlist1MerkleRoot(bytes32) should be declared external:
 - ForgottenRunesWarriorsMinter.setMintlist1MerkleRoot(bytes32) (contracts/ForgottenRunesWarriorsMinter.sol#505-507)
setMintlist2MerkleRoot(bytes32) should be declared external:
 - ForgottenRunesWarriorsMinter.setMintlist2MerkleRoot(bytes32) (contracts/ForgottenRunesWarriorsMinter.sol#513-515)
setClaimlistMerkleRoot(bytes32) should be declared external:
 - ForgottenRunesWarriorsMinter.setClaimlistMerkleRoot(bytes32) (contracts/ForgottenRunesWarriorsMinter.sol#520-522)
setStartPrice(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setStartPrice(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#550-552)
setLowestPrice(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setLowestPrice(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#557-559)
setDaPriceCurveLength(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setDaPriceCurveLength(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#564-566)
setDaDropInterval(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setDaDropInterval(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#571-573)
setFinalPrice(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setFinalPrice(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#579-581)
setMaxDaSupply(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setMaxDaSupply(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#586-588)
setMaxForSale(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setMaxForSale(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#593-595)
setMaxForClaim(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.setMaxForClaim(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#600-602)
withdraw(uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.withdraw(uint256) (contracts/ForgottenRunesWarriorsMinter.sol#608-611)
withdrawAll() should be declared external:
 - ForgottenRunesWarriorsMinter.withdrawAll() (contracts/ForgottenRunesWarriorsMinter.sol#616-619)
forwardERC20s(IERC20,uint256) should be declared external:
 - ForgottenRunesWarriorsMinter.forwardERC20s(IERC20,uint256) (contracts/ForgottenRunesWarriorsMinter.sol#627-630)

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

ForgottenRunesWarriorsGuild.sol:24:    uint256 public numMinted = 0;
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++) {

I suggest removing explicit initializations for default values.

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

ForgottenRunesWarriorsGuild.sol:1:pragma solidity ^0.8.0;
ForgottenRunesWarriorsMinter.sol:1:pragma solidity ^0.8.0;

Use msg.sender instead of OpenZeppelin's _msgSender() when meta-transactions capabilities aren't used

msg.sender costs 2 gas (CALLER opcode). _msgSender() represents the following:

function _msgSender() internal view virtual returns (address payable) {
  return msg.sender;
}

When no meta-transactions capabilities are used: msg.sender is enough.

See https://docs.openzeppelin.com/contracts/2.x/gsn for more information about GSN capabilities.

Consider replacing _msgSender() with msg.sender here:

ForgottenRunesWarriorsGuild.sol:101:        require(_msgSender() == minter, 'Not a minter');
ForgottenRunesWarriorsGuild.sol:115:            _isApprovedOrOwner(_msgSender(), tokenId),

In the solution, msg.sender is used everywhere else:

ForgottenRunesWarriorsGuild.sol:164:        require(payable(msg.sender).send(address(this).balance));
ForgottenRunesWarriorsGuild.sol:174:        require(address(msg.sender) != address(0));
ForgottenRunesWarriorsGuild.sol:175:        token.transfer(msg.sender, amount);
ForgottenRunesWarriorsMinter.sol:113:        setVaultAddress(msg.sender);
ForgottenRunesWarriorsMinter.sol:151:        daMinters.push(msg.sender);
ForgottenRunesWarriorsMinter.sol:152:        daAmountPaid[msg.sender] += msg.value;
ForgottenRunesWarriorsMinter.sol:153:        daNumMinted[msg.sender] += numWarriors;
ForgottenRunesWarriorsMinter.sol:163:            _mint(msg.sender);
ForgottenRunesWarriorsMinter.sol:182:        require(mintlistMinted[msg.sender] == false, 'Already minted');
ForgottenRunesWarriorsMinter.sol:183:        mintlistMinted[msg.sender] = true;
ForgottenRunesWarriorsMinter.sol:186:        bytes32 node = keccak256(abi.encodePacked(msg.sender));
ForgottenRunesWarriorsMinter.sol:194:        _mint(msg.sender);
ForgottenRunesWarriorsMinter.sol:221:            _mint(msg.sender);
ForgottenRunesWarriorsMinter.sol:238:        require(claimlistMinted[msg.sender] == false, 'Already claimed');
ForgottenRunesWarriorsMinter.sol:239:        claimlistMinted[msg.sender] = true;
ForgottenRunesWarriorsMinter.sol:242:        bytes32 node = keccak256(abi.encodePacked(msg.sender));
ForgottenRunesWarriorsMinter.sol:249:        _mint(msg.sender);
ForgottenRunesWarriorsMinter.sol:373:        _refundAddress(msg.sender);
ForgottenRunesWarriorsMinter.sol:628:        require(address(msg.sender) != address(0));
ForgottenRunesWarriorsMinter.sol:629:        token.transfer(msg.sender, amount);

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

ForgottenRunesWarriorsGuild.sol:68:        require(
ForgottenRunesWarriorsGuild.sol:100:        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
ForgottenRunesWarriorsGuild.sol:101:        require(_msgSender() == minter, 'Not a minter');
ForgottenRunesWarriorsGuild.sol:114:        require(
ForgottenRunesWarriorsGuild.sol:164:        require(payable(msg.sender).send(address(this).balance));
ForgottenRunesWarriorsGuild.sol:174:        require(address(msg.sender) != address(0));
ForgottenRunesWarriorsMinter.sol:136:        require(numSold < maxDaSupply, 'Auction sold out');
ForgottenRunesWarriorsMinter.sol:137:        require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
ForgottenRunesWarriorsMinter.sol:138:        require(daStarted(), 'Auction not started');
ForgottenRunesWarriorsMinter.sol:139:        require(!mintlistStarted(), 'Auction phase over');
ForgottenRunesWarriorsMinter.sol:140:        require(
ForgottenRunesWarriorsMinter.sol:146:        require(
ForgottenRunesWarriorsMinter.sol:177:        require(numSold < maxForSale, 'Sold out');
ForgottenRunesWarriorsMinter.sol:178:        require(mintlistStarted(), 'Mintlist phase not started');
ForgottenRunesWarriorsMinter.sol:179:        require(msg.value == finalPrice, 'Ether value incorrect');
ForgottenRunesWarriorsMinter.sol:182:        require(mintlistMinted[msg.sender] == false, 'Already minted');
ForgottenRunesWarriorsMinter.sol:187:        require(
ForgottenRunesWarriorsMinter.sol:207:        require(numSold < maxForSale, 'Sold out');
ForgottenRunesWarriorsMinter.sol:208:        require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
ForgottenRunesWarriorsMinter.sol:209:        require(publicStarted(), 'Public sale not started');
ForgottenRunesWarriorsMinter.sol:210:        require(
ForgottenRunesWarriorsMinter.sol:214:        require(
ForgottenRunesWarriorsMinter.sol:234:        require(numClaimed < maxForClaim, 'No more claims');
ForgottenRunesWarriorsMinter.sol:235:        require(claimsStarted(), 'Claim phase not started');
ForgottenRunesWarriorsMinter.sol:238:        require(claimlistMinted[msg.sender] == false, 'Already claimed');
ForgottenRunesWarriorsMinter.sol:243:        require(
ForgottenRunesWarriorsMinter.sol:258:        require(address(recipient) != address(0), 'address req');
ForgottenRunesWarriorsMinter.sol:372:        require(selfRefundsStarted(), 'Self refund period not started');
ForgottenRunesWarriorsMinter.sol:488:        require(
ForgottenRunesWarriorsMinter.sol:492:        require(
ForgottenRunesWarriorsMinter.sol:609:        require(address(vault) != address(0), 'no vault');
ForgottenRunesWarriorsMinter.sol:610:        require(payable(vault).send(_amount));
ForgottenRunesWarriorsMinter.sol:617:        require(address(vault) != address(0), 'no vault');
ForgottenRunesWarriorsMinter.sol:618:        require(payable(vault).send(address(this).balance));
ForgottenRunesWarriorsMinter.sol:628:        require(address(msg.sender) != address(0));

I suggest replacing revert strings with custom errors.