Yield Witch v2 contest - rbserver's results

[L-01] MISSING ZERO-ADDRESS CHECK

Addresses should be checked against address(0) to prevent unintended actions, unexpected loss of assets, etc. Please consider checking the following address inputs.

contracts\Witch.sol
  83: function point(bytes32 param, address value) external auth {
  141: function setAnotherWitch(address value, bool isWitch) external auth {
  176: function auction(bytes12 vaultId, address to)
  286-291:
    function payBase(
      bytes12 vaultId,
      address to,
      uint128 minInkOut,
      uint128 maxBaseIn
    )
  344-349:
    function payFYToken(
      bytes12 vaultId,
      address to,
      uint128 minInkOut,
      uint128 maxArtIn
    )
  528-532：
    function calcPayout(
      bytes12 vaultId,
      address to,
      uint256 maxArtIn
    )

[L-02] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers in the following code with constants.

contracts\Witch.sol
  102: require(initialOffer <= 1e18, "InitialOffer above 100%");
  103: require(proportion <= 1e18, "Proportion above 100%");
  105: initialOffer == 0 || initialOffer >= 0.01e18,
  108: require(proportion >= 0.01e18, "Proportion below 1%");
  162: if (auctioneerReward_ > 1e18) {
  163: revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);
  587: proportionNow = 1e18;
  591: uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

[N-01] REDUNDANT CAST

initialProportion does not need to be converted to uint256 because it is already stored as uint256 for the following code.

contracts\Witch.sol
  573-591:
    uint256 initialProportion = line_.initialOffer;
    ...
    proportionNow =
      uint256(initialProportion) +
      uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

[N-02] REVERT REASON CAN BE MORE EXACT

Because of the initialOffer == 0 condition, initialOffer can be 0, which is below 1%. The revert reason can clarify that initialOffer can also be 0.

contracts\Witch.sol
  104-107:
    require(
      initialOffer == 0 || initialOffer >= 0.01e18,
      "InitialOffer below 1%"
    );

[N-03] REVERT REASON CAN BE MORE DESCRIPTIVE

Instead of just mentioning "Unrecognized", the revert reason can describe what is unrecognized.

contracts\Witch.sol
  83-84:
    function point(bytes32 param, address value) external auth {
      require(param == "ladle", "Unrecognized"); 

[N-04] INCOMPLETE NATSPEC COMMENTS

NatSpec provides rich documentation for code. @param and/or @return are missing for the following NatSpec comments:

contracts\Witch.sol
  212-214:
    /// @dev Moves the vault ownership to the witch.
    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
    function _auctionStarted(bytes12 vaultId) internal virtual {
	
  220-229:
    /// @dev Calculates the auction initial values, the 2 non-trivial values are how much art must be repayed
    /// and what's the max ink that will be offered in exchange. For the realtime amount of ink that's on offer
    /// use `_calcPayout`
    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) {
	
  266-268:
    /// @dev Moves the vault ownership back to the original owner & clean internal state.
    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
    function _auctionEnded(bytes12 vaultId, address owner) internal virtual {
  
  385-391:
    /// @dev transfers funds from the ilkJoin to the liquidator (and potentially the auctioneer if they're differente people)
    function _payInk(
      DataTypes.Auction memory auction_,
      address to,
      uint256 liquidatorCut,
      uint256 auctioneerCut
    ) internal {
	
  407-414:
    /// @notice Update accounting on the Witch and on the Cauldron. Delete the auction and give back the vault if finished.
    /// This function doesn't verify the vaultId matches the vault and auction passed. Check before calling.
    function _updateAccounting(
      bytes12 vaultId,
      DataTypes.Auction memory auction_,
      uint256 inkOut,
      uint256 artIn
    ) internal {

  461-468:
    /// @dev Logs that a certain amount of a vault was liquidated
    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
    function _collateralBought(
      bytes12 vaultId,
      address buyer,
      uint256 ink,
      uint256 art
    ) internal virtual {
	
  561-566:
    /// @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) {

[N-05] @NOTICE PLACEMENT IN NATSPEC COMMENTS

It is a convention to place @notice above @dev and @param in NatSpec comments, which is not the case in the following code:

contracts\Witch.sol
  335-343:
    /// @dev Pay up to `maxArtIn` debt from a vault in liquidation using fyToken, getting at least `minInkOut` collateral.
    /// @notice If too much fyToken are offered, only the necessary amount are taken.
    /// @param vaultId Id of vault to buy
    /// @param to Receiver for the collateral bought
    /// @param maxArtIn Maximum amount of fyToken that will be paid
    /// @param minInkOut Minimum amount of collateral that must be received
    /// @return liquidatorCut Amount paid to `to`.
    /// @return auctioneerCut Amount paid to whomever started the auction. 0 if it's the same address that's calling this method
    /// @return artIn Amount of fyToken taken

[G-01] REQUIRE STATEMENT CAN BE PLACED AT START OF FUNCTION BODY IF POSSIBLE

When a require statement is placed at the start of the function body, the subsequent operations that cost more gas are prevented from running if the require statement reverts.

For the following code, require(cauldron.level(vaultId) < 0, "Not undercollateralized"); can be placed above DataTypes.Vault memory vault = cauldron.vaults(vaultId);.

contracts\Witch.sol
  176-189:
    function auction(bytes12 vaultId, address to)
      external
      returns (DataTypes.Auction memory auction_)
    {
      DataTypes.Vault memory vault = cauldron.vaults(vaultId);
      ...
      require(cauldron.level(vaultId) < 0, "Not undercollateralized");

For the following code, require(cauldron.level(vaultId) >= 0, "Undercollateralized"); can be placed above DataTypes.Auction storage auction_ = auctions[vaultId];.

contracts\Witch.sol
  253-256:
    function cancel(bytes12 vaultId) external {
      DataTypes.Auction storage auction_ = auctions[vaultId];
      require(auction_.start > 0, "Vault not under auction");
      require(cauldron.level(vaultId) >= 0, "Undercollateralized");

[G-02] memory KEYWORD CAN BE USED INSTEAD OF storage KEYWORD

When there is no need to make a reference to a state value, the memory keyword can be used instead of the storage keyword to save gas. Accessing storage using sload is more expensive than accessing memory using mload.

contracts\Witch.sol
  231: DataTypes.Line storage line = lines[vault.ilkId][series.baseId];
  254: DataTypes.Auction storage auction_ = auctions[vaultId]；

[G-03] UINT256 CAN BE USED INSTEAD OF UINT THAT ARE ARE LESS THAN 256 BITS

According to https://docs.soliditylang.org/en/v0.8.14/internals/layout_in_storage.html: "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." Please consider using uint256 where possible and converting these to uint that are less than 256 bits where required.

contracts\Witch.sol
  63: uint128 public auctioneerReward = 0.01e18;
  98: uint32 duration,
  129: uint128 max
  161: function setAuctioneerReward(uint128 auctioneerReward_) external auth {
  232: uint128 art = uint256(balances.art).wmul(line.proportion).u128();
  234: uint128 ink = (art == balances.art)
  289: uint128 minInkOut,
  303: uint128 artIn = uint128(
  347: uint128 minInkOut,

[G-04] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW OR UNDERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow or underflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

Because auctioneerCut is a proportion of liquidatorCut before separating auctioneerCut from liquidatorCut, then liquidatorCut -= auctioneerCut before the separation cannot underflow and liquidatorCut + auctioneerCut after the separation cannot overflow. Hence, both of them can be wrapped in unchecked {}.

contracts\Witch.sol
  597-598:
    auctioneerCut = liquidatorCut.wmul(auctioneerReward);
    liquidatorCut -= auctioneerCut;
	
  319: liquidatorCut + auctioneerCut,
  332: _collateralBought(vaultId, to, liquidatorCut + auctioneerCut, artIn);
  371: liquidatorCut + auctioneerCut,
  382: _collateralBought(vaultId, to, liquidatorCut + auctioneerCut, artIn);

[G-05] elapsed >= duration CAN BE USED INSTEAD OF elapsed > duration

For the following code, when elapsed == duration, uint256(initialProportion) + uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration)) = 1e18. Therefore, changing elapsed > duration to elapsed >= duration can avoid running extra operations to save gas. Moreover, >= comparison is cheaper than > comparison.

contracts\Witch.sol
  584-592:
    if (duration == type(uint32).max) {     // Interpreted as infinite duration
      proportionNow = initialProportion;
    } else if (elapsed > duration) {
      proportionNow = 1e18;
    } else {
      proportionNow =
        uint256(initialProportion) +
        uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));
    }

[G-06] X = X + Y AND X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y and x = x - y costs less gas than x += y and x -= y.

contracts\Witch.sol
  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;

[G-07] REDUNDANT TYPE CONVERSION CAN BE AVOIDED

Converting a variable type when not needed costs more unnecessary gas. Converting initialProportion and 1e18 - initialProportion require extra operations and more gas for the following code.

contracts\Witch.sol
  573-591:
    uint256 initialProportion = line_.initialOffer;
    ...
    proportionNow =
      uint256(initialProportion) +
      uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

[G-08] CUSTOM ERRORS CAN BE USED INSTEAD OF REVERT REASON STRINGS

Custom errors are more gas-efficient than revert reason strings.

contracts\Witch.sol
  84: require(param == "ladle", "Unrecognized");
  102: require(initialOffer <= 1e18, "InitialOffer above 100%");
  103: require(proportion <= 1e18, "Proportion above 100%");
  104-107:
    require(
      initialOffer == 0 || initialOffer >= 0.01e18,
      "InitialOffer below 1%"
    );
  108: require(proportion >= 0.01e18, "Proportion below 1%");
  189: require(cauldron.level(vaultId) < 0, "Not undercollateralized");
  200: require(limits_.sum <= limits_.max, "Collateral limit reached");
  255: require(auction_.start > 0, "Vault not under auction");
  256: require(cauldron.level(vaultId) >= 0, "Undercollateralized");
  300: require(auction_.start > 0, "Vault not under auction");
  313: require(liquidatorCut >= minInkOut, "Not enough bought");
  328: require(baseJoin != IJoin(address(0)), "Join not found");
  358: require(auction_.start > 0, "Vault not under auction");
  365: require(liquidatorCut >= minInkOut, "Not enough bought");
  395: require(ilkJoin != IJoin(address(0)), "Join not found");
  416: require(auction_.start > 0, "Vault not under auction");
  437-440:
    require(
      auction_.art - artIn >= debt.min * (10**debt.dec),
      "Leaves dust"
    );