Offers could exist below MinimumTotalValue

[pengocat]

Description

It seems like the intention of MinimumTotalValue is that all offers are above this value. However, there can be a scenario where this is not true. (comment from code: @notice Sets the minimumTotalValue value. All offers in existence must have a total value greater than this.)

Scenario

When the COO updates MinimumTotalValue, there is no check to see if there are existing bids that are below the new updated valued of MinimumTotalValue. This can create a situation where there are valid bids in the system that were created based on the old MinimumTotalValue, and the new bids don't satisfy the criteria of being higher than MinimumTotalValue.

The comment in OffersConfig.sol for function setMinimumTotalValue says 'All offers in existence must have a total value greater than this.' This may not hold in this situation.

Impact

Medium/Low

Reproduction

Set the MinimumTotalValue to say 0.0001 ETH. Create a bid for 0.0002 ETH for a token. Set MinimumTotalValue to 0.0005. There's now a valid bid in the system that falls below MinimumTotalValue.

Fix

When the COO calls this function, there should be a check on all active and valid bids. If these bids fall below the new MinimumTotalValue, they should be canceled.

[sunsetlover]

Think that’s just a poor choice of words in the comment. I assume this check just ensures that the eth coming in the contract = the eth coming out. Therefore, the comment only needs to hold at the time an offer is created. Plus it would be impossible to iterate through all current bids on chain to check this condition because you can’t grab which cats have offers and even if you could that would be an insane amount of gas.

[ghost]

@pengocat You state that .setMinimumTotalValue() should iterate over all current bids in existence.

However, that asks a Solidity function to iterate over an array of unbounded size. That very quickly causes the function to be impossible to call, since iterating over an array that is large enough will take up more gas than exists in any Ethereum gas block. Thus, no matter how high of a gas price CK sets, it will become impossible to call .setMinimumTotalValue() ever again. It is never good practice to expose a function to the vulnerability of being forced to either iterate over an unbounded array or throw.

There exist some solutions to this problem when an unbounded array is absolutely needed (see below), but those are not appropriate in this situation:
https://ethereum.stackexchange.com/questions/32467/dealing-with-unbounded-loops

[pengocat]

@sunsetlover @michaelKim4736 it is not about the choice of words, which is pretty clear. It is about the intent of the function. I checked to see where all this is used, and the scenario that I outlined doesn't actually break anything in the smart contracts. For example, if there was a check before accepting a bid that it was less than MinimumTotalValue, that would be an issue. However, there are no such checks.

My guess is, the intent of the check is to stop some spammy bids. This is a fair use of this function. I just wanted to bring it to your attention that the assumption that there are therefore no bids below this 'spam threshold' is not true. No such assumptions should be made e.g. while designing the product.

Iterating over an unbounded array is most certainly not needed to 'fix' this, as long as the function's use never makes the assumption that I outlined. Therefore the headline of the issue as well - it is always possible for offers to exist that are below the threshold, so that assumption should not be made in a) future smart contracts b) product decisions (e.g. filtering bids on the frontend below this threshold, say).

[hwrdtm]

Thanks @pengocat for your feedback.

You are right in that one of the reasons for introducing the minimumTotalValue was to "stop some spammy bids". The documentation associated with minimumTotalValue can probably be worded better to reflect that, yes, it is true that some offers in existence will be below the threshold after this parameter has been changed.

When there are such offers in existence, a concern is whether the Offers contract can take more than S * P from the Offer total. This is prevented as we store the offerCut and the unsuccessfulFee upon offer creation.

Thank you, though, for bringing our attention towards the documentation.