Forgotten Runes Warrior Guild contest - robee's results

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

ETH send return value is ignored while is gas limited

The use of send() / call() to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender or feeRecipient is a smart contract. Funds can potentially be lost if; 1. The smart contract fails to implement the payable fallback function 2. The fallback function uses more than 2300 gas units Different from .transfer(...), .send(...) and call(...) doesn't revert when it fails, and therefore its retrun value is extremely important. The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the transaction. A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now This is not just a best-practice advice since the return value isn't considered!!! If you would consider it then it was just a best practice.

Require with empty message

The following requires are with empty messages. This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: ForgottenRunesWarriorsMinter.sol, In line 610 with Empty Require message.
    Solidity file: ForgottenRunesWarriorsMinter.sol, In line 628 with Empty Require message.
    Solidity file: ForgottenRunesWarriorsGuild.sol, In line 164 with Empty Require message.
    Solidity file: ForgottenRunesWarriorsGuild.sol, In line 174 with Empty Require message.
    Solidity file: ForgottenRunesWarriorsMinter.sol, In line 618 with Empty Require message.

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    ForgottenRunesWarriorsMinter.sol.setVaultAddress _newVaultAddress
    ForgottenRunesWarriorsGuild.sol.setMinter newMinter
    ForgottenRunesWarriorsGuild.sol.mint recipient
    ForgottenRunesWarriorsMinter.sol.teamSummon recipient
    ForgottenRunesWarriorsGuild.sol.initialize newMinter

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Code instance:

Init function calls an owner function

    Init function that calls an onlyOwner function is problematic since sometimes the initializer or the one applies 
    the constructor isn't necessary the owner of the protocol. And if a contract does it then you might get a situation
    that all the onlyOwner functions are blocked since only the factory contract may use them but isn't necessary 
    support it. 
    
    

Code instances:

    ForgottenRunesWarriorsMinter.sol.constructor - calls setWarriorsAddress
    ForgottenRunesWarriorsMinter.sol.constructor - calls setVaultAddress
    ForgottenRunesWarriorsGuild.sol.constructor - calls setBaseURI
    ForgottenRunesWarriorsMinter.sol.constructor - calls setWethAddress

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    ForgottenRunesWarriorsMinter.sol: function _safeTransferETH parameter to isn't used. (_safeTransferETH is internal)
    ForgottenRunesWarriorsMinter.sol: function _safeTransferETH parameter value isn't used. (_safeTransferETH is internal)

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsMinter.sol#L400
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsMinter.sol#L629
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsGuild.sol#L175

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    ForgottenRunesWarriorsMinter.sol: function _safeTransferETH parameter to isn't used. (_safeTransferETH is internal)
    ForgottenRunesWarriorsMinter.sol: function _safeTransferETH parameter value isn't used. (_safeTransferETH is internal)

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsMinter.sol#L469
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsGuild.sol#L137
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsMinter.sol#L586
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsMinter.sol#L593
    https://github.com/code-423n4/2022-05-runes/tree/main/contracts/ForgottenRunesWarriorsGuild.sol#L129

ETH send return value is ignored while is gas limited

The use of send() / call() to send ETH may have unintended outcomes on the eth being sent to the receiver. Eth may be irretrievable or undelivered if the msg.sender or feeRecipient is a smart contract. Funds can potentially be lost if; 1. The smart contract fails to implement the payable fallback function 2. The fallback function uses more than 2300 gas units Different from .transfer(...), .send(...) and call(...) doesn't revert when it fails, and therefore its retrun value is extremely important. The latter situation may occur in the instance of gas cost changes. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the transaction. A detailed explanation of why relying on payable().transfer() may result in unexpected loss of eth can be found here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now This is not just a best-practice advice since the return value isn't considered!!! If you would consider it then it was just a best practice.

Code instances:

ForgottenRunesWarriorsGuild.sol, 163, require(payable(msg.sender).send(address(this).balance));

ForgottenRunesWarriorsMinter.sol, 617, require(payable(vault).send(address(this).balance));

ForgottenRunesWarriorsMinter.sol, 609, require(payable(vault).send(_amount));

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):

Code instances:

    ForgottenRunesWarriorsGuild.sol, 175 (forwardERC20s), token.transfer(msg.sender, amount);
    ForgottenRunesWarriorsMinter.sol, 629 (forwardERC20s), token.transfer(msg.sender, amount);

Unnecessary equals boolean

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

Code instances:

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

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    change to prefix increment and unchecked: ForgottenRunesWarriorsMinter.sol, i, 162
    change to prefix increment and unchecked: ForgottenRunesWarriorsMinter.sol, i, 259
    change to prefix increment and unchecked: ForgottenRunesWarriorsMinter.sol, i, 220
    change to prefix increment and unchecked: ForgottenRunesWarriorsMinter.sol, i, 355

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    ForgottenRunesWarriorsMinter.sol, 220
    ForgottenRunesWarriorsMinter.sol, 162
    ForgottenRunesWarriorsMinter.sol, 259

Storage double reading. Could save SLOAD

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

Code instances:

    ForgottenRunesWarriorsMinter.sol: maxDaSupply is read twice in bidSummon
    ForgottenRunesWarriorsMinter.sol: maxForSale is read twice in publicSummon

Unnecessary default assignment

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

Code instances:

    ForgottenRunesWarriorsMinter.sol (L#60) : uint256 public lowestPrice = 0.6 ether; 
    ForgottenRunesWarriorsGuild.sol (L#24) : uint256 public numMinted = 0; 

Use bytes32 instead of string to save gas whenever possible

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.

Code instance:

    ForgottenRunesWarriorsGuild.sol (L32), string public constant R = "Old men forget: yet all shall be forgot, But he'll remember with advantages What feats he did that day: then shall our names Familiar in his mouth as household words. This story shall the good man teach his son From this day to the ending of the world"; 

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instance:

    Solidity file: ForgottenRunesWarriorsMinter.sol, In line 146, Require message length to shorten: 34, The message: Ether value sent is not sufficient

Unnecessary Reentrancy Guards

Where there is onlyOwner or Initializer modifer, the reentrancy gaurd isn't necessary (unless you don't trust the owner or the deployer, which will lead to full security breakdown of the project and we believe this is not the case) This is a list we found of such occurrences:

Code instances:

    ForgottenRunesWarriorsMinter.sol no need both nonReentrant and onlyOwner modifiers in issueRefunds
    ForgottenRunesWarriorsMinter.sol no need both nonReentrant and onlyOwner modifiers in refundAddress

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    ForgottenRunesWarriorsMinter.sol, 211: change 'numWarriors > 0' to 'numWarriors != 0'
    ForgottenRunesWarriorsMinter.sol, 141: change 'numWarriors > 0' to 'numWarriors != 0'

Unnecessary cast

Code instance:

    address ForgottenRunesWarriorsMinter.sol.teamSummon - unnecessary casting address(recipient)

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instance:

    ForgottenRunesWarriorsMinter.sol (L#279) - if (block.timestamp >= daStartTime + daPriceCurveLength) { 

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    ForgottenRunesWarriorsMinter.sol, _safeTransferETH, { return success; }
    ForgottenRunesWarriorsGuild.sol, _baseURI, { return baseTokenURI; }