Yield Witch v2 contest - Meera's results

Overview

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");

Overview

Table of Contents:

• 1. Multiple accesses of a mapping/array should use a local variable cache
• 2. Use of the memory keyword when storage should be used
• 3. Unchecking arithmetics operations that can't underflow/overflow
• 4. Duplicated conditions should be refactored to a modifier or function to save deployment costs
• 5. Internal/Private functions only called once can be inlined to save gas
• 6. Use Custom Errors instead of Revert Strings to save Gas

1. Multiple accesses of a mapping/array should use a local variable cache

Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.

Affected code:

File: Witch.sol
125:     function setLimit(
126:         bytes6 ilkId,
127:         bytes6 baseId,
128:         uint128 max
129:     ) external auth {
+ 130:         DataTypes.Limits storage _limit = limits[ilkId][baseId];
- 130:         limits[ilkId][baseId] = DataTypes.Limits({
+ 130:         _limit = DataTypes.Limits({
131:             max: max,
- 132:             sum: limits[ilkId][baseId].sum // sum is initialized at zero, and doesn't change when changing any ilk parameters
+ 132:             sum: _limit.sum // sum is initialized at zero, and doesn't change when changing any ilk parameters
133:         });
134:         emit LimitSet(ilkId, baseId, max);
135:     }

2. Use of the memory keyword when storage should be used

Consider using a storage pointer instead of memory location here:

• https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L418

File: Witch.sol
- 418:         DataTypes.Limits memory limits_ = limits[auction_.ilkId][
+ 418:         DataTypes.Limits storage limits_ = limits[auction_.ilkId][
...
423:         {
424:             if (auction_.art == artIn) {
...
428:                 // Update limits - reduce it by the whole auction
429:                 limits_.sum -= auction_.ink;
430:             } else {
...
448:                 // Update limits - reduce it by whatever was bought
449:                 limits_.sum -= inkOut.u128();
450:             }
451:         }
452: 
- 453:         // Store limit changes
- 454:         limits[auction_.ilkId][auction_.baseId] = limits_;

3. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

• https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L443

File: Witch.sol
437:                 require(
438:                     auction_.art - artIn >= debt.min * (10**debt.dec),
439:                     "Leaves dust"
440:                 );
...
- 444:                 auction_.art -= artIn.u128(); //@audit should be unchecked due to L438
+ 444:                 unchecked { auction_.art -= artIn.u128(); }

4. Duplicated conditions should be refactored to a modifier or function to save deployment costs

• "Vault not under auction"

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");

• "Not enough bought"

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

5. Internal/Private functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Affected code:

• _auctionStarted()

contracts/Witch.sol:
  209:         _auctionStarted(vaultId);
  214:     function _auctionStarted(bytes12 vaultId) internal virtual {

6. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

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

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

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).

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences :

• "Vault not under auction"

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");

• "Not enough bought"

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

• "Join not found"

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