Forgotten Runes Warrior Guild contest - ellahi's results

Low

1. Unsafe ERC20 Operations

Impact

The return value of an external transfer/transferFrom call is not checked.

Proof of Concept

  /ForgottenRunesWarriorsGuild.sol::175 => token.transfer(msg.sender, amount);
  /ForgottenRunesWarriorsMinter.sol::402 => IERC20(weth).transfer(to, amount);
  /ForgottenRunesWarriorsMinter.sol::629 => token.transfer(msg.sender, amount);

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

Non-Critical

1. Natspec is incomplete.

Proof of Concept

ForgottenRunesWarriorsMinter.sol

    /**
     * @notice returns the current dutch auction price
     */
    function currentDaPrice() public view returns (uint256) {

    /**
     * @notice returns whether the dutch auction has started
     */
    function daStarted() public view returns (bool) {

    /**
     * @notice returns whether the mintlist has started
     */
    function mintlistStarted() public view returns (bool) {

    /**
     * @notice returns whether the public mint has started
     */
    function publicStarted() public view returns (bool) {

    /**
     * @notice returns whether the claims phase has started
     */
    function claimsStarted() public view returns (bool) {

    /**
     * @notice returns whether self refunds phase has started
     */
    function selfRefundsStarted() public view returns (bool) {

    /**
     * @notice returns the number of minter addresses in the DA phase (includes duplicates)
     */
    function numDaMinters() public view returns (uint256) {

Recommendation

Add natspec @return comments.

Tools used

Manual, Slither.

Gas

1. Contracts in scope are using unlocked pragma.

Impact

All the contracts in scope use pragma solidity ^0.8.0, allowing wide enough range of versions.

Proof of Concept

 ^0.8.0 (contracts/ForgottenRunesWarriorsGuild.sol#1)
 ^0.8.0 (contracts/ForgottenRunesWarriorsMinter.sol#1)

Recommendation

Consider locking compiler version, for example pragma solidity 0.8.6. This can have additional benefits, for example using custom errors to save gas and so forth.

2. Use != 0 instead of > 0 for Unsigned Integer Comparison.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

  /ForgottenRunesWarriorsMinter.sol::141 => numWarriors > 0 && numWarriors <= 20,
  /ForgottenRunesWarriorsMinter.sol::211 => numWarriors > 0 && numWarriors <= 20,
  /ForgottenRunesWarriorsMinter.sol::378 => if (owed > 0) {

Recommendation

Use != 0 instead of > 0.

3. No need to initialize variables with default values.

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  /ForgottenWarriorsGuild.sol::24 => uint256 public numMinted = 0; 
  /ForgottenRunesWarriorsMinter.sol::162 => for (uint256 i = 0; i < numWarriors; i++) {
  /ForgottenRunesWarriorsMinter.sol::220 => for (uint256 i = 0; i < numWarriors; i++) {
  /ForgottenRunesWarriorsMinter.sol::259 => for (uint256 i = 0; i < count; i++) {

Recommendation

Remove explicit zero initialization.

4. publicFunctions can beexternal`.

Impact

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

Proof of Concept

initialize(address) should be declared external:
        - 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)

Recommendation

Use the external attribute for functions never called from the contract.

5. ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Proof of Concept

  /ForgottenRunesWarriorsMinter.sol::162 => for (uint256 i = 0; i < numWarriors; i++) {
  /ForgottenRunesWarriorsMinter.sol::220 => for (uint256 i = 0; i < numWarriors; i++) {
  /ForgottenRunesWarriorsMinter.sol::259 => for (uint256 i = 0; i < count; i++) {
  /ForgottenRunesWarriorsMinter.sol::355 => for (uint256 i = startIdx; i < endIdx + 1; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

6. Use Custom Errors instead of Revert Strings.

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Recommendation

Use custom errors instead of revert strings.

Tools used

c4udit, manual, slither.