QA Report

[code423n4]

Overview Risk Rating	Number of issues
Low Risk	2
Non-Critical Risk	3

Table of Contents

• 1. Low Risk Issues
  • 1.1. Missing address(0) checks
  • 1.2. Unsafe casting may overflow
• 2. Non-Critical Issues
  • 2.1. Typos
  • 2.2. Open TODOS
  • 2.3. Use a constant instead of duplicating the same string or replace the following revert strings with Errors

1. Low Risk Issues

1.1. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

File: Witch.sol
59:     ICauldron public immutable cauldron;
...
71:     constructor(ICauldron cauldron_, ILadle ladle_) {
+ 72:     require(cauldron_ != address(0));
72:         cauldron = cauldron_;
73:         ladle = ladle_;
74:     }

1.2. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Even if the comment says "Overflow is fine", consider using OpenZeppelin's SafeCast library to prevent unexpected behaviors here:

Witch.sol:217:        emit Auctioned(vaultId, uint32(block.timestamp));
Witch.sol:241:                start: uint32(block.timestamp), // Overflow is fine
Witch.sol:582:            elapsed = uint32(block.timestamp) - uint256(auction_.start); // Overflow on block.timestamp is fine

File: Witch.sol
302:         // Find out how much debt is being repaid
303:         uint128 artIn = uint128(
304:             cauldron.debtFromBase(auction_.seriesId, maxBaseIn)
305:         );

2. Non-Critical Issues

2.1. Typos

• specialised

Witch.sol:213:    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
Witch.sol:267:    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
Witch.sol:462:    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

• differente

Witch.sol:385:    /// @dev transfers funds from the ilkJoin to the liquidator (and potentially the auctioneer if they're differente people)

• quoutes

Witch.sol:520:    /// @dev quoutes hoy much ink a liquidator is expected to get if it repays an `artIn` amount

2.2. Open TODOS

Consider resolving the TODOs before deploying.

Witch.sol:577:        // TODO: Replace this contract before then 😰

2.3. Use a constant instead of duplicating the same string or replace the following revert strings with Errors

Witch.sol:255:        require(auction_.start > 0, "Vault not under auction");
Witch.sol:300:        require(auction_.start > 0, "Vault not under auction");
Witch.sol:358:        require(auction_.start > 0, "Vault not under auction");
Witch.sol:416:        require(auction_.start > 0, "Vault not under auction");

Witch.sol:365:        require(liquidatorCut >= minInkOut, "Not enough bought");
Witch.sol:313:        require(liquidatorCut >= minInkOut, "Not enough bought");

Witch.sol:328:            require(baseJoin != IJoin(address(0)), "Join not found");
Witch.sol:395:            require(ilkJoin != IJoin(address(0)), "Join not found");

[alcueca]

Thanks for the typos, read the README, learn how to take a joke.

@bbonanno, we have an unnecessary cast on L303, though.

[ultrasecreth]

specialised is not a typo, it's British English :)