getGasPriceMinimum reverts when there is no rate #10932
Conversation
| @@ -175,7 +176,7 @@ contract GasPriceMinimum is | |||
| * For other addresses it returns gasPriceMinimum() mutiplied by | |||
| * the SortedOracles median of the token. It does not check tokenAddress is a valid fee currency. | |||
| * this function will never returns values less than ABSOLUTE_MINIMAL_GAS_PRICE. | |||
| * If Oracle rate doesn't exist, it returns ABSOLUTE_MINIMAL_GAS_PRICE. | |||
| * If Oracle rate doesn't exist, it reverts. | |||
There was a problem hiding this comment.
There's a question of what's the desired behavior. Sounds like in the past we intended to return ABSOLUTE_MINIMAL_GAS_PRICE (defined to be 1 wei).
- If that is the desired behavior, the fix should be instead to return 0 from the function above, instead of reverting.
- But likely it's a good idea to not return such a tiny gas price, in which case sounds like we're good to remove any references to
ABSOLUTE_GAS_PRICE_MINIMUM, including themaxcalculation below.
I guess the one thing to check is how would the celo-blockchain client react to a revert here?
There was a problem hiding this comment.
I think reverting is ok.
But sure, need to check what happens on the client
There was a problem hiding this comment.
The problem is that it can technically be 0 even for whitelisted tokens that do not use adapter. This is not yet supported by client but in long run it should be. Then it is up to user whether they will choose to pay with this token even though it is more expensive.
Personally I would keep the revert.
There was a problem hiding this comment.
I agree we should revert (like we've been doing, now it'd be more explicit).
@marekolszewski The comment it's a typo, should have been how Pavel left it now.
| @@ -165,6 +165,7 @@ contract GasPriceMinimum is | |||
| uint256 rateNumerator; | |||
| uint256 rateDenominator; | |||
| (rateNumerator, rateDenominator) = sortedOracles.medianRate(tokenAddress); | |||
| require(rateDenominator > 0, "Token has no rate"); | |||
There was a problem hiding this comment.
wouldn't it be cleaner for medianRate to do this require? If not we make every caller need to make this check
There was a problem hiding this comment.
actually... checking sorted oracles. var names looks odd
it should be:
rate, numOfRates =
There was a problem hiding this comment.
I moved it to medianRate and also renamed variable to medianRate and denominator
There was a problem hiding this comment.
wouldn't it be cleaner for
medianRateto do this require?
I think it makes sense for the oracle to have a non-reverting version of this function so consumers can decide what to do in the case of a missing rate.
There was a problem hiding this comment.
After checking blockchain client code:
Our options are:
- Revert when no medianRate exists - problem is that exchange rate of fee currency is consider 1:1 to CELO which will cause user to pay less/more than what they agreed to
- Keep it as it is (== returning 0) - problem is that if we whitelist new fee currency, but forget to add oracle feed, then we will panic the client since we are dividing by 0 and it has potential to shutdown whole Celo network
- Return uint256.max - which would make paying by this feeCurrency extremely expensive - we need to check whether there are no potential overflows
We agreed with @karlb to go with 3. for now
| @@ -175,7 +176,7 @@ contract GasPriceMinimum is | |||
| * For other addresses it returns gasPriceMinimum() mutiplied by | |||
| * the SortedOracles median of the token. It does not check tokenAddress is a valid fee currency. | |||
| * this function will never returns values less than ABSOLUTE_MINIMAL_GAS_PRICE. | |||
| * If Oracle rate doesn't exist, it returns ABSOLUTE_MINIMAL_GAS_PRICE. | |||
| * If Oracle rate doesn't exist, it reverts. | |||
There was a problem hiding this comment.
I think reverting is ok.
But sure, need to check what happens on the client
| (rateNumerator, rateDenominator) = sortedOracles.medianRate(tokenAddress); | ||
| return ((gasPriceMinimum() * rateNumerator) / rateDenominator); | ||
| (uint256 medianRate, uint256 denominator) = sortedOracles.medianRate(tokenAddress); | ||
| return ((gasPriceMinimum() * medianRate) / denominator); |
There was a problem hiding this comment.
Could be a good opportunity to replace this line for a remove https://github.com/celo-org/celo-monorepo/blob/release/core-contracts/11/packages/protocol/contracts/stability/SortedOracles.sol#L185
(reported here)
Description
Readable error message instead of panic when no medianRate for token exists
Addresses: https://github.com/celo-org/audit-a-findings/issues/8