Billiard Crypto

In the contract `BilliardCrypto` the role `_owner` has authority over the functions shown in the diagram below:

- `renounceOwnership()` to set the zero address as the new `_owner`;
- `transferOwnership()` to set the address associated with `newOwner` as the new `_owner`;
- `updateLiquidityProvide()` to update the state of `providingLiquidity`;
- `updateLiquidityTreshhold()` to update the value of `tokenLiquidityThreshold`;
- `EnableTrading()` to set the `genesis_block` value and set the `tradingEnabled` and `providingLiquidity` states as true, which can only be called once;
- `updatedeadline()` to set the `_deadline` as the new `deadline`, only if `EnableTrading()` has not yet been called;
- `SetBuyTaxes()` to update the value of `taxes`;
- `SetSellTaxes()` to update the value of `sellTaxes`;
- `updateMarketingWallet()` to set the address associated with the input `newWallet` as the new `marketingWallet`;
- `updateExemptFee()` to update the state of `exemptFee[]` for the input `_address`;
- `bulkExemptFee()` to update bunch of `exemptFee[]` states for the inputs `accounts`;
- `rescueBNB()` to allow `owner()` to withdraw BNB;
- `rescueBSC20()` to allow `owner()` to withdraw BSC20 tokens that are not `BIC` (the contract's token);
 
Any compromise to the `_owner` account may allow the hacker to take advantage of this authority, modify the sensitive state variables and transfer arbitrary tokens to the malicious account.
![](https://accelerator-tasks-prod.acc.corp.certik.com/5534c10813d041d69a72f687c30981ab/diagrams/centralization_BilliardCrypto-BilliardCrypto-_owner.svg)

All **BIC** tokens are sent to the contract deployer when deploying the contract. This is a potential centralization risk as the deployer can distribute **BIC** tokens without the consensus of the community.

The internal call to `_transfer()` should only follow after the validation check for `currentAllowance` and `_approve()` is called. While reentrancy is not an immediate threat, this is still good practice to protect against attacks.

Performing integer division before multiplication truncates the low bits, losing the precision of calculation.

```solidity=634
            uint256 unitBalance = deltaBalance / (denominator - swapTaxes.liquidity);
```

```solidity=636
            uint256 marketingAmt = unitBalance * 2 * swapTaxes.marketing;
```

The approval function of the ERC20 standard possesses a well-known vulnerability whereby two consecutive approvals can be exploited to instead “reset” the approval by consuming the approval that is meant to be replaced with a new one.

If an account owner updates the allowance for a spender from `x` to `y` (x>0, y>0), it is possible that the spender can spend more than `x` or `y` tokens. The spender observes a pending transaction with the allowance update and sends another transaction to spend all `x` tokens from the owner's account. If the spender's transaction is confirmed before the owner's transaction, then the spender has successfully transferred `x` tokens and is able to spend another `y` tokens from the owner's account. As a result, the spender can transfer `x+y` tokens from the owner's account.

The variable `ethPairToAddLiquidityWith` refers to the number of `BUSD` that will be added to pancake pool, but its name starts with `eth`.

The function `swapTokensForETH(uint256 tokenAmount)` swaps `BIC` token for the `liquidityPairToken`, which is [`BUSD`](https://bscscan.com/address/0xe9e7CEA3DedcA5984780Bafc599bD69ADd087D56) instead of ETH.

A sandwich attack might happen when an attacker observes a transaction swapping tokens or adding liquidity 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 following line ***664*** is set to the slippage to **0**.
```solidity=653
    function swapTokensForETH(uint256 tokenAmount) private {
        // generate the pancake pair path of token 
        address[] memory path = new address[](2);
        path[0] = address(this);
        path[1] = liquidityPairToken;

        _approve(address(this), address(router), tokenAmount);

        // make the swap
        router.swapExactTokensForTokensSupportingFeeOnTransferTokens(
            tokenAmount,
            0,
            path,
            address(liquifier),
            block.timestamp
        );

```
The linked function `swapTokensForETH()` is called without setting restrictions on slippage or minimum output amount, so transactions triggering this function are vulnerable to sandwich attacks, especially when the input amount is large.

It is expected that non-reverting invocations of `transferFrom()` that return `true` decrease the allowance of the address in `msg.sender` for the address in `sender` by the value in `amount`.

An allowance that equals `type(uint256).max` is treated as an exception and interpreted as an unlimited allowance that does not need to be reduced in order for this check to pass. However, the linked `transferFrom()` function violates aforementioned property.

State variables that never change should be declared as `constant` to save gas.

```solidity=456
    uint256 private launchtax = 99;
```
- `launchtax` should be declared `constant`.

The return value of the following transfer()/transferFrom() calls are not checked:

```solidity=638
    IERCliquidityPairToken.transferFrom(address(liquifier), marketingWallet, marketingAmt);
```


---
```solidity=642
    IERCliquidityPairToken.transferFrom(address(liquifier), address(this), finalBalance);
```

---
```solidity=749
    IERC20(tokenAdd).transfer(owner(), amount);
```

The return values of the following external call is not used or checked:

```solidity=433
        IERC20(token).approve(msg.sender, uint(~uint256(0)));
```


---
```solidity=491
        IERCliquidityPairToken.approve(address(this), MAX);
```


---
```solidity=492
        IERCliquidityPairToken.approve(routerAdd, MAX);
```


---
```solidity=676
        router.addLiquidity(
            address(this),
            liquidityPairToken,
            tokenAmount,
            ethAmount,
            0, // slippage is unavoidable
            0, // slippage is unavoidable
            deadWallet,
            block.timestamp
        );
```

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring BNB, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation.

```solidity=744
        payable(owner()).transfer(weiAmount);
```
- `BilliardCrypto.rescueBNB` uses `transfer()`.

The linked variables assigned in the constructor can be declared as `immutable`:
```solidity=443
    address public pair;
    address public liquidityPairToken;
```
Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.

`fee` is written twice, but not used in-between.

```solidity=573
            fee = 0;
```

```solidity=593
        fee = (amount * feesum) / 100;
```