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();
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
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
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,
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 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
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
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
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
Summary
Low Risk Issues
removeAddress()
is calledTotal: 12 instances over 6 issues
Non-critical Issues
approve()
not checkedpublic
functions not called by the contract should be declaredexternal
insteadconstant
s should be defined rather than using magic numbersindexed
fieldsTotal: 34 instances over 8 issues
Low Risk Issues
1. Batch-related functions will revert if
removeAddress()
is calledremoveAddress()
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 failThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L33-L37
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 theLiquidityReserve
. This code should check that they are in fact the sameThere is 1 instance of this issue:
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 failingThere is 1 instance of this issue:
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:
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:
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:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L36-L41
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 checkedNot all
IERC20
implementationsrevert()
when there's a failure inapprove()
. The function signature has aboolean
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 anythingThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L79
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:
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 declaredexternal
insteadContracts are allowed to override their parents' functions and change the visibility from
external
topublic
.There is 1 instance of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L370
4.
constant
s should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 3 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L73
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L74
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 functionsThere are 3 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L2
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L2
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L2
6. Typos
There are 10 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L315
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:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L46-L53
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L61-L65
8. Event is missing
indexed
fieldsEach
event
should use threeindexed
fields if there are three or more fieldsThere are 12 instances of this issue:
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L21
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L21
https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L15-L19