# Force Contracts Audit

*Scope*: [`Contact-audit`](https://github.com/FutureMarkt/Contact-Audit) repository at [`37a9527788b46b2e071d8f2702c0c9ed994ecf67`](https://github.com/FutureMarkt/Contact-Audit/tree/37a9527788b46b2e071d8f2702c0c9ed994ecf67) commit.

*Scope of second audit*: [`Contact-audit`](https://github.com/FutureMarkt/Contact-Audit) repository at [`134f0b71b16c6d8d8d1d1532e4b26a8deaac54cf`](https://github.com/FutureMarkt/Contact-Audit/tree/134f0b71b16c6d8d8d1d1532e4b26a8deaac54cf) commit.

## Issues

### Floating pragma

Consider to lock a Solidity compiler version over whole codebase to ensure reproducible build artifacts and mainnet deployment.

`./contracts/programs/Force.sol`

pragma solidity 0.8.14;

See [SWC-103](https://swcregistry.io/docs/SWC-103) for more information.

:::warning
Partially fixed, except `./contracts/programs/Statements.sol`, `./contracts/tokens/ForceToken.sol`, `./contracts/tokens/SFB.sol`, `./contracts/tokens/SFC.sol` files.
:::

### Debug libraries usage

Hardhat Console library is intended for debug purposes only and therefore all `hardhat/console.sol` imports and `console.log` calls should be completely stripped before mainnet deployment. There's a [couple](https://github.com/ItsNickBarry/hardhat-log-remover) [handy Hardhat libraries](https://github.com/wighawag/hardhat-preprocessor) for it.

:::warning
All debug function calls removed, except `hardhat/console.sol` import in `./contracts/programs/Statements.sol` file.
:::

### Inefficient usage of storage layout

Current structure layout take 4 256-bit storage slots:

`./contracts/programs/s6.sol`

struct structS6 {
    uint slot;
    uint lastChild1;
    uint lastChild2;
    uint frozenMoneyS6;
}

Each read/write of this structure will take 4 `SLOAD/SSTORE` EVM instructions (and 4 times more gas). Consider to pack structures to one storage slot e.g.

`./contracts/programs/s6.sol`

struct structS6 {
    uint16 slot;
    uint16 lastChild1;
    uint16 lastChild2;
    uint208 frozenMoneyS6;
}

That storage layout allow to cover up to ~16000 slot re-activation cycles using 4 times less gas per read/write, which may be more than enough for your business logic.

:::warning
Partially implemented. Some kind of structure packing is implemented (see explanation below).
:::

`./contracts/programs/Statements.sol`

struct StructS3 {
    uint128 slot;
    uint128 lastChild;
    uint256 frozenMoneyS3;
}

struct StructS6 {
    uint128 slot;
    uint64 lastChild1;
    uint64 lastChild2;
    uint256 frozenMoneyS6;
}

struct PresenceOf {
    uint128 lvl;
    uint128 slot;
    address ownerOfStruct;
    bool row;
}

struct LongTransferStorage {
    address to;
    uint256 amount;
}

Let's see `PresenceOf` structure storage layout for example. Currently it takes 128 + 128 + 20 + 8 = 284 bits. As EVM operates with 256-bit slots, it will take 2 read/write operations per structure. `lvl` field can be easily decreased to `uint8` (as there are only 12 levels), having 164 bits in result. Rest 92 bits can be used for additional fields.
Choose structure field sizes according to your business logic, domain model and common sense. I think that there are no person in existence able to perform 2^128^ S3/S6 cycles, and earn 2^256^ money, so each of these structures can be packed to single 256 bit slots.

### `uint256` type alias

Codebase is using `uint` to declare 256-bit integer unsigned number. Even though `uint` is just an alias for `uint256` and both represent the same type, consider to use only explicit `uint256` keyword. Future compiler versions can break backward compatibility and change underlying type to e.g. `uint128`. Prefer explicity over implicity.

:::warning
Partially fixed. `uint` aliases are still used in some places across the project.
:::

### Unnecessary usage of state variable

`./contracts/programs/programs.sol`

uint public firstPrice = 5 * 10 ** 18;

That state variable has constant value and used only once in according constructor. Consider marking it as `immutable` and pass to constructor arguments, or just convert item to `constant`.

:::warning
`firstPrice` variable still used only in contract constructor, but useless assignment was added on line 61.
:::

### Excessive usage of boolean comparison operator

`./contracts/programs/S3.sol`

require(activate[msg.sender][i] == true, "Previous level not activated");

`./contracts/programs/s6.sol`

require(activate[msg.sender][i] == true, "Previous level not activated");

`require` function expects boolean as first argument, so you just can pass `activate[msg.sender][i]` and `activateBoost[msg.sender][i]` respective variables without comparing them with `true`, e.g.

`./contracts/programs/S3.sol`

require(activate[msg.sender][i], "Previous level not activated");

:::warning
Issue fixed on indicated lines, but appears in new places (`./contracts/programs/Force.sol` on line 31, `./contracts/programs/S6.sol` on line 38 and `./contracts/programs/programs.sol` on line 37).
:::

### Unchecked ERC-20 transfers

`./contracts/programs/programs.sol`

tokenMFS.transferFrom(msg.sender, _parent, (_price - amoutSC)); // transfer token to me
tokenMFS.transferFrom(msg.sender, address(this), amoutSC); // transfer token to smart contract

Due to [loose definition](https://github.com/d-xo/weird-erc20#missing-return-values) of [ERC-20 standard](https://eips.ethereum.org/EIPS/eip-20) several tokens do not revert in case of `transfer` failure and return `false`. If `tokenMFS` works this way, one or none of these transfers can be successfully executed, that violates transaction atomicity principle. Always check `transfer` and `transferFrom` return values, or use OpenZeppelin's [`SafeERC20`](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol) handy helper for that purpose, e.g.

`./contracts/programs/programs.sol`

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

...

use SafeERC20 for ERC20;

...

tokenMFS.safeTransferFrom(msg.sender, _parent, (_price - amoutSC)); // transfer token to me
tokenMFS.safeTransferFrom(msg.sender, address(this), amoutSC); // transfer token to smart contract

:::success
Fixed.
:::

### Write after write

`./contracts/programs/s6.sol`

uint _spot;
if (_lastChild == 0) {
  _spot = 0;
}
_spot = _parentStruct.lastChild2 % 4; // THINK BECAUSE DUBLICATE ON SECOND LEVEL

`_spot` variable written on line 172, never read and written again right after on line 174. `_spot` is 0 by default, what makes condition on line 171 useless.

:::success
Fixed.
:::

### Incorrect file names casing

`./contracts/programs/Force.sol`

import "./S6.sol";

`./contracts/programs/S3.sol`

import "./Programs.sol";

You trying to import `S6.sol` and `Programs.sol` files but there are only `s6.sol` and `programs.sol` files exists. That may break build on case-sensitive OSes and file systems such as Linux. Rename files or change respective imports, or better use consistent file naming convention.

:::success
Fixed.
:::

### Re-activation don't enabled by default

`./contracts/programs/Referal.sol`

struct User {
    bool autoReCycle;
    bool autoUpgrade;
}

Boolean variables has `false` default value, so the new registered user will not have re-activation enabled by default.

:::success
Fixed.
:::

### Broken re-activation logic

`./contracts/programs/S3.sol`

// Last Child
if (_lastChild == 2) {
  // Check autorecycle
  if (users[_parent].autoReCycle) {
    _sendDevisionMoney(_parent, _price, 40);
  } else {
    tokenMFS.transferFrom(msg.sender, _parent, _price); // transfer token to parent
    if (_parent != owner()){
      updateS3(msg.sender, lvl); // update parents product
    }
  }
  _parentStruct.slot++;
}

- if upline user has auto re-activation *enabled*, he will got only 60 % of slot price (line 78) on new slot cycle;
- and vice versa: if upline user has auto re-activation *disabled*, he will got full slot price (line 80) on new slot cycle.

It looks like condition on line 77 should be inversed, but that doesn't explain the above 60/40 distribution. Project specification said that upline user got:

- full slot price if he has auto re-activation *enabled*,
- and only 75 % of slot price if he has auto re-activation *disabled*, rest of funds go to "product fund"

on new slot cycle.

:::success
Fixed.
:::

### Broken auto-upgrade logic

Similar to the previous issue:

`./contracts/programs/S3.sol`

// First Child
if (_lastChild == 0) {
  //Check autoUpgrade
  if (users[_parent].autoUpgrade) {
    _sendDevisionMoney(_parent, _price, 25);
  } else {
    _parentStruct.frozenMoneyS3 += _price;
    tokenMFS.transferFrom(msg.sender, address(this), _price);
  }
}

// Second Child
if (_lastChild == 1) {
  //Check autoUpgrade
  if (users[_parent].autoUpgrade) {
    _sendDevisionMoney(_parent, _price, 25);
  } else {
    _parentStruct.frozenMoneyS3 -= _price;

    // THINK
    tokenMFS.transfer(_parent, _price); // transfer frozen money
    tokenMFS.transferFrom(msg.sender, _parent, _price); // transfer money to parent
    // buy((lvl + 1)); // buy next lvl buy[lvl + 1]
  }
}

- if upline user has auto-upgrade *enabled*, he will got only 25 % of slot price (lines 52, 63) for each of new auto-upgrade spots;
- and vice versa: if upline user has auto re-activation *disabled*, he will got full slot price frozen on first auto-upgrade spot (line 54), and will got full slot price + frozen money if all of his auto-upgrade spots will be filled.

Also, there is no purchase of new slot on auto-upgrade, according line 70 is commented.

:::success
Fixed.
:::

### S3: Infinite loop/stack overflow

`./contracts/programs/S3.sol`

if (_parent != owner()){
  updateS3(msg.sender, lvl); // update parents product
}

Function `updateS3` calls itself with the same arguments. If `_parent` will never be equal `owner()`, it can cause stack overflow as that function will never finish. `updateS3(_parent, lvl)` maybe?

:::success
Fixed.
:::

### Get frozen money back if purchasing level without auto-upgrade

As project specification said, if only one of auto-upgrade spots is filled and user purchase next slot, he should got frozen money back.

:::success
Fixed.
:::

### Fixed slot prices

Slot prices table hard-coded in contract constructor, no price oracle used and there is no function to update table according to the price schedule from project specification.

`./contracts/programs/programs.sol`

/// Set products prices
for (uint j = 0; j < 12; j++) {
    prices.push(firstPrice * 2 ** j);
}

:::success
Fixed.
:::

### Dead code

Function `_getPositionLvl2` in `./contracts/programs/S6.sol` is never used and can be removed.

### Missing inheritance

It looks like `SFB` and `SFC` token contracts should be inherited respectively from `ISFB` and `ISFC` interfaces.

### Public function can be marked external

`buy()` function in `./contracts/programs/S6.sol` file can be marked as external, as it never called from contract itself.

### Unused state variable

State variable `innerFoundation` in `./contracts/programs/Statements.sol` file is never used, so it can be removed.

### Optimization: use bitmaps instead of array of booleans

This structure

`./contracts/programs/Referal.sol`

struct User {
    bool[12] autoReCycle;
    bool[12] autoUpgrade;
}

can be implemented as

struct User {
    uint16 autoRecycle;
    uint16 autoUpgrade;
    // Or in one bit map
    // uint24 autoRecycleAndUpgrade;
}

This allows you e.g. optimize registration function

`./contracts/programs/Referal.sol`

// Instead of 12 `SSTORE`s
users[msg.sender].autoReCycle = type(uint16).max;
users[msg.sender].autoUpgrade = type(uint16).max;

See [this](https://hiddentao.com/archives/2018/12/10/using-bitmaps-for-efficient-solidity-smart-contracts) and [this](https://soliditydeveloper.com/bitmaps) for detailed explanation.

### Use Yul IR compiler pipeline

Since the Solidity version used in project is higher than _0.8.13_, consider to enable Yul IR compiler pipeline. This will reduce contract sizes and gas usage from 5 to 7 %.

`./hardhat.config.js`

solidity: {
  version: "0.8.15",
  settings: {
    viaIR: true, // add this line
    optimizer: {
      enabled: true,
      runs: 200,
    },
  },
},