sherlock-audit / 2022-09-knox-judging

0 stars 0 forks source link

hansfriese - `OrderBook._remove()` doesn't update `index.length` properly. #144

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

hansfriese

medium

OrderBook._remove() doesn't update index.length properly.

Summary

OrderBook._remove() doesn't update index.length properly.

Vulnerability Detail

OrderBook._remove() doesn't update index.length properly.

OrderBook._remove() is used to remove the existing order from the tree and index.length should be 0 when the original tree is empty.

index.length = index.length > 0 ? index.length - 1 : 1;

Impact

OrderBook._remove() updates index.length wrongly and _length() will return the wrong result.

Code Snippet

https://github.com/sherlock-audit/2022-09-knox/blob/main/knox-contracts/contracts/auction/OrderBook.sol#L240

Tool used

Manual Review

Recommendation

We should change this line like below.

index.length = index.length > 0 ? index.length - 1 : 0;
0xCourtney commented 1 year ago

The AVLtree is 1 indexed by design.