Yield Witch v2 contest - c3phas's results

QA

Natspec is incomplete

File: Witch.sol line 173-179 Missing @return

    /// @dev Put an undercollateralized vault up for liquidation
    /// @param vaultId Id of vault to liquidate
    /// @param to Receiver of the auctioneer reward
    function auction(bytes12 vaultId, address to)
        external
        returns (DataTypes.Auction memory auction_)
    {

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: Witch.sol line 102

        require(initialOffer <= 1e18, "InitialOffer above 100%");

1e18

File: Witch.sol line 103

        require(proportion <= 1e18, "Proportion above 100%");

1e18

File: Witch.sol line 105

        require(
            initialOffer == 0 || initialOffer >= 0.01e18,
            "InitialOffer below 1%"
        );

01e18

File: Witch.sol line 108

        require(proportion >= 0.01e18, "Proportion below 1%");

0.01e18

File: Witch.sol line 162 1e18

        if (auctioneerReward_ > 1e18) {

File: Witch.sol line 163 1e18

          revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);

File: Witch.sol line 587 1e18

            proportionNow = 1e18;

File: Witch.sol line 591 1e18

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

FINDINGS

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

see Source

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

File:Witch.sol line 84

        require(param == "ladle", "Unrecognized");

File:Witch.sol line 102,103

        require(initialOffer <= 1e18, "InitialOffer above 100%");
        require(proportion <= 1e18, "Proportion above 100%");

File:Witch.sol line 104-108

        require(
            initialOffer == 0 || initialOffer >= 0.01e18,
            "InitialOffer below 1%"
        );
        require(proportion >= 0.01e18, "Proportion below 1%");

File:Witch.sol line 189

        require(cauldron.level(vaultId) < 0, "Not undercollateralized");

File:Witch.sol line 200

        require(limits_.sum <= limits_.max, "Collateral limit reached");

File:Witch.sol line 255&256

        require(auction_.start > 0, "Vault not under auction");
        require(cauldron.level(vaultId) >= 0, "Undercollateralized");

File:Witch.sol line 300

        require(auction_.start > 0, "Vault not under auction");

File:Witch.sol line 313

        require(liquidatorCut >= minInkOut, "Not enough bought");

File:Witch.sol line 328

            require(baseJoin != IJoin(address(0)), "Join not found");

File:Witch.sol line 358

        require(auction_.start > 0, "Vault not under auction");

File:Witch.sol line 365

        require(liquidatorCut >= minInkOut, "Not enough bought");

File:Witch.sol line 395

            require(ilkJoin != IJoin(address(0)), "Join not found");

File:Witch.sol line 416

        require(auction_.start > 0, "Vault not under auction");

File:Witch.sol line 437

                require(
                    auction_.art - artIn >= debt.min * (10**debt.dec),
                    "Leaves dust"
                );

Usage of uints/ints smaller than 32 bytes (256 bits) in function arguments incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

NB: Some functions have been truncated where necessary to just show affected parts of the code

Witch.sol.setAuctioneerReward : uint128 auctioneerReward_ should use uint256 and downcast where necessary(saves ~73 gas)

File: Witch.sol line 126-136

    function setLimit(
        bytes6 ilkId,
        bytes6 baseId,
        uint128 max
    ) external auth {
        limits[ilkId][baseId] = DataTypes.Limits({
            max: max,
            sum: limits[ilkId][baseId].sum // sum is initialized at zero, and doesn't change when changing any ilk parameters
        });
        emit LimitSet(ilkId, baseId, max);
    }

Since variables are not packed inside a function, there is no point in using something like uint128 in favor of uint256

Witch.sol.setAuctioneerReward : uint128 auctioneerReward_ should use uint256 and downcast where necessary(saves ~69 gas)

File: Witch.sol line 161

    function setAuctioneerReward(uint128 auctioneerReward_) external auth {
        if (auctioneerReward_ > 1e18) {
            revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);
        }
        auctioneerReward = auctioneerReward_;
        emit AuctioneerRewardSet(auctioneerReward_);
    }

uint128 auctioneerReward_ should be modified to uint256 auctioneerReward_ and downcast if neccessary

Witch.sol.payBase : uint128 minInkOut and maxBaseIn should use uint256 and downcast where necessary(saves ~235 gas)

File: Witch.sol line 286-297

    function payBase(
        bytes12 vaultId,
        address to,
        uint128 minInkOut,
        uint128 maxBaseIn
    )
        external
        returns (
            uint256 liquidatorCut,
            uint256 auctioneerCut,
            uint256 baseIn
        )
		...

Witch.sol.payFYToken : uint128 minInkOut and maxArtIn should use uint256 and downcast where necessary(saves ~161 gas)

File: Witch.sol line 344-356

    function payFYToken(
        bytes12 vaultId,
        address to,
        uint128 minInkOut,
        uint128 maxArtIn
    )
        external
        returns (
            uint256 liquidatorCut,
            uint256 auctioneerCut,
            uint256 artIn
        )
    {