Forgotten Runes Warrior Guild contest - kebabsec's results

1. Using transfer instead of safeTransfer (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L629 and https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L402) could result in a loss of funds in the case of failed transfers, due to transfer not having the require statement to ensure the transfer went through properly. We suggest to either implement and use safeTransfer, or add those checks necessary to ensure everything is handled correctly, specially since one of the functions exists to be able to recover tokens in the case of someone accidentally transfering an ERC20 token to the contract.

2. Unused Variable In (https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L32) a string is saved, we assume that this is for some frontend usage, but creating variables is very expensive and therefore we suggest it’s removed and stored in the frontend, if that’s the intention behind it.

3. Unprotected initialize function The function called initialize (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L52) does not have have checks to prevent it from being called more than once. Even though this is protected by the onlyOwner modifier, we suggest that a library, like openzeppelin’s Initalizable.sol (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L52) is used, specially because this function is redundant due to the fact that the function setMinter (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L137) is public and can be called directly.

4. Unnecessary require check In the function to recover ERC20 tokens, forwardERC20s there’s a require check (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L628), supposedly preventing the caller to be the 0 address. Since the function is protected by the onlyOwner modifie and the owner can’t be the 0 address, as it would be impossible to call this function regardless, this is not needed and you can safely remove that check.

5. Typos In (https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L476) “mintlst” should be “mintlist”.

1. Gas

Pre increments are less expensive than post increments (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#L162, https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L355)