Yield Witch v2 contest - Deivitto's results

Missing indexed event parameters

a. Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

b. Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

AuctioneerRewardSet event do not use indexed parameters.

c. Github Permalinks:

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

d. Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Y - event is missing indexed fields L57

Return value of functions ignored

a. Summary:

The return values of functions need to be checked for the appropriate values/conditions to determine error conditions appropriately.

b. Details:

baseJoin.join(msg.sender, baseIn.u128()); ignores returns value, also no event emited to know if the assets had been moved

c. Github Permalinks:

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

d. Mitigation:

Consider checking return values of the functions to detect error codes/conditions and verify expected values. Also the use of events in the movement of assets is something to consider.

Use of magic numbers is confusing and risky

a. Summary:

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

b. Details:

The following values are hardcoded and would be more readable and maintainable, as a future developer that inherit or improves the code may missunderstand or generate a typo on this values. This can be avoid if declared as a constant or used within a function -1e18 to constant L102, L103, L105, L162, L163, L233, L438 -0.01e18 to constant L63, L105, L108 -10** to function L587, L591

c. Github Permalinks:

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L63 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L102 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L103 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 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L162-163 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 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

d. Mitigation:

Replace magic hardcoded numbers with declared constants.

Missing Natspec

a. Summary:

Missing Natspec comments affect readability and maintainability of a codebase.

b. Details:

Function auction missing return descriptor in the natspec

c. Github Permalinks:

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

d. Mitigation:

Add @return descriptor

Open TODOs

a. Summary:

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

b. Details:

The code includes a TODO already done that affects readiability and focus on the readers/auditors of the contracts

c. Github Permalinks:

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

d. Mitigation:

Remove already done TODO

Solc-version

a. Summary:

solc-version 0.8.14 is too new for being recommended for deployment

b. Details:

New versions of solidity may add new interesting features and some fixes, however, as they are new, they are not tested enough, this may lead into unexpected behaviors or bugs

c. Github Permalinks:

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

d. Mitigation:

Take into account news about new bugs related to version 0.8.14 and/or consider using a previous version more tested in case you don’t need new features. This is informational.

Commented out code

a. Summary:

code that are not used should be removed or may lead to confusion while reading

b. Details:

in this case the code doesn't tell exactly on that point what is going on, it should be on the comment of the function or in the exact piece of code

c. Github Permalinks:

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

d. Mitigation:

Remove or move to a proper place the commented code if it's useful

Suggestion of name as asked in the code

a. Suggestions

• aquelarre
• witchList
• witchesList
• witches,

b. Github Permalinks:

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

use of custom errors rather than revert() / require()

summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

github permalinks

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L84 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L102 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L103 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L104-L106 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L108 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#L200 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L255 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L256 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L300 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L313 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L328 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L358 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L365 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L395 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L416 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L437-440

mitigation

replace each require by an error

use of > 0 costs more than != 0 on uints

summary

using > 0 costs 6 more gas than != 0 when used on a require() statement as negative numbers are not allowed in uint values

details

• start is uint32
• liquidatorCut and auctioneerCut are uint256

github permalinks

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L255 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L300 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L358 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L393 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L398 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L416

mitigation

replace > 0 to != 0 for extra gas savings by each time is called the condition

duplicated require() check should be refactored

summary

duplicated require() / revert() checks should be refactored to a modifier or function to save gas

details

• "Vault not under auction" check done 4 times. Lines: L255, L300, L358, L416
• "Not enough bought" check done 2 times. Lines: L313, L365
• "Join not found" check done 2 times. Lines: L328, L395

github permalinks

https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L255 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L300 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L358 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L416 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L313 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L365 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L328 https://github.com/code-423n4/2022-07-yield/blob/main/contracts/Witch.sol#L395

mitigation

refactor this checks to different functions to save gas