Forgotten Runes Warrior Guild contest - shenwilly's results

Lines of code

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

Vulnerability details

Impact

The owner of ForgottenRunesWarriorsMinter.sol contract can transfer all funds in the contract, including the refunds intended to be sent back to DA minters.

While it is unlikely for the owner to steal users' funds, the owner key can still be compromised.

Proof of Concept

• Owner key is compromised by Attacker.
• Attacker waits until minting is done.
• Attacker calls setVaultAddress, setting their address as the new vault.
• Attacker calls withdrawAll, stealing all funds including refunds.

Recommended Mitigation Steps

• Remove setVaultAddress function so fund can't be stolen even if key is compromised.
• Use multi-sig for the ownership.

The code heavily relies on the owner to properly setup and manage the contract during and after the minting process. While the team is fully doxxed, there is still the risk of compromised key and faulty script deployment.

Consider removing setter functions that are unlikely to be called once deployment is done, and add more sanity checks when setting critical variables (ex: minimum/maximum values, zero-address checks).

Low Risk Vulnerabilities

1. Mintlist phase could start at the highest price

In case of DA not minting out, setFinalPrice need to be manually set by the owner.

If there's a problem that prevents the owner from setting the final price before the Mintlist or Public phase started, unaware users might mint at the highest price 2.5e before owner sets the correct price, causing reputational risk for the team.

Recommended Mitigation

Consider adding a new variable isfinalPriceSet to track whether finalPrice has been changed (whether through DA minting out or manually set) and only allow mintlist/public mint when it is set to true.

// bidSummon()
...
if (numSold == maxDaSupply) {
    // optimistic: save gas by not setting on every mint, but will
    // require manual `setFinalPrice` before refunds if da falls short
    finalPrice = currentPrice;
    isfinalPriceSet = true;
}

function setFinalPrice(uint256 _newPrice) public onlyOwner {
    finalPrice = _newPrice;
    isfinalPriceSet = true;
}

function mintlistStarted() public view returns (bool) {
    return isfinalPriceSet && block.timestamp > mintlistStartTime;
}
function publicStarted() public view returns (bool) {
    return isfinalPriceSet && block.timestamp > publicStartTime;
}

2. Missing newDaStartTime input validation

As the DA phase is expected to launch first, there should be a check for this in setPhaseTimes in case of faulty deployment script setting the wrong value for newDaStartTime.

Recommended Mitigation

Add the check for newDaStartTime:

require(
    newMintlistStartTime >= newDaStartTime,
    'Set mintlist after dutch auction'
);

// or alternatively, a stricter check which includes the minimum DA duration:
require(
    newMintlistStartTime >= newDaStartTime + daPriceCurveLength,
    'Set mintlist after dutch auction'
);

3. WETH could get stuck in unsupported contract

When refunding contract minters that didn't implement token transfers, the WETH will be stuck forever in the contract. Consider using a pull approach and let the minters claim the refund by themselves.

In case the contract didn't implement selfRefund, it's still safe as team can withdraw the unclaimed refund and manually transfer it to the minter.

Recommended Mitigation

Consider using pull approach by reducing daAmountRefunded on failed transfer and let them manually call selfRefund instead.

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

1. Unnecessary zero-address checks in ForgottenRunesWarriorsGuild.forwardERC20s

It's almost impossible for msg.sender to be address zero so L174 can safely be removed.

2. Unnecessary zero-address check in ForgottenRunesWarriorsMinter.forwardERC20s

It's almost impossible for msg.sender to be address zero so L628 can safely be removed.

3. Unnecessary zero-address check in ForgottenRunesWarriorsMinter.teamSummon

OpenZeppelin guards against minting to zero address so L258 can safely be removed.