datschill
commented
1 year ago This is not a bug, it’s a feature. What if a User make an Order A with nonce = 10 and Order B = 11, and the Order B is taken before Order A. With this suggested feature, Order A wouldn’t be matchable ? That would be a problem. Our solution doesn’t seem to cause any problem, nor confusions if the purpose of our ‘nonce’ is well defined. Plus, the signature is made with the orderHash, which include _hashTypedDataV4(), which contains the chainId and smart contract address. So there is no ‘same hash problem’
zimu
medium
Nonce of Order/SellOrder does not fit its orginal meaning
Summary
Nonce of Order/SellOrder does not fit its orginal meaning. I.e., for the same user, an order B is maked and matched later than order A, but the nonce of order B can be smaller than order A‘s.
Vulnerability Detail
For example of Order,
struct Orderis defined in https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L33-L44All the parameters of Order is user-defined, including the nonce parameter that only requires greater or equal to minimumValidNonce without any self-increasing mechanism.
It would possibly make an out of time-ordered nonce sequence for an user's orders, causing some misleading sort for showing and tracking.
Impact
An out of time-ordered nonce sequence can cause some misleading sort for showing and tracking users' orders.
Code Snippet
https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L38 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L63 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L621-L627 https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L633-L639
Tool used
Manual Review
Recommendation
To add a self-increasing mechanism for
Order.nonceandSellOrder.nonce. And it is better to add a chain ID tostruct OrderandSellOrderto avoid the same hash problem if bullvbear plans to deploy on multi-chains in the future.