Yieldy contest - hubble's results

Lines of code

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

Vulnerability details

In Yieldy contract, while calling _storeRebase() in function rebase(), updatedTotalSupply is passed instead of currentTotalSupply.

Filing this as medium risk , due to two impacts, in the way this parameter is used in _storeRebase() function.

1. The rebasePercent is under-calculated, and in the LogRebase Event, wrong (lesser than actual) value is emitted.

2. The rebases[] storage value will have wrong values, with totalStakedBefore & totalStakedAfter having same value.

So actual changes in rebase cant be reconstructed via historical info (via both events & storage).

Recommended Mitigation Steps

Modify the line#100 to as below in rebase() function

_storeRebase(currentTotalSupply, _profit, _epoch);

Summary of Findings for Low / Non-Critical issues

• L-01 : Unnecessary check for balance in _requestWithdrawalFromTokemak() function
• L-02 : Users can lose all their stakingToken while using instantUnstake()
• L-03 : Timelock functionality missing while updating critical protocol parameters
• L-04 : Missing nullcheck in setEpochDuration may have detrimental effects
• L-05 : Possibility of losing control of Staking contract via renounceOwnership()
• L-06 : Possibility of losing control of Staking contract via transferOwnership()

Details L-01

Title : Unnecessary check for balance in _requestWithdrawalFromTokemak() function

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

Refering to the lines#324 in Staking.sol contract, function _requestWithdrawalFromTokemak().

    // the only way balance < _amount is when using unstakeAllFromTokemak
    uint256 amountToRequest = balance < _amount ? balance : _amount;

The check is being done for unstakeAllFromTokmak as mentioned in the comment. But in unstakeAllFromTokemak() the value of tokePoolBalance that is being passed is same as given below in _requestWithdrawalFromTokemak. uint256 balance = ITokePool(TOKE_POOL).balanceOf(address(this));

Hence the condition of balance < _amount will never hold good. Hence this line is unnecessary.

Recommended Mitigation Steps

Modified function as below.

    function _requestWithdrawalFromTokemak(uint256 _amount) internal {
        if (_amount > 0) tokePoolContract.requestWithdrawal(_amount);
    }

Details L-02

Title : Users can lose all their stakingToken while using instantUnstake()

In LiquidityReserve.sol there is a fee paid by user on using instantUnstake() to get their stakingToken instantly. If the admin behaves maliciously, they can set the fee to BASIS_POINTS i.e., 100%, which will result in 0 payout to users using instantUnstake()

Recommended Mitigation Steps

A reasonable max value of 5% or 10% can be used to check while setting the fee in setFee() function.

Details L-03

Title : Timelock functionality missing while updating critical protocol parameters

Contract Staking.sol

line#235 function setCoolDownPeriod(uint256 _vestingPeriod) line#217 function setEpochDuration(uint256 duration) line#167 function setAffiliateFee(uint256 _affiliateFee)

LiquidityReserve.sol

line#92 function setFee(uint256 _fee)

These functions that make critical changes should have a timelock, so that users can see the upcoming changes and have time to react to these changes.

Impact

Stable and trustworthy protocol.

Proof of Concept

https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing https://github.com/code-423n4/2022-01-sandclock-findings/issues/178

Recommended Mitigation Steps

Add timelock for critical changes which will effect user experience and decisions.

Details L-04

Title : Missing nullcheck in setEpochDuration may have detrimental effects

Because null check is missing in setEpochDuration(), its possible to set a value of 0 to epoch.duration In the eventuality of this set to 0, every external call like stake, unstake, etc., which calls rebase() will increment the epoch which will be detrimental and break functionality like warmup, cooldown, etc.,

Recommended Mitigation Steps

Add null check along with a reasonable min value to be checked while setting the epoch.duration value.

Details L-05

Title : Possibility of losing control of Staking contract via renounceOwnership()

In Staking.sol the Staking contract inherits OwnableUpgradeable, which has the function renounceOwnership() If by mistake or maliciously the renounceOwnership() is called, there is a possibility of losing control on the Staking contract.

Recommended Mitigation Steps

Override this renounceOwnership() function in Staking.sol by making it a no-op.

Details L-06

Title : Possibility of losing control of Staking contract via transferOwnership()

In Staking.sol the Staking contract inherits OwnableUpgradeable, which has the function transferOwnership() If by mistake a wrong address is used in transferOwnership(), then there is a possibility of losing control on the Staking contract.

Recommended Mitigation Steps

Best practice is to use two step process to transfer ownership 1st Step : setNewOwner << setting/proposing the address of the new Owner 2nd Step : acceptOwnership << the new Owner will execute this function to complete the ownership transfer