# BBB:Unsecure.sol audit
## Summary: [H-01] No one can be an OWNER.
### Vulnerability Detail
`address owner = msg.sender` defines msg.sender to be owner, but no one can be owner because `constructor` is not used
### Impact
Cannot set functions that only the owner can execute.

### Code Snippet

 13 address owner = msg.sender;

### Tool used
Manual Review

### Recommendation
Set the owner with `constructor`

constructor {
    address owner = msg.sender;
}

If the owner is not set, it is very dangerous because the function of the owner will be executed by someone who is not the owner.
So the danger level is set to 'high'.

## Summary: [H-02] Cannot execute anyone `function addApprovedTokens` because it is private

### Vulnerability Detail
The function is set as public that can be executed by the owner, but no one can execute it because it is private.

### Impact
No one can set the white list token if this function is private.

### Code Snippet

41 function addApprovedTokens(address _token) private {
42    if (msg.sender != owner) revert();
43    approvedTokens.push(_token);
44  }

### Tool used
Manual Review

### Recommendation
Change `private` to `public`

## Summary: [H-03] User will not receive rewards for previous deposits.

### Vulnerability Detail
If a user repeats a deposit more than twice in a row before executing a withdrawal, the deposit will be overwritten and the user will only receive the last deposit of the Reward.
Also, the previous amount of reward will not be received.

### Impact
The user wouldn't get the Reward that the user suppose to obtain.

### Code Snippet

function deposit(uint _amount, address _token, bool _isETH) public {
    if (!_isXXX(_token, whitelist)) revert();
    DepostInfo memory depositInfo;
    TransferInfo memory info = TransferInfo({
        isETH: _isETH,
        token: _token;
        from: msg.sender, 
        amount: _amount,
        to: address(this),
    });

    _tokenTransfer(info);
    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;
  }

If this function deposit is executed twice in a row, the deposit is overwritten.
Also, the function withdraw get reward executed is calculated by referring to the overwritten depositAmt, so only the one based on the last deposit record can get the Reward.

function getReward(address token) public view returns (uint reward) {
    uint amount = depositAmt[msg.sender][token].amount;
    uint lastTime = depositAmt[msg.sender][token].lastTime;
   reward = (REWARD_RATE / (block.timestamp - lastTime)) * amount;
  }

### Tool used
Manual Review

### Recommendation
Change `depositAmt` to be able to add to `amount`.

95 depositInfo.amount = depositAmt[msg.sender][_token].amount + _amount;

## Summary: [H-04] User can withdraw more than the deposited amount. And users can earn Reward as many times as they want.

### Vulnerability Detail
Amount:
Since the value of `depositAmt[msg.sender][token].amout` is not updated in the withdraw function, the User can continue to withdraw as long as the `token` specified in this contract remains.

Reward:
If the User enters 0 for the `amount` argument and withdraws it, the `Reward` can be obtained many times without withdrawing the `amount`, as long as the `tokenBBB` remains.
This is because the value of `depositAmt`, which is also used in `getReward`, is not updated.

### Impact
e.g.
Amount:
1. This contract has 100USDC. Eve has deposited 10USDC.
2. Eve executes `function withdraw(_amout(10),_token(USDC))`.
3. Eve withdraws 10 USDC and also earns a calculated Reward.
4. However, since `depositAmt[msg.sender][_token].amount` is not updated, Eve can withdraw again and again until the USDC owned by this contract is reached.

Reward:
1. Eve enter 0 for amaunt,token enter something.
2. The `amount` is not withdrawn, but the Reward is calculated and the BBBtoken can be earned.

### Code Snippet

function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
  ) public {
    if (!_isXXX(_token, whitelist)) revert();
    TransferInfo memory info = TransferInfo({
        isETH: _isETH,
        token: _token;
        from: address(this), 
        amount: _amount,
        to: _to,
    });
    uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
    require(_info.amount < canWithdrawAmount, "ERROR");
    canWithdrawAmount = 0;
    _tokenTransfer(_info);
    uint rewardAmount = getReward(_token);
    IERC20(BBBToken).transfer(msg.sender, rewardAmount);
  }

### Tool used
Manual Review

### Recommendation
Added confirmation that `amount` is non-zero
Instead of updating `canWithdrawAmount`, update `depositAmt[msg.sender][_token].amount`.

    uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
    require(_info.amount, "ERROR"); // throw error when '_amount' is zero
    require(_info.amount < canWithdrawAmount, "ERROR");
    uint rewardAmount = getReward(_token);
    depositAmt[msg.sender][_token].amount -= _info.amount; // update deposited amount
    _tokenTransfer(_info);
    IERC20(BBBToken).transfer(msg.sender, rewardAmount);

Note:Change the order of line `113` to `119` to follow the C-E-I pattern.

## Summary: [M-01] No initial value set for argument `i`

### Vulnerability Detail
`function _isXXX` checks if the address array `_xxx` contains the address `_token`.
However, this function fails because the initial value is not set.

### Impact

### Code Snippet

     for (uint i; i < length; ) {
       if (_token == _xxx[i]) return true;
       unchecked {
         ++i;
       }

### Tool used
Manual Review

### Recommendation
Set initial values for `i`

     for (uint i = 0; i < length; ) {
       if (_token == _xxx[i]) return true;
       unchecked {
        ++i;
       }

## Summary: [M-02] User can deposit with reentrancy and such deposit is locked in this contract forever.

### Vulnerability Detail
As same as the issue H-04, the order of token deposit does not follow the C-F-I pattern. Because of that, user can reentrance and deposit tokens, and deposited tokens via reentrancing is not reflected to the usre's deposit info, so they are not able to withdraw such deposit.

### Impact
User's deposit via reentrance gets locked to this contract.

### Tool used
Manual Review

### Recommendation
Fix the order of line `93` to `96` to follow the C-E-I pattern.
#### Before

    _tokenTransfer(info);
    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;

#### After

    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;
    _tokenTransfer(info);

## Summary: [M-03] Possibly overflowing due to time calculations.
### Vulnerability Detail
If you calculate "time" using value uint40, there is a potential for overflow.

For example, time * 2 will be stored into uint40, while the result might be larger than 40 bits. For this purpose only, it is already better to just keep the stuff in uint256s. As to gas cost, keeping all your data aligned to 256 bits, will actually yield lower gas-cost.

### Impact
Being at risk of getting an overflow in one of your calculations, as a result of the compiler choosing the shortest type which is sufficient for the operation.

### Code Snippet

// line of 94 
    depositInfo.lastTime = uint40(block.timestamp);

// line of 56 
    reward = (REWARD_RATE / (block.timestamp - lastTime)) * amount;

### Tool used
none

### Recommendation
don't declare uint40 for "time" but use uint256

#### Before

// line of 94 
    depositInfo.lastTime = uint40

#### After

// line of 94 
    depositInfo.lastTime = uint256(block.timestamp);

## Summary: [M-04] Validation Errors
### Vulnerability Detail
The validation in the `withdraw` function is to check if the argument `token` is in the whitelist.
The `withdraw` function should check if `msg.sender` has ever `deposited` with this `token`.

### Impact
Even if the function is on the white list, it is not checked if the `msg.sender` that executes the function is a `token` that has been `deposit`.

### Code Snippet

if (!_isXXX(_token, whitelist)) revert();

### Tool used
Manual Review

### Recommendation
Add this validation.

if (!_isXXX(_token, whitelist)) revert();
if (!depositAmt[msg.sender][_token]) revert();

## Summary: [M-05] Reward calculation is incorrect.
### Vulnerability Detail
The calculation is that the longer a user is on deposit, the less the reward.
Could this be a mistake in the formula?

### Impact
e.g.
A:
REWARD_RATE = 50
block.timestamp = 2022
lastTime = 2021
amount = 100
__reward = 500__

B:
REWARD_RATE = 50
block.timestamp = 2023
lastTime = 2021
amount = 100
__reward = 250__

B has been deposited for a longer period of time, but A, who has been deposited for a shorter period of time, is more remunerated.
This eliminates the need to deposit for a longer period of time.

### Code Snippet

 reward = (REWARD_RATE / (block.timestamp - lastTime)) * amount;

### Tool used
Manual Review

### Recommendation

reward = (block.timestarmp- lastTime) * amount * (REWARD_RATE / 100) 
// Divide by 100 because REWARD_RATE is the reward rate.

A:
REWARD_RATE = 50
block.timestamp = 2022
lastTime = 2021
amount = 100
__reward = 50__

B:
REWARD_RATE = 50
block.timestamp = 2023
lastTime = 2021
amount = 100
__reward = 100__

Now B, who has a longer deposit period, will receive a larger reward.
Isn't this what this contract wanted to do?

## Summary:[M-06] Mistake of type

### Vulnerability Detail
The `isETH` in `struct TransferInfo` is `uint`.

### Impact
Should be a true/false value whether or not it is ETH

### Code Snippet

 struct TransferInfo {
    uint isETH;         /// 32 bytes
    uint amount;        /// 32 bytes
    address token;      /// 20 bytes
    address from;       /// 20 bytes
    address to;         /// 20 bytes
  }

### Tool used
Manual Review

### Recommendation

 struct TransferInfo {
    bool isETH;         /// 32 bytes
    uint amount;        /// 32 bytes
    address token;      /// 20 bytes
    address from;       /// 20 bytes
    address to;         /// 20 bytes
  }

## Summary:[M-07] Requires the payable modifier

### Vulnerability Detail
If `isETH` is `true`, `payable` modifier is needed because `value` is transferred by `call`.

### Impact
Cannot remit in case of ETH.

### Code Snippet

function _tokenTransfer(TransferInfo memory _info) private {
    if (_info.isETH) {
      (bool success, ) = _info.to.call{ value: _info.amount }("");
      require(success, "Failed");
    } else {
      IERC20(_info.token).transferFrom(_info.from, _info.to, _info.amount);
    }
  }

### Tool used
Manual Review

### Recommendation

function _tokenTransfer(TransferInfo memory _info) private payable {
    if (_info.isETH) {
      (bool success, ) = _info.to.call{ value: _info.amount }("");
      require(success, "Failed");
    } else {
      IERC20(_info.token).transferFrom(_info.from, _info.to, _info.amount);
    }
  }