BulkRenewal.rentPrice potentially unsafe calculation

[barakman]

In the aforementioned function:

    function rentPrice(
        string[] calldata names,
        uint256 duration
    ) external view override returns (uint256 total) {
        ETHRegistrarController controller = getController();
        uint256 length = names.length;
        for (uint256 i = 0; i < length; ) {
            IPriceOracle.Price memory price = controller.rentPrice(
                names[i],
                duration
            );
            unchecked {
                ++i;
                total += (price.base + price.premium);
            }
        }
    }

The two unchecked additions in total += (price.base + price.premium) seem potentially unsafe.
In addition to that, the added-value in applying them seems somewhere between negligible and meaningless.

Let's start with the "potentially unsafe" part:
As the value of price comes from an oracle contract, which is presumably not 100% under your system control, it sounds plausible that it could be subjected to external manipulation.
As such, someone could theoretically manipulate it to return very large values, leading the unchecked calculation above to overflow and return a radically-incorrect result.
Whoever relies on this result in their system, will potentially be subjected to some sort of exploit.

As to the "negligible or meaningless added-value" part:
The improvement in gas cost here is by and large negligible, since the cost of regular addition is around 87 gas units, while the cost of unchecked addition is around 21 gas units. There are two additions per iteration here, so using unchecked saves (87 - 21) * 2 = 132 gas units per iteration.
Moreover, the improvement in gas cost here could even be meaningless altogether, since this function by itself is a read function, which means that it doesn't consume ANY gas to begin with, unless called from a write function in another contract, which as far as I could see in your codebase, it is not (though I understand that it is possibly designated as part of your system's API for 3rd-party contracts).

My reason for posting this in the first place is actually a different piece of code which appears right after the one above:

    function renewAll(
        string[] calldata names,
        uint256 duration
    ) external payable override {
        ...
        uint256 total;
        for (uint256 i = 0; i < length; ) {
            ...
            unchecked {
                ++i;
                total += totalPrice;
            }
        }
        ...
    }

This function seems very similar to the previous one, with the primary difference between them being that this one is a write function. One minor issue here, is that the local variable total doesn't seem to be used for anything whatsoever. However being minor, it nevertheless begs the question of whether this entire contract has ever been audited to begin with.
With this question in mind, I've figured that it might be worth to point out the other issue.

Regardless, note that you've ended up "saving" gas where gas is not even used (in a read function), and wasting gas on a redundant calculation where gas IS used (in a write function).