Forgotten Runes Warrior Guild contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L253-L262

Vulnerability details

Currently there are no actual restrictions on the number of warriors in teamSummon, which can break up the stated supply composition.

Setting severity to medium as while the function is Owner controlled, the improper teamSummon use can break up the announced distribution mechanics, which is the main goal of the system.

Proof of Concept

There is a supply schedule announced:

https://github.com/code-423n4/2022-05-runes#supply-breakdown

Any number of NFTs up to total supply cap can be minted anytime with teamSummon:

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L253-L262

     * @notice Mint a Warrior (owner only)
     * @param recipient address the address of the recipient
     * @param count uint256 of the number of warriors you're trying to mint
     */
    function teamSummon(address recipient, uint256 count) external onlyOwner {
        require(address(recipient) != address(0), 'address req');
        for (uint256 i = 0; i < count; i++) {
            _mint(recipient);
        }
    }

If by any chance teamSummon be called for more than 710 = 16000 - 14190 - 1100 warriors, the stated supply composition will not be met.

That is permanent as there is no time limit for phases after DA.

Recommended Mitigation Steps

Introduce the team cap variable and control the number of NFTs minted with teamSummon.

1. Initialization can be rerun to reset the minter in Guild contract (low)

As the Guild-Minter link is essential to collection's economics any material change after the initial setup can break the system, and that is possible now.

As a simplest example, setting new minter after the action start when some NFTs were minted will reduce maximum issuance cap for the new minter.

Proof of Concept

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L48-L54

    /**
     * @dev Convenient way to initialize the contract
     * @param newMinter address of the minter contract
     */
    function initialize(address newMinter) public onlyOwner {
        setMinter(newMinter);
    }

Recommended Mitigation Steps

Consider making setMinter() a one time action, requiring current minter to be zero.

2. Minter's configuration variables can be reset after DA and subsequent phases start (low)

Besides final price and vault address all other configuration variables shouldn't be available for any change after auction phase has started as it will break the system mechanics in the various ways.

For example, setting warriors to any other NFT contract after first mint will split the collection and increase the maximum available number of warriors (i.e. hard cap will be breached).

Proof of Concept

Now all configuration variables can be changed by owner at any time:

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L438-L602

For example, is NFT address be reset at any point after auction start, which can break the mechanics of the system:

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L531-L538

    /**
     * @notice set the warriors token address
     */
    function setWarriorsAddress(
        IForgottenRunesWarriorsGuild _newWarriorsAddress
    ) public onlyOwner {
        warriors = _newWarriorsAddress;
    }

Recommended Mitigation Steps

It looks like that all the config variables besides final price and vault address are not subject to change after DA start, so consider requiring block.timestamp <= daStartTime in the corresponding setters.

3. Unsafe transfer is used in rescue (low)

Transfer without result check is used for an arbitrary ERC20 in onlyOwner rescue function.

The function can be rerun, but the lack of execution control makes its use ambiguous.

Proof of Concept

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L175-L175

        token.transfer(msg.sender, amount);

Recommended Mitigation Steps

Require transfer success or consider safeTransfer as not all the tokens revert on failure.

4. Floating pragma of two types is used across the system (non-critical)

Impact

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

Proof of Concept

Interfaces use pragma solidity ^0.8.6:

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/interfaces/IForgottenRunesWarriorsGuild.sol#L5-L5

Both contracts use pragma solidity ^0.8.0:

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L1-L1

Recommended Mitigation Steps

Consider fixing the version to 0.8.x across all the codebase.

5. Max int representation is error-prone (non-critical)

0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff is used as max int representation in ForgottenRunesWarriorsMinter.

There are better ways that reduce operational error probability and improve readability.

Proof of Concept

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L16-L35

    /// @notice The start timestamp for the Dutch Auction (DA) sale and price
    uint256 public daStartTime =
        0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice The start timestamp for mintlisters
    /// @dev This is the end of DA phase. No more DA bids when this is hit
    uint256 public mintlistStartTime =
        0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice The start timestamp for the public sale
    uint256 public publicStartTime =
        0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice The start timestamp for the claims
    uint256 public claimsStartTime =
        0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

    /// @notice The start timestamp for self refunds
    uint256 public selfRefundsStartTime =
        0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;

Recommended Mitigation Steps

For compilers after 0.6.8 it's a kind of optimal to use type(uint256).max:

https://forum.openzeppelin.com/t/using-the-maximum-integer-in-solidity/3000/13