Forgotten Runes Warrior Guild contest - hake's results

Lines of code

ForgottenRunesWarriorsMinter.sol#L480

Vulnerability details

Impact

If publicStartTime or mintlistStartTime happen to overlap with daStartTime all users will be able to buy tokens concurrently.

In case of high demand leaving a big overlap between mintlistStartTime, publicStartTime and daStartTime may generate high gas prices and sell out before some members in the mint list have a chance to purchase their mint and defeating the puporse of having different phases.

Proof of Concept

The functions to set StartTime#L441-L464 have no checks that prevent them to all run or start at the same time.

And the checks in setPhaseTimes also allow total overlap.

require(
    newPublicStartTime >= newMintlistStartTime,
    'Set public after mintlist');
require(
    newClaimsStartTime >= newPublicStartTime,
    'Set claims after public');

newPublicStartTime can be set equal to newMintlistStartTime, and this may lead to a sell out before intended in case of high demand.

A worse scenario would be mistakenly starting publicStartTime or/and mintlistStartTime at the same time as daStartTime.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend setting a minimum time window between daStartime and mintlistStartTime so mistakenly setting mintlistStartTime to a time too early is not a possibility. Same goes for mintlistStartTime in regards for publicStartTime and so on.

Is always better to prevent mistakes from happening than having to deal with them on the spot.

LOW

L-01: No zero address check

setWarriorsAddress

setWethAddress

setVaultAddress

Implementing zero address checks could prevent from having to spend extra gas to call these functions again.

L-02: initialize is redundant and can be called any number of times

ForgottenRunesWarriorsGuild.sol#L52

Function name initialize suggests it should only be able to be called once, which is not the case. It is also redundant since all it does is call setMinter.

L-03: forwardERC20s could be exploited by ERC777 or malicious ERC20

ForgottenRunesWarriorsGuild.sol#L173

Attacker could deposit malicious ERC20 or ERC777 token that can take control of the execution flow and potentially do something harmful.

L-04: No zero addres checks in refundAddress could lead to spending gas for nothing.

ForgottenRunesWarriorsMinter.sol#364

If owner accidentally calls refundAddress with address zero no funds will be burned but gas will be spent for no reason.

I suggest implement a zero address check to avoid wasting gas.

Example:

function refundAddress(address minter) public onlyOwner nonReentrant {
    require(minter != address(0));
    _refundAddress(minter);
}

L-04: Floating pragma

Using a floating pragma (^) might result in the contract being deployed with a version it was not tested with and might result in bugs that affect the contract system negatively. In addition, older compilers might be susceptible to some bugs. A list of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

Locking the pragma (deleting the ^) helps to ensure that contracts do not accidentally get deployed using an outdated compiler version or a version it was not tested with. I recommend changing the solidity version pragma to the latest version to enforce the use of an up-to-date compiler.

NON-CRITICAL

N-01: Many functions could be set to external for better code clarity.

The following has been copied from slither:

ForgottenRunesWarriorsGuild.initialize(address) (contracts/ForgottenRunesWarriorsGuild.sol#52-54) exists(uint256) should be declared external:

• ForgottenRunesWarriorsGuild.exists(uint256) (contracts/ForgottenRunesWarriorsGuild.sol#85-87) mint(address) should be declared external:
• ForgottenRunesWarriorsGuild.mint(address) (contracts/ForgottenRunesWarriorsGuild.sol#94-106) burn(uint256) should be declared external:
• ForgottenRunesWarriorsGuild.burn(uint256) (contracts/ForgottenRunesWarriorsGuild.sol#113-119) 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)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external

GAS

G-01: for loop gas optimization

ForgottenRunesWarriorsMinter.sol#L162 ForgottenRunesWarriorsMinter.sol#L220 ForgottenRunesWarriorsMinter.sol#L259 ForgottenRunesWarriorsMinter.sol#L355

Unnecessary to update uint to default value of zero. Prefix instead of postfix increments and unchecked increments can save gas.

Taking all changes into account I recommend change a loop like this:

for (uint256 i = 0; i < numWarriors; i++) {
    _mint(msg.sender);
}

To this:

for (uint256 i; i < numWarriors;) {
    _mint(msg.sender);
    unchecked { ++i; }
}

G-02: Parameters order could save gas on mint fail

ForgottenRunesWarriorsMinter.sol#L163

Parameters daMinters, daAmountPaid, daNumMinted and numSold are not necessary to _mint. Therefore gas could be saved in case _mint reverts if _mint was set before such parameters. Check example below.

for (uint256 i = 0; i < numWarriors; i++) {
    _mint(msg.sender);
}

daMinters.push(msg.sender);
daAmountPaid[msg.sender] += msg.value;
daNumMinted[msg.sender] += numWarriors;
numSold += numWarriors;

if (numSold == maxDaSupply) {
    // optimistic: save gas by not setting on every mint, but will
    // require manual `setFinalPrice` before refunds if da falls short
    finalPrice = currentPrice;
}

G-03: No need to use _msgSender

ForgottenRunesWarriorsGuild.sol#L101

The use of _msgSender when there is no implementation of a meta transaction mechanism that uses it, such as EIP-2771, very slightly increases gas consumption.

I suggest replacing _msgSender with msg.sender if there is no mechanism to support meta-transactions like EIP-2771 implemented.

G-04: Gas can be saved by shortening syntax

ForgottenRunesWarriorsMinter.sol#L182

Gas could be saved by changing:

require(mintlistMinted[msg.sender] == false, 'Already minted');

To:

require(!mintlistMinted[msg.sender], 'Already minted');

G-05: Lore R contains very large string

ForgottenRunesWarriorsGuild.sol#L33

Having in mind that this might be unnegotiable I suggest shortening Lore R to less than 32 bytes to save gas.