Forgotten Runes Warrior Guild contest - ilan's results

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

L03 Call{value:amt}("") should be used instead of send()

In ForgottenRunesWarriorsGuild.withdrawAll.

transfer() and send() should be avoided because they take a hard dependency on gas costs by forwarding a fixed amount of gas: 2300. Gas costs of opcodes can change thus the fallback function can change to consume more than 2300 gas, hence transfer() and send() would fail.

Recommended Mitigation Steps

(bool success, ) = payable(msg.sender).call{value:address(this).balance)}("");
require(success, "Transfer failed");

NC01 Missing error message for some require statements

Some require statements in this contract are missing an error message. One should be added.

ForgottenRunesWarriorsMinter.forwardERC20s ForgottenRunesWarriorsMinter.withdraw ForgottenRunesWarriorsMinter.withdrawAll

NC02 Boolean constants can be used directly and do not need to be compare to true or false

instead of checking mintlistMinted[msg.sender] == false, check !mintlistMinted[msg.sender] in mintlistSummon claimSummon

NCO3 Unused function

ForgottenRunesWarriorsGuild._baseURI (contracts/ForgottenRunesWarriorsGuild.sol#76-78) is never used and should be removed

L01 xStarted() functions should have inclusive comparison

the phase time should include the start time (also inclusive comparison saves gas)

ForgottenRunesWarriorsMinter.daStarted() ForgottenRunesWarriorsMinter.mintlistStarted() ForgottenRunesWarriorsMinter.publicStarted() ForgottenRunesWarriorsMinter.claimsStarted() ForgottenRunesWarriorsMinter.selfRefundsStarted() ForgottenRunesWarriorsGuild.withdrawAll() ForgottenRunesWarriorsGuild.forwardERC20s()

For example: block.timestamp > daStartTime -> block.timestamp >= daStartTime

L02 Unchecked return value of token.transfer(msg.sender, amount)

In forwardERC20s:

require(address(msg.sender) != address(0));
token.transfer(msg.sender, amount);

although there is no serious vulnerability here, the return value should be checked by wrapping the transfer with require so the transaction will revert if the token transfer was not successful.

Recommended Mitigation Steps

require(token.transfer(msg.sender, amount), "transfer failed")

L03 Call{value:amt}("") should be used instead of send()

In ForgottenRunesWarriorsGuild.withdrawAll.

transfer() and send() should be avoided because they take a hard dependency on gas costs by forwarding a fixed amount of gas: 2300. Gas costs of opcodes can change thus the fallback function can change to consume more than 2300 gas, hence transfer() and send() would fail.

Recommended Mitigation Steps

(bool success, ) = payable(msg.sender).call{value:address(this).balance)}("");
require(success, "Transfer failed");

G01 Change i++ to ++i

i++ is generally more expensive because it must increment a value and return the old value, so it may require holding two numbers in memory. ++i only uses one number in memory.

Example:

for (uint256 i = startIdx; i < endIdx + 1; i++) -> for (uint256 i = startIdx; i < endIdx + 1; ++i)

example in code

G02 No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to 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.

Example:

uint256 i = 0 -> uint256 i

example in code

G03 functions should be decalred external

public functions that are never called by the contract should be declared external to save gas.

for example ForgottenRunesWarriorsGuild.tokenURI