Counter offer is not implemented correctly

[code423n4]

Lines of code

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

Vulnerability details

Impact

acceptCounterOffer is not verifying if the original order has already been filled. In case maker makes a counter offer and by the time counter offer is called, some user has already filled the original order then both original and counter offer will be filled. Maker will unnecessary have 2 positions (when maker only wanted 1). This means if user was incurring loss on this order then the loss would get doubled

Proof of Concept

1. Makes makes an Original order O1
2. Maker calls acceptCounterOffer with a counter order O2
3. By the time acceptCounterOffer by maker could execute, User X already called fillOrder on Order O1
4. So Order O1 gets filled and then due to counter offer order O2 also gets filled

Recommended Mitigation Steps

Add below check in acceptCounterOffer to verify if original order is not already filled

bytes32 orderHash = hashOrder(originalOrder);
require(ownerOf(uint256(orderHash)) == address(0), "Original order already filled");

[GalloDaSballo]

You cannot fillOrder of an order whose NFT was already minted, the tx will revert at _mint

[csanuragjain]

@GalloDaSballo Correct but in this case, the counter order hash should differ from the original one. An example will be user created order O1 as PUT Long with premium 1. Later he created a counter order O2 with premium 6 to lure more users. In this case hash for both orders will differ and hence _mint should work as different NFT will be given for each order. Can you please suggest

[GalloDaSballo]

The warden is showing that if you sign 2 orders:

• OrderA - Original
• OrderB - Counter

The SC has no guarantee as to whether one, or both will be filled, OrderA could be filled, then Cancelled, then B Or Order B could be filled and A cancelled.

It seems like acceptCounterOffer is calling cancel which requires the maker to call it, but then calls fillOrder that requires the taker (msg.sender) to be someone else.

I think a judge + sponsor will have to figure this out

[outdoteth]

can confirm that this is an issue

[outdoteth]

Duplicate: It’s possible to fill an order twice by accepting a counter offer for an already filled order: https://github.com/code-423n4/2022-06-putty-findings/issues/44