FilOzone / FilOzone/filecoin-pay

Audit Fix L06: RateChangeQueue clearing behavior

Open
#266 7 comments 0 reactions 1 assignee Claimed by @wjmelements View on GitHub
bug
Dominant language
Solidity
Stars
8
Forks
13
PR merge metrics
No merged PRs in 30d

Description

Audit ref: `[FIL-1132b525-L06]`

PR #86 introduced unreachable code. Upon dequeue of the rate change queue down to zero elements we try to reset the array completely with some fancy evm assembly zeroing out the changes dynamic array slot.

The reason for this as far as I can tell is code cleanliness. Super-theoretically it makes overflowing the head and changes.length slots easier to reason about. But reasonably-theoretically we could never overflow these 256 bit ints if we ran for the age of the universe.

Either removing this case entirely and letting the array's length grow without resetting to zero OR doing head++ first and then checking isEmpty condition are acceptable. I lean towards just letting array grow to remove a tiny bit of code from the expensive settlement operation.

Note that if KAMT behaved like the naive AMT datastructure used in other places in filecoin actors there would be a significant state overhead to operating with larger array sizes. There is a known inefficiency where the AMT adds a greater number of internal nodes eagerly when adding a bigger integer key than a small integer key. This is not the case with the HAMT which symmetrically handles all keys since all keys are hashed. The KAMT is a modification of the core HAMT logic and does not suffer from this inefficiency.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.