Forgotten Runes Warrior Guild contest - p4st13r4's results

Judge has assessed an item in Issue #143 as Medium risk. The relevant finding follows:

[L-04] _safeTransferETH() should perform simple ETH transfers and don’t forward 30k gas

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L412

Being a simple funds transfer, having a fallback of a WETH deposit, there should be no extra gas involved when potentially transferring the control to a smart contract. We suggest replacing the call and its 30_000 gas limit with a send() or transfer() call, to prevent possible reentrancy by malicious actors

Low Risk Issues

[L-01] mintlistSummon() can be called after mintlist phase has ended

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L171

Currently, phase 2 is enforced only with this check:

require(mintlistStarted(), 'Mintlist phase not started');

However, there is no check that ensures that the next phase is not started. We suggest adding the following check:

require(!publicStarted(), 'Mintlist phase over');

[L-02] publicSummon() can be called after public phase has ended

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L201

Currently, phase 33 is enforced only with this check:

require(publicStarted(), 'Public sale not started');

However, there is no check that ensures that the next phase is not started. We suggest adding the following check:

require(!claimsStarted(), 'Public sale over');

[L-03] selfRefund() should not be called when contract is paused

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L371

All the functions regarding the NFT sale through all the phases contain the whenNotPaused modifier, so if any issue should arise, the contract owner can safely pause the core functions of the contract. For the same reasons, since refunds should happen between phase 1 and 2 and can be performed by users autonomously, we suggest adding whenNotPaused here as well

[L-04] _safeTransferETH() should perform simple ETH transfers and don’t forward 30k gas

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L412

Being a simple funds transfer, having a fallback of a WETH deposit, there should be no extra gas involved when potentially transferring the control to a smart contract. We suggest replacing the call and its 30_000 gas limit with a send() or transfer() call, to prevent possible reentrancy by malicious actors

Non-critical Issues

[N-01] Useless initialize() function

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

contracts/ForgottenRunesWarriorsGuild.sol contains an initialize function which is not useful at all because it is just a proxy for setMinter and it is deceiving because it does not have a modifier that prevents it from being called again. We suggest removing it and just call setMinter manually after contract deploy

[N-02] withdrawAll() has a useless payable modifier

Link: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L616

The function does not deal with msg.value because it’s not supposed to receive any ETH. We suggest removing the payable modifier, and possibly replace its implementation with a simpler one:

function withdrawAll() public payable onlyOwner {
    withdraw(address(this).balance);
}

File contracts/ForgottenRunesWarriorsGuild.sol

[G-01] Unnecessary zero-address check

function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
    // @audit unnecessary check
    require(address(msg.sender) != address(0));
    token.transfer(msg.sender, amount);
}

msg.sender will never be the zero address, so this is probably the result of some copy-paste and can be safely removed

File contracts/ForgottenRunesWarriorsMinter.sol

[G-02] Long revert strings

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas if/when the revert condition is met. Example of long revert strings: You can summon no more than 20 Warriors at a time

We suggest using shorter errors or switch to solidity custom errors

[G-03] Avoid reading the same variables from storage multiple times

In bidSummon() the variables numSold and maxDaSupply are each read from storage 3 times. The code can be optimized by minimizing the number of SLOADs: SLOADs are expensive (100 gas) compared to MLOADs (3 gas).

[G-04] Remove reentrancy check from issueRefunds() and refundAddress()

These two functions can only be called by a privileged account since they got the onlyOwner. In order to save has during refunds phase we suggest removing this check, given it performs three reads from storage

[G-05] Unnecessary zero-address check

function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
    // @audit unnecessary check
    require(address(msg.sender) != address(0));
    token.transfer(msg.sender, amount);
}

msg.sender will never be the zero address, so this is probably the result of some copy-paste and can be safely removed