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

5 stars 0 forks source link

QA Report #333

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago
  1. batchFillOrder does not work properly as it does not have payable mutability. Also there is no unit test for this function
  2. In fillOrder function and exercise function, the case when the order base asset is not WETH and the msg.value > 0 makes user losing ether. Consider transfers the ether amount not needed back to users, or let users withdraw it.
  3. Consider using safeMint in fillOrder in case the maker or taker is a contract to make sure the contract is aware of the position minted
outdoteth commented 2 years ago

In fillOrder function and exercise function, the case when the order base asset is not WETH and the msg.value > 0 makes user losing ether. Consider transfers the ether amount not needed back to users, or let users withdraw it.

Duplicate: Native ETH can be lost if it’s not utilised in exercise and fillOrder: https://github.com/code-423n4/2022-06-putty-findings/issues/226

Consider using safeMint in fillOrder in case the maker or taker is a contract to make sure the contract is aware of the position minted

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: https://github.com/code-423n4/2022-06-putty-findings/issues/327

HickupHH3 commented 2 years ago

disagree with (1). It's supposed to not have payable because otherwise msg.value check will be looped. Invalidating this report as (2) and (3) are upgraded to med severity dups.