# [KY H-1]`depositAmt`が上書きされる ## 要約 `deposit()`内でdeposit Amountを保存するが、引き出し終わる前に同じトークンを預け入れると、最新の預け入れ金額に上書きされてしまう ## バグの詳細 PoCとしては以下となる 1. Aliceが10USDT depositする `deposit(10, USDT contract address, false)` 2. depositAmt[Alice][USDT contract address] = 10となる 3. Aliceが20USDT depositする `deposit(10, USDT contract address, false)` 4. deposit[Alice][USDT contract address] = 20となる AliceがwithdrawできるUSDTは最大でも20となる。 先に預けた10USDTがコントラクトの中にロックされてしまう ## インパクト コントラクトの中にユーザーの資金がロックされて、ユーザーにとって損失があるので重大 ## 対象となるコード https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L96 ## 使用したツール 特になし ## 改善方法 あるトークンをdepositしている状態であるのなら、同じトークンを`deposit()`することができないようにチェックする。 Check depositAmt[msg.sender][_token] == 0 ```solidity function deposit(uint _amount, address _token, bool _isETH) public { require(depositAmt[msg.sender][_token] == 0, "has already depositted"); // ...... } ``` # [KY M-1]`deposit()`関数にpayableがついていない ## 要約 `deposit()`はEtherをBBB contractに送金する場合もあるのに、payable identifierがついていない ## バグの詳細 `deposit()`が第3引数をtrueとして実行することは、Etherがコントラクトにdepositされることを意味する。 しかし、payable identifierがないためEhterをコントラクトに送ることができない。 ## インパクト Attackされることはないが、ドキュメントの仕様と大きく異なる ## 対象となるコード https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L82 ## 使用したツール 特になし ## 改善方法 `deposit()`にpayable identifierを付ける。 ```solidity function deposit(uint _amount, address _token, bool _isETH) public payable { ``` # [KY M-2]BBB contractに`fallback()`がない ## 要約 `deposit()`が実行されるとBBB contractはEtherを受け取ろう場合があるが、fallback()がないため、受け取れずにrevertする ## バグの詳細 `deposit()`が第3引数をtrueとして実行することは、`_tokenTransfer()`を呼び出してEtherがコントラクトに送金されようとすることを意味する https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L126 しかし、BBBにfallback()がないためEhterをコントラクトに送ることができない。 ## インパクト Attackされることはないが、ドキュメントの仕様と大きく異なる ## 対象となるコード https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L126 ## 使用したツール 特になし ## 改善方法 BBB contract内にfallback関数を追加する ```solidity fallback() external payable {} ``` # [KY M-3]不適切なCheckによって`deposit()`でEtherを送金できなくなっている ## 要約 `deposit()`でEtherを預けようとしても、83行目のckeckが不適切なので、送金できない https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L83 ## バグの詳細 `deposit()`が第3引数をtrueとして実行することは、`_tokenTransfer()`を呼び出してEtherがコントラクトに送金されようとすることを意味する https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L83 しかし、83行目のckeckで、Etherは`whiteList`に含まれていないためrevertしてしまう。 `if (!_isXXX(_token, whitelist)) revert();` ## インパクト Attackされることはないが、ドキュメントの仕様と大きく異なる ## 対象となるコード https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L83 ## 使用したツール 特になし ## 改善方法 Ether以外のTokenが送信されているときのみ、whiteListと照合するチェックを行う ```solidity if (!_isEther && !_isXXX(_token, whitelist)) revert(); ``` # [KY H-2]Checkが不足しているため、預入額とEther送金額が一致しない可能性がある ## 要約 `deposit()`でEtherを預けた値(msg.value)と、depositAmtに格納された預入額が一致しない場合がある https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L95 ## バグの詳細 `deposit()`が第3引数をtrueとして実行することは、`_tokenTransfer()`を呼び出してEtherがコントラクトに送金されようとすることを意味する その際に、`_tokenTransfer()`でも以下の様に_info.amount分のEtherを送金している。 ```solidity 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); } } } ``` しかし、_info.amountが、`deposit()`の際のmsg.valueと一致するとは限らない。 そのため、以下のようなAttackを行うことが可能 1. deposit(1000, address(0), true){msg.value : 1 wei} で実行 2. withdraw(msg.sender,900,true,address(0)) を実行 3. 899wei分Attackerは儲かることができる ## インパクト Attackerが余分に引き出しすることが可能なので、重大なエラー ## 対象となるコード https://gist.github.com/Tomosuke0930/5624c8fde570aae10e2cec00c4c2758a#file-unsecure-sol-L82 ## 使用したツール 特になし ## 改善方法 Ether送信されているときは、以下のCheckを追加する Check msg.value == _amount ```solidity require(msg.value == _amount, "incorrect amount of ether"); ```