fix: check supply queue size

[Jean-Grimal]

Fixes #217

I wondered if it deserved a natspec comment (on why we bound the size of the supply queue), but since the error (which is MaxQueueSizeExceeded) is already very explicit, I thought it wasn't worth it.

wdyt ?

[MerlinEgalite]

to me it's enough

[QGarchery]

Just thought about this but actually this isn't enough to ensure that the supply queue does not go above the max size. This is because the _setCap also pushes an element to the queue. Should we also revert in this case ? Or should we remove that feature from _setCap ?

[MerlinEgalite]

Just thought about this but actually this isn't enough to ensure that the supply queue does not go above the max size. This is because the _setCap also pushes an element to the queue. Should we also revert in this case ? Or should we remove that feature from _setCap ?

Are you sure? We're checking that the withdrawQueue length is not greater than MAX_QUEUE_SIZE. And to me the supplyQueue is a subset of withdrawQueue.

[MerlinEgalite]

Could be a good invariant btw

[QGarchery]

Are you sure? We're checking that the withdrawQueue length is not greater than MAX_QUEUE_SIZE. And to me the supplyQueue is a subset of withdrawQueue.

Yes I think so, here is a counter-example:

1. Suppose there is only one market with non-zero cap at the start, call it M1.
   So supplyQueue = [M1], same as withdrawQueue.
2. Then an allocator changes the supply queue to be of max size:
   supplyQueue = [M1, M1, ..., M1] and we still have withdrawQueue = [M1].
3. Now another market gets whitelisted, call it M2.
   So we get withdrawQueue = [M1, M2], which is ok because it's less than the max size. But we also have supplyQueue = [M1, M1, ..., M1, M2] which is bigger than the max size

[MerlinEgalite]

Oh yes you can have duplicates in supplyQueue that's unfortunate... So we should enforce it for both I think in _setCap

[Jean-Grimal]

I'm not sure we should do it for both. IMO we shouldn't revert when enabling a market only because there is duplicates in the supply queue, although MAX_QUEUE_SIZE hasn't been reached for withdraw queue.

Maybe we should just also forbid duplicates in the supply queue. What is the gas cost of doing so ?

[QGarchery]

I'm not sure we should do it for both. IMO we shouldn't revert when enabling a market only because there is duplicates in the supply queue, although MAX_QUEUE_SIZE hasn't been reached for withdraw queue.

I disagree with omitting the check in _setCap: the purpose is to ensure that allocators cannot grief deposits. So either do both checks to ensure that, or do no check and trust allocators.

Maybe we should just also forbid duplicates in the supply queue.

I believe this is still not enough for this issue, because the supply queue can contain markets that were previously enabled but have now been removed from the withdraw queue

[MerlinEgalite]

Can we update this branch with the second check?

outdated