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

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 differing compiler version numbers. 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

The contract is serving as the underlying entity to interact with third party protocols. The scope of the audit treats 3rd party entities as black boxes and assume their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, migrating to new LP pools, etc.

Third Party Dependencies

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

In contract `AllocationStaking`, codes at Line 99 and 193 miss error messages.

In contract `Admin`, code at Line 49 miss an error message.

In contract `SalesFactory`, code at Line 36 miss an error message.

In contract `BrewerySale`, codes at Line 124, 141, 234, 244, 245, 502 miss error messages.

Missing Error Messages

In the contract `Admin`, the role `admin` has authority over the following functions:
- addAdmin
- removeAdmin

In the contract `SalesFactory`, the role `admin` has authority over the following functions:
- setAllocationStaking
- deploySale

In the contract `AllocationStaking`, the role `owner` has authority over the following functions:
- setSalesFactory
- add
- set

In the contract `BrewerySale`, the role `saleOwner` has authority over the following functions:
- depositTokens
- withdrawEarningsAndLeftover
- withdrawEarnings
- withdrawLeftover

the role `admin` has authority over the following functions:
- setVestingParams
- shiftVestingUnlockingTimes
- setSaleParams
- setSaleToken
- setRegistrationTime
- setSaleStart
- updateTokenPriceInETH
- postponeSale
- extendRegistrationPeriod
- setCap

Any compromise to these accounts may allow a hacker to take advantage of this authority.

Centralization Related Risks

The given input `_admins` is missing the check for the non-zero address, and the array length should more than 0. If no admin being set in construct function, the contract `Admin` will be useless.
```solidity
constructor (address [] memory _admins) public {
    for(uint i = 0; i < _admins.length; i++) {
        admins.push(_admins[i]);
        isAdmin[_admins[i]] = true;
    }
}
```

Missing Input Validation for `_admins`

In contract `AllocationStaking`, the given input `_pid` in the following functions needs to be checked if the index is valid.
- set()
- deposited()
- pending()
- setTokensUnlockTime()
- updatePool()
- deposit()
- withdraw()
- compound()
- emergencyWithdraw()
- getPendingAndDepositedForUsers()

Missing Validation for `_pid`

In contract `AllocationStaking`, the following functions miss the input validation. Address should be checked before assignment to make sure it is not zero addresses.
- initialize()
- deposited()
- pending()

Missing Zero Address Validation

In contract `AllocationStaking`, when adding a new LP pool, the function `add` just add the new pool without checking whether the new LP exists. Only comment in the code to warn the owner is surely not enough.
```solidity
// Add a new lp to the pool. Can only be called by the owner.
// DO NOT add the same LP token more than once. Rewards will be messed up if you do.
function add(uint256 _allocPoint, IERC20 _lpToken, bool _withUpdate) public onlyOwner {
...
```

Missing Validation for Same Pool

The related condition is not correct, since `user.amount` is always  `>= 0`.
```solidity=291
require(user.amount >= 0, "User does not have anything staked.");
```

Incorrect `require` Condition

The design of compound is wrong. The staking tokens of different pools are different LP tokens while the reward token is an ERC20 token. The compound mechanism only can be used when the staking token and the reward token are the same. 

If using the compound mechanism falsely, all the rewards and staking tokens amount will be messed up.  Those who call the function `compound` can withdraw more LP tokens than their staking LP amount. The total LP supply of the pool is not increased, but their recorded LP amount is increased. When they withdraw more LP tokens, other users may have no LP token to withdraw.  

Wrong Design of Compound

The function `erc20Transfer()` does not use the `safeTransfer()` from `SafeERC20`. If the function `transfer()` fails to transfer without reverting and just returns false, the function still goes on which will cause mistakes.
```solidity
function erc20Transfer(address _to, uint256 _amount) internal {
    erc20.transfer(_to, _amount);
    paidOut += _amount;
}
```

`safeTransfer` Not Used

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)

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. 
If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. 

Since the `pool.lpToken` is unknown, the functions `deposit`, `withdraw` and `emergencyWithdraw` need a `ReentrancyGuard` to prevent potential reentrancy attack.

Potential Reentrancy Attack

SafeMath from OpenZeppelin is not used in the following functions which makes them possible for overflow/underflow and will lead to an inaccurate calculation result.

- `fund()`
- `erc20Transfer()`

SafeMath Not Used

It is unusual that the emergency withdrawals are also subject to time limit. It is better to remove this time limit `user.tokensUnlockTime <= block.timestamp`.
```solidity=308
// Withdraw without caring about rewards. EMERGENCY ONLY.
function emergencyWithdraw(uint256 _pid) public {
    PoolInfo storage pool = poolInfo[_pid];
    UserInfo storage user = userInfo[_pid][msg.sender];
    require(user.tokensUnlockTime <= block.timestamp,
        "Emergency withdraw blocked during sale and cooldown period.");
...
```

Emergency Withdrawals Subject to Time Limit

The contract `BrewerySale` has a base contract `ReentrancyGuard`, but never uses the modifier `nonReentrant` from `ReentrancyGuard`.

Unused Base Contract `ReentrancyGuard`

The related condition is not correct, since `sale.saleStart` has already added `timeToShift`.
```solidity=323
function postponeSale(uint256 timeToShift) external onlyAdmin {
    require(
        block.timestamp < sale.saleStart,
        "sale already started."
    );
    //  postpone registration start time
    sale.saleStart = sale.saleStart.add(timeToShift);
    require(
        sale.saleStart + timeToShift < sale.saleEnd,
        "Start time can not be greater than end time."
    );
}
```

The owner can update token price to any price at any time. This may raise the community's concerns about the centralization issue.

Update Token Price Without Limit

If the sale owner does not deposit tokens to the contract `BrewerySale`, and the buyers still call the function `participate` to buy the token, they can not withdraw any token finally. So it is important to ensure that the token has already been deposited in function `participate`.

Participating Missing `tokensDeposited` Check

The function `participate` does not check if it is sold out or not when updating `sale.totalTokensSold`. The user may send the ETH and get no token.

No Sold-out Check

The function `withdrawMultiplePortions` misses check for the `sale.tokensUnlockTime` like the function `withdrawTokens`.

Missing Check `sale.tokensUnlockTime`

The input `amount` in function `participate` is signed by admin. Can you please tell us how you decide the participating amount of one user?

Controversy about Participating Amount

The amount of `totalSupply_` tokens that are minted to the contract deployer,  may raise the community's concerns about the centralization issue.

Token Minted To Centralized Address

The contract `BreToken` only has function `decreaseAllowance` but does not have function `increaseAllowance`. Usually these two functions come in pairs.

Missing Function `increaseAllowance`

The variables `saleOwnerToSale` and `tokenToSale` are never used.

The event `SaleOwnerAndTokenSetInFactory` is never used.

Useless Variables and Event

In contract `SalesFactory`, the constructor misses the input validation. Address should be checked before assignment to make sure it is not zero addresses.

Function `getAllSales` only checks the `endIndex > startIndex`, without checking `endIndex <= allSales.length`.

Boba Brewery

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

27

0

7

0

20