# Day 7
以下yoki記載
# [yoki] addApprovedTokens, addWhitelistで同じトークンを複数登録することが可能
## 要約
addApprovedTokens, addWhitelistで重複してトークン登録することが可能なので、`_isXXX`関数におけるオーバーフローが生じる。
## バグの詳細
addApprovedTokens, addWhitelistでは重複のチェックなく配列に格納しています。
## インパクト
`_isXXX`関数内において、`unchecked`を用いて加算しているので`addApprovedTokens`や`addWhitelist`の要素数が増えるとオーバーフローとなる可能性がある。
## 対象となるコード
```.sol
function addWhitelist(address _token) public {
if (!_isXXX(_token, approvedTokens)) revert();
whitelist.push(_token);
}
```
```.sol
function addApprovedTokens(address _token) public {
if (msg.sender != owner) revert();
approvedTokens.push(_token);
}
```
## 使用したツール
特になし
## 改善方法
配列にpushする前に追加対象の値外既に存在するか確認する。
# [yoki] isETHをtrueにしてETH以外のトークンを指定すること（またはその逆）が可能である
## 要約
## バグの詳細
`deposit`や`withdraw`関数にて`_isETH`を引数として持つが、
* ETHを扱うが`_isETH`を`false`
* ETHを扱わないが`_isETH`を`true`
といったリクエストが可能である。
## インパクト
例えば、`deposit`関数利用時に任意のトークンを指定しと`_isETH`を`true`とした場合にはETHの送金処理が実行されることになる。
## 対象となるコード
```.sol
function deposit(uint _amount, address _token, bool _isETH) public payable {
```
```.sol
function withdraw(
address _to,
uint _amount,
bool _isETH,
address _token
) public payable {
```
```.sol
function _tokenTransfer(TransferInfo memory _info) private {
if (_info.isETH) {
emit DebugLogEvent("debug: This is eth");
(bool success, ) = _info.to.call{ value: _info.amount }("");
require(success, "Failed");
} else {
emit DebugLogEvent("debug: This is not eth");
IERC20(_info.token).transferFrom(_info.from, _info.to, _info.amount);
}
}
```
## 使用したツール
特になし
## 改善方法
ETHかどうかの判別をインターフェイスに持たせるのではなく、`msg.value`の値を利用してその判別を行う。
---
以下mabukさん記載
# ETHのdepositが出来ない
## 要約
ETHをコントラクトにdeposit関数で送金すると必ずエラーとなる。つまり誰もETHをdeposit出来ない。
## バグの詳細
コントラクトのコンパイルは出来るが、この実装だとコントラクトはETHの送金を受け取れない。
ETHの送金を受け取るにはdepositはpayableであり、かつreceive関数またはfallback関数が必要。
## インパクト
ETHのdepositが必ず失敗するので誰もETHのdepositが出来ない。
## 対象となるコード
```solidity
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;
}
```
## 使用したツール
hardhat
## 改善方法
- receive関数の追加
- depositにpayable修飾子を追加
```solidity
//before
function deposit(uint _amount, address _token, bool _isETH) public {
//after
receive() external payable {}
function deposit(uint _amount, address _token, bool _isETH) public payable {
```
# 送金するETHより小さい値を引数に指定してdeposit出来る
## 要約
ETHの場合、ウォレットから出ていく値とdepositに指定出来る値が異なっていてもエラーにならない
## バグの詳細
1. depositのamountを0.5ethに設定する
2. ウォレットからの送金額を1.0ethに設定する
3. depositを実行
4. コントラクトの残高は1.0eth増える
5. ウォレットの残高は1.0eth減る
## インパクト
depositを行うユーザのethがGOXする可能性がある
## 対象となるコード
```solidity
function deposit(
uint256 _amount,
address _token,
bool _isETH
)
```
## 使用したツール
hardhat
## 改善方法
2つある
1. ETHと他のトークンのdeposit関数を分ける
2. ETHの場合はdeposit時にamountを設定不可とする
# 残高がある時にdepositを呼ぶと資金を失う
## 要約
*前提: depositは残高が0の場合のみ受け付けるという仕様(何度も呼んで残高を増やしていくことは想定外とする)*
以前にdepositを行い、残高がまだある状態で再度depositを行うと、以前の残高は失われ新しい残高で上書きされる
## バグの詳細
1. 9/20に100トークンのデポジットを行う
2. lastTime:9/20, amount:100トークンが記録される
3. 9/30に10トークンのデポジットを行う
4. lastTime:9/20, amount:100トークンが記録は消え、lastTime:9/30, amount:10トークンの記録だけが残る
## インパクト
ユーザーが資産を失う可能性がある
## 対象となるコード
```solidity
depositInfo.lastTime = uint40(block.timestamp);
depositInfo.amount = _amount;
depositAmt[msg.sender][_token] = depositInfo;
```
## 使用したツール
特になし
## 改善方法
残高が無いもしくは0である場合以外はエラーとする
---
以下hamaupさん記載
# [GTeam 0X] withdraw()預金残高を超えても引き出しができてしまう。
## 要約
withdraw()で、預金残高を超えても引き出しができてしまう。
引き出しをした際に、預金残高を更新するように提案する。
## バグの詳細
withdraw()を預金残高以下の引き出し額に指定して実行した場合、預金残高を保存している
depositAmt[msg.sender][_token].amountの値が更新されない。
もう一度、withdraw()を実行した際に、また同じ預金残高の値を使用して引き出し処理をしてしまう。
## インパクト
コントラクト残高がなくなるまで引き出しされてしまう。
## 対象となるコード
```solidity=
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);
}
```
## 使用したツール
特になし
## 改善方法
withdraw() 引き出し額を差し引いて預金残高 `depositAmt[msg.sender][_token].amount` を更新する。
#### before
```solidity=
uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
canWithdrawAmount = 0;
```
#### after
```solidity=
uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
canWithdrawAmount = 0;
depositAmt[msg.sender][_token].amount -= _amount
```
# [GTeam 0X] withdraw()引き出し額を0に指定した場合でもrewardを受け取れてしまう。
## 要約
withdraw()引き出し額を0に指定した場合でもrewardを受け取れてしまう。
その場合は、報酬金`reward` を送れないようにする。
## バグの詳細
少しでも預金残高がある場合、withdraw()で引き出し額 `_amount` を0に指定しても、報酬額`reward` を決定され送信されてしまう。
1. 少額をdeposit()する。
2. withdraw()引き出し額 `_amount` を0に指定する
3. 引き出し額 `_amount` は、転送する額に`info.amount`保持される。
4. 預金残高 `depositAmt[msg.sender][_token].amount` が1でもある場合、
以下条件パスする。
```solidity=
require(_info.amount < canWithdrawAmount, "ERROR");
```
5. getReward()内で預金残高で報酬金`reward` を決定
6. withdraw()を実行した者`msg.sender`に報酬金`reward` を送信
## インパクト
引き出し額 `_amount` を0に指定して引き出しを繰り返されると用意している報酬額`reward`がすべてなくなってしまう。
## 対象となるコード
```solidity=
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);
}
```
## 使用したツール
特になし
## 改善方法
# [GTeam 0X]の改善方法を併用して、引き出し額 `_amount` が0では報酬金`reward` を送れないようにする。
#### before
```solidity=
uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
canWithdrawAmount = 0;
```
#### after
```solidity=
uint canWithdrawAmount = depositAmt[msg.sender][_token].amount;
require(_info.amount < canWithdrawAmount, "ERROR");
require(_info.amount > 0, "ERROR");
canWithdrawAmount = 0;
depositAmt[msg.sender][_token].amount -= _amount
```
[GTeam 03] 預け入れから引き出しまでの期間が長いほど報酬金`reward`が少なくなる。
## 要約
預け入れから引き出しまでの期間が長いほど報酬金`reward`が少なくなってしまい、即座に預け入れ引き出しを繰り返す利用方法が増えてしまう。
報酬金`reward`の計算方法とdeposit()のlasttimeの値の保存方法の検討を提案する。
## バグの詳細
報酬金`reward`計算式です。
```solidity=
reward = (REWARD_RATE / (block.timestamp - lastTime)) * amount;
```
1. 初めてdeposit()を実行した時に、lastTimeにタイムスタンプが入ります。
deposit()
```solidity=
depositInfo.lastTime = uint40(block.timestamp);
```
2. widthdraw()で報奨金計算 getReward（）を呼び出した時に、lastTimeにタイムスタンプが入ります。
getReward（）
```solidity=
uint lastTime = depositAmt[msg.sender][token].lastTime;
```
3. 報奨金が計算されます。その際、widthdraw()で報奨金計算した時から初めてdeposit()を実行した時の差分が分母に入ります。
getReward（）
```solidity=
reward = (REWARD_RATE / (block.timestamp - lastTime)) * amount;
```
例えば、初めてdeposit()を実行した後に、もう一度deposit()をした場合、lastTimeが2回目に実行したタイムスタンプに置き換えられる。
deposit()
```solidity=
uint lastTime = depositAmt[msg.sender][token].lastTime;
```
その場合、現在のタイムスタンプから初めてdeposit()を実行した期間より現在のタイムスタンプから2回目にdeposit()を実行した期間の方が短い、つまり報酬金計算式の分母が小さくなるため、預け入れから引き出しまでの期間が長いほど報酬金`reward`が少なくなってしまう。
また報酬金計算式内にblock.timestampを使用しているので、誤差が出る可能性があり、不正なマイナーによる改ざんもありセキュリティ面で安全ではない。
deposit()のlasttimeの値は、預け入れする度に上書きされてしまう。引き出しを行わず、預け入れを繰り返し行った場合、最後の預け入れをした時のタイムスタンプに置き換わってしまう。
## インパクト
短く預け引き出す方が報酬が高くなってしまうため、即座に預け入れ引き出しを繰り返す利用方法が増えてしまう。
## 対象となるコード
```solidity=
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;
}
```
## 使用したツール
特になし
## 改善方法
報酬金`reward`の計算方法とdeposit()のlasttimeへの値の保存方法の検討を提案する。
Sign in with Wallet
Connect another wallet