Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

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.

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