QA Report

[code423n4]

Summary

Low Risk Issues

	Issue	Instances
1	Batch-related functions will revert if removeAddress() is called	2
2	Staking contract's token not verified to be the same token as the staking token	1
3	Missing infinite approval functionality	1
4	Missing checks that the end time matches the duration	1
5	Missing input validations and timelocks	5
6	Front-runable initializer	2

Total: 12 instances over 6 issues

Non-critical Issues

	Issue	Instances
1	Return values of approve() not checked	2
2	Misleading variable names	1
3	public functions not called by the contract should be declared external instead	1
4	constants should be defined rather than using magic numbers	3
5	Use a more recent version of solidity	3
6	Typos	10
7	NatSpec is incomplete	2
8	Event is missing indexed fields	12

Total: 34 instances over 8 issues

Low Risk Issues

1. Batch-related functions will revert if removeAddress() is called

removeAddress() removes entries from the storage array that holds batch contract addresses, but it doesn't fill in the deleted entries with a replacement value. When other functions hit these null entries and try to call functions on the zero address, they'll revert, causing the whole function to fail

There are 2 instances of this issue:

File: src/contracts/BatchRequests.sol   #1

33       function canBatchContracts() external view returns (Batch[] memory) {
34           uint256 contractsLength = contracts.length;
35           Batch[] memory batch = new Batch[](contractsLength);
36           for (uint256 i; i < contractsLength; ) {
37:              bool canBatch = IStaking(contracts[i]).canBatchTransactions();

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L33-L37

File: src/contracts/BatchRequests.sol   #2

50       function canBatchContractByIndex(uint256 _index)
51           external
52           view
53           returns (address, bool)
54       {
55           return (
56               contracts[_index],
57:              IStaking(contracts[_index]).canBatchTransactions()

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L50-L57

2. Staking contract's token not verified to be the same token as the staking token

There may be a mismatch between the token that _stakingContract is in charge of, and the actual token used by the LiquidityReserve. This code should check that they are in fact the same

There is 1 instance of this issue:

File: src/contracts/LiquidityReserve.sol   #1

57       function enableLiquidityReserve(address _stakingContract)
58           external
59           onlyOwner
60       {
61           require(!isReserveEnabled, "Already enabled");
62           require(_stakingContract != address(0), "Invalid address");
63   
64           uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
65               msg.sender
66           );
67           // require address has minimum liquidity
68           require(
69               stakingTokenBalance >= MINIMUM_LIQUIDITY,
70               "Not enough staking tokens"
71           );
72:          stakingContract = _stakingContract;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L57-L72

3. Missing infinite approval functionality

Most other contracts in the repository use type(uint256).max to mean infinite approval, rather than a specific approval amount. Not doing the same thing here will mean inconsistent behavior between the components, will mean that approvals will eventually run down to zero, and will mean that there will be hard-to-track-down issues when things eventually start failing

There is 1 instance of this issue:

File: src/contracts/Yieldy.sol   #1

210          require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
211  
212          uint256 newValue = _allowances[_from][msg.sender] - _value;
213          _allowances[_from][msg.sender] = newValue;
214:         emit Approval(_from, msg.sender, newValue);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L210-L214

4. Missing checks that the end time matches the duration

There is 1 instance of this issue:

File: src/contracts/Staking.sol   #1

95               duration: _epochDuration,
96               number: 1,
97               timestamp: block.timestamp, // we know about the issues surrounding block.timestamp, using it here will not cause any problems
98:              endTime: _firstEpochEndTime,

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L95-L98

5. Missing input validations and timelocks

The following instances are missing checks for zero addresses and or valid ranges for values. Even if the DAO is the one setting these values, it's important to add sanity checks in case someone does a fat-finger operation that is missed by DAO participants who may not be very technical. There are also no timelocks involved, which should be rectified

There are 5 instances of this issue:

File: src/contracts/Staking.sol

217      function setEpochDuration(uint256 duration) external onlyOwner {
218          epoch.duration = duration;
219          emit LogSetEpochDuration(block.number, duration);
220:     }

167      function setAffiliateFee(uint256 _affiliateFee) external onlyOwner {
168          affiliateFee = _affiliateFee;
169          emit LogSetAffiliateFee(block.number, _affiliateFee);
170:     }

217      function setEpochDuration(uint256 duration) external onlyOwner {
218          epoch.duration = duration;
219          emit LogSetEpochDuration(block.number, duration);
220:     }

226      function setWarmUpPeriod(uint256 _vestingPeriod) external onlyOwner {
227          warmUpPeriod = _vestingPeriod;
228          emit LogSetWarmUpPeriod(block.number, _vestingPeriod);
229:     }

235      function setCoolDownPeriod(uint256 _vestingPeriod) external onlyOwner {
236          coolDownPeriod = _vestingPeriod;
237          emit LogSetCoolDownPeriod(block.number, _vestingPeriod);
238:     }

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L217-L220

6. Front-runable initializer

There is nothing preventing another account from calling the initializer before the contract owner. In the best case, the owner is forced to waste gas and re-deploy. In the worst case, the owner does not notice that his/her call reverts, and everyone starts using a contract under the control of an attacker

There are 2 instances of this issue:

File: src/contracts/LiquidityReserve.sol   #1

36       function initialize(
37           string memory _tokenName,
38           string memory _tokenSymbol,
39           address _stakingToken,
40           address _rewardToken
41:      ) external initializer {

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L36-L41

File: src/contracts/Yieldy.sol   #2

29       function initialize(
30           string memory _tokenName,
31           string memory _tokenSymbol,
32           uint8 _decimal
33:      ) external initializer {

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L29-L33

Non-critical Issues

1. Return values of approve() not checked

Not all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

There are 2 instances of this issue:

File: src/contracts/Staking.sol   #1

79:               IERC20(TOKE_POOL).approve(CURVE_POOL, type(uint256).max);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L79

File: src/contracts/Staking.sol   #2

83:           IERC20(STAKING_TOKEN).approve(TOKE_POOL, type(uint256).max);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L83

2. Misleading variable names

There is 1 instance of this issue:

File: src/contracts/LiquidityReserve.sol   #1

/// @audit code is no longer FOX-specific
112:         uint256 lrFoxSupply = totalSupply();

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L112

3. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There is 1 instance of this issue:

File: src/contracts/Staking.sol   #1

370:      function unstakeAllFromTokemak() public onlyOwner {

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L370

4. constants should be defined rather than using magic numbers

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

There are 3 instances of this issue:

File: src/contracts/Staking.sol   #1

/// @audit 0x9008D19f58AAbD9eD0D60971565AA8510560ab41
73:           COW_SETTLEMENT = 0x9008D19f58AAbD9eD0D60971565AA8510560ab41;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L73

File: src/contracts/Staking.sol   #2

/// @audit 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110
74:           COW_RELAYER = 0xC92E8bdf79f0507f65a392b0ab4667716BFE0110;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L74

File: src/contracts/Staking.sol   #3

/// @audit 12
76:           timeLeftToRequestWithdrawal = 12 hours;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L76

5. Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

There are 3 instances of this issue:

File: src/contracts/LiquidityReserve.sol   #1

2:    pragma solidity 0.8.9;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L2

File: src/contracts/Migration.sol   #2

2:    pragma solidity 0.8.9;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L2

File: src/contracts/Staking.sol   #3

2:    pragma solidity 0.8.9;

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L2

6. Typos

There are 10 instances of this issue:

File: src/contracts/Staking.sol

/// @audit withdrawRequest
315:          @notice creates a withdrawRequest with Tokemak

/// @audit requestedWithdraws
316:          @dev requestedWithdraws take 1 tokemak cycle to be available for withdraw

/// @audit cycleTime
348:          @notice checks TOKE's cycleTime is within duration to batch the transactions

/// @audit requestWithdraw
367:          @notice owner function to requestWithdraw all FOX from tokemak in case of an attack on tokemak

/// @audit requestWithdraw
368:          @dev this bypasses the normal flow of sending a withdrawal request and allows the owner to requestWithdraw entire pool balance

/// @audit asdf
675:          // prevent unstaking if override due to vulnerabilities asdf

/// @audit autoRebase
767:       * @dev this is function is called from claimFromTokemak if the autoRebase bool is set to true

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L315

File: src/contracts/Yieldy.sol

/// @audit profit_
74:           @notice increases Yieldy supply to increase staking balances relative to profit_

/// @audit co
232:          @notice called from the staking contract co create Yieldy tokens

/// @audit co
262:          @notice called from the staking contract co burn Yieldy tokens

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L74

7. NatSpec is incomplete

There are 2 instances of this issue:

File: src/contracts/BatchRequests.sol   #1

/// @audit Missing: '@param _index'
46        /**
47            @notice shows if contracts can batch by index
48            @return (address, bool)
49         */
50        function canBatchContractByIndex(uint256 _index)
51            external
52            view
53:           returns (address, bool)

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L46-L53

File: src/contracts/BatchRequests.sol   #2

/// @audit Missing: '@param _index'
61        /**
62            @notice get address in contracts by index
63            @return address
64         */
65:       function getAddressByIndex(uint256 _index) external view returns (address) {

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L61-L65

8. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There are 12 instances of this issue:

File: src/contracts/LiquidityReserve.sol

21:       event FeeChanged(uint256 fee);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L21

File: src/contracts/Staking.sol

21:       event LogSetEpochDuration(uint256 indexed blockNumber, uint256 duration);

22:       event LogSetWarmUpPeriod(uint256 indexed blockNumber, uint256 period);

23:       event LogSetCoolDownPeriod(uint256 indexed blockNumber, uint256 period);

24:       event LogSetPauseStaking(uint256 indexed blockNumber, bool shouldPause);

25:       event LogSetPauseUnstaking(uint256 indexed blockNumber, bool shouldPause);

26        event LogSetPauseInstantUnstaking(
27            uint256 indexed blockNumber,
28            bool shouldPause
29:       );

30        event LogSetAffiliateAddress(
31            uint256 indexed blockNumber,
32            address affilateAddress
33:       );

34:       event LogSetAffiliateFee(uint256 indexed blockNumber, uint256 fee);

36:       event LogSetCurvePool(address indexed curvePool, int128 to, int128 from);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L21

File: src/contracts/Yieldy.sol

15        event LogSupply(
16            uint256 indexed epoch,
17            uint256 timestamp,
18            uint256 totalSupply
19:       );

21:       event LogRebase(uint256 indexed epoch, uint256 rebase, uint256 index);

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L15-L19

[moose-code]

Low risk issues:
1 - Agree
2 - Agree
3 - Agree
4 - Agree
5 - Agree
6 -Agree

Informational:
1- Agree
2 -Agree
3 - Agree
4 - Strongly agree, this is very helpful.
5 - Agree
6 - Agree
7 - Agree
8 - Agree