hats-finance / Tokemak-0x4a2d708ea6b0c04186ecb774cfad1e50fb5efc0b

0 stars 0 forks source link

NavTracking.sol #7

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x11469b756c7b30d3c3bac10eaeff6466d3cb8f04066b13d65c81cb492c13ecca Severity: low

Description: Description\ The insert function of the NavTracker to insert a new share based on a timestamp, handling values based on indexes of a history array, that may get upgraded unintentionally.

Attack Scenario\ At the end of the insertion, we increase the len variable of the State, in order to match the non-zero values of the history array. The condition for incrementing it is that it is less than the defined MAX size, but it is never explicitly blocked for inserting at max size. Thus even if our len reaches the max size, we can still insert values, meaning we will update the timestamp, the indexes but not the length. E.g when len finally reaches the defined MAX, we would not increment len, but the function does not revert due to size reach.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Recommendation: revert when the len reaches the MAX cap to disable insertions.

PlamenTSV commented 7 months ago

Thinking back on this, it could be MED depending on the impact of overriding values in the array.

codenutt commented 7 months ago

NavTracking is only meant to track the last 90 days of nav values to support the "Lookback Test" described in the docs. It is implemented as a ring buffer and so overwriting values is expected.