Forgotten Runes Warrior Guild contest - TerrierLover's results

ForgottenRunesWarriorsGuild.sol

Naming of constants should use capital letters with underscores.

According to the solidity document, the recommendation of naming of the state variable is using mixedCase.

https://docs.soliditylang.org/en/v0.8.11/style-guide.html#local-and-state-variable-names

Use mixedCase. Examples: totalSupply, remainingSupply, balancesOf, creatorAddress, isPreSale, tokenExchangeRate.

Currently, METADATA_PROVENANCE_HASH is a state variable, and it seems not following the recommendation by solidity.

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

string public METADATA_PROVENANCE_HASH = '';

It should use mixedCase as the document suggests like this.

string public metadataProvenanceHash = '';

No need to override tokenURI function

tokenURI function is defined in the code, but @openzeppelin/contracts/token/ERC721/ERC721.sol already has full implementation.

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

function tokenURI(uint256 tokenId)
    public
    view
    virtual
    override
    returns (string memory)
{
    require(
        _exists(tokenId),
        'ERC721Metadata: URI query for nonexistent token'
    );

    return string(abi.encodePacked(baseTokenURI, tokenId.toString()));
}

The above mentioned codes can be reduced while keeping the almost same functionality.

Here is a definition of the ERC721 codebase. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L93-L98

function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
    require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token");

    string memory baseURI = _baseURI();
    return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : "";
}

The difference is that the OZ's ERC721 checks if the baseURI is defined or not. This additional check seems to be a good check to keep.

Using either _msgSender() or msg.sender only for consistency

msg.sender is used at following codes. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L164 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L174-L175

On the other hand, _msgSender() is used here. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L101

The code should use either msg.sender or _msgSender() for the consistency.

---

ForgottenRunesWarriorsMinter.sol

Extra parentheses exist

These codes have parentheses wrapping currentPrice * numWarriors or finalPrice * numWarriors.

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

require(
    msg.value >= (currentPrice * numWarriors),
    'Ether value sent is not sufficient'
);

Technicaly these parentheses are not required. It can remove them like this:

require(
    msg.value >= currentPrice * numWarriors,
    'Ether value sent is not sufficient'
);

---

Should pay attention to which ERC20 token forwardERC20s function allows

forwardERC20s function at both ForgottenRunesWarriorsGuild and ForgottenRunesWarriorsMinter accepts any address. If the attacker sets the malicious ERC20 whose transfer function has malicious code, it potentially steal WETH stored in ForgottenRunesWarriorsMinter contract. Only owner can call forwardERC20s, so the owner should be careful which ERC20 token is used in this function.

ForgottenRunesWarriorsGuild.sol

address function is not needed for converting msg.sender to address

address function used on msg.sender.

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

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

The type of msg.sender is address, so there is no need to use address function on msg.sender. Removing address can reduce the gas fee.

No need to override tokenURI function

tokenURI function is defined in the code, but @openzeppelin/contracts/token/ERC721/ERC721.sol already has full implementation.

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

function tokenURI(uint256 tokenId)
    public
    view
    virtual
    override
    returns (string memory)
{
    require(
        _exists(tokenId),
        'ERC721Metadata: URI query for nonexistent token'
    );

    return string(abi.encodePacked(baseTokenURI, tokenId.toString()));
}

The above mentioned codes can be reduced while keeping the almost same functionality.

Here is a definition of the ERC721 codebase. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L93-L98

function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
    require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token");

    string memory baseURI = _baseURI();
    return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : "";
}

Not implementing tokenURI again can reduce the deployment gas fee.

address(0) check seems to be not needed at forwardERC20s function

This part checks if msg.sender is not address(0).

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

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

I cannot think of a case where msg.sender becomes 0. In addition, this function is only called by owner, and owner will not be address(0), so this check itself seems not needed. Not having a check can reduce the gas cost.

No need to set 0 at numMinted state variable

numMinted is set to 0. The default value of uint256 is 0, so there is no need to set 0 at numMinted variable.

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

uint256 public numMinted = 0;

It can be rewritten like this:

uint256 public numMinted;

This can remove the gas cost.

Using unchecked at mint functioin can reduce gas cost

Since numMinted will not be greater than MAX_WARRIORS.

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

require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
...
numMinted += 1;

Therefore, numMinted += 1 will not be overflown. So it can be rewritten like this to reduce the gas cost.

unchecked {
    numMinted += 1;
}

---

ForgottenRunesWarriorsMinter.sol

Use != 0 instead of > 0 to reduce gas cost

Following codes use > 0 on uint256 variable. uint256 will never be less than 0, so it can be replaced with != 0. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L141

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

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

By using != 0 instead of > 0, it can reduce gas cost.

No need to set = 0 for uint256 variable

Following codes set uint256 i = 0. 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

The default value of uint256 variable is 0, so there is no need to set = 0. It can be rewritten like this:

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

Using unchecked in several for loops

In the for loop, i++ is used at the following codes.

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

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

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

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

At each for loop, i < numWarriors or i < count or i < endIdx is set as the end condition. numWarriors or count will not be overflown. Therefore, it can be rewritten like this to reduce gas cost.

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

address(0) check seems to be not needed at forwardERC20s function

This part checks if msg.sender is not address(0).

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

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

I cannot think of a case where msg.sender becomes 0. In addition, this function is only called by owner, and owner will not be address(0), so this check itself seems not needed. Not having a check can reduce the gas cost.

address function is not needed for converting msg.sender to address

address function used on msg.sender or variable whose type is address at several places.

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

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

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

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

require(address(vault) != address(0), 'no vault');

The type of msg.sender is address, so there is no need to use address function on msg.sender. Removing address can reduce the gas fee. It can be written as follows:

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

require(vault != address(0), 'no vault');

Using ! instead of == false can reduce gas cost

Following codes use == false. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L182 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L238

By using !, it can reduce the gas cost.

require(!mintlistMinted[msg.sender], 'Already minted');

Using unchecked at bidSummon and publicSummon functions can reduce gas cost

Following code can be wrapped by unchecked. Since the code has require(numSold + numWarriors <= maxDaSupply, 'Not enough remaining') check, numSold + numWarriors will not be overflown.

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

numSold += numWarriors;

It can write like this to reduce the gas cost.

unchecked {
    numSold += numWarriors;
}

Using unchecked at mintlistSummon and claimSummon functions can reduce gas cost

Following code can be wrapped by unchecked. Since the code has require(numSold < maxForSale, 'Sold out'), it will not be overflown. https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L193 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L248

numSold += 1;

It can write like this to reduce the gas cost.

unchecked {
    numSold += 1;
}