Forgotten Runes Warrior Guild contest - AuditsAreUS's results

Lines of code

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

Vulnerability details

Impact

The function forwardERC20s() transfers ERC20 tokens out of the contract to the owner. However, it does not properly handle non-standard ERC20 tokens such as USDT which do not return a bool when the transfer is called.

The issue is that token is of type IERC20 which expects a bool to be returned. However USDT does not return any data. Thus, when token.transfer(msg.sender, amount); attempts to decode the bool return parameter (even though it's not used it still attempts to decode it) the decoding fails and the transaction reverts.

The impact is that it is impossible to transfer non-standard ERC20 tokens out of the contract.

Proof of Concept

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

Recommended Mitigation Steps

Consider using OpenZepplin's SafeERC20 for the transfer calls.

Lines of code

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

Vulnerability details

Impact

It is possible for the owner to withdraw all Ether from the protocol including the amount that should be refunded to the Dutch Auction participants.

This poses a centralisation risk and undermines the effectiveness of the Dutch Auction.

Proof of Concept

The functions withdraw() and withdrawAll() allow the owner to withdraw any arbitrary amount of Ether from the contract.

    function withdraw(uint256 _amount) public onlyOwner {
        require(address(vault) != address(0), 'no vault');
        require(payable(vault).send(_amount));
    }

    /**
     * @notice Withdraw all funds to the vault
     */
    function withdrawAll() public payable onlyOwner {
        require(address(vault) != address(0), 'no vault');
        require(payable(vault).send(address(this).balance));
    }

Recommended Mitigation Steps

Consider preventing the owner from withdrawing amounts that are attributed to the dutch auction.

This can be done by counting the number of warriors sold in the auction in addition to the total amount paid for in the auction stage. The withdrawable amount by the owner will be withdrawable = address(this).balance - numSoldInDa * finalPrice.

Note withdrawals should only be allowed to be made after the finalPrice is set and the finalPrice should not be modifiable after it has been set.

Lines of code

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

Vulnerability details

Impact

It is possible for the owners to modify the variables startPrice and lowestPrice during an auction.

Modifying these variables will cause the price mechanics of the curve to change. This will significantly impact the finalPrice of the auction and also how much each future user will pay during the auction including the users who have already bid.

Proof of Concept

For example a user wishing to pay a maximum of 1 ETH bids part way through the auction when current price has decreased to 1 ETH. If the auction continues and the price continues to drop the users will be attributed a refund at the end of the auction. However if the owner modifies the lowestPrice to increase it to above 1 ETH and the auction does not sell out, the user will not be able to receive a refund since the lowestPrice is now greater than the amount the user paid. Thus, refundOwed() will overflow on the line 391 return daAmountPaid[minter] - totalCostOfMints - refundsPaidAlready; cause any refund transaction to revert.

    function setStartPrice(uint256 _newPrice) public onlyOwner {
        startPrice = _newPrice;
    }

    /**
     * @notice set the dutch auction lowest price
     */
    function setLowestPrice(uint256 _newPrice) public onlyOwner {
        lowestPrice = _newPrice;
    }

Recommended Mitigation Steps

We reocmmend removing the functions setStartPrice() and setLowestPrice() such that they can only be called during the constructor.

Alternatively only allow these variables to be set before the auction has started.

Unused Pause Functions

The functions pause() and unpause() in ForgottenRunesWarriorsMinter.sol are not used.

Consider removing the functions and also the Pausable inheritance.

Unnecessarily Permissive Functions

The following functions allow changes from the owner that could significantly distrupt the contract execution.

It is recommended each of the following functions is removed.

• setWarriorAddress()
• setWethAddress()
• setMaxDaSupply()
• setMaxForSale()
• setMaxClaimFor()
• setDaPriceCurveLength()
• setDaDropInterval()

The impact of changing any of these functions can cause a DoS attack on the contract. Whether this is deliberate of by misconfiguration it could prevent normal execution of the sale.

Start Time Functions Should Be Validated

Each of the following functions allows modifying the start time of different phases without verifying that any of the phases and valid and non-overlapping.

• setDaStartTime()
• setMintListStartTime()
• setPublicStartTime()
• setClaimStartTime()

Consider removing these functions in favour of setPhaseTimes() which should set the storage variables directly

Furthermore, add the additional requirements to setPhaseTimes(). First that the times can only be set once. Then that the daStartTime is at a time greater than or equal to now such that it has not already passed and finally that the mintlist phase occurs after the dutch auctions.

If the mintlist phase occurs before the dutch auction then finalPrice will still be zero and users will not have to pay for the warriors.

require(daStartTime == (uint256).max)
require(newDaStartTime >= now)
require(newMintlistStartTime > newDaStartTime + daPriceCurveLength);

mintlistSummon() Should Require finalPrice To Be Set

If mintlistSummon() is called before finalPrice is set then the users will not need to pay for the warriors.

We recommend adding the requirement that the finalPrice > 0 before this function may be called.

If issueRefunds() or refundAddress() is called before finalPrice Is Set Users Get A Full Refund

If owners call issueRefunds() or refundAddress() before finalPrice is set users get a full refund.

We recommend adding the requirement that finalPrice > 0 in each of these functions to prevent over paying bidders.

Division Before Multiplication In currentDaPrice()

On lines 284-285 there is a division before a second division which may introduce rounding errors.

        uint256 dropPerStep = (startPrice - lowestPrice) /
            (daPriceCurveLength / daDropInterval);

Consider performing a multiplication before the division. e.g.

        uint256 dropPerStep = (startPrice - lowestPrice) * daDropInterval / daPriceCurveLength;