# C-Team-BBB-Bug-Report

## [C-Team H-01] リエントランシー攻撃によってコントラクト内のETHが盗まれる脆弱性

### 要約
攻撃者が`withDraw()`を実行する時にリエントランシー攻撃によってコントラクトにプールされている資金を全て盗むことができる。

### バグの詳細
1. 攻撃者が`_isETH = true` として `withDraw()`を実行する
2. `_tokenTransfer()`が実行され、ETHが攻撃者のコントラクトアドレスへ送金される
3. このコントラクトアドレス内の`fallback`で`withDraw()`を再実行(リエントランシー)する
4. BBBコントラクト内のETHがなくなるまで`withDraw()`が実行され続ける

### インパクト
- コントラクト内のほぼ全てのETHが盗まれる可能性がある。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L113-L116

uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
canWithdrawAmount = 0;
_tokenTransfer(_info);

### 使用したツール
特になし pen & paper

### 改善方法
1. @openzeppelin/contracts/security/ReentrancyGuard.sol(https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol)をimport、継承する
2. `withdraw()`にnonReentrantを付ける
3. CEIパターンに沿うようにコードの順番を変える

// Before
function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
  ) public {
  ...
  canWithdrawAmount = 0;
  _tokenTransfer(info);
  uint256 rewardAmount = getReward(_token);
  IERC20(BBBToken).transfer(msg.sender, rewardAmount);
}

// After
function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
  ) public nonReentrant {
  ...
  uint256 rewardAmount = getReward(_token);
  canWithdrawAmount = 0;
  IERC20(BBBToken).transfer(msg.sender, rewardAmount);
  _tokenTransfer(info);
}

*補足
ユーザーの資金が盗まれることはなさそうだが `deposit()` もリエントランシー攻撃が可能なため、上記と同様に修正するべきである

// Before
function deposit(
        uint256 _amount,
        address _token,
        bool _isETH
    ) public {
    ...
    _tokenTransfer(info);
    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;
}

// After
function deposit(
        uint256 _amount,
        address _token,
        bool _isETH
    ) public nonReentrant {
    ...
    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;
    _tokenTransfer(info);
}

## [C-Team H-02] コントラクト内の資金とBBBトークンを不正に取得可能な脆弱性

### 要約
`withDraw()`実行時、`depositAmt[msg.sender][_token].amount`を更新していない。そのため、入金額に関わらず何度でも`withDraw()`を実行して資産およびBBBを取得できてしまう。

### バグの詳細
1. Aliceが`withDraw()`を実行する
2. Aliceの残高が変更されずに`_tokenTransfer()`が実行され、資金がAliceへ送金される
3. BBBトークンがAliceに送られる
4. Aliceは同様の操作を残高を変更せずに何度も実行でき、ほぼ全ての資金とBBBトークンが不正に引き出されてしまう。

### インパクト
- コントラクト内のほぼ全ての資金が盗まれる可能性がある。
- コントラクト内のほぼ全てのBBBトークンが盗まれる可能性がある。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L113-L116

uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
canWithdrawAmount = 0;
_tokenTransfer(_info);

### 使用したツール
特になし pen & paper

### 改善方法
1. `withDraw()`関数の最初に、引数の`_amount`が0より大きいかチェックし、0ならrevertする
2. `depositAmt[msg.sender][_token].amount`から`_amount`を引いてから、`_tokenTransfer()`する
3. CEIパターンに沿うようにコードの順番を変える

function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
  ) public { // ) public nonReentrant { 
    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);
  }

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

## [C-Team M-01] コンパイルエラーでBBBコントラクトをデプロイできない（`struct TransferInfo.isETH`に`uint`型以外を代入している箇所がある）

### 要約
コンパイルエラーでBBBコントラクトをデプロイできない（`struct TransferInfo.isETH`に`uint`型以外を代入している箇所がある）

### バグの詳細
コンパイルエラーでBBBコントラクトをデプロイできない（`struct TransferInfo.isETH`に`uint`型以外を代入している箇所がある）

### インパクト
- コンパイルエラーでコントラクトをデプロイできない

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L28

### 使用したツール
特になし pen & paper

### 改善方法
TransferInfoストラクト内の`isETH`の型を`bool`に変更する。

// Before
struct TransferInfo {
    uint256 isETH;
    ...
}

// After
struct TransferInfo {
    bool isETH;
    ...
}

## [C-Team M-02] 利用可能なトークンを追加することができない（DoS）

### 要約
利用可能なトークンを追加する関数`addApprovedTokens()`が`private`かつ、BBBコントラクト内でこの関数を呼び出している箇所もないので誰も操作できない。

### バグの詳細
利用可能なトークンを追加する関数`addApprovedTokens()`が`private`で定義されているため、BBBコントラクト以外から呼び出せない。BBBコントラクト内で呼び出している関数もないため、デプロイ後に誰も利用可能なトークンを追加することができない。

### インパクト
- オーナーが`addApprovedTokens`にトークンを登録できず、ユーザーが`deposit()`でコントラクトに入金することができないため、DoSとなる。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L41

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

### 使用したツール
特になし pen & paper

### 改善方法
`addApprovedTokens` の `private`を、`public`または、`external`に置き換える。

// Before
function addApprovedTokens(address _token) private {
    if (msg.sender != owner) revert();
    approvedTokens.push(_token);
}

// After
function addApprovedTokens(address _token) public {
    if (msg.sender != owner) revert();
    approvedTokens.push(_token);
}

## [C-Team M-03] `_isXXX()`がガスリミットによってrevertされてしまう可能性がある

### 要約
`approvedTokens`, `whitelist`は可変長配列であり、これらをfor文で処理することは大量のガス消費によるtxリバートの原因となる可能性がある。

### バグの詳細
1. `deposit()`を実行する
2. `_isXXX()`が実行され、最大で配列`approvedTokens` or `whitelist` 長さ分処理が実行される
3. `approvedTokens` or `whitelist` 配列が長くなるにつれて、処理量が大きくなり、ガスリミットに達してしまう可能性がある

### インパクト
すべてのユーザーが `deposit()` できなくなる恐れがある。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L14-L15
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L59-L71
- approvedTokens, whitelistを宣言している箇所

    address[] approvedTokens; /// JPYC, USDC, USDTのみがownerからapproveされます
    address[] whitelist;

- 先の配列をfor文でループ処理している箇所

function _isXXX(
        address _token,
        address[] memory _xxx
    ) private pure returns (bool) {
        uint length = _xxx.length;
        for (uint i; i < length; ) {
            if (_token == _xxx[i]) return true;
            unchecked {
                ++i;
            }
        }
        return false;
    }

### 使用したツール
特になし pen & paper

### 改善方法
1. `approvedTokens`と`whitelist`の管理を配列からmappingに変更する。
2. `_isXXX()`を削除し、`require(approvedTokens[_token])`で確認する。

// Before
address[] approvedTokens;
address[] whitelist;

function deposit(
        uint256 _amount,
        address _token,
        bool _isETH
    ) public {
    if (!_isXXX(_token, whitelist)) revert();
}

function withdraw(
        address _to,
        uint256 _amount,
        bool _isETH,
        address _token
) public {
    if (!_isXXX(_token, whitelist)) revert();
}

// After
mapping(address => bool) approvedTokens;
mapping(address => bool) whitelist;

function deposit(
        uint256 _amount,
        address _token,
        bool _isETH
    ) public {
   require(approvedTokens[_token])
}

function withdraw(
        address _to,
        uint256 _amount,
        bool _isETH,
        address _token
) public {
   require(approvedTokens[_token])
}

## [C-Team M-04] 利用可能なトークンの脆弱性によって、資産の盗難やDoSを引き起こされる可能性がある

### 要約
一度トークンをホワイトリストに登録すると削除する方法がない。そのため、登録したトークンに脆弱性が見つかると対処できない可能性がある。また、ホワイトリストは誰でも実行可能な関数のため、登録したくないトークンであったとしても、悪意のある人に登録されてしまう可能性がある。

### バグの詳細
`addWhitelist()`が`public`であるため、ユーザーは`addWhitelist()`を呼ぶことによって既に`approvedTokens`に追加されたトークンを自由にホワイトリストすることができる。`approvedTokens`に追加されていながら`whitelist`に追加されていなかったコントラクトに重大な脆弱性が生じた場合、そのコントラクトが`whitelist`に追加され、意図せずそれらを利用することで資産を失ったり、DoSが生じる可能性がある。

### インパクト
予想外の挙動により、資産を盗まれたり、DoSなどが起こる可能性がある。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L77

function addWhitelist(address _token) public {
    if (!_isXXX(_token, approvedTokens)) revert();
    whitelist.push(_token);
}

### 使用したツール
特になし pen & paper

### 改善方法
1. `addWhitelist()`をowner権限で実行するように変更する。
2. オーナーが`whitelist`からトークンを削除できる関数の実装の検討をする。

function addWhitelist(address _token) public {
    if (msg.sender != owner) revert();
    if (!_isXXX(_token, approvedTokens)) revert();
    whitelist.push(_token);
  }

function removeWhitelist(address _token) public {
    if (msg.sender != owner) revert();
    if (!_isXXX(_token, whitelist)) revert();
    uint length = whitelist.length;
    for (uint i; i < length; ) {
        if (_token == whitelist[i]) {
            whitelist[i] = whitelist[length-1];
            whitelist.pop();
        }
      unchecked {
        ++i;
      }
    }
  }

## [C-Team M-05] Etherを入金できない

### 要約
`deposit()`関数は`payable`修飾子がついていないので、etherを扱うことができない。にも関わらず、etherを送金する処理が存在する。

### バグの詳細
`deposit()`関数は`payable`修飾子がついていないので、etherを扱うことができない。にも関わらず、etherを送金する処理が存在する。

### インパクト
BBBコントラクトでユーザーがetherを取り扱うことができない。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L82

  function deposit(uint _amount, address _token, bool _isETH) public {

### 使用したツール
特になし pen & paper

### 改善方法
`deposit`インターフェースに`payable`を追加する。この時、トークンを利用する際にetherを誤送金しないようチェックすることが望ましい。

// Before
function deposit(uint _amount, address _token, bool _isETH) public {
    ...
}

// After
function deposit(uint _amount, address _token, bool _isETH) public payable {
    if(_isETH == false && msg.value != 0) revert();
    ...
}

## [C-Team M-06] Etherを引き出すことができない

### 要約
Etherを引き出す際、BBBコントラクトアドレスはpayableでないので引き出すことができない

### バグの詳細
1. `withdraw()`を実行して、Etherを引き出そうとする
2. `_tokenTransfer()` で`_info.to.call()`が実行され、BBBコントラクトアドレスへEtherを送金しようとする
3. BBBコントラクトアドレスはpayableなコントラクトではないので、Etherを送金できず失敗する

### インパクト
ユーザーが、自身が`deposit`したetherを引き出すことができない。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L124-L127

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

### 使用したツール
特になし pen & paper

### 改善方法
1. `to.call{value: amount}("")`でETHを送金するときの`to`を`payable`にする。
2.

// Before
(bool success, ) = _info.to.call{value: _info.amount}("");

// After
(bool success, ) = payable(_info.to).call{value: _info.amount}("");

## [C-Team M-07] `deposit()`を使用して、同じトークンを再入金すると、depositInfoが都度更新されてしまう

### 要約
すでに入金したトークンを、`deposit()`で再入金する際に、`depositInfo.amount`には再入金したトークンの量が追加され、`depositInfo.lastTime`は変更されないのが望まれますが、現状ではどちらも新しいデータで上書きされてしまうため、それまでのトークンや報酬をユーザーが失ってしまいます。

### バグの詳細
1． Aliceが午前9時に1000JPYCをコントラクトに入金する
2． `depositInfo`に、現時刻と1000JPYCが保存される
3． Aliceが午前10時に50JPYCをコントラクトに再入金する
4． `depositInfo`に、現時刻と50JPYCが保存される
この間にwithDraw()は行っていないとすると、2で`depositInfo`に記録した「午前9時、1000JPYC」のデータは上書きされ、4の「午前10時、50JPYC」のデータのみが残ります。

### インパクト
`depositInfo`のデータの上書きは、それまでに入金したトークンの所有権をユーザーが失うこと、また、ユーザーが獲得予定の報酬の減額も意味し、長期間トークンをコントラクトに収めておくインセンティブも消失します。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L94-L96

function deposit(uint _amount, address _token, bool _isETH) public {
    ///...
    depositInfo.lastTime = uint40(block.timestamp);
    depositInfo.amount = _amount;
    depositAmt[msg.sender][_token] = depositInfo;
}

### 使用したツール
特になし 筆と紙

### 改善方法
1. `depositAmt[msg.sender][_token].amount`が0かのチェックを追加する
2. 0なら、そのトークンでの初めての入金か、そのトークンで入金したことはあるものの全て引き出しているかのどちらかなので、現時刻を記録する
3. 0でないなら、そのトークンの過去の入金分が残っている状態なので、`depositInfo.lastTime`は変更しない
4. `depositInfo.amount`には入金額を加算する。

function deposit(uint _amount, address _token, bool _isETH) public {
    ///...
    if(depositAmt[msg.sender][_token].amount == 0){
        depositInfo.lastTime = uint40(block.timestamp);
    }
    depositInfo.amount += _amount;
    depositAmt[msg.sender][_token] = depositInfo;
}

追記
報酬は、入金期間とトークンの量が重み付けされ、更に引き出されるトークンの量に応じて算出されることが望ましいと考えられますが、現状では最初に少額の入金をし、十分時間が経過してから、同じトークンで多額の入金をしすぐに全額引き出すと、さも多額の入金が長期間されていたかのように報酬が跳ね上がってしまいます。
この対策としては、
1. ユーザーからあるトークンが入金され、同じユーザーが同じトークンで再入金するときは、それまで入金したトークンを全て引き出してもらいその分の報酬を得てもらってから、再入金してもらえるようにロジックを変更する
2. ユーザーから入金される毎にdepositInfoを作成し、個別に管理する
などが考えられます。ご検討下さい。

## [C-Team M-08] ユーザーの資金が凍結される可能性がある

### 要約
ユーザーが資金を引き出す際、ERC20未対応のコントラクトアドレスへ送信してしまうとそのERC20が引き出せなくなってしまう。

### バグの詳細
`withdraw()`内で実行される`_tokenTransfer()`で、_toのアドレスがコントラクトであり、かつERC20の送金機能を持たない場合、そのコントラクトにロックされてしまうことになり、ユーザーの資金が凍結される可能性があります。

### インパクト

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L124-L132

function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
) public {
    ...
    _tokenTransfer(_info);
    ...
}

### 使用したツール
特になし pen & paper

### 改善方法
_toのアドレスがコントラクトでないことを確認してください。

function withdraw(
    address _to,
    uint _amount,
    bool _isETH,
    address _token
) public {
    require(_to.code.length == 0);
    ...
    _tokenTransfer(_info);
    ...
}

## [C-Team M-09] 報酬が正しく算出されない

### 要約
本プログラムでは、`getReward()`にて報酬が算出されていますが、solidityの言語仕様により小数点以下が切り捨てられてしまうことで、ユーザーは妥当な報酬を得られなくなります。

### バグの詳細
ユーザーがあるトークンを入金する
1. 1秒後にそのトークンを全額引き出した場合
この時、ユーザーは最大の報酬を手にすることができる
2. 51秒以降に全額を引き出した場合
この時、報酬計算における分数は分母の方が大きくなるため、小数点以下が切り捨てられ分数部分が0となり、0の乗算となるので、引き出すトークンの量に何を入れても報酬は0となる

### インパクト
報酬体系は、多額の入金や長期の入金のインセンティブとなるよう設計されることが、ユーザー、オーナーの双方から期待されているはずですが、現状の設計ですと、入金期間が長くなるほど、入金額にかかわらず報酬は0になってしまい、逆に、多額の入金後に即引き出しを行うことで大量の報酬を得ることができてしまいます。

### 対象となるコード
https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L56

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

### 使用したツール
特になし pen & paper

### 改善方法
好ましい算出方法は以下の通りと考えられます。
1. amountの乗算を前に持ってくる
2. 経過時間の計算式を分子に持ってくる
3. REWARD_RATEは割合であるとネーミングから推測されるので、REWARD_RATEを100で割ったものを乗算する。

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