Forgotten Runes Warrior Guild contest - sorrynotsorry's results

Lines of code

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L399-L404 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsMinter.sol#L627-L631 https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L173-L176

Vulnerability details

Impact

Incorrect ERC20 interface Incorrect return values for ERC20 functions. A contract compiled with Solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.

Proof of Concept

    function _safeTransferETHWithFallback(address to, uint256 amount) internal {
        if (!_safeTransferETH(to, amount)) {
            IWETH(weth).deposit{value: amount}();
            IERC20(weth).transfer(to, amount);
        }
    }

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

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

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

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

https://github.com/code-423n4/2022-05-runes/blob/060b4f82b79c8308fe65674a39a07c44fa586cd3/contracts/ForgottenRunesWarriorsGuild.sol#L173-L176

Token.transfer does not return a boolean. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC20 interface implementation.Alice's contract is unable to interact with Bob's contract.

Reference

Tools Used

Manual Review

Recommended Mitigation Steps

Set the appropriate return values and types for the defined ERC20 functions.

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

• transfer and send methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract participates in the auction. Reference Link -1, Reference Link -2

QA (LOW / NONCRITICAL)

• Floating Pragma used in ForgottenRunesWarriorsGuild.sol, ForgottenRunesWarriorsMinter.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) 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. Reference

• It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks. Different versions of Solidity compilers are used (0.8.0. and 0.8.6)

• At ForgottenRunesWarriorsMinter.sol There is no address(0) check for setWarriorsAddress() function. Also to avoid errors, the new address can be checked against existing address like require(_newWarriorsAddress != warriors, "err")

• There are no events on the codebase except the OZ libraries. For the transparency and trackability options, it should be emitting events. Events aid in the visibility of contract state changes. This information can be used on the dApp frontend. Reference

• SPDX license identifier not provided in both source files for ForgottenRunesWarriorsGuild.sol, ForgottenRunesWarriorsMinter.sol. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.

• There are many condition check requirements for ForgottenRunesWarriorsGuild.sol, ForgottenRunesWarriorsMinter.sol. require statements are added to the required methods insde code snippets with @audit-info mark as below.

For ForgottenRunesWarriorsMinter.sol;

function setDaStartTime(uint256 _newTime) public onlyOwner {
    require(_newTime > daStartTime, "err" ); @audit-info <<-- Added require statement
    daStartTime = _newTime;
}

function setMintlistStartTime(uint256 _newTime) public onlyOwner {
    require(_newTime > mintlistStartTime, "err" ); @audit-info <<-- Added require statement
    mintlistStartTime = _newTime;
}

function setPublicStartTime(uint256 _newTime) public onlyOwner {
    require(_newTime > publicStartTime, "err" ); @audit-info <<-- Added require statement
    publicStartTime = _newTime;
}

function setClaimsStartTime(uint256 _newTime) public onlyOwner {
    require(_newTime > claimsStartTime, "err" ); @audit-info <<-- Added require statement
    claimsStartTime = _newTime;
}

function setSelfRefundsStartTime(uint256 _newTime) public onlyOwner {
    require(_newTime > selfRefundsStartTime, "err" ); @audit-info <<-- Added require statement
    selfRefundsStartTime = _newTime;
}

function setMintlist2MerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
    require(newMerkleRoot != mintlist2MerkleRoot, "Existing mintlist2MerkleRoot" ); @audit-info <<-- Added require statement
    require(newMerkleRoot != bytes32(0), "Can't set to Zero" ); @audit-info <<-- Added require statement   
    mintlist2MerkleRoot = newMerkleRoot;
}

function setClaimlistMerkleRoot(bytes32 newMerkleRoot) public onlyOwner {
    require(newMerkleRoot != claimlistMerkleRoot, "Existing claimlistMerkleRoot" ); @audit-info <<-- Added require statement
    claimlistMerkleRoot = newMerkleRoot; 
}

function setWarriorsAddress(
    IForgottenRunesWarriorsGuild _newWarriorsAddress
) public onlyOwner {
    require(_newWarriorsAddress != warriors, "Existing Warrior Address" ); @audit-info <<-- Added require statement    
    require(_newWarriorsAddress != address(0), "Can't set to address(0)" ); @audit-info <<-- Added require statement   
    warriors = _newWarriorsAddress; 
}

function setStartPrice(uint256 _newPrice) public onlyOwner {
    require(_newPrice != 0, "Price can't be zero" ); @audit-info <<-- Added require statement 
    require(_newPrice != startPrice, "Existing price" ); @audit-info <<-- Added require statement 
    require(_newPrice > lowestPrice, "Must be GT lowestPrice" ); @audit-info <<-- Added require statement 
    startPrice = _newPrice; 
}

function setLowestPrice(uint256 _newPrice) public onlyOwner {
    require(_newPrice != 0, "Price can't be zero" ); @audit-info <<-- Added require statement 
    require(_newPrice != lowestPrice, "Existing price" ); @audit-info <<-- Added require statement 
    require(_newPrice < startPrice, "Must be LT startPrice" ); @audit-info <<-- Added require statement 
    lowestPrice = _newPrice;
}

function setDaPriceCurveLength(uint256 _newTime) public onlyOwner {
    require(_newTime != 0, "Can't set to zero"); @audit-info <<-- Added require statement 
    require(_newTime != daPriceCurveLength, "Existing Curve Length"); @audit-info <<-- Added require statement 
    require(_newTime <= (mintlistStartTime - daStartTime), "MintlistSummon has started"); @audit-info <<-- Added require statement 
    daPriceCurveLength = _newTime; 
}

function setDaDropInterval(uint256 _newTime) public onlyOwner {
    require(_newTime != 0, "Can't set to zero"); @audit-info <<-- Added require statement 
    require(_newTime != daDropInterval, "Existing drop rate"); @audit-info <<-- Added require statement 
    daDropInterval = _newTime;
}

function setFinalPrice(uint256 _newPrice) public onlyOwner {
    require(_newPrice != 0, "Can't airdrop on FCFS basis"); @audit-info <<-- Added require statement 
    require(_newPrice != finalPrice, "Existing final price"); @audit-info <<-- Added require statement 
    require(_newPrice < finalPrice, "There should be a discount to sell those out"); @audit-info <<-- Added require statement 
    finalPrice = _newPrice;
}

function setMaxForSale(uint256 _newSupply) public onlyOwner {
    require(_newSupply != 0, "Can't set to zero"); @audit-info <<-- Added require statement 
    require(_newSupply != maxForSale, "Existing supply"); @audit-info <<-- Added require statement 
    require(_newSupply > numSold, "Can't be LT sold NFT's"); @audit-info <<-- Added require statement 
    maxForSale = _newSupply;
}

function setMaxForClaim(uint256 _newSupply) public onlyOwner {
    require(_newSupply != 0, "Can't set to zero"); @audit-info <<-- Added require statement 
    require(_newSupply != maxForClaim, "Can't set to zero"); @audit-info <<-- Added require statement 
    require(_newSupply > numClaimed, "Can't be LT total claimed"); @audit-info <<-- Added require statement 
    maxForClaim = _newSupply;
}

function setVaultAddress(address _newVaultAddress) public onlyOwner { 
    require(_newVaultAddress != address(0), "Can't set to zero"); @audit-info <<-- Added require statement 
    vault = _newVaultAddress; 
}

function withdraw(uint256 _amount) public onlyOwner {
    require(address(vault) != address(0), 'no vault');
    require(_amount <= address(this).balance,"Insufficient balance");@audit-info <<-- Added require statement 
    require(payable(vault).send(_amount)); 
    }

For ForgottenRunesWarriorsGuild.sol;

function initialize(address newMinter) public onlyOwner {
    require(newMinter != address(0), "No address(0) allowed"); @audit-info <<-- Added require statement 
    setMinter(newMinter);
}

function mint(address recipient) 
        public
        override
        nonReentrant
        returns (uint256)
    {   
        require(recipient != address(0), "No minting to zero address"); @audit-info <<-- Added require statement 
        require(recipient != address(this), "No minting to contract address"); @audit-info <<-- Added require statement 
        require(numMinted < MAX_WARRIORS, 'All warriors have been summoned');
        require(_msgSender() == minter, 'Not a minter');
        uint256 tokenId = numMinted;
        _safeMint(recipient, tokenId);
        numMinted += 1;
        return tokenId;
}

function setBaseURI(string memory newBaseURI) public onlyOwner {
    require(newBaseURI.length != 0, "No empty strings"); @audit-info <<-- Added require statement 
    require(newBaseURI != baseTokenURI , "No empty strings"); @audit-info <<-- Added require statement 
    baseTokenURI = newBaseURI; 
}

function setMinter(address newMinter) public onlyOwner {
    require(newMinter != minter , "Same minter"); @audit-info <<-- Added require statement 
    require(newMinter != address(0) , "No zero address"); @audit-info <<-- Added require statement 
    minter = newMinter; 
}

function setProvenanceHash(string memory newHash) public onlyOwner {
    require(newHash.length != 0, "No empty strings"); @audit-info <<-- Added require statement 
    require(newHash != METADATA_PROVENANCE_HASH , "Existing hash"); @audit-info <<-- Added require statement 
    METADATA_PROVENANCE_HASH = newHash; 
}

• The require statements in following LOC's don't throw error messages on revert. When transactions revert, the users won't receive error messages indicating the cause of the failure; ForgottenRunesWarriorsGuild.sol#L165, ForgottenRunesWarriorsGuild.sol#L174, ForgottenRunesWarriorsGuild.sol#L175, ForgottenRunesWarriorsMinter.sol#L611 , ForgottenRunesWarriorsMinter.sol#L619, ForgottenRunesWarriorsMinter.sol#L629

• The require statement on ForgottenRunesWarriorsGuild.sol#L174 is reduntant since onlyOwner modifier is in action and msg.sender can't be address(0) (tautology). Same issue with ForgottenRunesWarriorsMinter.sol#L629 at forwardERC20s() function.

• At ForgottenRunesWarriorsGuild.sol#L73 ,Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Instead abi.encode() can be used. Reference Link

• transfer and send methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract participates in the auction. Reference Link -1, Reference Link -2

• At ForgottenRunesWarriorsMinter.sol#L112 , setWethAddress() function is redundant as this well known address can be declared constant / immutable or initialized at the constructor / initializer.

• There should be a require statement as below to check whether the claimlistMerkleRoot is initialized in line ForgottenRunesWarriorsMinter.sol#L242 else the claimSummon() function will revert.

require(claimlistMerkleRoot != bytes32(0), "claimlistMerkleRoot is not set");

• At ForgottenRunesWarriorsMinter.sol#L403, _safeTransferETHWithFallback() function, the success return is not checked. While the call should never fail when the contract addresses are correct, it's still recommended checking the success return value of these calls.

• _safeTransferETH() function of ForgottenRunesWarriorsMinter.sol has 30.000 gas limit to prevent the contract logic to pass to the other side if the callee is a contract. However, it still may not be sufficient if the receiver is a contract fallback even without malicious intentions. Also there should not be any hardcoded gas limits in calls. Reference

• setWethAddress() of ForgottenRunesWarriorsMinter.sol is reduntant since original WETH address will be used in the contract as verified in Discord. This can be achieved via constructor / initializer or setting it to immutable / constant variable.

• At ForgottenRunesWarriorsMinter.sol#L304 , the logic should be;

return block.timestamp >= daStartTime;

instead of;

return block.timestamp > daStartTime;

• block.timestamp is used on many places at the scoped contracts. Hoewever, this can be manipulated by malicious miners. Reference

• It would be more consistent and safe in boundries if the summon phase start and expire times can be explicitly set as state variables. The contract is lacking of expire times of these windows and dependent to block.timestamp only.

GAS OPTIMIZATIONS

• The team can consider to use immutable instead of constant for MAX_WARRIORS variable in line ForgottenRunesWarriorsGuild.sol#L21

• Appropriate Auction Variables (not the mappings) can be included in a struct in uint32 rather than allocating every variable a slot (uint256). This will save ton of a gas.

• WETH address is said to be used as official WETH. So the variable of WETH address in line ForgottenRunesWarriorsMinter.sol#L54 can be directly be immutable as address public immutable WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;

• Error strings more than 32 bytes consume much more gas as in line ForgottenRunesWarriorsGuild.sol#L70. Team can consider to reduce the message length AND use custom errors for each revert scenario which will save lots of gas.

• The logic can be inlined in refundAddress() function instead of calling _refundAddress() function in ForgottenRunesWarriorsMinter.sol#L366

• At ForgottenRunesWarriorsMinter.sol#L296 the subtraction can be uncheckedsince the condition is assesed in ForgottenRunesWarriorsMinter.sol#L293

• selfRefund() function visibility is set as public where onlyOwner modifier is not present. To save gas, the visibility can be set to external .

• Many functions are set with public visibility and onlyOwner modifier. However, these can be set in external visibility to save gas since the owner will be msg.sender, not the contract itself as per OZ Ownable.sol, at the time of contract deployment.

• There can be a nested mapping(address => mapping(uint256 => purchases)) public minters for the minters and sales in order to save more gas.

• The looping should not have state variable inside to avoid SLOADS. Reference