QA Report

[code423n4]

[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

[alcueca]

4 useful, but for God's sake read the README. I wrote it for a reason.