Yield Witch v2 contest - TomJ's results

Table of Contents

Low Risk Issues

• Immutable addresses should 0-Check

Non-critical Issues

• Define Magic Numbers to Constant
• Event is Missing Indexed Fields
• TYPO
• Incomplete NatSpec

 

Low Risk Issues

Immutable addresses should 0-Check

Issue

I recommend adding check of 0-address for immutable addresses. Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

PoC

• Witch.sol: immutable address of "cauldron" https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L71-L74

Mitigation

Add 0-address check for above immutable address.

 

Non-critical Issues

Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

1. Magic Number: 1e18

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

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L102-L103 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L162-L163 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L587 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L591

2. Magic Number: 0.01e18

./Witch.sol:105:            initialOffer == 0 || initialOffer >= 0.01e18,
./Witch.sol:108:        require(proportion >= 0.01e18, "Proportion below 1%");

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

3. Magic Number: 10

./Witch.sol:233:        if (art < debt.min * (10**debt.dec)) art = balances.art;
./Witch.sol:438:                    auction_.art - artIn >= debt.min * (10**debt.dec),

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

Mitigation

Define magic numbers to constant.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./Witch.sol:34-38:    event Bought(bytes12 indexed vaultId, address indexed buyer, uint256 ink, uint256 art);
./Witch.sol:43-49:    event LineSet(bytes6 indexed ilkId, bytes6 indexed baseId, uint32 duration, uint64 proportion, uint64 initialOffer);
./Witch.sol:50:    event LimitSet(bytes6 indexed ilkId, bytes6 indexed baseId, uint128 max);
./Witch.sol:51:    event AnotherWitchSet(address indexed value, bool isWitch);
./Witch.sol:52-56:    event IgnoredPairSet(bytes6 indexed ilkId, bytes6 indexed baseId, bool ignore);
./Witch.sol:57:    event AuctioneerRewardSet(uint128 auctioneerReward);

Mitigation

Use all 3 index fields when possible.

 

TYPO

Issue

Some typo was found in the following.

PoC

1. Witch.sol: Change "overriden" to "overridden" https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L213 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L267 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L462

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

2. Witch.sol: Change "differente" to "different" https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L385

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

3. Witch.sol: Change "quoutes" to "quotes" and "hoy" to "how" https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L520

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

Mitigation

Change to correct spelling as mentioned in above PoC

 

Incomplete NatSpec

Issue

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces. However there were places where this NatSpec was incomplete.

PoC

1. no @param for vaultId https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L212-L214

2. no @param for vault, series, to, balances, debt and no @return https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L220-L229

3. no @param for vaultId and owner https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L266-L268

4. no @param for auction_, to, liquidatorCut, auctioneerCut https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L385-L390

5. no @param for vaultId, auction_, inkOut, artIn https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L407-L413

6. no @param for vaultId, buyer, ink, art https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L461-L467

7. no @ param for auction_, to, artIn and @return for liquidatorCut, auctioneerCut https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L561-L566

Mitigation

Complete the missing NatSpec as shown in above PoC

 

Table of Contents

• Minimize the Number of SLOADs by Caching State Variable
• Defined Variables Used Only Once
• Duplicate require() Checks Should be a Modifier or a Function
• Internal Function Called Only Once can be Inlined
• Use Calldata instead of Memory for Read Only Function Parameters
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• != 0 costs less gass than > 0
• Use Custom Errors to Save Gas

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC

1. Cache variable cauldron of auction() in Witch.sol 5 SLOADs to 1 SLOAD, 1 MSTORE and 5 MLOAD https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L180 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L184 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L189 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L191-L192

2. Cache variable cauldron of payBase() in Witch.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOAD https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L304 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L309

3. Cache variable cauldron of calcPayout() in Witch.sol 4 SLOADs to 1 SLOAD, 1 MSTORE and 4 MLOAD https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L542 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L546-L548

Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC

1. Remove 'inkAtEnd' variable of _calcPayout function in Witch.sol Mitigation: Delete line 594 and replace line 595 like shown below

Originally:
594:        uint256 inkAtEnd = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink);
595:        liquidatorCut = inkAtEnd.wmul(proportionNow);

Change to:
595:        liquidatorCut = uint256(artIn).wdiv(auction_.art).wmul(auction_.ink).wmul(proportionNow);

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L594-L595

Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

./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:313:        require(liquidatorCut >= minInkOut, "Not enough bought");
./Witch.sol:366:        require(liquidatorCut >= minInkOut, "Not enough bought");

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Internal Function Called Only Once can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

1. _auctionStarted() in Witch.sol _auctionStarted function called only once at line 209 Function: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L214-L218 Called at: https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L209

Mitigation

I recommend to not define above functions and instead inline it to place it is called.

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

./Witch.sol:224:        DataTypes.Vault memory vault,
./Witch.sol:225:        DataTypes.Series memory series,
./Witch.sol:227:        DataTypes.Balances memory balances,
./Witch.sol:228:        DataTypes.Debt memory debt
./Witch.sol:387:        DataTypes.Auction memory auction_,
./Witch.sol:411:        DataTypes.Auction memory auction_,
./Witch.sol:563:        DataTypes.Auction memory auction_,

Mitigation

Change memory to calldata

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, it acctually cost more gas to use elements smaller than 32 bytes. Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

./Witch.sol:28:    error AuctioneerRewardTooHigh(uint128 max, uint128 actual);
./Witch.sol:46:        uint32 duration,
./Witch.sol:47:        uint64 proportion,
./Witch.sol:48:        uint64 initialOffer
./Witch.sol:50:    event LimitSet(bytes6 indexed ilkId, bytes6 indexed baseId, uint128 max);
./Witch.sol:57:    event AuctioneerRewardSet(uint128 auctioneerReward);
./Witch.sol:63:    uint128 public auctioneerReward = 0.01e18;
./Witch.sol:98:        uint32 duration,
./Witch.sol:99:        uint64 proportion,
./Witch.sol:100:        uint64 initialOffer
./Witch.sol:129:        uint128 max
./Witch.sol:161:    function setAuctioneerReward(uint128 auctioneerReward_) external auth {
./Witch.sol:232:        uint128 art = uint256(balances.art).wmul(line.proportion).u128();
./Witch.sol:234:        uint128 ink = (art == balances.art)
./Witch.sol:289:        uint128 minInkOut,
./Witch.sol:290:        uint128 maxBaseIn
./Witch.sol:303:        uint128 artIn = uint128(
./Witch.sol:347:        uint128 minInkOut,
./Witch.sol:348:        uint128 maxArtIn

Mitigation

I suggest using uint256 instead of anything smaller.

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

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

Mitigation

I suggest changing > 0 to != 0

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

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

Mitigation

I suggest implementing custom errors to save gas.