Read the findings of the 2022 Web3 Security Report

The given input are missing the check for the non-zero address:
- `MilkyMaker`.`constructor()`: `factory`, `dest`, `milky` and `wwada`;
- `MilkyMaker`.`setBridge()`: `token`, and `bridge`;
- `MasterMilker`.`constructor()`: `devaddr`;
- `MasterMilker`.`dev()`: `devaddr`;

Lack of Input Validation

A [documented issue](https://slowmist.medium.com/slowmist-a-brief-analysis-of-the-story-of-the-sushi-swap-attack-c7bc6709adea) exists within the `MilkyMaker.convert()` function, allowing an external attacker to steal funds.

For the remaining explanation, `SLP` corresponds to the Pair `WETH/USDT`. \
The steps for an attack are:
1. An attacker creates `SLP1` which is a pair for `SLP/WETH`; this pool is created (and therefore controlled) by the attacker;
2. The attacker calls `convert(SLP, WETH)`; \
When this function is called, the `_swap()` function will be called, this call will retrieve the associated LP for `SLP/WETH` (which is the malicious `SLP1`), and put it in the `pair` variable.
```solidity=230
IUniswapV2Pair pair = IUniswapV2Pair(
    factory.getPair(fromToken, toToken)
);
```
3. All the `SLP` will be swapped via the `SLP/WETH` pool, which is controlled by the attacker. \
In this case, the attacker can use a small amount of `WETH` to retrieve all the `SLP`.
```solidity=243
IERC20(fromToken).safeTransfer(address(pair), amountIn);
pair.swap(0, amountOut, to, new bytes(0));
```
4. As SLP is a regular pool, there will be many `SLP` tokens (from users’ swapping in `MilkySwap`).
5. At the end of the attack, the attacker will have received most of the `SLP`, that he can later swap for USDT or WETH.

Issue with the `convert()` function from SushiSwap

In the contract `MasterMilker`, the role `owner` has authority over the following functions:
- function `add()`: Add a LP to the Masterchef pool;
- function `set()`: For a given pool, modify the allocation of `MILKY`;
- function `updateEmissionRate()`: Modify the rate emission of `MILKY`.

Any compromise to the `owner` account may allow a hacker to take advantage of this authority and steal funds from the contract.
The attacker could :
1. Create two fake Tokens, that he would fully own;
2. Create a pair for the two tokens, and deposit the tokens to get fake LP;
3. Call `add()` from `MasterMilker` to add his malicious LP, with a maximum `_allocPoint`, then reduce the `_allocPoint` of other pools with the `set()` function, and increase the amount of Milky minted each block with `updateEmissionRate()`;
4. Call `deposit()` on his fake LP;
5. After a few minutes, once the attacker gathered enough `Milky`, withdraw his rewards and dump them to make a profit.

In the contract `MilkyToken`, the role `owner` has authority over the following functions:
- function `mint()`: Mint `Milky tokens`;

Any compromise to the `owner` account may allow a hacker to take advantage of this authority and get an infinite amount of tokens. Note: the owner of the milky token should be the MatsterMilk contract. If correctly set, it will not cause actual problem to the project. The status of this issue will be updated after deployment upon request.

In the contract `MilkyMaker`, the role `owner` has authority over the following functions:
- function `setBridge()`: Configure the `_bridges` variable for a token.

In the contract `Ownable`, the role `owner` has authority over the following functions:
- function `transferOwnership()`: Change the ownership of the contract.

In the contract `CreamyToken`, the role `admin` has authority over the following functions:
- `commit_transfer_ownership()`: Transfer ownership of `CreamyToken` contract;
- `apply_transfer_ownership()`: Apply the changes from `commit_transfer_ownership()`;
- `commit_smart_wallet_checker()`: Set the `future_smart_wallet_checker` variable, an external contract to check for approved smart contract wallets;
- `apply_smart_wallet_checker()`: Apply the changes from `commit_smart_wallet_checker()`.

In the contract `FeeDistributor`, the role `admin` has authority over the following functions:
- `checkpoint_token()`: Update the token checkpoint. Can only be called by the admin during the setup of the initial distribution;
- `commit_admin()`: Transfer ownership of `FeeDistributor` contract;
- `apply_admin()`: Apply the changes from `commit_admin()`;
- `toggle_allow_checkpoint_token()`: Toggle permission for checkpointing by any account;
- `kill_me()`: Kill the contract;
- `recover_balance()`: Recover ERC20 tokens from the contract.

Centralization Related Risks

The function `deposit_for(_addr: address, _value: uint256)` allows anyone to deposit tokens on the behalf of another user, under the conditions that the final user:
- Has funds already locked in the contract;
- Has approved the `CreamyToken` contract to spend his tokens.

For example, if Alice has approved the `CreamyToken` contract to spend all of her tokens, and if Alice has half of her tokens already locked in the contract for a period of time; then Bob can call the `deposit_for()` function to lock the other half of Alice's tokens in the contract.

This can cause an undesirable situation where a malicious user calls this function to lock other people funds.

Possibility to Lock Other Users Funds

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 `MasterMilker` 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

The [Solidity documentation](https://docs.soliditylang.org/en/v0.8.7/security-considerations.html) suggests that a smart contract should follow the `Checks-Effects-Interactions` pattern. 
However, the `emergencyWithdraw()` function violates the `Checks-Effects-Interactions` pattern by having external calls (`safeTransfer()`) before event emissions and state variables changes (Effects).
```solidity=244
      pool.lpToken.safeTransfer(address(msg.sender), user.amount); // TODO - CEI / re-entrancy
      emit EmergencyWithdraw(msg.sender, _pid, user.amount);
      user.amount = 0;
      user.rewardDebt = 0;
```

If the `safeTransfer()` function of the LP tokens implemented a callback function to the receiver, the receiver could re-enter the `emergencyWithdraw()` function to drain all the LP tokens within the contract.

Check Effect Interaction Pattern Violated

The linked `transfer()` invocations do not check the function call's return value, which should yield a `true` result in the case of proper ERC-20 implementation.
```solidity=253
        if (_amount > milkyBal) {
            milky.transfer(_to, milkyBal);
        } else {
            milky.transfer(_to, _amount);
        }
```


Unchecked Value of ERC-20 `transfer()` Call

In the `add()` and `set()` functions, the flag `_withUpdate` determines if all the pools will be updated. This reliance might lead to significant loss of the reward.

For illustration, assume we have 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 set to `true`.
- distribute the reward and update the pool.
- add the given pool information

Case 2: `_withUpdate` is set to `false`.
- add the given pool information
 
(Note: While we focused on the `add()` function, both the `add()` and `set()` functions update `totalAllocPoint`, which is used in calculation of pool rewards in the function `updatePool()`


* In Case 1, reward for the first pool is updated in the call to `updatePool()` where `milkyReward = multiplier.mul(milkyPerBlock).mul(pool.allocPoint).div(totalAllocPoint))`.
* In Case 2, an update `totalAllocPoint = totalAllocPoint.add(_allocPoint)` is done first. Then `updatePool()` calculates the reward for the first pool: `milkyReward = multiplier.mul(milkyPerBlock).mul(pool.allocPoint).div(totalAllocPoint))` Because the second pool is sharing rewards with the first one, the amount of reward for the first pool becomes half as much as that in the first case.

Potential Loss of Pool Rewards

A sandwich attack might happen when an attacker observes a transaction swapping tokens without setting restrictions on slippage or minimum output amount. The attacker can manipulate the exchange rate by frontrunning (before the transaction being attacked) a transaction to purchase one of the assets and make profits by backrunning (after the transaction being attacked) a transaction to sell the asset.

The function `_swap()` directly interacts with UniswapV2Pair and doesn't set restrictions on slippage or minimum output amount, so transactions triggering this function are vulnerable to sandwich attacks, especially when the input amount is large:


Potential Sandwich Attacks

In the `MilkyMaker` contract, Wrapped ADA is under the variable `wwada` instead of simply `wada`.

Typo

`MilkyToken` is a fork of `SushiToken` contract.

Regarding the Voting mechanisms, the contracts that were forked have known [vulnerabilities](https://medium.com/valixconsulting/sushiswap-voting-vulnerability-of-sushi-token-and-its-forks-56f220d4c9ba) :
1. Voting Amplification : Because delegated tokens are not locked in the contract, it is possible for a user to delegate his tokens to an address multiple times, by sending the tokens to a new address, and re-delegate from the new address.
2. Voting Displacement : 
When redelegation of tokens occur, the amount of the moved votes is determined by the delegator’s current `MILKY` balance, not the previously delegated amount of `MILKY`, so the attackers can manipulate the incorrect number of the votes movement by getting more `MILKY`.
3. Redelegation failure : 
When redelegation of tokens occur, the amount of the moved votes is determined by the delegator’s current `MILKY` balance, not the previously delegated amount of `MILKY`, so if the delegator has more `MILKY` than the amount of delegated `MILKY`, the operation below will fail (integer underlow):
```solidity
uint256 srcRepNew = srcRepOld.sub(amount);
```

Issues With SushiSwap Voting System

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

Improper Usage of `public` and `external` Type

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to different compiler versions. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

Within the `Ownable` contract, two methods exist to modify ownership:
- **Direct** method: User A sends ownership to User B, User B becomes immediately the new owner.
- **Indirect** method (Pull over Push): User A sends ownership is sent to User B, but User A remains owner. User B calls  `claimOwnership()` to become the new owner.

An issue can arise if the owner initiates the **indirect** method to give ownership to User A, but finally decides to use the **direct** method to send the owner to User B.
Here is the scenario:
1. Owner decides to changes owner to Alice, with the **indirect** method (`direct=false` in `transferOwnership()`).
The following variable is updated : `pendingOwner = ALICE;`. Alice is supposed to call `claimOwnership()` to finalize the process.
2. Owner changes his mind, and decides to change the owner **directly** and to put Bob, Bob is the new owner.
3. Alice can still call `claimOwnership()` and retrieve ownership from Bob.

Overlap of Ownership

MilkySwap

WETH9.sol

interfaces

UQ112x112.sol

UniswapV2Router02.sol

libraries

IUniswapV2Router01.sol

Math.sol

IERC20.sol

UniswapV2ERC20.sol

MasterMilker.sol

UniswapV2Library.sol

IMasterMilker.sol

UniswapV2Pair.sol

IRewarder.sol

IUniswapV2Pair.sol

SafeMath.sol

IWETH.sol

IUniswapV2Router02.sol

MilkyMaker.sol

Ownable.sol

TransferHelper.sol

IUniswapV2ERC20.sol

UniswapV2Factory.sol

MilkyToken.sol

SafeERC20.sol

SignedSafeMath.sol

IUniswapV2Factory.sol

IUniswapV2Callee.sol

Multicall2.sol

From DeFi to scam awareness, this site arms beginners with the tools they need to thrive in crypto - Cointelegraph

DeFi auditor nets $40,000 for identifying Uniswap vulnerability

DeFi Analyst Predicts $2.00 Per Token For Both Uniglo.io And Ripple In 2023 | NewsBTC

DeFi Exchange Mango's $114M Exploit Was 'Market Manipulation,' Not a Hack, Ex-FBI Special Agent Says

Disney hiring transaction lawyer for ‘aggressive’ NFT and DeFi plans

Gibraltar Will Focus on Stablecoins and DeFi Lending After Market Turmoil

Brett Harrison raises $5 million for DeFi platform aimed at institutions: Bloomberg

Hacker Exploits BNB Chain-Based DeFi Protocol Ankr

DeFi-Focused Startup Blue Comes Out of Stealth With $3.2M Raise

Presales Like Uniglo.io (GLO) Are Generating Demand As Ethereum (ETH) Merge Looks To Benefit DeFi And UniSwap (UNI) | NewsBTC

From neglecting security to bad tokenomics, DeFi has played a hand in its own decline

2023 Investment Guide: DeFi Giants Celsius Network (CEL) and Aave (AAVE) | Bitcoinist.com

Nasdaq's Crypto Move, $160M DeFi Hack, Hong Kong CBDC + More News

Coinbase CEO: Regulate centralized actors but leave DeFi alone

Comparing Axie Infinity and Cardano to Big Eyes Coin Next-Gen DeFi Services

DeFi user loses $3.4 million of GMX tokens in phishing attack

Circle, Uniswap Research Says DeFi Can Solve $2 Trillion FX Risk Problem

Aptos DeFi project under fire, asks community for confidence vote

French Central Bank CBDC Projects Aim to Manage DeFi Liquidity, Settle Tokenized Assets

Hackers drain $8M in assets from Bitkeep wallets in latest DeFi exploit

Composable Finance announces the official launch of Picasso, the infrastructure layer enabling interoperable DeFi | NewsBTC

MilkySwap

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

14

0

2

1

11