Forgotten Runes Warrior Guild contest - 0xDjango's results

Lines of code

Included below

Vulnerability details

Impact

There are several important setter functions that lack validation on either the value itself or the timing on which the function can be called. It seems that the developer intentionally wrote these contracts with flexibility in mind, so I am submitting all of these findings as a single vulnerability under the umbrella of "functions unsafely lacking validation".

Example #1 - This initialize() function lacks the initializer keyword.

The following initialize function can be called by the owner as many times as desired. This function sets the minter contract that is allowed to call important functions such as mint(). The owner can accidently or intentionally set this to a contract address that is not the correct minter contract, effectively DOS'ing the minting process.

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

Example #2 - The Phase times can be set incorrectly or updated mid-phase

These setter functions have no validation. Therefore, the phase times can either be set incorrectly (e.g. claim phase before dutch auction) or the time can be updated during the phase to effectively end it.

These functions are set to public though maybe they should be set to internal and called within setPhaseTimes() as this function contains validation.

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

Example #3 - The Merkle Roots can be updated at any time

Changing these merkle roots can DOS the event, if desired.

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

Example #4 - These addresses can be updated at any time

Setting these addresses to contracts that don't support the required functions will DOS the event. In the case of the warrior address, setting this to another compliant contract effectively allows the users to mint knockoff NFTs.

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

Example #5 - Setting these variables incorrectly will DOS minting

If the owner intentionally or accidently passes 0 for these setters, all minting will be DOS'ed.

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

Tools Used

Manual review.

Recommended Mitigation Steps

Add validation for each of these functions. They should be validated both for proper values as well as the time frame that the call is allowed.

Lines of code

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

Vulnerability details

Impact

There are no internal accounting variables keeping track of how many team summons have been performed, nor are there any checks that the team summons are performed after all of the public phases have been completed. This can lead to less than the desired amount of NFTs available to the public.

Tools Used

Manual review.

Recommended Mitigation Steps

Add internal account variables to ensure that the number of team summons performed is less than the desired allotment of team summons. If this is not desired in the case that there are more leftover than anticipated, at least add a check that the team summons can not be performed until after the claim phase to ensure that the amount of team summons does not interfere with the number of NFTs that should be available to the public.

Overall, the two contracts that make up this codebase perform the necessary functions well but lack checks for most major actions. Several variables are able to be written and re-written at any time which can cause the contracts to stop functioning correctly. This report will not consist of these issues, but instead will highlight low severity issues and general comments to further strengthen the contracts.

Issue #1 (Low) - Time checks should be inclusive

There are several checks to determine if the desired phase has started. All of these checks require that block.timestamp > startTime though the startTime should be included in the check. These should be changed to block.timestamp >= startTime. For example, if you tell someone that an event starts at 12:00pm, you should allow them to participate at 12:00pm, not 12:01.

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

Issue #2 (Low) - Vault address should be checked for address(0)

It is important to check that the vault address is not set to address(0) to ensure that no funds are lost.

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

Issue #3 (Low) - Floating Pragma

Contracts contain a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions. In the case of the forked contacts, I recommend deploying with the exact version that the current live versions were deployed with.

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

Issue #4 (Low) - Use of incorrect interface

The contract is using the correct IWETH interface for depositing WETH, but then uses the IERC20 interface for transferring the WETH. This does not cause any functional or security issues, but since IWETH contains its own transfer function, it would make sense to keep it consistent.

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

Optimization #1 - Several functions can be made external instead of public

Since these functions do not have any references within the codebase, they may be made external. External functions consume roughly half of the gas compared to public.

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

Optimization #2 - For loop optimization

These for loops can be optimized by incrementing i within unchecked{}. The new pattern would be:

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

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L162-L164 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L220-L222 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L259-L261

Optimization #3 - Set the parameter numWarriors to smaller uint

Since the maximum number of warriors to be summoned is 20, this will easily fit within uint8

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

Optimization #4 - dropPerStep does not need to be calculated every time currentDaPrice() is called

The value for dropPerStep does not change. Therefore, this value can be written and read instead of recalculated each time.

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