Forgotten Runes Warrior Guild contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

Using Solidity's transfer function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

The sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

Proof of Concept

ForgottenRunesWarriorsGuild.sol#L164
ForgottenRunesWarriorsMinter.sol#L610
ForgottenRunesWarriorsMinter.sol#L618

Tools Used

Manual review

Recommended mitigation steps

Use Solidity's low-level call() function or the sendValue function available in OpenZeppelin Contract’s Address library to send Ether.

QA Report

Table of Contents

• Non-Critical Findings
  • [NC-01] - Spelling mistakes
• Low Risk
  • [L-01] - Use of floating pragma
  • [L-02]: Events not emitted for important state changes
  • [L-03] - Unnecessary payable
  • [L-04]: Zero-address checks are missing
  • [L-05]: Validate daDropInterval in setDaDropInterval()
  • [L-06]: Validate upper limit for finalPrice in setFinalPrice()

Non-Critical Findings

[NC-01] - Spelling mistakes

Description

Spelling mistakes found.

Findings

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

* @param newMintlistStartTime uint256 the mintlst phase start time

mintlst

Recommended mitigation steps

Fix spelling mistakes.

Low Risk

[L-01] - Use of floating pragma

Description

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

Findings

ForgottenRunesWarriorsGuild.sol#L1
ForgottenRunesWarriorsMinter.sol#L1

Recommended mitigation steps

Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.

[L-02]: Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

ForgottenRunesWarriorsMinter.sol#L441
ForgottenRunesWarriorsMinter.sol#L448
ForgottenRunesWarriorsMinter.sol#L455
ForgottenRunesWarriorsMinter.sol#L462
ForgottenRunesWarriorsMinter.sol#L469
ForgottenRunesWarriorsMinter.sol#L505
ForgottenRunesWarriorsMinter.sol#L513
ForgottenRunesWarriorsMinter.sol#L520
ForgottenRunesWarriorsMinter.sol#L550
ForgottenRunesWarriorsMinter.sol#L557
ForgottenRunesWarriorsMinter.sol#L564
ForgottenRunesWarriorsMinter.sol#L571
ForgottenRunesWarriorsMinter.sol#L579
ForgottenRunesWarriorsMinter.sol#L586
ForgottenRunesWarriorsMinter.sol#L593
ForgottenRunesWarriorsMinter.sol#L600

Recommended mitigation steps

Emit events for state variable changes.

[L-03] - Unnecessary payable

Description

Functions which do not use msg.value do not need payable modifier.

Findings

ForgottenRunesWarriorsGuild.sol#L163
ForgottenRunesWarriorsMinter.sol#L616

Recommended mitigation steps

Remove payable modifier.

[L-04]: Zero-address checks are missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

ForgottenRunesWarriorsMinter.sol#L528

vault = _newVaultAddress;

ForgottenRunesWarriorsMinter.sol#L537

warriors = _newWarriorsAddress;

ForgottenRunesWarriorsMinter.sol#L544

weth = _newWethAddress;

ForgottenRunesWarriorsGuild.sol#L138

minter = newMinter;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_newVaultAddress != address(0), "Zero-address");

[L-05]: Validate daDropInterval in setDaDropInterval()

Description

Setting daDropInterval to a value of 0 results in division by zero in following cases:

ForgottenRunesWarriorsMinter.sol#L285

uint256 dropPerStep = (startPrice - lowestPrice) /
  (daPriceCurveLength / daDropInterval);

ForgottenRunesWarriorsMinter.sol#L288

uint256 steps = elapsed / daDropInterval;

Findings

ForgottenRunesWarriorsMinter.sol#L572

Recommended mitigation steps

Validate _newTime to make sure value is != 0:

require(_newTime != 0, "Zero");

[L-06]: Validate upper limit for finalPrice in setFinalPrice()

Description

As the dutch auction start price is defined as 2.5 ether, this should be used as an upper bound for finalPrice to prevent setting an arbitrary high price.

Findings

ForgottenRunesWarriorsMinter.sol#L580

Recommended mitigation steps

Validate _newPrice to be < 2.5 ether:

require(_newPrice <= 2.5 ether, "Limit reached");