Forgotten Runes Warrior Guild contest - rfa's results

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

Title: Using SafeERC20 library in ForgottenRunesWarriorsMinter.sol

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

There are some token which are not implementing current ERC20 standard (example: USDT, OmiseGo and BNB). Using SafeERC20 library will be nice to prevent them stuck when the transaction failed.

using SafeERC20 for IERC20;


    function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
        require(address(msg.sender) != address(0));
        IERC20(token).safeTransfer(msg.sender, amount);

Lines of code

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

Vulnerability details

Impact

Posibilty of DOS at withdraw() and withdrawAll functions

Proof of Concept

payable.send() is considered the go-to solution for making payments to EOAs or other smart contracts. But since the amount of gas it forward to the called smart contract is very low, the called smart contract can easily run out of gas. This would make the payment impossible.

This might possible if the receiving contract is't designed carefully not to exceed the gas limit. Those function has a critical role to withdraw all the funds. So it is better to use payable.call

Tools Used

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

Use call() method instead of send()

QA REPORTS

Title: Implement check effect interaction when calling _safemint()

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L103-L104 Although I can't found any vulnerability in current implementation, its better doing state update before doing external call to prevent reentrancy

RECOMMENDED MITIGATION STEP:

	numMinted += 1; // @audit-info: Better updating state before _safeMint()
        _safeMint(recipient, tokenId);

Title: Unnecessary initialize() function

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L52-L54 The function doesn't have any modifier (as how most initializer function implemented) which make it called only once when the contract is deployed, and it also have same goal as directly call setMinter() function. I recommend to initialize the minter in the constructor() (To prevent minter == address(0)), removing initial function, and use setMinter() instead of calling initialize() to set minter which can save both deployment and execution gas fee:

RECOMMENDED MITIGATION STEP:

    constructor(string memory baseURI, address newMinter)
        ERC721('ForgottenRunesWarriorsGuild', 'WARRIORS')
    {
        setBaseURI(baseURI);
	setMinter(newMinter);
    }

Title: Throw false error

Occurrences: 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

If the user input 0 it will throw error message: 'You can summon no more than 20 Warriors at a time' I recommend to change the error message: 'You can summon 0 or no more than 20 Warriors at a time'

Title: No check that _newTime > daStartTime (+ expected duration)

https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L448-L450 If mintListStartTime < daStartTime (+ expected duration) is happened, bidSummon() will be DOS function. It's quite easy to handle, but for better implementation I recommend to check it

    function setMintlistStartTime(uint256 _newTime) public onlyOwner {
	require(_newTime > daStartTime + duration); //@auditInfo: var duration(estimated) can be determined by admin/owner. 
        mintlistStartTime = _newTime;
    }

It also can be implemented on all function which set publicStartTime and claimsStartTime https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L455-L464

Those implementation were implemented in setPhaseTime function (only for newPublicStartTime and newClaimsStartTime). But not when we call each function alone.

Title: Using SafeERC20 library in ForgottenRunesWarriorsMinter.sol

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

There are some token which are not implementing current ERC20 standard (example: USDT, OmiseGo and BNB). Using SafeERC20 library will be nice to prevent them stuck when the transaction failed.

using SafeERC20 for IERC20;


    function forwardERC20s(IERC20 token, uint256 amount) public onlyOwner {
        require(address(msg.sender) != address(0));
        IERC20(token).safeTransfer(msg.sender, amount);

GAS FINDINGS

Title: Initializing var with default value

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

By declaring var by not set its default value (0 for uint) can save deployment gas cost Change to:

uint256 public numMinted;

Also for i inside for(): Occurrences: 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#L259-L261

Title: Using calldata to store string parameter

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

Using calldata to store read only string can save gas:

    constructor(string calldata baseURI) {}

Title: Using msg.sender instead _msgSender()

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

Calling msg.sender directly can save more gas instead calling it through function:

        require(msg.sender == minter, 'Not a minter');

Title: Preventing SLOAD in mint() function

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

By caching numMinted earlier in the function can save gas by preventing SLOAD if require() condition is true

RECOMMENDED MITIGATION STEP

	uint256 tokenId = numMinted;
        require(tokenId < MAX_WARRIORS, 'All warriors have been summoned'); //@audit-info: prevent SLOAD here
        require(_msgSender() == minter, 'Not a minter');
        
        _safeMint(recipient, tokenId);
        numMinted += 1;
        return tokenId;

Title: Using && in require is not effective

Occurrences: 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

Using && is consuming 15 more execution gas fee although its cost more on deployment. I recommend to use multiple require() to save gas:

RECOMMENDED MItiGATION STEP:

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

Title: Using prefix increment and unchecked for i in a for() loop

Occurrences: 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#L259-L261

Using prefix increment and unchecked for i can save execution gas fee:

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

Title: Using ++var to do +1 increment

Occurrences: 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

Change:

        numSold += 1;

To:

	++numSold;

It saves 67 execution gas cost per call

Title: Unnecessary checks numSold < maxForsale

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

numSold < maxForSale (L207) will always true if numSold + numWarriors <= maxForSale == true. I recommend to remove L207

Title: Some functions visibility can set to external

Occurrences: 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#L85 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L94 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L113 https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsGuild.sol#L163

Setting the visibility to external instead of public is more effective