Putty contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

In the EIP721, onERC721Received is a safeguard to avoid NFTs to be stuck on contracts. But here it could turn out to actually create this situation: it may happen that the NFTs are stuck on PuttyV2 if a user uses a contract not suited to receive NFTs to create and fill orders.

Proof of Concept

An example scenario would be:

• Alice uses a contract to create a short put option.
• Bob fills the order. A long NFT is minted to Alice's contract but there is no check
• Bob exercise the option
• Alice cannot withdraw: here contract cannot receive NFTs

Recommended Mitigation Steps

Check in the appropriate case if the maker or msg.sender is a contract and if it is the case, if they can receive NFTs

Lines of code

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

Vulnerability details

Impact

Fees are applied during withdraw, but can change between the time the order is filled and its terms are agreed upon and the withdrawal time, leading to a loss of the expected funds for the concerned users.

Proof of Concept

The scenario would be:

• Alice and Bob agrees to fill an order at a time fees are 0.1%
• During the duration of the option, fees are increased to 3%
• At withdrawal they'll pay 3% of the strike, although they wouldn't have created the order in the first place with such fees

Recommended Mitigation Steps

Mitigation could be:

• Store the fees in Order and verify that they are correct when the order is filled, so they are hardcoded in the struct
• Add a timestamp: this wouldn't fully mitigate but would still be better than the current setup
• Keep past fees and fee change timestamps in memory (for example in an array) to be able to retrieve the creation time fees at withdrawal

[Low - 01] - Incorrect comment

Here it says:

"It fills the counter offer, and then cancels the original order that the counter offer was made for”.

In the code it's actually the opposite:

"It cancels the original order the counter offer was made for, and then fills the counter offer.”.

[Gas - 01] - Use Custom Errors

Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors: see https://blog.soliditylang.org/2021/04/21/custom-errors/. This would save both deployment and runtime cost.

[Gas - 02] - Directly settle short orders in exercise

In exercise, why not directly settling the deal ?

The gas overhead would be minimal has the transfers are already done, and you’d save a ton of gas in the overall flow. Instead of doing a 2 time transfer msg.sender -> contract during exercise and then contract -> maker or taker during withdraw, you could directly do the accounting, burn the position and do the correct transfers in exercise. There would still be a withdraw function, but only for non-exercised options.

Especially as the order is immutable, we already know during exercise than withdraw will be called, so this optimisation is worth doing to me, although it requires some work.