Back to reassor's contests

Forgotten Runes Warrior Guild contest - reassor's results

16,000 Warrior NFTs sold in a phased Dutch Auction.

General Information

Platform: Code4rena

Start Date: 03/05/2022

Pot Size: $30,000 USDC

Total HM: 6

Participants: 93

Period: 3 days

Judge: gzeon

Id: 118

League: ETH

Forgotten Runes

Findings Distribution

Researcher Performance

Rank: 20/93

Findings: 4

Award: $515.73

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryForgotten Runes

Findings Information

🌟 Selected for report: throttle

Also found by: 0xDjango, BowTiedWardens, WatchPug, defsec, dipp, fatherOfBlocks, gzeon, hake, reassor, shung, unforgiven

Labels

bug
duplicate
2 (Med Risk)
upgraded by judge

Awards

296.7571 USDC - $296.76

External Links

Link to GitHub issue

Judge has assessed an item in Issue #44 as Medium risk. The relevant finding follows:

3. Missing/Invalid validation checks

Risk

Low

Impact

Function ForgottenRunesWarriorsMinter.setPhaseTimes setups times for different phases of minting. Phases should be launched one after the other.

Missing check:

  • newDaStartTime < newMintlistStartTime - making sure that Phase DA is before Mintlist Phase

Invalid checks that might lead to unintentional overlapping of phases:

  • newPublicStartTime >= newMintlistStartTime should use > instead of >=
  • newClaimsStartTime >= newPublicStartTime should > instead of >=

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add missing check newDaStartTime > newMintlistStartTime and fix invalid checks comparisons from >= to >.

#0 - gzeoneth

2022-06-20T17:34:55Z

Duplicate of #27

Findings Information

🌟 Selected for report: leastwood

Also found by: 0xliumin, Czar102, IllIllI, TrungOre, VAD37, WatchPug, berndartmueller, cccz, gzeon, hickuphh3, horsefacts, ilan, joestakey, m9800, p4st13r4, peritoflores, reassor, rfa, robee, sorrynotsorry, tintin, z3s

Awards

48.5872 USDC - $48.59

Labels

bug
duplicate
2 (Med Risk)
upgraded by judge

External Links

Link to GitHub issue

Judge has assessed an item in Issue #44 as Medium risk. The relevant finding follows:

1. Usage of legacy ETH transfer function

Risk

Low

Impact

Contract ForgottenRunesWarriors for withdrawing ETH to vault uses send function, which has a fixed gas stipend and can fail.

The reason behind this is that, after the Istanbul hardfork, any smart contract that uses transfer() or send() is taking a hard dependency on a gas costs by forwarding a fixed amount of gas (2300). This forwards 2300 gas, which may not be enough if the recipient is a contract and the cost of gas changes.

Proof of Concept

ForgottenRunesWarriorsMinter:

ForgottenRunesWarriorsGuild:

Tools Used

Manual Review / VSCode

It is recommended to use safe wrapper library, such as the OpenZeppelin Address library’s and its sendValue function that forwards sufficient gas for the transfer regardless of the underlying OPCODE gas costs.

#0 - gzeoneth

2022-06-18T19:19:28Z

Duplicate of #254

Findings Information

🌟 Selected for report: defsec

Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0x52, 0xDjango, 0xf15ers, 0xkatana, 0xliumin, AuditsAreUS, BowTiedWardens, CertoraInc, Cr4ckM3, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, Picodes, Ruhum, TerrierLover, TrungOre, VAD37, WatchPug, berndartmueller, broccolirob, catchup, cccz, cryptphi, csanuragjain, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, hubble, hyh, ilan, joestakey, kebabsec, kenta, kenzo, leastwood, m9800, marximimus, minhquanym, oyc_109, p4st13r4, pauliax, pedroais, peritoflores, plotchy, rajatbeladiya, reassor, rfa, robee, rotcivegaf, samruna, shenwilly, shung, simon135, sorrynotsorry, sseefried, teddav, throttle, tintin, unforgiven, z3s

Awards

149.3609 USDC - $149.36

Labels

bug
QA (Quality Assurance)
sponsor confirmed

External Links

Link to GitHub issue

1. Usage of legacy ETH transfer function

Risk

Low

Impact

Contract ForgottenRunesWarriors for withdrawing ETH to vault uses send function, which has a fixed gas stipend and can fail.

The reason behind this is that, after the Istanbul hardfork, any smart contract that uses transfer() or send() is taking a hard dependency on a gas costs by forwarding a fixed amount of gas (2300). This forwards 2300 gas, which may not be enough if the recipient is a contract and the cost of gas changes.

Proof of Concept

ForgottenRunesWarriorsMinter:

ForgottenRunesWarriorsGuild:

Tools Used

Manual Review / VSCode

It is recommended to use safe wrapper library, such as the OpenZeppelin Address library’s and its sendValue function that forwards sufficient gas for the transfer regardless of the underlying OPCODE gas costs.

2. Missing zero address checks

Risk

Low

Impact

Multiple functions in ForgottenRunesWarriorsMinter and ForgottenRunesWarriorsGuild contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

ForgottenRunesWarriorsMinter:

ForgottenRunesWarriorsGuild:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

3. Missing/Invalid validation checks

Risk

Low

Impact

Function ForgottenRunesWarriorsMinter.setPhaseTimes setups times for different phases of minting. Phases should be launched one after the other.

Missing check:

  • newDaStartTime < newMintlistStartTime - making sure that Phase DA is before Mintlist Phase

Invalid checks that might lead to unintentional overlapping of phases:

  • newPublicStartTime >= newMintlistStartTime should use > instead of >=
  • newClaimsStartTime >= newPublicStartTime should > instead of >=

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add missing check newDaStartTime > newMintlistStartTime and fix invalid checks comparisons from >= to >.

4. ERC20 Return values not checked

Risk

Low

Impact

The IERC20.transfer() function returns a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Furthermore, tokens that do not correctly implement the EIP20 standard, like USDT which does not return a success boolean, will revert.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to use OpenZeppelin's SafeERC20 with the safeTransfer function that handles the return value check as well as non-standard-compliant tokens.

5. Owner - critical address change

Risk

Low

Impact

Changing critical addresses such as ownership of ForgottenRunesWarriorsMinter and ForgottenRunesWarriorsGuild contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for passing ownership for ForgottenRunesWarriorsMinter and ForgottenRunesWarriorsGuild contracts.

6. Missing error messages

Risk

Non-Critical

Impact

Contracts ForgottenRunesWarriorsMinter and ForgottenRunesWarriorsGuild are missing error messages for multiple of require statements. Lack of clear error messages might lead to confusion in case of transactions being rejected.

Proof of Concept

ForgottenRunesWarriorsMinter.sol:

ForgottenRunesWarriorsGuild.sol

Tools Used

Manual Review / VSCode

It is recommended to add error messages to listed require statements.

7. The contracts use unlocked pragma

Risk

Non-Critical

Impact

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

Proof of Concept

All the contracts in scope use unlocked pragma:

Tools Used

Manual Review / VSCode

Consider using a single compiler version for compiling both contracts, for example 0.8.10

8. Obsolete function ForgottenRunesWarriorsGuild.withdrawAll

Risk

Non-Critical

Impact

Contract ForgottenRunesWarriorsGuild implements withdrawAll function that allows withdrawing all ether from the contract if it was accidentally sent to it. Function withdrawAll is the only payable function in the whole ForgottenRunesWarriorsGuild contract which makes it impossible to receive "accidental" ether.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to remove withdrawAll function from ForgottenRunesWarriorsGuild contract.

9. Missing events

Risk

Non-Critical

Impact

Contracts ForgottenRunesWarriorsGuild and ForgottenRunesWarriorsMinter are not implementing events for multiple critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

ForgottenRunesWarriorsGuild

ForgottenRunesWarriorsMinter:

Tools Used

Manual Review / VSCode

It is recommended to go through listed functions and add emitting events for the one that change storage variables and should be monitored.

10. Usage of boolean values

Risk

Non-Critical

Impact

Contract ForgottenRunesWarriorsMinter in functions mintlistSummon and claimSummon uses false boolean expression for require statement in functions.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to remove false expression: require(!mintlistMinted[msg.sender], 'Already minted'); and require(!claimlistMinted[msg.sender], 'Already claimed');

11. Use constant max uint256 as a default value for timestamp variables

Risk

Non-Critical

Impact

Contract ForgottenRunesWarriorsMinter as a default value for daStartTime, mintlistStartTime, publicStartTime, claimsStartTime and selfRefundsStartTime uses maximum value of uint256 variable written in hexadecimal format. It makes code less readable and might lead to accidental typos in their values.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add constant value MAX_UINT = type(uint256).max and then reuse it as a default value for "timestamp" variables.

12. Risk of centralization

Risk

Non-Critical

Impact

Protocol is highly centralized and depends on the good will of the team. Owner of the contract(s) can withdraw all deposited funds at any time, can start and stop minting and can mint arbitrary number of NFTs. This might lead to trust issues and users losses if the owner account will get compromised.

The issue has been downgraded to non-critical since the team is aware of the risk and has accepted it.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to decentralize protocol by adding multiple safeguards and limit power of the contract(s) owner.

#0 - cryppadotta

2022-05-05T12:44:27Z

Good advice in here. Will fix several of these

Findings Information

🌟 Selected for report: BowTiedWardens

Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xNazgul, 0xProf, 0xc0ffEE, 0xf15ers, 0xkatana, 0xliumin, ACai, AlleyCat, CertoraInc, Cityscape, Cr4ckM3, DavidGialdi, Dinddle, FSchmoede, Funen, GimelSec, Hawkeye, IllIllI, Kulk0, M0ndoHEHE, MaratCerby, MiloTruck, Picodes, RoiEvenHaim, Tadashi, TerrierLover, TrungOre, VAD37, WatchPug, antonttc, catchup, defsec, delfin454000, dirk_y, eccentricexit, ellahi, fatherOfBlocks, gzeon, hake, hansfriese, hickuphh3, horsefacts, ilan, joestakey, kebabsec, kenta, kenzo, marximimus, minhquanym, noobie, oyc_109, p4st13r4, pauliax, rajatbeladiya, reassor, rfa, robee, rotcivegaf, saian, samruna, shenwilly, shung, simon135, slywaters, sorrynotsorry, throttle, unforgiven, z3s

Awards

21.02 USDC - $21.02

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

1. Cache calculation outside of loop

Impact

Function ForgottenRunesWarriorsMinter.issueRefunds is running for loop with refunds but is performing calculation inside the loop endIdx + 1.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to cache the calculation outside of the loop

uint256 end = endIdx + 1;

2. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper that with > 0.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to use != 0 instead of > 0.

3. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

ForgottenRunesWarriorsMinter:

ForgottenRunesWarriorsGuild:

Tools Used

Manual Review / VSCode

It is recommended to decrease revert messages to maximum 32 bytes. Alternatively custom error types should be used.

4. ++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).

Proof of Concept

ForgottenRunesWarriorsMinter:

Tools Used

Manual Review / VSCode

It is recommended to use ++i instead of i++ to increment value of an unsigned integer variable.

5. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. In ForgottenRunesWarriorsMinter contract are multiple for statements that use i iterator that cannot overflow or underflow but consumes additional gas for checks.

Proof of Concept

ForgottenRunesWarriorsMinter:

Tools Used

Manual Review / VSCode

It is recommended wrap incrementing of i with unchecked block:

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

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

As an example

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

can be replaced with

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

Proof of Concept

ForgottenRunesWarriorsMinter:

Tools Used

Manual Review / VSCode

It is recommended to remove explicit initializations with default values.

7. Obsolete require statements

Impact

Contract ForgottenRunesWarriorsMinter in functions bidSummon and publicSummon has redundant require statements that consume additional gas.

If the second require and numWarriors > 0 statements in bidSummon are true then the first is also true:

    require(numSold < maxDaSupply, 'Auction sold out');
    require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
    (..)
    require(
        numWarriors > 0 && numWarriors <= 20,
        'You can summon no more than 20 Warriors at a time'
    );

If the second require and numWarriors > 0 statements in publicSummon are true then the first is also true:

    require(numSold < maxForSale, 'Sold out');
    require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
    (..)
    require(
        numWarriors > 0 && numWarriors <= 20,
        'You can summon no more than 20 Warriors at a time'
    );

Proof of Concept

ForgottenRunesWarriorsMinter:

Tools Used

Manual Review / VSCode

It is recommended to:

  • remove require(numSold < maxDaSupply, 'Auction sold out'); statement from bidSummon
  • remove require(numSold < maxForSale, 'Sold out'); statement from publicSummon
AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter