Yield Witch v2 contest - IllIllI's results

Summary

Low Risk Issues

Total: 3 instances over 3 issues

Non-critical Issues

Total: 27 instances over 8 issues

Low Risk Issues

[L‑01] setLine() parameters inconsistently followed

In setLine() there's a require() that ensures that the initial offer is either equal to zero, or greater than 1%. It stands to reason that therefore, offers less than 1% are considered dust and are not actionable. If this is the case, then in the else-block on line 589, does not follow this same dust-skipping logic when initialProportion is set to zero, and will waste time offering proportions that nobody will take. This may lead to auctions taking longer than necessary, and more funds being lost. It is incorrect state handling and therefore of Low risk. If there was some other meaning behind the conditions, there should be a require() enforcing it

There is 1 instance of this issue:

File: contracts/Witch.sol

584          if (duration == type(uint32).max) {     // Interpreted as infinite duration
585              proportionNow = initialProportion;
586          } else if (elapsed > duration) {
587              proportionNow = 1e18;
588          } else {
589              proportionNow =
590                  uint256(initialProportion) +
591                  uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));
592:         }

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

[L‑02] Events will contain the wrong timestamp in the future

While the timestamp field in the event might not affect on-chain processing, it will impact off-chain tools that have to parse it. This is incorrect state handling and therefore of Low risk

There is 1 instance of this issue:

File: contracts/Witch.sol

217:         emit Auctioned(vaultId, uint32(block.timestamp));

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

[L‑03] Integer overflow due to casting will cause contract accounting to break

When block.timestamp becomes larger than type(uint32).max, the cast on line 582 will overflow, causing the elapsed time calculation to be extremely large and wrong if the auction start time was before the wrap. This will cause the proportion to be greater than 100%, and will allow a liquidator to earn a massive fee. Comments in code are not sufficient to prevent client fund loss, and relying on Google calendar is obviously not either. This should be a Medium, but I'm guessing the sponsor will argue that it's already documented here in the code (though it needs to be in the README.md and in Yield's risks documentation), so it's not worth while to write up the whole thing just to have it downgraded by a judge that decides not to follow that rule.

There is 1 instance of this issue:

File: contracts/Witch.sol

575          // If the world has not turned to ashes and darkness, auctions will malfunction on
576          // the 7th of February 2106, at 06:28:16 GMT
577          // TODO: Replace this contract before then 😰
578          // UPDATE: Added reminder to Google calendar ✅
579          uint256 elapsed;
580          uint256 proportionNow;
581          unchecked {
582              elapsed = uint32(block.timestamp) - uint256(auction_.start); // Overflow on block.timestamp is fine
583:         }

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

Non-critical Issues

[N‑01] Incomplete documentation

The infinite duration comment should be in NatSpec, not a normal comment hidden in the code

There is 1 instance of this issue:

File: contracts/Witch.sol

584:         if (duration == type(uint32).max) {     // Interpreted as infinite duration

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

[N‑02] Timestamps in events are redundant

block.timestamp and block.number are added to event information by default so adding them manually wastes gas and is redundant

There is 1 instance of this issue:

File: contracts/Witch.sol

217:         emit Auctioned(vaultId, uint32(block.timestamp));

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

[N‑03] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 8 instances of this issue:

File: contracts/Witch.sol

/// @audit 1e18
102:          require(initialOffer <= 1e18, "InitialOffer above 100%");

/// @audit 1e18
103:          require(proportion <= 1e18, "Proportion above 100%");

/// @audit 0.01e18
105:              initialOffer == 0 || initialOffer >= 0.01e18,

/// @audit 0.01e18
108:          require(proportion >= 0.01e18, "Proportion below 1%");

/// @audit 1e18
162:          if (auctioneerReward_ > 1e18) {

/// @audit 1e18
163:              revert AuctioneerRewardTooHigh(1e18, auctioneerReward_);

/// @audit 1e18
587:              proportionNow = 1e18;

/// @audit 1e18
591:                  uint256(1e18 - initialProportion).wmul(elapsed.wdiv(duration));

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

[N‑04] Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There are 2 instances of this issue:

File: contracts/Witch.sol

/// @audit uint256(initialProportion)
590:                  uint256(initialProportion) +

/// @audit uint256(artIn)
594:          uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);

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

[N‑05] Typos

There are 6 instances of this issue:

File: contracts/Witch.sol

/// @audit overriden
213:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

/// @audit repayed
220:      /// @dev Calculates the auction initial values, the 2 non-trivial values are how much art must be repayed

/// @audit overriden
267:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

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

/// @audit overriden
462:      /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties

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

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

[N‑06] NatSpec is incomplete

There is 1 instance of this issue:

File: contracts/Witch.sol

/// @audit Missing: '@return'
174       /// @param vaultId Id of vault to liquidate
175       /// @param to Receiver of the auctioneer reward
176       function auction(bytes12 vaultId, address to)
177           external
178:          returns (DataTypes.Auction memory auction_)

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

[N‑07] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (threefields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question

There are 6 instances of this issue:

File: contracts/Witch.sol

33        event Bought(
34            bytes12 indexed vaultId,
35            address indexed buyer,
36            uint256 ink,
37            uint256 art
38:       );

43        event LineSet(
44            bytes6 indexed ilkId,
45            bytes6 indexed baseId,
46            uint32 duration,
47            uint64 proportion,
48            uint64 initialOffer
49:       );

50:       event LimitSet(bytes6 indexed ilkId, bytes6 indexed baseId, uint128 max);

51:       event AnotherWitchSet(address indexed value, bool isWitch);

52        event IgnoredPairSet(
53            bytes6 indexed ilkId,
54            bytes6 indexed baseId,
55            bool ignore
56:       );

57:       event AuctioneerRewardSet(uint128 auctioneerReward);

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

[N‑08] Duplicated require()/revert() checks should be refactored to a modifier or function

The compiler will inline the function, which will avoid JUMP instructions usually associated with functions

There are 2 instances of this issue:

File: contracts/Witch.sol

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

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

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

Summary

Gas Optimizations

Total: 37 instances over 10 issues

Gas Optimizations

[G‑01] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There is 1 instance of this issue:

File: contracts/Witch.sol

66        mapping(bytes6 => mapping(bytes6 => DataTypes.Line)) public lines;
67        mapping(bytes6 => mapping(bytes6 => DataTypes.Limits)) public limits;
68        mapping(address => bool) public otherWitches;
69:       mapping(bytes6 => mapping(bytes6 => bool)) public ignoredPairs;

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

[G‑02] Using storage instead of memory for structs/arrays saves gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

There is 1 instance of this issue:

File: contracts/Witch.sol

197           DataTypes.Limits memory limits_ = limits[vault.ilkId][
198               series.baseId
199:          ];

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

diff --git a/contracts/Witch.sol b/contracts/Witch.sol
index f98dd6a..ccf9822 100644
--- a/contracts/Witch.sol
+++ b/contracts/Witch.sol
@@ -194,15 +194,15 @@ contract Witch is AccessControl {
         // There is a limit on how much collateral can be concurrently put at auction, but it is a soft limit.
         // If the limit has been surpassed, no more vaults of that collateral can be put for auction.
         // This avoids the scenario where some vaults might be too large to be auctioned.
-        DataTypes.Limits memory limits_ = limits[vault.ilkId][
+        DataTypes.Limits storage limits_ = limits[vault.ilkId][
             series.baseId
         ];
-        require(limits_.sum <= limits_.max, "Collateral limit reached");
+        uint128 lsum_ = limits_.sum;
+        require(lsum_ <= limits_.max, "Collateral limit reached");
 
         auction_ = _calcAuction(vault, series, to, balances, debt);
 
-        limits_.sum += auction_.ink;
-        limits[vault.ilkId][series.baseId] = limits_;
+        limits_.sum = lsum_ + auction_.ink;
 
         auctions[vaultId] = auction_;
 

diff --git a/gas_before b/gas_after
index 68d894d..749b496 100644
--- a/gas_before
+++ b/gas_after
@@ -3,11 +3,11 @@
 ╞════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
 │ Deployment Cost                    ┆ Deployment Size ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ 3076398                            ┆ 15658           ┆       ┆        ┆       ┆         │
+│ 3062982                            ┆ 15591           ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ Function Name                      ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ auction                            ┆ 4219            ┆ 70111 ┆ 81848  ┆ 91148 ┆ 30      │
+│ auction                            ┆ 4219            ┆ 69956 ┆ 81665  ┆ 90965 ┆ 30      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ auctioneerReward                   ┆ 426             ┆ 426   ┆ 426    ┆ 426   ┆ 1       │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

[G‑03] The result of a function call should be cached rather than re-calling the function

The instances below point to the second+ call of the function within a single function

There are 3 instances of this issue:

File: contracts/Witch.sol

/// @audit inkOut.u128()
450:                  limits_.sum -= inkOut.u128();

/// @audit inkOut.u128()
/// @audit artIn.u128()
458:          cauldron.slurp(vaultId, inkOut.u128(), artIn.u128());

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

diff --git a/contracts/Witch.sol b/contracts/Witch.sol
index f98dd6a..c2196e8 100644
--- a/contracts/Witch.sol
+++ b/contracts/Witch.sol
@@ -421,6 +421,7 @@ contract Witch is AccessControl {
         ];
 
         // Update local auction
+        uint128 io128_;
         {
             if (auction_.art == artIn) {
                 // If there is no debt left, return the vault with the collateral to the owner
@@ -428,6 +429,7 @@ contract Witch is AccessControl {
 
                 // Update limits - reduce it by the whole auction
                 limits_.sum -= auction_.ink;
+                io128_ = inkOut.u128();
             } else {
                 // Ensure enough dust is left
                 DataTypes.Debt memory debt = cauldron.debt(
@@ -439,15 +441,16 @@ contract Witch is AccessControl {
                     "Leaves dust"
                 );
 
+                io128_ = inkOut.u128();
                 // Update the auction
-                auction_.ink -= inkOut.u128();
+                auction_.ink -= io128_;
                 auction_.art -= artIn.u128();
 
                 // Store auction changes
                 auctions[vaultId] = auction_;
 
                 // Update limits - reduce it by whatever was bought
-                limits_.sum -= inkOut.u128();
+                limits_.sum -= io128_;
             }
         }
 
@@ -455,7 +458,7 @@ contract Witch is AccessControl {
         limits[auction_.ilkId][auction_.baseId] = limits_;
 
         // Update accounting at Cauldron
-        cauldron.slurp(vaultId, inkOut.u128(), artIn.u128());
+        cauldron.slurp(vaultId, io128_, artIn.u128());
     }
 
     /// @dev Logs that a certain amount of a vault was liquidated

diff --git a/gas_before b/gas_after
index 68d894d..7867bad 100644
--- a/gas_before
+++ b/gas_after
@@ -3,7 +3,7 @@
 ╞════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
 │ Deployment Cost                    ┆ Deployment Size ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ 3076398                            ┆ 15658           ┆       ┆        ┆       ┆         │
+│ 3075198                            ┆ 15652           ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ Function Name                      ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
@@ -29,9 +29,9 @@
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ otherWitches                       ┆ 570             ┆ 570   ┆ 570    ┆ 570   ┆ 1       │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ payBase                            ┆ 7632            ┆ 20431 ┆ 22355  ┆ 26694 ┆ 10      │
+│ payBase                            ┆ 7632            ┆ 20373 ┆ 22357  ┆ 26544 ┆ 10      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ payFYToken                         ┆ 7507            ┆ 19219 ┆ 20648  ┆ 27910 ┆ 10      │
+│ payFYToken                         ┆ 7504            ┆ 19161 ┆ 20654  ┆ 27760 ┆ 10      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ point                              ┆ 2934            ┆ 5098  ┆ 2942   ┆ 9420  ┆ 3       │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

[G‑04] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There is 1 instance of this issue:

File: contracts/Witch.sol

/// @audit point(), setLine(), setLimit(), setAnotherWitch(), setIgnoredPair(), setAuctioneerReward(), auction(), cancel(), payBase(), payFYToken(), calcPayout()
19:   contract Witch is AccessControl {

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

[G‑05] Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 2 instances of this issue:

File: contracts/Witch.sol

68:       mapping(address => bool) public otherWitches;

69:       mapping(bytes6 => mapping(bytes6 => bool)) public ignoredPairs;

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

[G‑06] >= costs less gas than >

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

There are 3 instances of this issue:

File: contracts/Witch.sol

308:          artIn = artIn > auction_.art ? auction_.art : artIn;

361:          artIn = maxArtIn > auction_.art ? auction_.art : maxArtIn;

556:          artIn = maxArtIn > auction_.art ? auction_.art : maxArtIn;

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

[G‑07] Usage of uints/ints smaller than 32 bytes (256 bits) 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.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs an extra 28 gas as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

There are 2 instances of this issue:

File: contracts/Witch.sol

/// @audit uint128 art
233:          if (art < debt.min * (10**debt.dec)) art = balances.art;

/// @audit uint128 artIn
308:          artIn = artIn > auction_.art ? auction_.art : artIn;

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

diff --git a/contracts/Witch.sol b/contracts/Witch.sol
index f98dd6a..066e912 100644
--- a/contracts/Witch.sol
+++ b/contracts/Witch.sol
@@ -229,11 +229,11 @@ contract Witch is AccessControl {
     ) internal view returns (DataTypes.Auction memory) {
         // We store the proportion of the vault to auction, which is the whole vault if the debt would be below dust.
         DataTypes.Line storage line = lines[vault.ilkId][series.baseId];
-        uint128 art = uint256(balances.art).wmul(line.proportion).u128();
+        uint256 art = uint256(balances.art).wmul(line.proportion);
         if (art < debt.min * (10**debt.dec)) art = balances.art;
-        uint128 ink = (art == balances.art)
+        uint256 ink = (art == balances.art)
             ? balances.ink
-            : uint256(balances.ink).wmul(line.proportion).u128();
+            : uint256(balances.ink).wmul(line.proportion);
 
         return
             DataTypes.Auction({
@@ -242,8 +242,8 @@ contract Witch is AccessControl {
                 seriesId: vault.seriesId,
                 baseId: series.baseId,
                 ilkId: vault.ilkId,
-                art: art,
-                ink: ink,
+                art: art.u128(),
+                ink: ink.u128(),
                 auctioneer: to
             });
     }

diff --git a/gas_before b/gas_after
index 68d894d..4f7212a 100644
--- a/gas_before
+++ b/gas_after
@@ -3,17 +3,17 @@
 ╞════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
 │ Deployment Cost                    ┆ Deployment Size ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ 3076398                            ┆ 15658           ┆       ┆        ┆       ┆         │
+│ 3077398                            ┆ 15663           ┆       ┆        ┆       ┆         │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ Function Name                      ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ auction                            ┆ 4219            ┆ 70111 ┆ 81848  ┆ 91148 ┆ 30      │
+│ auction                            ┆ 4219            ┆ 70086 ┆ 81815  ┆ 91115 ┆ 30      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ auctioneerReward                   ┆ 426             ┆ 426   ┆ 426    ┆ 426   ┆ 1       │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ auctions                           ┆ 1244            ┆ 1244  ┆ 1244   ┆ 1244  ┆ 20      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ calcPayout                         ┆ 3627            ┆ 10420 ┆ 14603  ┆ 19151 ┆ 29      │
+│ calcPayout                         ┆ 3627            ┆ 10415 ┆ 14603  ┆ 19118 ┆ 29      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ cancel                             ┆ 2736            ┆ 9683  ┆ 5675   ┆ 20639 ┆ 3       │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

[G‑08] require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.

There is 1 instance of this issue:

File: contracts/Witch.sol

/// @audit expensive op on line 105
108:          require(proportion >= 0.01e18, "Proportion below 1%");

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

[G‑09] Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 17 instances of this issue:

File: contracts/Witch.sol

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

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

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

104           require(
105               initialOffer == 0 || initialOffer >= 0.01e18,
106               "InitialOffer below 1%"
107:          );

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                   require(
438                       auction_.art - artIn >= debt.min * (10**debt.dec),
439                       "Leaves dust"
440:                  );

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

[G‑10] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 6 instances of this issue:

File: contracts/Witch.sol

83:       function point(bytes32 param, address value) external auth {

95        function setLine(
96            bytes6 ilkId,
97            bytes6 baseId,
98            uint32 duration,
99            uint64 proportion,
100           uint64 initialOffer
101:      ) external auth {

126       function setLimit(
127           bytes6 ilkId,
128           bytes6 baseId,
129           uint128 max
130:      ) external auth {

141:      function setAnotherWitch(address value, bool isWitch) external auth {

150       function setIgnoredPair(
151           bytes6 ilkId,
152           bytes6 baseId,
153           bool ignore
154:      ) external auth {

161:      function setAuctioneerReward(uint128 auctioneerReward_) external auth {

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