Forgotten Runes Warrior Guild contest - kenzo's results

Lines of code

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

Vulnerability details

The mintonomics blog post and the contest readme both state that in the mintlist phase, minters will have 24 hours to mint 1 warrior each. But there is no such time limit in the code. Once the mintlist is open, it is open indefinitely.

Impact

Mintlist phase is not open for just 24 hours. Mintlisted addresses can mint from the mintlist even while the public sale is going on. So this will be unfair for the public participants. As the audit readme states "The goal of this code review and bounty is to ensure that the contract can perform safely according to the communities expectations and what is communicated in that blog post", I think this missing time limit can be considered incorrect implementation - function incorrect as to specs.

Proof of Concept

By looking at the mintlistSummon function, one can see that there is no 24h time limit, unlike what the mintonomics blog post and the contest readme state that should be.

Recommended Mitigation Steps

Add the time limit.

Improving issueRefunds (AKA: almost paying for your C4 audit)

This simple change will save 44-60% of your gas, meaning 5-10 ETH.

This is the basic logic for refunding addresses (comment lines removed):

    function _refundAddress(address minter) private {
        uint256 owed = refundOwed(minter);
        if (owed > 0) {
            daAmountRefunded[minter] += owed;
            _safeTransferETHWithFallback(minter, owed);
        }
    }
    function refundOwed(address minter) public view returns (uint256) {
        uint256 totalCostOfMints = finalPrice * daNumMinted[minter];
        uint256 refundsPaidAlready = daAmountRefunded[minter];
        return daAmountPaid[minter] - totalCostOfMints - refundsPaidAlready;
    }

To know that the minter has been refunded, instead of storing daAmountRefunded[minter] - an expensive operation - we can update daAmountPaid[minter] back to 0, to receive a gas refund instead. This does mean that the amount the user originally paid is not saved any more after the refund, but there is no use for it. daAmountPaid is only used for issuing refunds, and will not be updated further after DA phase has ended, and the refunds won't start before DA phase has ended. So I believe it is fine to edit.

So I changed the functions to the following (note - now daAmountPaid should probably be renamed to something like daPaymentToBoConsideredForRefund):

    function _refundAddress(address minter) private {
        uint256 owed = refundOwed(minter);
        if (owed > 0) {
            daAmountPaid[minter] = 0;
            _safeTransferETHWithFallback(minter, owed);
        }
    }

    function refundOwed(address minter) public view returns (uint256) {
        if (daAmountPaid[minter] == 0) {
            return 0;
        }

        uint256 totalCostOfMints = finalPrice * daNumMinted[minter];
        return daAmountPaid[minter] - totalCostOfMints;
    }

According to the single refund gasPrint function, this change saves 44% of gas. From 109219 to 60236. When issuing 8000 refunds this translates to 5-10 ETH. Using the bulk 100-address gas test, the function shows even more savings - 59%.

issueRefunds loop

For an additional small saving (roughly $240 for 8000 addresses), change the issueRefunds loop from this:

        for (uint256 i = startIdx; i < endIdx + 1; i++) {
            _refundAddress(daMinters[i]);
        }
    }

to this:

        for (uint256 i = startIdx; i <= endIdx;) {
            _refundAddress(daMinters[i]);
            unchecked { ++i; }
        }