Forgotten Runes Warrior Guild contest - rotcivegaf's results

QA reports (low/non-critical)

Contests: Forgotten Runes Warrior Guild

Autor: Rotcivegaf

Scope:

Interfaces:

• IForgottenRunesWarriorsGuild.sol
• IForgottenRunesWarriorsMinter.sol
• IWETH.sol

Contracts:

• ForgottenRunesWarriorsGuild.sol
• ForgottenRunesWarriorsMinter.sol

Non-critical

NC-01) Unused import

In ForgottenRunesWarriorsMinter.sol, the import '@openzeppelin/contracts/token/ERC721/ERC721.sol'; is unused

NC-02) address vault to address payable address

The variable address vault in ForgottenRunesWarriorsMinter.sol can be payable This variable is used to send ETH and this also avoid the cast to address payable

• L51: address public vault; to address payable public vault;
• L113: setVaultAddress(msg.sender); to setVaultAddress(payable(msg.sender));
• L257: function setVaultAddress(address _newVaultAddress) public onlyOwner { to function setVaultAddress(address payable _newVaultAddress) public onlyOwner {
• L610: require(payable(vault).send(_amount)); to require(vault.send(_amount));
• L618: require(payable(vault).send(address(this).balance)); to require(vault.send(address(this).balance));

NC-03) address public weth; to IWETH public weth;

In ForgottenRunesWarriorsMinter.sol L54, the weth variable can be IWETH and IWETH interface can inherit from IERC20 interface This avoid cast from address to IWETH

NC-04) Typo

In ForgottenRunesWarriorsGuild.sol L58, * @param tokenId uint256 ID of the token to be minted to * @param tokenId uint256 ID of the token minted

NC-05) Missing commenting

The following variables and functions are missing commenting as describe below: In ForgottenRunesWarriorsGuild.sol:

• L32: string public constant R
• L76: function _baseURI()

In ForgottenRunesWarriorsMinter.sol:

• L376: function _refundAddress(address)
• L264: function _mint(address)
• L275: The function currentDaPrice() should have more documentation of equations and the curve

NC-06) The summon limit its easy to break

The limit of 20 warriors(ERC721 token) at a time in L130 bidSummon(uint256) and L201 publicSummon(uint256) its easy to break, a contract could be used to mint more than 20 at a time

NC-07) The R contract variable

Improve the name of variable R in ForgottenRunesWarriorsGuild.sol L32 In the contest video said "put a little bit of lore within all of all our contracts" and the minter ForgottenRunesWarriorsMinter.sol dont have this variable Recommend remove this variable to clean the code and spend less gas, can put this information in a comment

NC-08) The METADATA_PROVENANCE_HASH contract variable

In ForgottenRunesWarriorsGuild.sol L34, can storage this information in tokenURI variable and baseTokenURI function, also can remove the L145 setProvenanceHash function

NC-09) Set vars in the constructor

Recommend set daStartTime, mintlistStartTime, publicStartTime, claimsStartTime, selfRefundsStartTime variables in the construtor with real values, and remove the initialize on the contract

NC-10) Setter function for start and lowest price

Recommend a function to set start/lowest price with a startPrice > lowestPrice require

function setStartLowestPrices(uint256 _startPrice, uint256 _lowestPrice) external onlyOwner {
    require(_startPrice > _lowestPrice, "The start price lower than lowest price");
    startPrice = _startPrice;
    lowestPrice = _lowestPrice;
}

NC-11) Use _ to the start of the parameter of the functions

Like in the setPhaseTimes:

function setPhaseTimes(
    uint256 _newDaStartTime,
    uint256 _newSelfRefundsStartTime,
    uint256 _newMintlistStartTime,
    uint256 _newPublicStartTime,
    uint256 _newClaimsStartTime
) public onlyOwner {

This avoids confusion with contract variables

NC-12) Group in struct the contract variables

Group the contract variables in a struct, in example the times variables

struct Time {
  uint32 daStart;
  uint32 mintlistStart;
  uint32 publicStart;
  uint32 claimsStart;
  uint32 selfRefundsStart;

  uint32 daPriceCurveLength;
  uint32 daDropInterval;
}

Time public timeVars;

This save gas and clean the code

Low

L-01) The setWethAddress function can be delete

In ForgottenRunesWarriorsMinter.sol, The WETH address should never change and L54 weth can be immutable

L-02) In ForgottenRunesWarriorsGuild.sol the L152 uploadImage and L157 uploadAttributes functions are empty

Its a good idea storage the encoded data on-chain but recommend emit and event with the id of the token and the data

L-03) Do not have any event

Each function that changes the state of the contract should have an associated event

L-04) Remove initialize function

In ForgottenRunesWarriorsGuild.sol, remove L52 initialize function and use directly L137 setMinter function

L-05) Redundant require

In ForgottenRunesWarriorsGuild.sol L173 and ForgottenRunesWarriorsMinter.sol L628, the require of the forwardERC20s function, require(address(msg.sender) != address(0)); dont do anything, can be remove this require, also dont have revert message

Also in this function add address to parameter for transfer the tokens directly to who sent it and use the @openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol library to transfer the tokens

import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';

contract ... {
  using SafeERC20 for IERC20;

  ...

  /**
    * @dev Again, ERC20s should not be sent to this contract, but if someone
    * does, it's nice to be able to recover them
    * @param _token IERC20 the token address
    * @param _to address who receives the tokens
    * @param _amount uint256 the amount to send
    */
  function forwardERC20s(IERC20 _token, address _to, uint256 _amount) external onlyOwner {
      require(_to != address(0), "ForgottenRunesWarriorsGuild: transfer to the zero address");
      _token.safeTransfer(_to, _amount);
  }

  ...

}

L-06) Remove L163 withdrawAll function

In the ForgottenRunesWarriorsGuild.sol the function L163 withdrawAll:

• Use transfer instead of send
• The require dont have revert message
• The function withdrawAll should be non payable

However, the contract cant receibe ETH because dont have a receive/fallback payable function except the withdrawAll function and this function return the balance of the contract to the sender(the owner), in other words the withdrawAll function do nothing

Note: A contract can use selfdestruct to send ETH to the ForgottenRunesWarriorsGuild contract, but why would someone do this?

L-07) Redundant require

In ForgottenRunesWarriorsMinter.sol:

• L136 require(numSold < maxDaSupply, 'Auction sold out'); Redundant require, in the next line check L137 numSold + numWarriors <= maxDaSupply and numWarriors > 0 => numSold < maxDaSupply
• L207 require(numSold < maxForSale, 'Sold out'); Redundant require, in the next line check L208 numSold + numWarriors <= maxForSale and numWarriors > 0 => numSold < maxForSale

L-08) Redundant require(address(recipient) != address(0), 'address req');

In L258 the require is redundant, the ERC721 check the receive its not the address(0)

L-09) Miss not address(0) require

The functions setWarriorsAddress(IForgottenRunesWarriorsGuild) and setWethAddress(address) should have a not address(0) check like require(<address> != address(0), "<address> is zero address");

L-10) Can remove ReentrancyGuard.sol

In ForgottenRunesWarriorsGuild.sol, L94 function mint(address): If move the _safeMint(recipient, tokenId); to the end or use _mint instead of _safemint can remove the nonReentrant modifier

Recommend:

function mint(address _recipient)
    public
    override
    returns (uint256)
{
    require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
    require(_msgSender() == minter, 'Not a minter');
    uint256 tokenId = numMinted;
    numMinted += 1;
    _safeMint(_recipient, tokenId);
    return tokenId;
}

note: The minter should have the responsibility to worry about the reentrancy attacks

In ForgottenRunesWarriorsMinter.sol: In the functions L130 bidSummon(uint256), L171 mintlistSummon(bytes32[]), L201 publicSummon(uint256), L229 claimSummon(bytes32[]) the mint is in the end of the function In the functions L350 issueRefunds(uint256,uint256), L364 refundAddress(address), nonReentrant, L371 selfRefund() the .call is in the end of the function

L-11) Modifie setPhaseTimes function

In ForgottenRunesWarriorsMinter.sol, L480 setPhaseTimes:

• Add new parameter to claimsStartTime
• Add a require(newSelfRefundsStartTime > newMintlistStartTime, 'Set selfRefundsStart after mintlist');
• Set selfRefundsStartTime, setSelfRefundsStartTime(newSelfRefundsStartTime);

L-12) Add require on setPhaseTimes function

In ForgottenRunesWarriorsMinter.sol, L480 setPhaseTimes:

• Add a require(newMintlistStartTime > newDaStartTime, 'Set mintlist after daStart');

L-01) Modifie the refund logic

In ForgottenRunesWarriorsMinter.sol:

• Delete: L73 daMinters array, L76 daAmountPaid mappings, L79 daAmountRefunded mapping, L82 daNumMinted mapping, L337 numDaMinters() function
• Add BidSummon event to save the minters addresses, numWarriors and value
• In toDAMinter mapping save the minters info
• The toDAMinter mapping using DAMinter struct how saves the paid amount and the number of minted, in uint240 and uint16 to save gas(total 256 => a bytes32 instead of 2 * bytes32)

event BidSummon(address indexed minter, uint256 numWarriors, uint256 value);

struct DAMinter {
    /// @notice Tracks the total amount paid
    uint240 amountPaid;
    /// @notice Tracks the total count of NFTs minted
    uint16 numMinted;
}

/// @notice Tracks the information of DA minters
mapping(address => DAMinter) public toDAMinter;

• Assign the info to the toDAMinter mapping
• Emit the BidSummon event

function bidSummon(uint256 numWarriors)
    external
    payable
    nonReentrant
    whenNotPaused
{
    require(numSold < maxDaSupply, 'Auction sold out');
    require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
    require(daStarted(), 'Auction not started');
    require(!mintlistStarted(), 'Auction phase over');
    require(
        numWarriors > 0 && numWarriors <= 20,
        'You can summon no more than 20 Warriors at a time'
    );

    uint256 currentPrice = currentDaPrice();
    require(
        msg.value >= (currentPrice * numWarriors),
        'Ether value sent is not sufficient'
    );

    toDAMinter[msg.sender].amountPaid += uint240(msg.value);
    toDAMinter[msg.sender].numMinted += uint16(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;
    }

    emit BidSummon(msg.sender, numWarriors, msg.value);

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

• Modifie the refunds logic

/**
  * @notice issues refunds for the accounts in minters
  * @param minters array of addresses to refund
  */
function refundAddresses(address[] calldata minters) external onlyOwner {
    uint256 mintersLength = minters.length;
    for (uint256 i; i < mintersLength;) {
        _refundAddress(minters[i]);
        unchecked {
            ++i;
        }
    }
}

/**
  * @notice issues a refund for the address
  * @param minter address the address to refund
  */
function refundAddress(address minter) external onlyOwner {
    _refundAddress(minter);
}

/**
  * @notice refunds msg.sender what they're owed
  */
function selfRefund() public {
    require(selfRefundsStarted(), 'Self refund period not started');
    _refundAddress(msg.sender);
}

/**
  * @notice refunds a minter what they're owed
  * @dev the toDAMinter od the minter is deleted
  * @param minter address the address of the account that wants a refund
  */
function _refundAddress(address minter) private {
    uint256 owed = refundOwed(minter);
    if (owed > 0) {
        delete toDAMinter[minter];
        _safeTransferETHWithFallback(minter, owed);
    }
}

/**
  * @notice returns the amount owed the address
  * @param minter address the address of the account that wants a refund
  */
function refundOwed(address minter) public view returns (uint256) {
    unchecked {
        return toDAMinter[minter].amountPaid - (finalPrice * toDAMinter[minter].numMinted);
    }
}

Warning:

• If the owner refunds a minter its convenient that the DA phase was completed
• Its convenient that the finalPrice does not change when start the refunds

Note: Use The Graph, Web3 or Ethers to get and process the events and get all minters addresses to use in refundAddresses(address[]) function

Gas reports

Contests: Forgotten Runes Warrior Guild

Autor: Rotcivegaf

Scope:

Interfaces:

• IForgottenRunesWarriorsGuild.sol
• IForgottenRunesWarriorsMinter.sol
• IWETH.sol

Contracts:

• ForgottenRunesWarriorsGuild.sol
• ForgottenRunesWarriorsMinter.sol

G-01) The contracts use unlocked pragma

• ForgottenRunesWarriorsGuild.sol
• ForgottenRunesWarriorsMinter.sol
• IForgottenRunesWarriorsGuild.sol
• IForgottenRunesWarriorsMinter.sol
• IWETH.sol

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Consider locking compiler version, for example pragma solidity 0.8.6.

Ref link: The contracts use unlocked pragma #181

G-02) No need to initialize variables with default values

In ForgottenRunesWarriorsGuild.sol:

• L24: uint256 public numMinted
• L36: string public METADATA_PROVENANCE_HASH

In ForgottenRunesWarriorsMinter.sol:

• L162, L220, L259: uint256 i

G-03) Redundant type casting

Avoid cast to save gas and clean code

In ForgottenRunesWarriorsGuild.sol:

• L174: address(msg.sender) to msg.sender

In ForgottenRunesWarriorsMinter.sol:

• L258: address(recipient) to recipient
• L609, L617: address(vault) to vault
• L628: address(msg.sender) to msg.sender

G-04) Public functions to external functions

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions, when dont have parameters.

In ForgottenRunesWarriorsGuild.sol:

• L52: initialize(address)
• L85: exists(uint256)
• L94: mint(address)
• L113: burn(uint256)
• L145: setProvenanceHash(string)
• L173: forwardERC20s(IERC20,uint256)

In ForgottenRunesWarriorsMinter.sol:

• L350: issueRefunds(uint256,uint256)
• L364: refundAddress(address)
• L505: setMintlist1MerkleRoot(bytes32)
• L513: setMintlist2MerkleRoot(bytes32)
• L520: setClaimlistMerkleRoot(bytes32)
• L550: setStartPrice(uint256)
• L557: setLowestPrice(uint256)
• L564: setDaPriceCurveLength(uint256)
• L571: setDaDropInterval(uint256)
• L579: setFinalPrice(uint256)
• L586: setMaxDaSupply(uint256)
• L593: setMaxForSale(uint256)
• L600: setMaxForClaim(uint256)
• L608: withdraw(uint256)
• L616: forwardERC20s(IERC20,amount)

G-05) Internal functions to private functions

The following functions could be set private to save gas and improve code quality, in ForgottenRunesWarriorsMinter.sol:

• L399: _safeTransferETHWithFallback(address,uint256)
• L412: _safeTransferETH(address,uint256)

G-06) _msgSender() to msg.sender

Use msg.sender to save gas in ForgottenRunesWarriorsGuild.sol:

• L101: require(_msgSender() == minter, 'Not a minter'); to require(msg.sender == minter, 'Not a minter');
• L115: _isApprovedOrOwner(_msgSender(), tokenId), to _isApprovedOrOwner(msg.sender, tokenId),

G-07) <var>++ or <var> += 1 to ++<var>

When the value of the post-loop increment/decrement is not stored or used in any calculations, the prefix increment/decrement operators (++<var>/--<var>) cost less gas PER LOOP than the postfix increment/decrement operators (<var>++/<var>--)

Use ++<var> rather than <var>++ to save gas

In ForgottenRunesWarriorsGuild.sol:

• L162, L220: for (uint256 i = 0; i < numWarriors; i++) { to for (uint256 i = 0; i < numWarriors; ++i) {
• L259: for (uint256 i = 0; i < count; i++) { to for (uint256 i = 0; i < count; ++i) {
• L355: for (uint256 i = startIdx; i < endIdx + 1; i++) { to for (uint256 i = startIdx; i < endIdx + 1; ++i) {
• L192: numSold += 1; to ++numSold;
• L248: numClaimed += 1; to ++numClaimed;

In ForgottenRunesWarriorsMinter.sol:

• L104: numMinted += 1; to ++numMinted;

G-08) Use unchecked for operations not expected to overflow

In ForgottenRunesWarriorsGuild.sol:

• L104: numMinted += 1; to

  unchecked {
      numMinted += 1;
  }

In ForgottenRunesWarriorsMinter.sol:

• L152:

  daAmountPaid[msg.sender] += msg.value;
  daNumMinted[msg.sender] += numWarriors;
  numSold += numWarriors;

  to

  unchecked {
      daAmountPaid[msg.sender] += msg.value;
      daNumMinted[msg.sender] += numWarriors;
      numSold += numWarriors;
  }

  The sum of value cant overflow => daNumMinted, numSold and cant overflow, daAmountPaid > numSold

• L162, L220, L259, L355:

  for (...; i++) {
      ...
  }

  to

  unchecked {
      for (...;) {
          ...
          unchecked {
              i++;
          }
  }

• L193, L248: <var> += 1; to

  unchecked {
      <var> += 1;
  }

• L219: numSold += numWarriors; to

  unchecked {
      numSold += numWarriors;
  }

• L275: All mathematics inside of currentDaPrice() function can be unchecked

• L379: daAmountRefunded[minter] += owed; to

  unchecked {
      daAmountRefunded[minter] += owed;
  }

• L388: All mathematics inside of refundOwed() function can be unchecked

G-09) Use !<bool> instead of <bool> == false

In ForgottenRunesWarriorsMinter.sol, L182 require(mintlistMinted[msg.sender] == false, 'Already minted'); to require(!mintlistMinted[msg.sender], 'Already minted'); to save gas

G-10) Use local variables to cache results of storage reads

In ForgottenRunesWarriorsGuild.sol:

• L94: In function mint(address) mload numMinted in the begin of the function and use tokenId in the MAX_WARRIORS require

  function mint(address recipient)
      public
      override
      nonReentrant
      returns (uint256)
  {
      uint256 tokenId = numMinted;
      require(tokenId < MAX_WARRIORS, 'All warriors have been summoned');
      require(_msgSender() == minter, 'Not a minter');
      _safeMint(recipient, tokenId);
      numMinted += 1;
      return tokenId;
  }

In ForgottenRunesWarriorsMinter.sol:

• L130: function bidSummon(uint256) variable maxDaSupply
• L275: function currentDaPrice() variable: lowestPrice sload 5 times startPrice sload 4 times daStartTime sload 2 times daPriceCurveLength sload 2 times daDropInterval sload 2 times

G-11) Use <= to save gas

In ForgottenRunesWarriorsMinter.sol L355: for (uint256 i = startIdx; i < endIdx + 1; i++) { to for (uint256 i = startIdx; i <= endIdx; i++) { to save gas