search

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

5 stars 0 forks source link

both order side positions don’t exist is not checked #345

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The flow in fillOrder() as documented in https://github.com/code-423n4/2022-06-putty/blob/main/contracts/spec/fillOrder.md expects the check order side positions doesn't exist . However the fillOrder() function does not have a require check for this. This allows for the same side positions to be created multiple times leading to loss of funds or assets for either maker or sender.

Proof of Concept

  1. Call fillOrder()
  2. side positions created for order.maker and msg.sender
  3. call flows into transfers of premium(to msg.sender) and strikes (to contract)
  4. call flows into transfers of assets from either maker or taker to the contract
  5. fillOrder() is called again, since there is no check if side positions already exist, it flows into step 2.
  6. step 3 and 4 are repeated, leading to transfers for positions the maker or sender already owns.

Tools Used

Manual

Recommended Mitigation Steps

As documented, there should be a check for both order side positions doesn't exist.

kirk-baird commented 2 years ago

This case is covered in _mint() which ensures the ID hasn't already been minted. The ID is the order hash and so is unique for each order https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L13

outdoteth commented 2 years ago

confirming what @kirk-baird said.