RTokenAsset price estimation accounts for margin of error twice

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/RTokenAsset.sol#L53-L72
https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/RTokenAsset.sol#L100-L115

Vulnerability details

RTokenAsset estimates the price by multiplying the BU (basket unit) price estimation by the estimation of baskets held (then dividing by total supply).
The issue is that both BU and baskets held account for price margin of error, widening the range of the price more than necessary.

Impact

This would increase the high estimation of the price and decrease the lower estimation.
This would impact:

• Setting a lower min price for trading (possibly selling the asset for less than its value)
• Preventing the sell of the asset (lotLow falling below the min trade volume)
• Misestimation of the basket range on the 'parent' RToken

Proof of Concept

• Both tryPrice() and lotPrice() use this method of multiplying basket unit price by basket range then dividing by total supply
• BU price accounts for oracle error
• As for the basket range - whenever one of the collaterals is missing (i.e. less than baskets needed) it estimates the value of anything above the min baskets held, and when doing that it estimates for oracle error as well.

Consider the following scenario:

• We have a basket composed of 1 ETH token and 1 USD token (cUSDCv2)
• cUSDCv2 defaults and the backup token AAVE-USDC kicks in
• Before trading rebalances things we have 0 AAVE-USDC
• This means that we'd be estimating the low price of the ETH we're accounting for margin of error at least twice:
  • Within the basketRange() we're dividing the ETH's low price by buPriceHigh
  • Then we multiply again by buPriceLow

(there's also some duplication within the basketRange() but that function isn't in scope, what is is scope is the additional margin of error when multiplying by buPriceLow).

Recommended Mitigation Steps

I think the best way to mitigate this would be to use a dedicated function to estimate the price, I don't see an easy way to fix this while using the existing functions.

Assessed type

Other

[c4-judge]

thereksfour marked the issue as primary issue

[tbrent]

Currently contemplating switching BasketHandler.price()/lotPrice() to return a point estimate, since it is only ever used by RTokenAsset and RecollateralizationLib to back out a UoA value to a BU value.

(or equivalently, using the basketHandler.price() midpoint)

[tbrent]

@0xA5DF on further thought I'm not so sure this is a bug, or at least, I don't think one could do better. Consider the following:

• When an RToken is 100% collateralized (or expects to regain it), basketRange().top == basketRange().bottom. But there still needs to be uncertainty associated with the RToken price. It doesn't make sense for the RToken price estimate to be a single point estimate given there are price uncertainties associated with the backing tokens.
• The behavior you're describing only occurs when basketRange() has a non-zero delta between top and bottom. The delta exists due to potential clearing prices during the trading that will occur on the way to recollateralization. After all that occurs, there is then an additional uncertainty that comes from pricing the tokens that will eventually back the RToken. So it seems right to me to take the oracleError into account twice, for balances that are expected to be traded.

As for the impact statements, there are a few things I'd point out:

- Setting a lower min price for trading (possibly selling the asset for less than its value)
- Preventing the sell of the asset (lotLow falling below the min trade volume)
- Misestimation of the basket range on the 'parent' RToken

1. Any RToken sitting in the BackingManager is dissolved as a first step before rebalance() trading. RToken will therefore never be bought or sold by the BackingManager, only ever by the RevenueTraders, and RevenueTraders do not pay attention to minTradeVolume.
2. The trading mechanisms are intentionally resilient to under/over-pricing. Batch auctions have good price discovery as long as there is competition, and the dutch auctions will cover the entire distance between the "best price" and "worst price" for the pair, and then some. The impact for dutch auctions would be less precision in the overall clearing price due to larger drops in price per-block. The degree to which it can be said that the asset was sold for less than its true value is thus extremely small, and as implied by point 1 this can only happen for revenue auctions.

[0xA5DF]

So it seems right to me to take the oracleError into account twice, for balances that are expected to be traded.

I agree there's some sense to it, but:

• The required trading can be a very small percentage of the total basket value, e.g. we have 99 cUSDC and 1 aUSDC and the aUSDC is the one failing. In this case only 1% will be traded while we account for an oracle error for the whole basket.
• Notice that the same thing happens upwards, i.e. we account for the oracle error twice when calculating the high price. Do we expect to get more value by trading? we might argue that yes, but I think most cases we lose some value by trading (though I'm not sure what's the impact of the high price being to high)

Any RToken sitting in the BackingManager is dissolved as a first step before rebalance()

My understanding was that RTokenAsset is for cases when you have one RToken that holds another RToken as an asset, if this isn't the case then I agree this isn't relevant for rebalancing.

The trading mechanisms are intentionally resilient to under/over-pricing.

I agree the mechanism will work well for most of the time, but during busy and high gas price periods this might fail and this is when you need the minimum price to kick in.
Also notice that Dutch trades might have less participants when selling a high volume since it requires to buy the whole batch at once (iinm I read somewhere in the docs that this is the reason we need EasyAuction as well), this is also a case where you need the min price protection mechanism.

[tbrent]

Good points. I was ignoring the fact that RTokens may hold other RTokens as assets. In the case of CurveStableRTokenMetapoolCollateral, it's also possible for an RToken to hold an LP token for a pool that contains a different RToken as one of its tokens. Currently for example this pool is a collateral token in the RToken hyUSD.

The point about the uncertainty being applied to the entire basket because of the use of basketsHeld.bottom is good as well. If the basket is DISABLED or directly after it is changed, for example, this value would be 0, so the uncertainty would be applied to all token balances. This is something we were aware of in the context of a single RToken iteratively recollateralizing (because each step raises basketsHeld.bottom, decreasing uncertainty) but it's true that when it comes to one RToken pricing another RToken it seems like it could lead to poor behavior.

For upwards pricing it feels like less of a concern to me, because range.top is bounded at rToken.basketsNeeded().

It's worth noting though that all this discussion has been in the absence of any RSR stake. In practice all RTokens are overcollateralized by RSR. If the overcollateralization is at least 2%, and the avg oracleError for the collateral tokens is 1%, then ~no double counting occurs because range.top ~= range.bottom. Only ETH+ today has such a low overcollateralization; the other 4 RTokens listed on register.app are overcollateralized 7-24%. Still, the protocol should function well when the Distributor is set up with 0% of revenue going to stakers.

Thoughts on ways this issue could be mitigated? I thought I had an idea but after I looked into it more I don't think it would work.

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[0xA5DF]

Thoughts on ways this issue could be mitigated?

Maybe the most simple solution would be to calculate the total value of the assets that the protocol holds (capped to BU price), and then multiply by baskets needed and divide by totalSupply.

I was thinking of modifying RecollateralizationLibP1.basketRange() to calculate the price rather than baskets the protocol holds, but I think it'd just be a more complicated way to calculate the above.

[tbrent]

Maybe the most simple solution would be to calculate the total value of the assets that the protocol holds (capped to BU price), and then multiply by baskets needed and divide by totalSupply.

The issue with an approach like this is that it's agnostic of where we are in the collateralization process. Balances that are disjoint with the current basket are treated the same as balances that are overlapping. This really comes down to the question of when and where to apply maxTradeSlippage and subtract out minTradeVolume, which is what the current basketRange() implementation aims to do.

[tbrent]

We've discussed internally and where we're coming down is that we think this issue should be acknowledged but that it is a Medium and not a High. We want to acknowledge the issue because while we were aware of the double-counting of oracleError in the context of a single RToken pricing itself, we hadn't considered it in the context of a parent-child relationship, and in that case it is importantly different. However, it seems more like a Medium because the trading mechanisms are resilient to mild mispricing. The expected downside outcome would be a trade occuring via DutchTrade and the block-by-block price dropping faster than necessary, possibly resulting in more slippage, but this would likely be very small and on the order of ~0.1%, and only for the impacted balance held in the child RToken.

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report