Forgotten Runes Warrior Guild contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Being a registered US-based company isn't really a guarantee for not rugging your users. You're still able to take out all the contract's funds at any time and leave. Especially non-US-based users will have a hard time taking legal action. Even for US-based people, it should be difficult. AFAIK there are no past verdicts.

So instead, you should limit the withdraw() and withdrawAll() functions.

Originally I included this in my QA report. But, since this affects actual user funds I submit it as a MED issue. The chance of the owner rugging the users with this function is there. However small that number might be. Being a US-based company is no excuse for introducing these risks in your contract.

Proof of Concept

The owner calls withdrawAll() to take out all the ETH after the dutch auction. Users won't be able to call the selfRefund() function since there is no ETH left.

Tools Used

none

Recommended Mitigation Steps

At the worst, the total amount to be refunded should be (startPrice - finalPrice) * numSold. The withdrawal functions should only allow you to take out anything beyond that.

Since it is difficult to know whether everybody claimed their refund, I'd propose to introduce a timestamp at which refunds aren't possible anymore, e.g 30 days. Anybody who hasn't claimed their refund after that gives it up.

function withdraw(uint256 _amount) public onlyOwner {
    uint reserved;
    if (block.timestamp < selfRefundsStartTime + 30 days) {
      uint reserved = (startPrice - finalPrice) * numSold;
    }
    require(_amount <= address(this).balance - reserved);
    require(address(vault) != address(0), 'no vault');
    require(payable(vault).send(_amount));
}

function withdrawAll() public payable onlyOwner {
    uint reserved;
    if (block.timestamp < selfRefundsStartTime + 30 days) {
      uint reserved = (startPrice - finalPrice) * numSold;
    }
    require(address(vault) != address(0), 'no vault');
    require(payable(vault).send(address(this).balance - reserved));
}

There are better solutions than that. For example, you can keep track of the total amount owed by having an array that stores all the addresses that bought a token through the dutch auction. Then you can loop through that array and check daAmountPaid and daNumMinted to find out how much they paid. From there you can derive the total amount that should be refunded. But, that's a little more work.

Report

The contract has many configurable properties. All of them are critical to the launch and beyond. There are almost no constraints on the configuration. Before the sale, while it's still running, and after it as well.

In the current NFT space we've seen how the owners are able to abuse the lack of constraints to rug their users. My report consists mostly of these constraints and why they should be added. Evaluating the risk of these is a little difficult. They all depend on the actions of the contract's creator. In the past, those were considered LOW at best so I decided to stick them all into the QA submission.

I think they have benefits for the creators and the users. They decrease the chance of errors in the configuration and allow the user to trust that there won't be any shenanigans with the sale or the collection itself.

• Set constraints for sale time configurations
• Self refund start time isn't set in setPhaseTimes()
• Allows owner to mint more tokens than planned
• Owner is able to freely set the final price
• Don't allow the anything to be configured after the sale has started
• Don't allow the WETH address to be changed
• Don't allow the address of the ERC721 token to be changed

Set constraints for sale time configurations

As you remarked yourself, the configuration of the sale has to be done correctly.

While you can trust in yourself to configure them correctly, it makes sense to put constraints into the code to only allow certain values. You decrease the risk of errors and increase the trust of your users.

In the setPhaseTimes() function you try to do that but you're missing some critical ones IMO.

Currently, you have the following constraints:

require(
    newPublicStartTime >= newMintlistStartTime,
    'Set public after mintlist'
);
require(
    newClaimsStartTime >= newPublicStartTime,
    'Set claims after public'
);

But what you're missing is:

• newDaStartTime <= newMintlistStartTime. If the DA start time is not smaller than the mintlist's nobody will be able to mint through the auction because of the require statement here
• newMintlistStartTime - newDaStartTime >= daPriceCurveLength. Otherwise, there won't be enough time for the auction to reach the lowest price point.

In addition to the setPhaseTimes() function, there's the possibility to set each individual price point through a separate function. There you don't have any constraints at all. I'd encourage you to add them there as well.

Self refund start time isn't set in setPhaseTimes()

The setPhaseTimes() function is used as a convenience function to configure all the timestamp related variables. But, it doesn't set the sefRefundsStartTime.

Self refunds are supposed to be a safety mechanism for users to trigger the refund themselves if the owner doesn't do it. According to the README, self refunds should be possible after phase 1. Thus, it's related to the daStartTime variable and should be configured at the same place.

Add:

require(newSelfRefundsStartTime >= daStartTime + daPriceCurveLength);
setSelfRefundsStartTime(newSelfRefundsStartTime);

Allows owner to mint more tokens than planned

There are two ways for the owner of the contract to mint tokens for themselves unrelated to the public sale:

• through the [teamSummon()](https://github.com/code-423n4/2022-05-runes/blob/main/contracts/ForgottenRunesWarriorsMinter.sol#L257-L262) function
• or by setting the minter to their own address with setMinter() and then minting through mint()

In both cases there's only one limiting factor. The max supply set to 16000. Technically, the owner is able to mint all 16000 for themselves. But, you obviously wouldn't do that. There's no value in it. In the README it says that the owner is supposed to get about ~709 tokens distributed accross multiple parties.

I'd advise to set strict limits in the code to only allow the specified mints for the owner instead of having a general mint function for owners. It builds trust with your users and protects you from accidentally minting too many.

One easy approach would be to keep track of tokens minted by the owner and set a limit to teamSummon() like this:

function teamSummon(address recipient, uint256 count) external onlyOwner {
    require(ownerMints + count <= 709);
    ownerMints += count;
    require(address(recipient) != address(0), 'address req');
    for (uint256 i = 0; i < count; i++) {
        _mint(recipient);
    }
}

Owner is able to freely set the final price

Again, a constraint issue. In your current setup, the finalPrice is only set if the auction sells all the tokens. Otherwise, the owner sets the value.

But, you allow yourself to set any arbitrary price in setFinalPrice(). Instead, it should only be possible to set it to the value of lowestPrice. That would be the actual final possible price.

Change it to:

function setFinalPrice() public onlyOwner {
    finalPrice = lowestPrice;
}

Don't allow the anything to be configured after the sale has started

Currently, the owner is able to freely configure anything even after the dutch auction has started. That's against the interest of the user and shouldn't be allowed. The owner could increase the sale price, or decrease the number of tokens to be sold. They could end the auction early and let it run for longer.

Add the following limitation to the configuration functions (works because of the initial value of daStartTime):

require(block.timestamp < daStartTime);

Following functions should have the limitation:

• setDaStartTime
• setMintlistStartTime
• setPublicStartTime
• setClaimsStartTime
• setSelfRefundsStartTime
• setPhaseTimes
• setMintlist1MerkleRoot
• setMintlist2MerkleRoot
• setClaimlistMerkleRoot
• setStartPrice
• setLowestPrice
• setDaPriceCurveLength
• setDaDropInterval
• setMaxDaSupply
• setMaxForSale
• setMaxForClaim

When the dutch auction starts everything should be already configured. The auction itself but also all the other phases which start after it.

Don't allow the WETH address to be changed

There's no scenario where the address of the WETH contract changes. There's no benefit in allowing the variable to be changed after deployment. Instead, set it directly in the constructor and remove the setWethAddress() function:

constructor(IForgottenRunesWarriorsGuild _warriors, address _weth) {
    weth = _weth;
    setWarriorsAddress(_warriors);
    setVaultAddress(msg.sender);
}

Also, make the weth state variable immutable to save gas.

Don't allow the address of the ERC721 token to be changed

There's no scenario where the address of the token would ever change. So this is the same thing as the WETH issue mentioned above.

I'd suggest to remove the function and set the value in the constructor directly.