Audit Fix 1: RateChangeQueue clearing behavior

[ZenGround0]

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.

[Chaitu-Tatipamula]

can i take this up, happy to put up a PR for Option A if you'd like.

[BigLep]

For the record, this came out of #253 but wasn't deemed gritical for GA, which is why it has an MX priority.

[wjmelements]

This check is redundant:

require(queue.head < c.length, "Queue is empty");

Because solidity will do bounds checking on the next line:

RateChange memory change = c[queue.head];

If we wanted a custom error we can make the subsequent access unchecked

[wjmelements]

overflow these 256 bit ints if we ran for the age of the universe.

overflowing is less of a concern than collision, though both are improbable

[wjmelements]

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.

The main benefit of not letting it grow is clearing out head. We would prefer to reduce accumulated storage. An empty queue should be empty. So, I would prefer to replace the isEmpty check to fix the clearing behavior.

[ZenGround0]

We would prefer to reduce accumulated storage. An empty queue should be empty. So, I would prefer to replace the isEmpty check to fix the clearing behavior.

I think it would be slightly better to do this on zeroing out after finalization here so we can avoid the unnecessary checking during every settlement.