QA Report

[code423n4]

C4-001 : Critical changes should use two-step procedure - Non Critical

C4-002 : Low level calls with solidity version 0.8.14 can result in optimizer bug. - LOW

C4-003 : Use of Block.timestamp - Non-critical

C4-004: Centralization Risk - LOW

C4-005: Open TODOs - Non-critical

C4-006 : Implement check effect interaction - LOW

C4-007 : Divide before multiply - LOW

C4-008 : Missing zero-address check in the setter functions and constructor - Low

ISSUES

C4-001 : Critical changes should use two-step procedure

Impact - NON CRITICAL

The critical procedures should be two step process. The contracts inherit OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

1. Navigate to the following contract.

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

Tools Used

Code Review

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

C4-002 : Low level calls with solidity version 0.8.x can result in optimiser bug.

Impact

The protocol is using low level calls with solidity version 0.8.x which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Proof of Concept

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.15.

C4-003 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

1. Navigate to the following contract.

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

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-004 : Centralization Risk

Impact - LOW

Admin role has absolute power across Witch contracts with several auth functions. There is no ability to change admin to a new address or renounce it which is helpful for lost/compromised admin keys or to delegate control to a different governance/DAO address in future.

The project does not use the widely used OpenZeppelin Ownable library which provides transfer/renounce functions to mitigate such compromised/accidental situations with admin keys. This makes admin role/key a single-point of failure.

Code Location

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

Recommended Mitigation Steps

Ensure admins are reasonably redundant/independent (3/7 or 5/9) multisigs and add transfer/renounce functionality for admin. Consider using OpenZeppelin’s Ownable library.

C4-005 : Open TODOs

Impact - LOW

Open TODOs can point to architecture or programming issues that still need to be resolved.

Location

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

Recommended Mitigation Steps

Consider resolving the TODOs before deploying.

C4-006 : Implement check effect interaction

Impact - LOW

There is no impact to the funds but to align with best practices, it is always better to update internal state before any external function calls.

Location

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

Recommended Mitigation Steps

Call external interaction before accounting. (Update the internal accounting before transferring the tokens out.)

C4-007 : Divide before multiply

Impact - LOW

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Location

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

Recommended Mitigation Steps

In general, it's usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous. Consider ordering multiplication before division.

C4-008 : Missing zero-address check in the setter functions in the constructor - Low

Impact

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept

1. Navigate to the following contracts.

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

[alcueca]

None useful

[PierrickGT]

Labelled as invalid cause the warden didn't provide any useful suggestions.