Forgotten Runes Warrior Guild contest - hubble's results

Summary of Findings for Low / Non-Critical issues

• L-01 : Event messages missing for critical functions

• L-02 : Owner can set the minter to any new address anytime

• L-03 : Missing input validation in set MerkleRoot functions

• L-04 : Null check in constructor parameters missing

• L-05 : Null check in all the set values for price missing

• L-06 : Input validation missing while setting max values for maxDaSupply, maxForSale & maxForClaim

• L-07 : Missing input validation in function setPhaseTimes

• NC-01 : require statement always true in function forwardERC20s()

• NC-02 : Redundant require statement in bidSummon and publicSummon

• NC-03 : withdrawAll does not need payable keyword

Details L-01

Title : Event messages missing for critical functions

There are no events generated in any of the functions. Some critical functions may need some events to be emitted.

Impact

Tracking of any protocol changes is not possible, and transparency missing.

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : setFinalPrice(), setPhaseTimes(), etc.,

Recommended Mitigation Steps

Add relevant event messages to critical functions required for tracking, or transparency

Details L-02

Title : Owner can set the minter to any new address anytime.

The initialize function is not protected by any modifier like initializer, so its possible to call this function multiple times, and setting the minter address to different values.

Impact

Setting the new minter to a new address may cause duplicate or redundant issuance of NFTs.

Proof of Concept

Contract : ForgottenRunesWarriorsGuild.sol
Function : initialize and setMinter functions

Recommended Mitigation Steps

Protect the initialize function by an initializer modifier from Openzeppelin, and set the minter state variable in the initialize function. Along with this remove the setMinter function.

Details L-03

Title : Missing input validation in set MerkleRoot functions

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : setMintlist1MerkleRoot(), setMintlist2MerkleRoot() & setClaimlistMerkleRoot()

Recommended Mitigation Steps

Add below validation in the above set functions require(newMerkleRoot != bytes32(0), 'Not valid root');

Details L-04

Title : Null check in constructor parameters missing

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol
Function : constructor()

    constructor(IForgottenRunesWarriorsGuild _warriors, address _weth) {
        setWarriorsAddress(_warriors);
        setWethAddress(_weth);
        setVaultAddress(msg.sender);
    }

Recommended Mitigation Steps

Add null check for the _warriors and _weth address parameters. Alternately add null check in respective functions called in the constructor

Details L-05

Title : Null check in all the set values for price missing

Wrongly setting the price value to 0 may cause loss of value of the NFT minted.

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol
Functions : setFinalPrice(), setLowestPrice() & setStartPrice()

Recommended Mitigation Steps

Add require statement to each of the set functions with appropriate min values.

Details L-06

Title : Input validation missing while setting max values for maxDaSupply, maxForSale & maxForClaim

Impact

Wrongly setting the values can cause minting of more number of NFTs than planned

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : maxDaSupply, maxForSale & maxForClaim

Recommended Mitigation Steps

Define a new max value and add appropriate require statements as input validation in each of the functions

   uint256 public maxTotal = 8000+6190+1100;
   require(_newSupply+maxForSale+maxForClaim <= maxTotal, "Minting more than permissible Supply")

Details L-07

Title : Missing input validation in function setPhaseTimes

There is a possibility of setting the phase times wrongly.

Impact

Mixing up the phase times may cause problems for minting for various groups/entities.

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : setPhaseTimes()

Recommended Mitigation Steps

Check for null values and all values should be in future timestamp Also add check for newDaStartTime < newMintlistStartTime < newPublicStartTime < newClaimsStartTime

Details NC-01

Title : require statement always true in function forwardERC20s()

Proof of Concept

Contract : ForgottenRunesWarriorsGuild.sol
Function : forwardERC20s

below line in the function is always true, so unnecessary line 173 , require(address(msg.sender) != address(0));

Recommended Mitigation Steps

Remove the above mentioned require statement

Details NC-02

Title : Redundant require statement in bidSummon and publicSummon

The line 136 require statement below is made redundant due to line 137 require statement. The line 207 require statement below is made redundant due to line 208 require statement.

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : function bidSummon()

line 136 require(numSold < maxDaSupply, 'Auction sold out'); line 137 require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');

Function : function publicSummon()

line 207 require(numSold < maxForSale, 'Sold out'); line 208 require(numSold + numWarriors <= maxForSale, 'Not enough remaining');

Recommended Mitigation Steps

remove the redundant require statements in both the functions

Details NC-03

Title : withdrawAll does not need payable keyword

Proof of Concept

Contract : ForgottenRunesWarriorsMinter.sol Function : line 616 function withdrawAll() public payable onlyOwner { ... }

Recommended Mitigation Steps

remove the payable keyword