code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L500

Vulnerability details

Impact

Fees are being sent directly as ERC20 token transfers to the admin/DAO address within PuttyV2.withdraw.

If the fee token transfer reverts, the PuttyV2.withdraw transaction reverts as a whole and assets can not be withdrawn for expired puts or exercised calls.

Proof of Concept

PuttyV2.withdraw

ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);

ERC20 token transfers of fees to the owner() address are subject to reverting token transfers, hence withdrawals are brought to a halt.

Reasons for reverting ERC20 token transfers (see Weird ERC20 Tokens):

Blacklisted addresses - The owner could theoretically be blocked and prevented from transferring tokens
ERC777 token is used as order.baseAsset - Receiving owner() is a contract and does not implement the ERC777 function tokensReceived. Transaction reverts

Tools Used

Manual review

Recommended mitigation steps

Consider implementing a pull pattern where fees are not sent directly to the admin/DAO address, instead fees are kept in the PuttyV2 contract and the admin/DAO sweeps (withdraws) fee token balances from time to time.

outdoteth commented 2 years ago

Report: Owner can DOS withdraw() for ERC777 tokens

HickupHH3 commented 2 years ago

dup of #296