Day 5 G-team - HackMD

# Day 5 G-team ## [H-01] put 行使時ではなく、put 失効時に手数料が差し引かれている。 ### ■ カテゴリー ### ■ 条件 * PuttyV2.solL#495に存在する条件が、order.isCallが falseで isExercisedがfalseの場合 * feeAmountは fee > 0の場合 ### ■ ハッキングの詳細 PuttyV2.solでは、行使時ではなく、putの失効時に手数料が差し引かれています。 setFee()関数のコメント欄に「行使時に適用される手数料率」とありますが、これはポジションが行使される（または行使された）時のみ、手数料額が行使額から差し引かれることを意味しています。しかし、PuttyV2.solL#495-L503の関数withdraw()では、Put ポジションが行使されずに失効していても手数料が差し引かれています。また、関数exercise()において、Put ポジションの権利行使が行われ、権利行使が呼び出される際に、order.strikeから手数料が差し引かれないようにしました（PuttyV2.solL#451)。 ### ■ 修正方法 PuttyV2.sol#L498の if条件を(fee > 0 && order.isCall && isExercised)で更新。 ```solidity= if(fee> 0) { feeAmount=(order.strike*fee)/ 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ``` ```solidity= if (fee > 0 && order.isCall && isExercised) { feeAmount=(order.strike*fee)/ 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ``` PuttyV2.sol#L451でput 行使、strike 移行後のfeeAmount計算と控除を以下のように追加。 ```solidity= uint256 feeAmount = 0; if (fee > 0) { feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); ``` ## [H-02] 元の注文とカウンターオファーの注文の2つが約定されてしまう ### ■ カテゴリーフロントランニング ### ■ 条件 acceptCounterOfferが発生したとき必ず ### ■ ハッキングの詳細 acceptCounterOfferが発生したときにフロントランニングでfillOrderを挟み込む(どうやるのかは分かっていません)。そうすとfillOrderもacceptCounterOfferも成功してしまう。 ### ■ 修正方法以下などの方法で約定済みの注文は、キャンセルは失敗するようにする ```solidity= //before function cancel(Order memory order) public { require(msg.sender == order.maker, "Not your order"); //after function cancel(Order memory order) public { require(msg.sender == order.maker, "Not your order"); require(_ownerOf[uint256(orderHash)] == 0, "約定済みです") ``` ## [H-3] 空でないfloorでショートコール注文を作成すると、オプションの行使と引き出しが不可能になります ### ■ カテゴリー Context? ### ■ 条件 * floorのロングコールオプションが約定し、且つfloorでショートコール注文を作成する ### ■ ハッキングの詳細メーカー (A) がロングコールを作成し、テイカー (B) がそれを埋め、フロアトークン (XYZ) をパテに転送するとします。メーカー (C) が floorTokens (XYZ) でショートコールを作成すると、テイカー (D) は、XYZ がすでにパテに常駐しているため、ロングコールを埋めて行使することができます。ただし、これにより、A と B の間で作成されたオプションペアが無効になります。A は行使できず、B は撤回できない。 ### ■ 修正方法ショートコールの場合はfloorTokensを許容しない ```.sol require(order.floorTokens.length == 0, "Short call cant have floorTokens"); ``` ## [H-4] ゼロストライクコールオプションは、テイカーからプレミアムを盗むために利用可能 ### ■ カテゴリー ERC20 ### ■ 条件 ### ■ ハッキングの詳細 ERC20の中には、ゼロ金額の送金を許可しないものもあり、order.baseAsset はそのような資産になる可能性があります。悪意のあるメーカーは有効期限が切れるのを待って資産を引き出し、事実上無料でプレミアムを回収できるため、メーカーはテイカーから盗むことが可能 ### ■ 修正方法 strikeが0より大きいことをチェックする ```.sol if (order.strike > 0) { ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike); + } ``` ## [M-01] 悪質なトークン契約により注文がロックされる可能性があります。 ### ■ カテゴリー ERC20,ERC721 ### ■ 条件 * 攻撃者が悪意のあるERC20またはNFTを用意している * 攻撃者は、注文を作成し、アドレスに悪意のあるERC20またはNFTを設定する ### ■ ハッキングの詳細以下のいずれかのフィールドに悪意のあるトークンコントラクトを含ませることで、注文のexercise()withdraw()を実行させないようにすることが可能です。 * baseAsset * floorTokens[] * erc20Assets[] * erc721Assets[] 攻撃者は、注文を作成し、そのアドレスの1つを攻撃者の実行できる悪意のあるコントラクトに設定することができる。攻撃者は、ユーザーの注文を許可した後、常に悪意のあるコントラクト上の変数をトグルしてその注文を戻せます。望ましくないポジションにある場合（例えば、ショートして価格が上昇した場合）、注文がexercise()されるのを防ぐことによって利益を得ます。時間切れか価格が下がるのを待ち、悪意のあるトークンで転送が行われるようにします。 withdraw()関数も信頼できない外部アドレスに呼び出されるため、同様の攻撃が可能です。この場合、攻撃者はオプションを行使して、他のユーザーがNFTまたはERC20トークンを要求できないようにすることができます。 #### exercise() ロングオーダーを行使し、それを表すロングポジションNFTをburnします。 #### withdraw() ショートオーダーから資産を引き出し、またショートポジションをburnします。 ### ■ 修正方法承認されたERC20トークンまたはERC721アドレスコントラクトをホワイトリスト化を検討 ## [M-02] ガス不足による引き出し失敗 ### ■ カテゴリーガス欠 ### ■ 条件引き出すトークンの数を送信するだけのガスが足りない ### ■ ハッキングの詳細トークンの数だけループを回して引き出しを行っている。転送されるトークンの数に制限はない。ガスの必要量も変わることがある。つまり資産が転送中に多くのガスを使用する場合提供された資産では権利を行使できないかもしれない。オプションの買い手はプレミアムを支払っているのに、権利を行使できずアセットを手に入れられない。 ### ■ 修正方法ひとつの取引でのトークンの上限を設けるかループを回さず一度で送信できるようにする ## [M-3] プットオプションの売り手は、0 or存在しないトークンを指定することで行使を防ぐことが可能 ### ■ カテゴリー Access Controll ### ■ 条件 * 金額が０や存在しないアドレスを指定した注文をする ### ■ ハッキングの詳細プットオプションの買い手は、その権利行使価格を取得できるという特権のために、オプションプレミアムを売り手に支払います。彼らが「プット」を実行できない場合、彼らはプレミアムを無料で支払ったことになり、本質的に彼らから資金を盗まれたことになります。 → 従って売り手が行使できないプットオプションを作成できると問題になる例えば・・・ * 売り手が`order.erc20Assets`に金額ゼロのものを含めるか、現在そのアドレスにコードがない資産を指定する場合この場合`revert`が起こる ### ■ 修正方法 fillOrder() 中に資産の金額とアドレスを確認し、トークンが存在するかどうかをチェックする ## [M-04] プットオプションは手数料無料 ### ■ カテゴリー ### ■ 条件コールオプションを行使した場合 ### ■ ハッキングの詳細コールオプションは手数料が発生するが、プットオプションでは手数料が発生しない。 #### プットオプションプット・オプションが行使されると、行使者はorder.baseAsset建ての行使価格（当初はショートポジションの保有者から預かったもの）を受け取ります。 ```solidity= // transfer strike from putty to exerciser ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike); ``` #### コールオプションコールオプションが行使された場合、行使者は権利行使価格をPuttyに送信し、ショートポジションの保有者は権利行使額を引き出すことができるようになります。 ```solidity= // transfer strike to owner if put is expired or call is exercised if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) { // send the fee to the admin/DAO if fee is greater than 0% uint256 feeAmount = 0; if (fee > 0) { feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); // @audit DoS due to reverting erc20 token transfer (weird erc20 tokens, blacklisted or paused owner; erc777 hook on owner receiver side can prevent transfer hence reverting and preventing withdrawal) - use pull pattern @high // @audit zero value token transfers can revert. Small strike prices and low fee can lead to rounding down to 0 - check feeAmount > 0 @high // @audit should not take fees if renounced owner (zero address) as fees can not be withdrawn @medium } ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount); // @audit fee should not be paid if strike is simply returned to short owner for expired put @high return; } ``` ### ■ 修正方法行使されたプットオプションにも手数料を課す。 #### before ```solidity= ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike); ``` #### after ```solidity= feeAmount = (order.strike * fee) / 1000; ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike -feeAmount); ``` ## [M-05] fillOrder()とexercise()にてコントラクトに送られたEtherを永遠にロックする可能性 ### ■ カテゴリー GOX ### ■ 条件結果的にEtherを必要としない処理の場合でEtherを送る ### ■ ハッキングの詳細 payableの関数内でEtherが不要なコードパスがある。このコードパスをEtherを付けて実行できるが、そのEtherはコントラクト内にロックされるだけ ### ■ 修正方法 payableの関数内でEtherが不要なコードパス3箇所に `require(0 == msg.value)` を加える例 ```solidity // before if (order.isLong{ ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium); } else { // after if (order.isLong{ require(0 == msg.value); ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium); } else { ``` ## [M-6] 契約所有者は、ユーザーがストライキを撤回するのをブロックする可能性があります ### ■ カテゴリー Denial of Service ### ■ 条件 * pullで資金を引き出せない ### ■ ハッキングの詳細ユーザーが Putty 契約でエスクローされたストライキを撤回する場合、Putty はストライク金額から一定の手数料を請求します。料金は最初に契約所有者に送られ、その後残りのストライキ額がユーザーに送られます。ユーザーがストライキの撤回 → Puttyに手数料支払い → 契約所有者に支払い（ここを拒否できる可能性がある） → ユーザーが残りのストライキ額を取得 1. ownerをzero addressにする * ERC20ではこれは`revert`される 2. baseAssetがERC77トークンの場合 * tokensReceivedが呼ばれたタイミングで`revert`が可能 ### ■ 修正方法 [withdrawal pattern](https://docs.soliditylang.org/en/v0.8.15/common-patterns.html#withdrawal-from-contracts)を採用する ## [M-07] 攻撃者は、ERC721をサポートしないNFT（CRYPTOPUNKなど）に対してshort put のオプション注文を作成でき、ユーザーは注文を履行することができますが、オプションを行使することはできません。 ### ■ カテゴリー ERC721 ### ■ 条件 ERC721をサポートしないNFTを指定してshort put　オプション注文を作成した場合 ### ■ ハッキングの詳細 Puttyは、solmateのERC721.safeTransferFromを使用しており、NFTコントラクトがonERC721Receivedを実装していることが必要です。 punksやrocksのようなOG NFTの場合、失敗します。 ### ■ 修正方法順番にnftsにホワイトリストを追加したり、cryptopunkなど OG NFTをサポートすることを検討してください。 ## [M-08] 関数の重複でerc20AssetsやbaseAssetがERC20ではなくERC721になってしまう ### ■ カテゴリートークン変異 ### ■ 条件 baseAssetやERC20をtransferFromする ### ■ ハッキングの詳細 ERC721のtransferFrom()が呼ばれることでbaseAseetとERC20がERC721として処理されてしまう。その事によりERC721として扱われるようになり、権利行使や引き出しが行えなくなる ### ■ 修正方法 ERC721, ERC20それぞれのWLを用意する。 WLを元に明確にERC721かERC20どちらのtransferFrom()を使うか振り分ける ## [M-9] 契約は手数料なしでフラッシュローンプールとして機能する ### ■ カテゴリー Reentrancy？ ### ■ 条件 ### ■ ハッキングの詳細手数料なしのフラッシュローンとして利用できる ### ■ 修正方法 reentrancyチェックをする？ ## [M-10] PUTTYのポジショントークンがERC721以外の受け取り側にミントされる可能性がある。 ### ■ カテゴリー ERC721 ### ■ 条件 ERC721トークン以外でfillOrderのmintを使用した場合 ### ■ ハッキングの詳細 Puttyはコードベース全体でERC721safeTransferと safeTransferFromを使用して、ERC721トークンがERC721以外の受け取り側に転送されないようにしています。しかし、fillOrderの最初の位置のmintはsafeMintではなく mintを使用し、受け取り側がERC721トークン転送を受け入れるかどうかをチェックしません。メーカーやテイカーがERC721トークンを受け取ることができないコントラクトである場合、そのオプションポジションはロックされ、譲渡できない可能性があります。受け取り側のコントラクトがPuttyと対話する仕組みを提供しない場合、ポジションの行使や資産の引き出しができなくなります。 ### ■ 修正方法 SolmateのERC721#safeMintの requireチェックを独自のmint関数で実装することを検討してください。 #### before ```solidity= function _mint(address to, uint256 id) internal override {. require(to!= address(0), "INVALID_RECIPIENT"); require(_ownerOf[id] == address(0), "ALREADY_MINTED"); _ownerOf[id] = to; emit Transfer(address(0), to, id); } ``` #### after ```solidity= function _safeMint(address to, uint256 id) internal virtual {. _mint(to, id)です。 require( to.code. ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") == ==ERC721TokenReceiver(to). ERC721TokenReceiver.onERC721Received.selector, "unsafe_recipient" ); } ``` リエントランシー攻撃対策も検討する。 ## [M-11] コントラクトオーナーによって手数料の書き換えがユーザーの同意無しで行える ### ■ カテゴリープロジェクトの信頼 ### ■ 条件コントラクトオーナーであればはいつでも誰の許可も不要 ### ■ ハッキングの詳細利用者は1%の手数料だということで、オプション取引をしていた。しかし、いつの間にかその手数料を3%にしていた。 1%だと思って権利行使したら3%取られていた。ということがありえる作りになっている。 ### ■ 修正方法手段1: 手数料はハードコードする手段2: 手数料が変更された時間を記録しておく(修正ではないが少なくとも今よりは多少マシ) 手段3: 時間と手数料をペアで記録しておき、手数料を変更しても変更前の取引はその当時の手数料で権利行使が行えるようにする ## [M-12] 行使価格が小さいオプションは 0 に切り捨てられ、資産の引き出しを防ぐことが可能 ### ■ カテゴリー ERC20 ### ■ 条件 * ERC20 * 且つトークン数量が0となる ### ■ ハッキングの詳細一部の ERC-20 トークンは、ゼロ値トークンの転送と復帰をサポートしていません。このようなトークンを order.baseAsset として使用すると、オプションストライクがかなり少なく、プロトコル手数料率が低いため、0に切り捨てられ、それらのポジションの資産の引き出しが妨げられる可能性がある ### ■ 修正方法 0以上の場合のみ送金処理を行う。 ```.sol if (feeAmount > 0) { ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); } ``` ## [M-13] 不正なメーカーが注文期間を0に設定できる。 ### ■ カテゴリー ### ■ 条件 * メーカーが注文期間ゼロの注文を作成する ### ■ ハッキングの詳細悪意のあるメーカーは、最小注文期間を0に設定することができ、これは注文が成立した後、即座に失効することを意味します。テイカーは引出しオプションのみを取得し、それも権利行使価格の手数料がかかるので、テイカーはこの無意味な取引で損をすることになる。 * メーカーが注文期間ゼロの注文を作成 * テイカーはこの注文を満たしたが、継続時間が0であったため、注文は即座に失効した。 * テイカーはstrike 価格の手数料で引き出しをする唯一のオプションを取得します。 ### ■ 修正方法少なくとも x 日間の継続時間を強制する。 ## [M-14] 注文キャンセルのフロントランニングとオフチェーンDBへの依存 ### ■ カテゴリーフロントランニング ### ■ 条件 2つある 1. キャンセル関数を呼ぶと、それをトリガーにしてフロントランニング出来る 2. キャンセル関数を呼んでも、その関数はオフチェーンDBが必要なためオフチェーンDBが落ちてるとキャンセルとならない。そしてその注文データをなんらかの形で保持しているユーザーには注文を約定できてしまう ### ■ ハッキングの詳細 #### 1.キャンセルの前に約定を差し込めるブロック内にキャンセルが発生したら、それよりも前に約定をさしこみ、結果的にキャンセルできず、約定される。 #### 2.キャンセル出来るはずの注文がキャンセルできないキャンセルしたつもりが、オフチェーンDBが落ちているなどの原因でキャンセルに必要なデータを取り出せずキャンセルが出来なかった場合、他のユーザーがその注文を約定できてしまう。他のユーザーはオフチェーンのDBではなく、別のところに保持していおいたデータを利用して注文を約定している。 ### ■ 修正方法 cancel関数とは別に、すべての注文をキャンセル出来るようにする。例えばすべての注文のキャンセルはオフチェーンDB使わず単純に注文カウンターなどの状態変数を変更することで全注文をキャンセルとして扱うなど。 ## [M-15] ゼロストライクコールオプションは、システム料金の支払いを回避可能 ### ■ カテゴリー Context? ### ■ 条件 * ゼロストライクコールオプションを作成する？ ### ■ ハッキングの詳細ゼロおよびゼロに近いストライクコールは、一般的なデリバティブタイプです。このようなデリバティブの場合、システムは手数料を受け取ることはありません。手数料は現在、オーダーストライクの一部として定式化されています。 ### ■ 修正方法手数料をオプションプレミアムにリンクする ## [M-16] Solidityのバージョン0.8.13には2つの既知の問題がPuttyV2に対してある ### ■ カテゴリー Soitityのバージョン ### ■ 条件 Solidityのバージョン0.8.13 ### ■ ハッキングの詳細影響範囲 - ABIエンコーディングの脆弱性 - hashOrder() - hashOppositeOrder() - インラインアセンブリのメモリ副作用に関する最適化のバグに関連する脆弱性 - 継承しているopenzeppelinがインラインアセンブリを利用 - 継承しているsolmateがインラインアセンブリを利用 ### ■ 修正方法 Solidityのバージョンを0.8.15に上げる