Yield Witch v2 contest - __141345__'s results

Variable name

The name value could be related to _address. value may confuse others with fund or numeric type.

    /// @dev Governance function to set other liquidation contracts that may have taken vaults already.
    /// @param value The address that may be set/unset as another witch
    /// @param isWitch Is this address a witch or not
    function setAnotherWitch(address value, bool isWitch) external auth {
        otherWitches[value] = isWitch;
        emit AnotherWitchSet(value, isWitch);
    }

NATSPEC not complete

    /// @notice Return how much collateral should be given out.
    function _calcPayout(
        DataTypes.Auction memory auction_,
        address to,
        uint256 artIn
    ) internal view returns (uint256 liquidatorCut, uint256 auctioneerCut) {

suggestion: Follow NATSPEC.

MAGICAL NUMBER CAN BE DOCUMENTED AND EXPLAINED

1e18 is used several times through the code. This may both obscure the purpose of the function and unnecessarily lead to potential error if the constants are changed during development.

102-108:
        require(initialOffer <= 1e18, "InitialOffer above 100%");
        require(proportion <= 1e18, "Proportion above 100%");
        require(
            initialOffer == 0 || initialOffer >= 0.01e18,
            "InitialOffer below 1%"
        );

162-164:
        if (auctioneerReward_ > 1e18) {
            revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);
        }

587:
        proportionNow = 1e18;

591:
        uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

Suggestion: Use constants as this would make the code more maintainable and readable while costing nothing gas-wise.

Arithmic operation order

The division is performed first, and multiplication later.

594:        uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);

It might be arithmically safer to do the multiplication first instead.

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata.

    function _calcAuction(
        DataTypes.Vault memory vault,
        DataTypes.Series memory series,
        address to,
        DataTypes.Balances memory balances,
        DataTypes.Debt memory debt
    ) internal view returns (DataTypes.Auction memory) {

These parameters passed in are not modified in the function.

Another place is _calcPayout():

    function _calcPayout(
        DataTypes.Auction memory auction_,
        address to,
        uint256 artIn
    ) internal view returns (uint256 liquidatorCut, uint256 auctioneerCut) {

Here

DataTypes.Auction memory auction_

can be delcated as

DataTypes.Auction calldata auction_

X = X - Y IS CHEAPER THAN X -= Y

The demo of the gas comparsion can be seen here.

204:        limits_.sum += auction_.ink;
259:        limits[auction_.ilkId][auction_.baseId].sum -= auction_.ink;
430:        limits_.sum -= auction_.ink;
443:        auction_.ink -= inkOut.u128();
444:        auction_.art -= artIn.u128();
450:        limits_.sum -= inkOut.u128();
598:        liquidatorCut -= auctioneerCut;

Suggestion: Consider use X = X - Y to save gas

> 0 can be replaced with != 0 when dealing with uint.

!= 0 costs less gas compared to > 0 for unsigned integers with the optimizer enabled (6 gas). Here is the code as a demo. Opcode discussion.

255:        require(auction_.start > 0, "Vault not under auction");
300:        require(auction_.start > 0, "Vault not under auction");
358:        require(auction_.start > 0, "Vault not under auction");
393:        if (liquidatorCut > 0) {
398:        if (auctioneerCut > 0) {
416:        require(auction_.start > 0, "Vault not under auction");

Suggestion:

• Changing > 0 with != 0.
• Enable the Optimizer.

Guaranteed non overflow/underflow can be unchecked

438:        auction_.art - artIn >= debt.min * (10**debt.dec)

Can be writen as

438:        unchecked { auction_.art - artIn >= debt.min * (10**debt.dec) }

Memory can be used instead of Storage inside a function

    function _calcAuction() {
        // ...
        DataTypes.Line storage line = lines[vault.ilkId][series.baseId];
        uint128 art = uint256(balances.art).wmul(line.proportion).u128();
        if (art < debt.min * (10**debt.dec)) art = balances.art;
        uint128 ink = (art == balances.art)
            ? balances.ink
            : uint256(balances.ink).wmul(line.proportion).u128();
    }

Since line is not used later outside, it can be declared as memory to save gas.

Or just cache line.proportion, which was referred to twice.

    uint64 memory proportion = line.proportion;

10**debt.dec can be saved to an immutable variable in the cauldron

233:
    function _calcAuction() {
        // ...
        if (art < debt.min * (10**debt.dec)) art = balances.art;
    }

438:
    function _updateAccounting() {
        // ...
                require(
                    auction_.art - artIn >= debt.min * (10**debt.dec),
                    "Leaves dust"
                );
    }

10**debt.dec is used in _calcAuction() and _updateAccounting(), these 2 functions seem frequently used.

Suggestion: Save 10**debt.dec to an immutable variable in the cauldron.