Read the findings of the 2022 Web3 Security Report

When the same LP token is added into a pool more than once in function `add()`, the total amount of reward in function `updatePool()` will be incorrectly calculated. The current implementation is relying on the operation correctness to avoid repeatedly adding the same LP token to the pool, as the function will only be called by the owner.

add() Function Not Restricted

When `msg.sender` calls `enterStaking()`, `monster` token will be minted to `msg.sender` when `pool.lpToken` is staked in the contract. However, if the `msg.sender` calls `emergencyWithdraw()`, the `pool.lpToken` can be transferred back to the `msg.sender` but the `monster` token that has been minted to the `msg.sender` will not be burnt.
Therefore, `msg.sender` can call `enterStaking()` and `emergencyWithdraw()` repeatedly to ultimately mint a huge amount of monster token, with just the same amount of `pool.lpToken`.

Logic Flaw In `emergencyWithdraw()`

In the contract `MasterChef`, the role, `_owner`, has the authority over the functions shown below,
- `updateMultiplier()`, to update bonus multiplier, which is the bonus multiplier for early yok makers.
- `add()`, to add a new lp to the pool.
- `set()`, to update the given pool's yok allocation points.
- `setStakingRewards()`, to set StakingRewards contract.
- `setMigrator()`, to set migrator contract.
- `updateYOKPerSecond()`, to update yok tokens created per second.

Any compromise to the privileged account which has access to `_owner` may allow the hacker to take advantage of this.


Centralization Risk in MasterChef.sol

In the functions `deposit()/withdraw()`, the users will retrieve the reward tokens, before depositing/withdrawing the lp tokens. The `yok` token is transferred to the retriever from the contract `MonsterToken` via the function `MonsterToken.safeCakeTransfer()`. However, we do not find the code which mints or transfers the `yok` token to the contract `MonsterToken`. The users may not get the full amount of rewards when the `yok` token balance in the contract `MonsterToken` is insufficient.

Source of reward token

Variable `BONUS_MULTIPLIER` and `updateYOKPerSecond` do not have reasonable boundaries in function `updateMultiplier()` and `updateYOKPerSecond()` so they can be changed arbitrarily after deploying.

Lack of reasonable boundary

The order of external call/transfer and storage manipulation must follow the check-effect-interaction pattern in the following functions:

- `deposit()`
- `withdraw()`
- `enterStaking()`
- `leaveStaking()`
- `emergencyWithdraw()`

Check Effect Interaction Pattern Violated

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user stakes 100 deflationary tokens (with a 10% transaction fee) in a MasterChef, only 90 tokens actually arrived in the contract. However, the user can still withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. 

The MasterChef takes the pool token balance(the `lpSupply`) into account when calculating the users' reward. An attacker can repeat the process of deposit and withdraw to lower the token balance(`lpSupply`) in a deflationary token pool and cause the contract to increase the reward amount. 

Reference:
https://thoreum-finance.medium.com/what-exploit-happened-today-for-gocerberus-and-garuda-also-for-lokum-ybear-piggy-caramelswap-3943ee23a39f

Incompatibility With Deflationary Tokens(Farming)

Some input variables should not be zero address, but do not have zero address validation.

For example,
- Variable `_yok`, `_monster` in `MasterChef.constructor()`
- Variable `_migrator` in `MasterChef.setMigrator()`
- Variable `_to` in `MasterChef.safeCakeTransfer()`

Lack of Zero Address Validation

The following code snippet in the functions	`MasterChef.add()` and `MasterChef.set()` update the pools and distribute the reward if `_withUpdate` evaluates to `true`:

```solidity
    if (_withUpdate) {
        massUpdatePools();
    }
```

However, in the case that `_withUpdate` evaluates to false, a significant loss of reward might happen.

The reward calculation in the function `MasterChef.updatePool()` is:

```solidity=225
        uint256 yokReward = multiplier.mul(yokPerSecond).mul(pool.allocPoint).div(totalAllocPoint);
```
Assuming there is only one pool with `pool.allocPoint == 50` and `totalAllocPoint == 50` at the beginning. Now we want to add another pool with `pool.allocPoint == 50`.

There will be two scenarios on calculating the pool reward, 

Case 1: _withUpdate is `true` value.
- Step 1, distribute the reward and update the pool.
- Step 2, add or set the given pool information.
In this case, the reward calculated in step 1 is `yokReward = multiplier.mul(yokPerSecond).mul(50).div(50)`, which is equivalent to `multiplier.mul(yokPerSecond)`.

Case 2: _withUpdate is `false` value.
- Step 1, add or set the given pool information.
In this case, the reward calculated later will be `yokReward = multiplier.mul(yokPerSecond).mul(50).div(100)`, which is half as much as the reward calculated in case 1.

Potential Loss of Pool Rewards

The linked variables do not conform to the standard naming convention of Solidity whereby functions and variable names utilize the format unless variables are declared as constant in which case they utilize the format.

Variable Naming Convention

There's no sanity check to validate if a pool is existing.

Recommended Explicit Pool Validity Checks

`setMigrator()` function can set migrator contract to any contract that is implemented from `IMigratorChef` interface by the owner. As result, invocation of `migrator.migrate()` in function migrate() may bring dangerous effects as it is unknown to the user. However, the project may lose the ability to upgrade and migrate if `setMigrator()` and `migrate()` are removed.

Unknown Implementation of migrator.migrate()

Function that affect the status of sensitive variables should be able to emit events as notifications to customers.

Missing Emit Events

The function that affects the status of sensitive variables should be able to emit events as notifications to customers.
- `setMigrator()`
- `updateMultiplier()`

In the contract `MonsterToken`, the role `_owner` has the authority over the following function:
- `mint()`, to mint Monster tokens to `_to` address.
- `burn()`, to burn Monster tokens from the `_from` address.
- `safeYOKTransfer()`, to transfer yok tokens to `_to` address.

Any compromise to the `_owner` account may allow the hacker to take advantage of this.

Centralization Risk in MonsterToken.sol

In the contract `Owned`, the role `_owner` has the authority over the following function:
- `nominateNewOwner()`, to nominate a new owner.

Any compromise to the `_owner` account may allow the hacker to take advantage of this.

Centralization Risk in Owned.sol

In the contract `Pausable`, the role `_owner` has the authority over the following function:
- `setPaused()`, to change the paused state of the contract.

Any compromise to the `_owner` account may allow the hacker to take advantage of this.

Centralization Risk in Pausable.sol

In the contract, `StakingRewards`, the role `_chef` has the authority over the functions:
- `stake()`, to stake LP tokens.
- `withdraw()`, to withdraw LP tokens.
- `emergencyWithdraw()`, to withdraw LP tokens without caring about rewards.
  
Any compromise to the privileged account which has access to `_chef` may allow the hacker to take advantage of this.

In the contract, `StakingRewards`, the role `_owner` has the authority over the functions:
- `updatePeriodFinish()`, to update finish period timestamp.
- `setRewardsDuration()`, to set rewards duration.
  
Any compromise to the privileged account which has access to `_owner` may allow the hacker to take advantage of this.

In the contract, `StakingRewards`, the role `rewardsDistribution` has the authority over the functions:
- `notifyRewardAmount()`
  
Any compromise to the privileged account which has access to `rewardsDistribution` may allow the hacker to take advantage of this.


Centralization Risk in StakingRewards.sol

Unlike the other functions with external calls in their implementations, the linked one is not modified by the `nonReentrant()` modifier.

Missing `nonReentrant()` Modifier

The role `owner` can set state variable `periodFinish` arbitrary large or small causing potential risks in fees and anti whale.

The function `notifyRewardAmount()` does not check if the reward pool has received the real reward to distribute. Some users can get more rewards if `rewardDistribution` set a large number of reward. Some users may earn few rewards. i.e. RewardDistribution can notify 1000 reward tokens even reward pool only received 100 rewards. In this case, the `rewardRate` will be significantly larger than the actual.

Function `notifyRewardAmount()` Does Not Check If It Receives The Reward

The given input is missing the sanity check for the non-zero address in the aforementioned line. 

Missing Zero Address Validation

Addresses should be checked before assigning to make sure they are not zero addresses.

Missing Input Validation

In the contract, `YokaiFactory`, the role, `feeToSetter`, has the authority over the functions shown in the diagram below.
- `setFeeTo()`, to set the `feeTo`, which is the dev fee receiver address.
- `setFeeToSetter()`, to transfer the `feeToSetter` role to another address.

Any compromise to the privileged account which has access to `feeToSetter` may allow the hacker to take advantage of this.

Centralization Risk in YokaiFactory.sol

The usage of `allPairs` array is as a counter to maintain the number of created pairs.

Unnecessary Array as Counter

In the contract `YOKToken`, the role `_owner` has the authority over the following function:
- `mint()`, the `_owner` can mint any amount of tokens.

Any compromise to the `_owner` account may allow the hacker to take advantage of this.

Centralization Risk in YOKToken.sol

In essence, YOKToken governance lets token holders delegate their voting power to another entity. However, if that token holder then transfers the tokens to someone else, the delegator still maintains their governance power. The second token holder can now delegate tokens once again, multiplying the delegator’s power by as much as necessary. The bug is that the token transfer does not call `_moveDelegates()`.

Does not move delegates while transferring token

In the contract `YOKVault`, the role `_owner` has the authority over the following function:
- `setAdmin()`, to set admin address.
- `setTreasury()`, to set treasury address.
  
Any compromise to the `_owner` account may allow the hacker to take advantage of this.

In the contract `YOKVault`, the role `admin` has the authority over the following function:
- `setPerformanceFee()`, to set performance fee.
- `setCallFee()`, to set call fee.
- `setWithdrawFee()`, to set withdraw fee.
- `setWithdrawFeePeriod()`, to set withdraw period.
- `emergencyWithdraw()`, to withdraw from MasterChef to Vault without caring about rewards.
- `inCaseTokensGetStuck()`, to withdraw unexpected tokens sent to yok Vault.
- `pause()`, to trigger stopped state.
- `unpause()`, to return to normal state.

Any compromise to the `admin` account may allow the hacker to take advantage of this.



Centralization Risk in YOKVault.sol

Under what circumstances will `diff < balWithdraw`? The `butterToken` deposited by the user will be staked by the contract to `masterChef` for mining to obtain rewards. The reward is also `butterToken`, and after the reward is withdrawn, it will be staked again to `masterChef`. The final balance in the contract should be greater than the amount deposited by the user.

Logic Issue  on Function `withdraw()`

In the contract, `YokaiPair`, the role, `factory`, has the authority over the functions shown in the diagram below.
- `initialize()`, to set the token addresses in the swap pair.

Any compromise to the privileged account which has access to `factory` may allow the hacker to take advantage of this.

Centralization Risk in YokaiPair.sol

If the value of `totalSupply` is 0, the following two division operations will fail due to the divide by 0 error, which ultimately make the invocation to `burn()` function fail.

```solidity=144
amount0 = liquidity.mul(balance0) / _totalSupply; // using balances ensures pro-rata distribution
amount1 = liquidity.mul(balance1) / _totalSupply; // using balances ensures pro-rata distribution
```

Divide by Zero

Libraries `UQ112x112`, `SafeMath` and `Math` will be inherited by the contract `YokaiPair` every time a new pair is created, which will cost extra gas on creating new pairs

Replace Libraries with Inherited Contract in Contract Template

The `assert()` function should only be used to test for internal errors, and to check invariants. The `require()` function should be used to ensure valid conditions, such as inputs, or contract state variables are met, or to validate return values from calls to external contracts.

YokaiSwap

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

37

0

37

0

0