Forgotten Runes Warrior Guild contest - joestakey's results

Lines of code

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

AVOID USING .SEND TO TRANSFER ETH

Twice in the minter contract, the .send() method is used to transfer ETH.

If gas costs are subject to change, then smart contracts can’t depend on any particular gas costs.

Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

If an upgrade makes the gas cost of the vault fallback function higher than 2300 gas unit, the withdraw functions will not work and a new vault will need to be deployed to avoid loss of funds.

Impact

Medium

Proof Of Concept

see this article

Tools Used

Manual Analysis

Mitigation

use .call() to send ETH.

QA Report

Table of Contents

• summary
• Missing error messages in require statements of various functions
• Setters should emit an event
• Setters should check the input value
• Unlocked pragma

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the absence of events in setters.

Missing error messages in require statements of various functions

PROBLEM

The use of informative error messages helps troubleshoot exceptional conditions during transaction failures or unexpected behavior. Otherwise, it can be misleading and waste crucial time during emergency conditions, even in functions with access control.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:611: require(payable(vault).send(_amount));
ForgottenRunesWarriorsMinter.sol:620: require(payable(vault).send(address(this).balance));

TOOLS USED

Manual Analysis

MITIGATION

Add a short but helpful error message in require statements.

Setters should emit an event

PROBLEM

All setters should emit an event, so the Dapps can detect important changes.

SEVERITY

Low

PROOF OF CONCEPT

None of the setters in either contract are emitting events

TOOLS USED

Manual Analysis

MITIGATION

Emit events in all setters.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address. In particular, setters that change the address of an ERC20 token used for transfers can, in case of a wrongful address being set, lead to loss of funds. In the case of an uint256, if the variable is used as a denominator in some functions, setting it as zero can cause the function to revert. Here, setDaDropInterval does not check the zero value while daDropInterval is a denominator in currentDaPrice(), which is a function called during the Dutch Auction Phase in bidSummon()

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:543: function setWethAddress(address _newWethAddress)

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check in setters, and a zero check in setDaDropInterval.

Unlocked pragma

PROBLEM

Both contracts have unlocked pragma (pragma solidity ^0.8.0) which are not fixed to a specific Solidity version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most.

SEVERITY

Low

PROOF OF CONCEPT

ForgottenRunesWarriorsGuild:1: pragma solidity ^0.8.0;
ForgottenRunesWarriorsMinter:1: pragma solidity ^0.8.0;

TOOLS USED

Manual Analysis

MITIGATION

Remove ^.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparisons with zero for unsigned integers
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Prefix increments
• Require instead of AND
• Signatures verifications are cheaper than Merkle Proofs
• unchecked arithmetic

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

scope: mint()

• numMinted is read twice:

ForgottenRunesWarriorsGuild.sol:100
ForgottenRunesWarriorsGuild.sol:102

ForgottenRunesWarriorsMinter.sol

scope: bidSummon()

• numSold is read twice:

ForgottenRunesWarriorsMinter.sol:136
ForgottenRunesWarriorsMinter.sol:137

• maxDaSupply is read three times:

ForgottenRunesWarriorsMinter.sol:136
ForgottenRunesWarriorsMinter.sol:137
ForgottenRunesWarriorsMinter.sol:156

scope: publicSummon()

• numSold is read twice:

ForgottenRunesWarriorsMinter.sol:207
ForgottenRunesWarriorsMinter.sol:208

• maxForSale is read three times:

ForgottenRunesWarriorsMinter.sol:207
ForgottenRunesWarriorsMinter.sol:208

scope: currentDaPrice()

• startPrice is read four times:

ForgottenRunesWarriorsMinter.sol:277
ForgottenRunesWarriorsMinter.sol:284
ForgottenRunesWarriorsMinter.sol:292
ForgottenRunesWarriorsMinter.sol:295

• lowestPrice is read five times:

ForgottenRunesWarriorsMinter.sol:281
ForgottenRunesWarriorsMinter.sol:284
ForgottenRunesWarriorsMinter.sol:293
ForgottenRunesWarriorsMinter.sol:296
ForgottenRunesWarriorsMinter.sol:296

• daPriceCurveLength is read twice:

ForgottenRunesWarriorsMinter.sol:279
ForgottenRunesWarriorsMinter.sol:285

• daDropInterval is read twice:

ForgottenRunesWarriorsMinter.sol:285
ForgottenRunesWarriorsMinter.sol:288

• daStartTime is read twice:

ForgottenRunesWarriorsMinter.sol:279
ForgottenRunesWarriorsMinter.sol:287

scope: _safeTransferETHWithFallback()

• weth is read twice:

ForgottenRunesWarriorsMinter.sol:401
ForgottenRunesWarriorsMinter.sol:402

scope: withdraw()

• vault is read twice:

ForgottenRunesWarriorsMinter.sol:609
ForgottenRunesWarriorsMinter.sol:610

scope: withdrawAll()

• vault is read twice:

ForgottenRunesWarriorsMinter.sol:617
ForgottenRunesWarriorsMinter.sol:618

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

scope: setBaseURI()

ForgottenRunesWarriorsGuild.sol:129: string memory newBaseURI

scope: setProvenanceHash()

ForgottenRunesWarriorsGuild.sol:241: string memory newHash

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparisons with zero for unsigned integers

IMPACT

>0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:141
ForgottenRunesWarriorsMinter.sol:211

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with !0

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:137
ForgottenRunesWarriorsMinter.sol:141
ForgottenRunesWarriorsMinter.sol:208
ForgottenRunesWarriorsMinter.sol:211

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-numSold + numWarriors <= maxForSale;
+numSold + numWarriors < maxForSale + 1;

Another option is to explicitly name the variable and incorporate the increment upon declaration to save one addition.

+numSold + numWarriors < maxForSalePlusOne;

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

ForgottenRunesWarriorsGuild.sol:42 string memory baseURI

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:110 IForgottenRunesWarriorsGuild _warriors, address _weth

TOOLS USED

Manual Analysis

MITIGATION

Hardcode the state variables with their initial values instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

ForgottenRunesWarriorsGuild.sol:68 require(
            _exists(tokenId),
            'ERC721Metadata: URI query for nonexistent token'
        );
ForgottenRunesWarriorsGuild.sol:100 require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
ForgottenRunesWarriorsGuild.sol:101 require(_msgSender() == minter, 'Not a minter');
ForgottenRunesWarriorsGuild.sol:114 require(
            _isApprovedOrOwner(_msgSender(), tokenId),
            'ERC721Burnable: caller is not owner nor approved'
        );
ForgottenRunesWarriorsGuild.sol:164 require(payable(msg.sender).send(address(this).balance));
ForgottenRunesWarriorsGuild.sol:174 require(address(msg.sender) != address(0));

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:136: require(numSold < maxDaSupply, 'Auction sold out');
ForgottenRunesWarriorsMinter.sol:137: require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining');
ForgottenRunesWarriorsMinter.sol:138:require(daStarted(), 'Auction not started');
ForgottenRunesWarriorsMinter.sol:139: require(!mintlistStarted(), 'Auction phase over');
ForgottenRunesWarriorsMinter.sol:140: require(
            numWarriors > 0 && numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );
ForgottenRunesWarriorsMinter.sol:146: require(
            msg.value >= (currentPrice * numWarriors),
            'Ether value sent is not sufficient'
        );
ForgottenRunesWarriorsMinter.sol:177: require(numSold < maxForSale, 'Sold out');
ForgottenRunesWarriorsMinter.sol:178: require(mintlistStarted(), 'Mintlist phase not started');
ForgottenRunesWarriorsMinter.sol:179: require(msg.value == finalPrice, 'Ether value incorrect');
ForgottenRunesWarriorsMinter.sol:182: require(mintlistMinted[msg.sender] == false, 'Already minted');
ForgottenRunesWarriorsMinter.sol:187: require(
            MerkleProof.verify(_merkleProof, mintlist1MerkleRoot, node) ||
                MerkleProof.verify(_merkleProof, mintlist2MerkleRoot, node),
            'Invalid proof'
        );
ForgottenRunesWarriorsMinter.sol:207: require(numSold < maxForSale, 'Sold out');
ForgottenRunesWarriorsMinter.sol:208: require(numSold + numWarriors <= maxForSale, 'Not enough remaining');
ForgottenRunesWarriorsMinter.sol:209: require(publicStarted(), 'Public sale not started');
ForgottenRunesWarriorsMinter.sol:210: require(
            numWarriors > 0 && numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );
ForgottenRunesWarriorsMinter.sol:214: require(
            msg.value == (finalPrice * numWarriors),
            'Ether value sent is incorrect'
        );
ForgottenRunesWarriorsMinter.sol:234: require(numClaimed < maxForClaim, 'No more claims');
ForgottenRunesWarriorsMinter.sol:235: require(claimsStarted(), 'Claim phase not started');
ForgottenRunesWarriorsMinter.sol:238: require(claimlistMinted[msg.sender] == false, 'Already claimed');
ForgottenRunesWarriorsMinter.sol:243: require(
            MerkleProof.verify(_merkleProof, claimlistMerkleRoot, node),
            'Invalid proof'
        );
ForgottenRunesWarriorsMinter.sol:258: require(address(recipient) != address(0), 'address req');
ForgottenRunesWarriorsMinter.sol:372: require(selfRefundsStarted(), 'Self refund period not started'); 
ForgottenRunesWarriorsMinter.sol:488: require(
            newPublicStartTime >= newMintlistStartTime,
            'Set public after mintlist'
        );
ForgottenRunesWarriorsMinter.sol:492: require(
            newClaimsStartTime >= newPublicStartTime,
            'Set claims after public'
        );
ForgottenRunesWarriorsMinter.sol:609: require(address(vault) != address(0), 'no vault');
ForgottenRunesWarriorsMinter.sol:611: require(payable(vault).send(_amount));
ForgottenRunesWarriorsMinter.sol:618: require(address(vault) != address(0), 'no vault');
ForgottenRunesWarriorsMinter.sol:619: require(payable(vault).send(address(this).balance));
ForgottenRunesWarriorsMinter.sol:630: require(address(msg.sender) != address(0));;

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in ForgottenRunesWarriorsMinter.sol:

Replace

require(address(msg.sender) != address(0));

with

if (address(msg.sender) == address(0)) {
		revert IsAddressZero();
}

and define the custom error in the contract

error IsAddressZero();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

ForgottenRunesWarriorsGuild.sol:24: uint256 public numMinted = 0;

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:162: uint256 i = 0;
ForgottenRunesWarriorsMinter.sol:220: uint256 i = 0;
ForgottenRunesWarriorsMinter.sol:259: uint256 i = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:162
ForgottenRunesWarriorsMinter.sol:220
ForgottenRunesWarriorsMinter.sol:259
ForgottenRunesWarriorsMinter.sol:355

TOOLS USED

Manual Analysis

MITIGATION

change i++ to ++i.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter:141:
require(
            numWarriors > 0 && numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );
ForgottenRunesWarriorsMinter:211:require(
            numWarriors > 0 && numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );

TOOLS USED

Manual Analysis

MITIGATION

Break down the require statement in multiple require statements (or better, use custom errors)

-require(
            numWarriors > 0 && numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );
+require(numWarriors > 0);
require(numWarriors <= 20,
            'You can summon no more than 20 Warriors at a time'
        );

Signatures Verifications are cheaper than Merkle Proofs

IMPACT

For presales/whitelists with over 127 participants, signature verification is cheaper than Merkle proofs.

PROOF OF CONCEPT

More information can be found in this article

TOOLS USED

Manual Analysis

MITIGATION

Use a signature verification system instead of merkle proofs to verify eligibility to mint.

-function claimSummon(bytes32[] calldata _merkleProof)
        external
        nonReentrant
        whenNotPaused
    {
        //function logic
        require(
            MerkleProof.verify(_merkleProof, claimlistMerkleRoot, node),
            'Invalid proof'
        );
+function claimSummon(bytes[] calldata _signature)
        external
        nonReentrant
        whenNotPaused
    {
        //function logic
        require(
        claimListSigningAddress ==
            keccak256(
                abi.encodePacked(
                    "\x19Ethereum Signed Message:\n32",
                    bytes32(uint256(uint160(msg.sender)))
                )
            ).recover(_signature),
        "not allowed"
    );

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

ForgottenRunesWarriorsGuild.sol

ForgottenRunesWarriorsGuild.sol:104: numMinted bounded by MAX_WARRIORS == 16000, overflow check unnecessary

ForgottenRunesWarriorsMinter.sol

ForgottenRunesWarriorsMinter.sol:284: lowestPrice is lower than startPrice, underflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block