sherlock-audit / 2022-11-bullvbear-judging

0 stars 0 forks source link

Mukund - The code don't use nonce to identify orders, instead it uses order hash which is not best practice and could cause some issues like: duplicate nonce for different orders, can't quickly identify cancelled orders without order's all info, can't cancel multiple orders, can't cancel order without knowing order's all parameters #124

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Mukund

medium

The code don't use nonce to identify orders, instead it uses order hash which is not best practice and could cause some issues like: duplicate nonce for different orders, can't quickly identify cancelled orders without order's all info, can't cancel multiple orders, can't cancel order without knowing order's all parameters

Summary

The code don't use nonce to identify orders, instead it uses order hash which is not best practice and could cause some issues like: duplicate nonce for different orders, can't quickly identify cancelled orders without order's all info, can't cancel multiple orders, can't cancel order without knowing order's all parameters

Vulnerability Detail

It's common practice in EIP-712 encoding to use nonce to identify singer messages and prevent replay attacks and create ability to make signed messages invalid. but in bvb uses order hash for identifying orders and cancelling them. this can cause some problems like:

  1. duplicate nonce for different orders
  2. can't quickly identify cancelled orders without order's all info
  3. can't cancel multiple orders
  4. can't cancel order without knowing order's all parameters This problems and similar problems are created because order hash is not iterable and also to calculate order hash it requires all order info. But nonce can be iterable and only the nonce of orders are required to identify orders.

    Impact

  5. duplicate nonce for different orders
  6. can't quickly identify cancelled orders without order's all info
  7. can't cancel multiple orders
  8. can't cancel order without knowing order's all parameters This problems and similar problems are created because order hash is not iterable and also to calculate order hash it requires all order info. But nonce can be iterable and only the nonce of orders are required to identify orders.

    Code Snippet

    https://github.com/sherlock-audit/2022-11-bullvbear/blob/main/bvb-protocol/src/BvbProtocol.sol#L585-L597

    Tool used

Manual Review

Recommendation

use nonce for identifying orders and use order hash and signature for validating orders.

datschill commented 1 year ago

Nonce is used as a salt + OpenSea and other big protocols also use complex method to cancel (need to know the full order)