Putty contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L573

Vulnerability details

Impact

When a user accepts a counter offer, he does not want that his original order is also exercised. In certain cases (floor options, options for ERC20 tokens), this could lead to situations where a user pays two times or has to deliver the underlying two times.

Proof of Concept

Bob posts a short put floor option for 2 Bored Apes offchain with a strike of 100 WETH. He then gets a counter offer from attacker Alice (same parameters, but a long put). However, just before he calls acceptCounterOffer, Alice calls fillOrder with the order from Bob. The acceptCounterOffer and fillOrder goes through and 200 WETH are deducted from Bob, although he only intended to go short on one option with 100 WETH.

Recommended Mitigation Steps

Consider checking if the order that will be cancelled was already filled. If that is the case, acceptCounterOffer should fail. This would also increase the utility of this function, as it would be different to a simple cancel and fillOrder (which a user could also do by himself).

Judge has assessed an item in Issue #45 as Medium risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

When order.strike and fee are both low values, (order.strike * fee) / 1000 can round down to 0, meaning feeAmount will be 0. A transfer of 0 fails for some ERC20 tokens (https://github.com/d-xo/weird-erc20), meaning that withdraw will never succeed. While the consequences would be severe (no withdrawal possible), the chances are relatively low in my opinion (combination of a very low strike price or a token with very little decimals, a low fee, and a token that treats 0 transfers in a weird way), so I rate the issue as medium.

Proof of Concept

For instance with order.strike = 950 and fee = 1, we have: feeAmount = (950 * 1) / 1000 = 0.

Recommended Mitigation Steps

Consider only initiating the transfer when the value is larger than 0. This is also beneficial if the token does not revert on a zero transfer, as it saves gas in such a case.

• There is no way to withdraw assets by only passing the NFT ID and without passing the order details. While this seems to be a delaborate design decision, I would prefer a function to do so. Imagine that those NFTs are traded on a secondary market (which makes a lot of sense for the use case, as the trading volume for options on secondary markets is usually high). A user buys an NFT and only has the ID. To withdraw, he now has to have all of the parameters of the original Order, although some of them do not even matter for his buying decision (e.g., he should not have to care which nonce was used for the original Order).
• Setting balanceOf to type(uint256).max is a deliberate design decision according to the docs, but can have negative consequences for NFT markets where the Putty NFTs are traded. If they use balanceOf internally (because they assume that the ERC721 standard is implemented according to the specifications), transfers may fail or the behaviour of the market place may be wrong.
• Inconsistency: In _transferERC20sIn, it is checked if the token is a contract, which is not done for _transferERC721sIn and _transferFloorsIn. Consider to check this either for all or none of the transfers.
• With regards to fees, we have the following possibilities: - Put option exercised: No fee deducted, whole strike transferred to exerciser - Put option expired: Fee deducted from payout of short - Call option exercised: Fee deducted from payout of short - Call option expired: No fee deducted While this is probably intended like that, it would be good to document this behavior, because there are other possibilites. My initial assumption, before reading the code, was that there is also a fee deducted from the exerciser when a put option is exercised.
• While it is mentioned that tokens which deduct a fee are not supported by the protocol, you could add a check (balance is increased by intended amount after transfer) to avoid that they are used. Currently, it would not be possible to exercise / withdraw because the transfer would fail, i.e. the tokens would be stuck.

• There are multiple places where the length of the array can be cached before the for loop and the i++ can be unchecked to save gas:
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L556
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L594
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L611
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L627
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L637
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L647
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L658
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L670
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L728
  • https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L742
• The subtraction https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L503 can be unchecked, feeAmount is per definition always lower than order.strike.