QA Report

[code423n4]

Low Risk

1. Missing pause functionality
2. Missing zero address checks
3. Critical address change

Non-Critical

4. Usage of not well-tested solidity version might contain undiscovered vulnerabilities
5. Missing/incomplete natspec comments

1. Missing pause functionality

Risk

Low

Impact

Contract Witch is missing pause functionality that could be used in case of security incidents and would block executing selected functions while the contract is paused.

Proof of Concept

Witch.sol:

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add functionality for pausing contract Witch and go through all publicly/externally accessible functions to decide which one should be forbidden from running while the contract is paused.

2. Missing zero address checks

Risk

Low

Impact

Multiple functions of Witch contract do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

Witch.sol:

• Missing check in constructor for cauldron_ and ladle_ addresses - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L71
• Missing check in point for value address - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L83
• Missing check in setAnotherWitch for value address - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L141
• Missing check in payBase for to address - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L288
• Missing check in payFYToken for to address - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L346

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

3. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

Witch.sol:

• Modifier auth inherited from AccessControl - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L19

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing ownership.

4. Usage of not well-tested solidity version might contain undiscovered vulnerabilities

Risk

Non-Critical

Impact

Using the latest versions might make contracts susceptible to undiscovered compiler bugs.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use more stable and tested solidity version such as 0.8.10.

5. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Contract Witch is missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

Witch.sol:

• Missing natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L71
• Missing @param vaultId natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L214
• Missing multiple @param and @return natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L220-L223
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L266-L268
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L385-L386
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L407-L409
• Missing multiple @param natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L461-L463
• Missing multiple @param and @return natspec - https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L561-L562

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

[alcueca]

Thanks for the suggestion for a pause functionality, and for the natspec check. Nothing else is useful or correct.