code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L268 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L436

Vulnerability details

Impact

Though the document says:

Rebase and fee-on-transfer tokens
There are various tokens with custom implementations of how user balances are updated over time. The most common of these are fee-on-transfer and rebase tokens. Due to the complexity and cost of persistent accounting we don't intend to support these tokens.

If the protocol uses non-fee-on-transfer tokens (e.g. USDC) but someday these tokens upgrade to fee-on-transfer tokens, transactions may fail and the protocol will lose tokens to compensate users for the execution.

Another scenario is, if Alice creates a long call option and Bob takes it by calling fillOrder, then the token is upgraded to fee-on-transfer token. In this case, Alice can call exercise to put order.strike amount into the protocol, but Bob may be unable to call withdraw to get order.strike amount due to the fee-on-transfer token.

Proof of Concept

The protocol will receive tokens in fillOrder, it should record amounts as afterAmount - beforeAmount rather than order.strike directly.

Another scenario is, if Alice creates a long call option and Bob takes it by calling fillOrder, then the token is upgraded to fee-on-transfer token. Now Alice can call exercise to put order.strike amount into the protocol: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L436

But Bob may be unable to call withdraw to get order.strike amount, or the protocol will lose a part of feeAmount: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L503

Tools Used

None

Recommended Mitigation Steps

Token Transfer Calculation Accuracy in this article: https://blog.chain.link/defi-security-best-practices/

outdoteth commented 2 years ago

All of the risks of not supporting fee-on-transfer tokens are already implicitly acknowledged in the README: https://github.com/outdoteth/putty-v2#rebase-and-fee-on-transfer-tokens

HickupHH3 commented 2 years ago

dup of #21

code-423n4 / 2022-06-putty-findings