Forgotten Runes Warrior Guild contest - catchup's results

Floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L1 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L1 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/interfaces/IForgottenRunesWarriorsGuild.sol#L5 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/interfaces/IForgottenRunesWarriorsMinter.sol#L5

Missing 0 address check

It is a good practice to include 0 address check while updating an important address. I suggest to include a zero address check for the functions below.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L52 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L137 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L527 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L543

Important changes should emit events

There are no events in the contracts. Functions which are executed only by privileged users and change important parameters should emit events.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L129 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L137 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L145 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L163 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L173 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L376 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L427 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L434 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L441 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L448 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L455 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L462 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L469 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L480 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L550 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L557 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L564 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L571 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L579 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L586 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L593 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L600 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L608 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L616

No controls when setting time parameters individually

It is commented in setPhaseTimes() that individual setter functions have no require statements because probably the intention is to set all the time parameters at once using setPhaseTimes(). However, I think it would be better to have necessary require statements in the individual setter functions, in case they can be called individually. If the controls are placed within the individual setter functions, no control would be necessary in the setPhaseTimes(). Therefore, I suggest to have:

• Check the _newtime against block.timestamp in setDaStartTime()
• Check the _newtime against block.timestamp and against daStartTime in setMintlistStartTime()
• Check the _newtime against block.timestamp and against mintlistStartTime in setPublicStartTime()
• Check the _newtime against block.timestamp and against publicStartTime in setClaimsStartTime()

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L441 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L448 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L455 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L462

for loops can be optimized

1- For loop index increments can be made unchecked. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Also postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L162 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L220 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L259 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L355

Recommended Mitigation Steps

I suggest to change the for loops like below: for (uint256 i; i < count; ) { _mint(recipient); unchecked { ++i; } }

Error string longer than 32 characters

Error reason strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L70 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L116 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L142 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L148 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L212

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L24 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L36 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L162 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L220 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L259

Non-zero amount check missing before transfers

It is a good practice to apply non-zero amount checks for transfers to avoid unnecessary executions.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L175 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L629

Booleans are more expensive than uint256

It is more expensive to operate using booleans because before every write operation an SLOAD is executed to read the contents of the slot. Therefore, it is cheaper to use uint256 instead of bool. On the other hand, using bool is better for the code readability. Hence, it is a tradeoff to be decided by the developers.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L85 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L88

Recommended Mitigation Steps

Consider replacing booleans with uint256 if gas efficiency overweighs code readability.

Public state variables can be private

Private state variables are cheaper than public state variables. There are many instances where public variables can be private.

Lines of code

Pretty much all the state variables, some of which are: https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L24 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L27 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L30 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L36 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L17 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L22 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L26 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L30 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L34 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L38 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L42

Public constant can be made private

MAX_WARRIORS is only used within the contract it is defined. Therefore, it is unnecessary to define it as public.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L21

Unused state variable

R state variable is not used anywhere, hence can be deleted

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L32

Unused internal function

_baseURI() is an internal function, but it is not called by any other functions, hence can be deleted.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L76

Some internal functions can be made private

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L399 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L413

dropPerStep can be calculated once and cached

Everytime bidSummon() is called, currentPrice is calculated using currentDaPrice(). dropPerStep is calculated in currentDaPrice() and this calculation uses fixed values. Therefore, dropPerStep can be calculated only once and stored in a state variable, potentially in the constructor and can be updated when one of the underlying calculation parameters is updated.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L284-285

Many state variables are reused multiple times in currentDaPrice()

startPrice is accessed 4 times, daStartTime 2 times, daPriceCurveLength 2 times, lowestPrice 5 times, daDropInterval 2 times within currentDaPrice() function. Caching these state variables and reading from stack would save a lot of SLOAD's.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L275-L297

Caching opportunities in bidSummon()

numSold is accessed 2 times and maxDaSupply 3 times within bidSummon() function. Caching these state variables and reading from stack would save SLOAD's.

Lines of code

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

Caching opportunities in publicSummon()

numSold is accessed 2 times and maxForSale 2 times within publicSummon() function. Caching these state variables and reading from stack would save SLOAD's.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L201-L223

Unused public function

numDaMinters() is used in the test scripts, but not within the real contracts. It can be deleted in the deployed code.

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L337