Contract Migration implements function moveFundsToUpgradedContract that is expected to move funds from OLD_CONTRACT to NEW_CONTRACT. The issue is that for withdrawing funds it uses instantUnstake that does not exist in Staking contract. Contract Staking implements instantUnstakeReserve and instantUnstakeCurve, while instantUnstake is being implemented by LiquidityReserve.
It is recommended to use ILiquidityReserve interface for OLD_CONTRACT and make sure that OLD_CONTRACT is a LiquidityReserve contract.
2. Missing threshold validation
Risk
Low
Impact
Contract Staking implements multiple functions for setting staking parameters. The issue is that these functions are missing basic threshold checks of received arguments which makes it risky that, either by accident or intentionally, the parameters will be set to values that will completely break Staking contract logic.
It is recommended to add threshold checks for listed parameters.
3. Missing approve(0) first
Risk
Low
Impact
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
Approve with a zero amount first before setting the actual amount.
4. Missing events
Risk
Low
Impact
Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.
It is recommended to add zero address checks for listed parameters.
6. 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.
It is recommended to implement two-step process for changing ownership.
7. Use max values
Risk
Non-Critical
Impact
Contract YieldStorage.sol uses bit negation to retrieve MAX_UINT256 and MAX_SUPPLY (max uint128), these values can be easily retrieved from type object.
1. Migration incorrect interface
Impact
Contract
Migration
implements functionmoveFundsToUpgradedContract
that is expected to move funds fromOLD_CONTRACT
toNEW_CONTRACT
. The issue is that for withdrawing funds it usesinstantUnstake
that does not exist inStaking
contract. ContractStaking
implementsinstantUnstakeReserve
andinstantUnstakeCurve
, whileinstantUnstake
is being implemented byLiquidityReserve
.Proof of Concept
Migration.sol
:Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to use
ILiquidityReserve
interface forOLD_CONTRACT
and make sure thatOLD_CONTRACT
is aLiquidityReserve
contract.2. Missing threshold validation
Risk
Low
Impact
Contract
Staking
implements multiple functions for setting staking parameters. The issue is that these functions are missing basic threshold checks of received arguments which makes it risky that, either by accident or intentionally, the parameters will be set to values that will completely breakStaking
contract logic.Proof of Concept
Staking.sol
:affiliateFee
for maximum value - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L167-L170duration
for minimum and maximum value - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L217-L220_vestingPeriod
for minimum and maximumwarmUpPeriod
value - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L226-L229_vestingPeriod
for minimum and maximumcoolDownPeriod
value - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L235-L238_timestamp
for minimum and maximumtimemLeftToRequestWithdrawal
value - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L246-L251Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to add threshold checks for listed parameters.
3. Missing approve(0) first
Risk
Low
Impact
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
Proof of Concept
Staking.sol
:LiquidityReserver.sol
:Migration.sol
:Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
Approve with a zero amount first before setting the actual amount.
4. Missing events
Risk
Low
Impact
Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.
Proof of Concept
Staking.sol
:claimFromTokemak
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L111transferToke
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L141setTimeLeftToRequestWithdrawal
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L246unstakeAllFromTokemak
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L370sendWithdrawalRequests
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L384stake
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L406claim
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L465claimWithdraw
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L485instantUnstakeReserve
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L571instantUnstakeCurve
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L600unstake
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L674rebase
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L701addRewardsForStakers
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L741preSign
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L769LiquidityReserve.sol
:enableLiquidityReserve
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L57addLiquidity
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L104removeLiquidity
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L161instantUnstake
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L188unstakeAllRewardTokens
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L214Migration.sol
:moveFundsToUpgradeContract
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L43BatchRequests.sol
:addAddress
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L81-L83removeAddress
function event - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L89-L99Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to add missing events to listed functions.
5. Missing zero address checks
Risk
Low
Impact
Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.
Proof of Concept
Staking.sol
:stake
for_recipient
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L406LiquidityReserve.sol
:instantUnstake
for_recipient
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L188BatchRequests.sol
:addAddress
for_address
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L81removeAddress
for_address
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L89Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to add zero address checks for listed parameters.
6. 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
Staking.sol
:owner
throughOwnableUpgradeable
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L18LiquidityReserve.sol
:owner
throughOwnableUpgradeable
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L16BatchRequests.sol
:owner
throughOwnable
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L8Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to implement two-step process for changing ownership.
7. Use max values
Risk
Non-Critical
Impact
Contract
YieldStorage.sol
uses bit negation to retrieveMAX_UINT256
andMAX_SUPPLY
(max uint128
), these values can be easily retrieved fromtype
object.Proof of Concept
YieldyStorage.sol
:Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to use
type(uint256).max
andtype(uint128).max
respectively.8. Use scientific notation
Risk
Non-Critical
Impact
Proof of Concept
StakingStorage.sol
:LiquidityReserveStorage.sol
:Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to use scientific notation such as
1e4
.9. Missing/incomplete natspec comments
Risk
Non-Critical
Impact
Contracts are missing natspec comments which makes code more difficult to read and prone to errors.
Proof of Concept
Staking.sol
:@param _curvePool
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L155@param _affiliateFee
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L163-L167@param _affiliateAddress
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L175@param _shouldPause
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L183-L187@param _shouldPause
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L193-L197@param _shouldPause
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L203-L207@param duration
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L213-L217@param _vestingPeriod
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L223-L226@param _vestingPeriod
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L232-L235@param _amount
and@param _recipient
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L402-L406@param stake
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L453-L456@param _recipient
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L461-L465@param _amount
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L511-L517Yieldy.sol
:@param _previsionCirculating
,@param _profit
and@param _epoch
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L105-L110@param _wallet
and@return
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L133-L138@param _amount
and@return
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L143-L147@param _credits
and@return
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L156-L160@param _to
,@param _value
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L177-L182@param _from
,@param _to
,@param _value
,@return
descriptions - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L198-L205@return
- https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L224-L227Migration.sol
:BatchRequests.sol
:@return
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L29-L33@param _index
and@return
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L46-L50@param _index
and@return
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L61-L65@return
description - https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L69-L73Tools Used
Manual Review / VSCode
Recommended Mitigation Steps
It is recommended to add missing natspec comments.