search

wyvernprotocol / wyvern-v3

Wyvern Protocol v3.1, Ethereum implementation
https://wyvernprotocol.com
MIT License
298 stars 121 forks source link

Bug report for StaticMarket.sol #68

Closed jackcpku closed 2 years ago

jackcpku commented 2 years ago

https://github.com/wyvernprotocol/wyvern-v3/blob/403f866940b4ef304d24c2147bd9503e89e1cec7/contracts/StaticMarket.sol#L136-L156

The return value should not be 1 since multiple ERC20 tokens are transferred. This will cause inconsistency with the meaning of variable fills.

MarvinJanssen commented 2 years ago

Orders involving an ERC721 cannot be partially filled, hence 1. Such orders should always have a maximumFill of 1. The fills variable corresponds to order fills, not tokens transferred, although both may coincide.

jackcpku commented 2 years ago

Orders involving an ERC721 cannot be partially filled, hence 1. Such orders should always have a maximumFill of 1. The fills variable corresponds to order fills, not tokens transferred, although both may coincide.

https://github.com/wyvernprotocol/wyvern-v3/blob/403f866940b4ef304d24c2147bd9503e89e1cec7/test/7-static-market-matching.js#L617-L622

Ok, so here in line 618, maximumFill should not be buyingPrice, instead be 1

MarvinJanssen commented 2 years ago

It appears so.