Reserve Protocol - Invitational - 0xA5DF's results

A permissionless platform to launch and govern asset-backed stable currencies.

General Information

Platform: Code4rena

Start Date: 25/07/2023

Pot Size: $80,100 USDC

Total HM: 18

Participants: 7

Period: 10 days

Judge: cccz

Total Solo HM: 15

Id: 268

League: ETH

Reserve

Findings Distribution

Researcher Performance

Rank: 6/7

Findings: 2

Award: $0.00

QA:

grade-b

🌟 Selected for report: 1

🚀 Solo Findings: 1

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Reserve

Findings Information

🌟 Selected for report: ronnyx2017

Also found by: 0xA5DF

Labels

bug

3 (High Risk)

satisfactory

sponsor confirmed

duplicate-23

Awards

Data not available

External Links

Link to GitHub issue

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/cbeth/CBETHCollateral.sol#L63

Vulnerability details

Whenever refresh() is called for a collateral it does a few checks, one of them is to ensure the collateral didn't depge. It does so by calling tryPrice() and checking that the returned parameter pegPrice (which is supposed to represent the current price of the collateral on the market) is within the limits of the peg. The issue here is that CBETHCollateral.tryPrice() always returns a constant 1 for pegPrice, meaning a depeg event wouldn't be detected.

Impact

Asset wouldn't be marked as iffy/disabled in case of a depeg, one of the impacts of that is that issuance would still be possible during a depeg. This would cause a loss of assets, since when the protocol would attempt to cover the deficit caused by the depeg it'd have a bigger deficit to cover for (this would probably come at the expense of RSR stakers or other RToken holders, depending on the case).

Proof of Concept

tryPrice() returns targetPerRef for pegPrice:

    function tryPrice()
        external
        view
        override
        returns (
            uint192 low,
            uint192 high,
            uint192 pegPrice
        )
    {
        // {UoA/tok} = {UoA/ref} * {ref/tok}
        uint192 p = chainlinkFeed.price(oracleTimeout).mul(
            refPerTokChainlinkFeed.price(refPerTokChainlinkTimeout)
        );
        uint192 err = p.mul(oracleError, CEIL);

        high = p + err;
        low = p - err;
        // assert(low <= high); obviously true just by inspection

        pegPrice = targetPerRef(); // {target/ref} ETH/ETH is always 1
    }

targetPerRef always returns 1:

    /// @return {target/ref} Quantity of whole target units per whole reference unit in the peg
    function targetPerRef() public view virtual returns (uint192) {
        return FIX_ONE;
    }

Recommended Mitigation Steps

Use an oracle for pegPrice

Assessed type

Other

#0 - c4-judge
2023-08-05T09:31:14Z

thereksfour marked the issue as primary issue

#1 - c4-sponsor
2023-08-08T19:43:56Z

tbrent marked the issue as sponsor confirmed

#2 - c4-judge
2023-09-05T06:13:03Z

thereksfour marked the issue as satisfactory

#3 - c4-judge
2023-09-05T06:13:07Z

thereksfour marked the issue as selected for report

#4 - c4-judge
2023-09-05T13:48:06Z

thereksfour marked the issue as not selected for report

#5 - c4-judge
2023-09-05T13:48:20Z

thereksfour marked the issue as duplicate of #23

Findings Information

🌟 Selected for report: 0xA5DF

Labels

bug

2 (Med Risk)

disagree with severity

downgraded by judge

primary issue

satisfactory

selected for report

sponsor acknowledged

M-03

Awards

Data not available

External Links

Link to GitHub issue

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

#0 - c4-judge
2023-08-05T10:20:49Z

thereksfour marked the issue as primary issue

#1 - tbrent
2023-08-09T15:44:46Z

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)

#2 - tbrent
2023-08-09T23:24:44Z

@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

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.
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.

#3 - 0xA5DF
2023-08-10T19:23:33Z

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.

#4 - tbrent
2023-08-10T21:14:45Z

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.

#5 - c4-sponsor
2023-08-10T21:14:50Z

tbrent marked the issue as sponsor acknowledged

#6 - 0xA5DF
2023-08-11T15:35:46Z

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.

#7 - tbrent
2023-08-11T17:05:44Z

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.

#8 - tbrent
2023-08-11T17:11:48Z

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.

#9 - c4-sponsor
2023-08-11T17:11:53Z

tbrent marked the issue as disagree with severity

#10 - c4-judge
2023-08-14T04:33:27Z

thereksfour changed the severity to 2 (Med Risk)

#11 - c4-judge
2023-09-05T06:28:18Z

thereksfour marked the issue as satisfactory

#12 - c4-judge
2023-09-05T06:28:22Z

thereksfour marked the issue as selected for report

Findings Information

🌟 Selected for report: auditor0517

Also found by: 0xA5DF, RaymondFam, bin2chen, carlitox477, ronnyx2017

Labels

bug

grade-b

QA (Quality Assurance)

Q-03

Awards

Data not available

External Links

Link to GitHub issue

`RTokenAsset.price()` should return FIX_MAX if this is the BU price

When the basketHandler.price() high price is FIX_MAX then the returned high price should be FIX_MAX as well. This isn't always the case since the BU price is then multiplied by the basket range and divided by total supply, in case that baskets needed is less than total supply (i.e. there was a previous haircut) then the returned value might be lower than FIX_MAX.

This might cause issues when trying to detect a faulty asset by the returned price value.

Replace the `assert()` at `RTokenAsset.tryPrice()` with a string or custom error

When assert reverts it reverts with an empty error, this means that when this is called from price() it'd be mistaken to be a revert due to out of gas error. Therefore it's best to replace it with a non-empty revert.

`STATIC_ATOKEN_IMPL` might be too long for a symbol

StaticATokenLM uses STATIC_ATOKEN_IMPL as the ERC20 symbol. While I'm not aware of any issue with using 18 characters for a symbol, most ERC20s use 3-6 characters, so it might be better to follow the common length and shorten the symbol.

Compound V3’s accrue account runs after the balance check

code The check for the balance check is done before calling accrueAccount(), so there might be a case where the user would have enough balance once accrueAccount() is called, but the function would revert because the balance is checked beforehand.

Side notes:

Changing this would break CEI pattern
My assumption that accrueAccount() increases user's balance might be wrong, I'm not very familiar with the way Compound V3 works.

#0 - c4-judge
2023-08-06T10:17:21Z

thereksfour marked the issue as grade-b

Reserve Protocol - Invitational - 0xA5DF's results

A permissionless platform to launch and govern asset-backed stable currencies.

General Information

Findings Distribution

Researcher Performance

External Links

[H-01] cbETH's fails to check for a depeg since `pegPrice` is always 1 - $0.00

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

#0 - c4-judge2023-08-05T09:31:14Z

#1 - c4-sponsor2023-08-08T19:43:56Z

#2 - c4-judge2023-09-05T06:13:03Z

#3 - c4-judge2023-09-05T06:13:07Z

#4 - c4-judge2023-09-05T13:48:06Z

#5 - c4-judge2023-09-05T13:48:20Z

🚀 [M-03] `RTokenAsset` price estimation accounts for margin of error twice - $0.00

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

#0 - c4-judge2023-08-05T10:20:49Z

#1 - tbrent2023-08-09T15:44:46Z

#2 - tbrent2023-08-09T23:24:44Z

#3 - 0xA5DF2023-08-10T19:23:33Z

#4 - tbrent2023-08-10T21:14:45Z

#5 - c4-sponsor2023-08-10T21:14:50Z

#6 - 0xA5DF2023-08-11T15:35:46Z

#7 - tbrent2023-08-11T17:05:44Z

#8 - tbrent2023-08-11T17:11:48Z

#9 - c4-sponsor2023-08-11T17:11:53Z

#10 - c4-judge2023-08-14T04:33:27Z

#11 - c4-judge2023-09-05T06:28:18Z

#12 - c4-judge2023-09-05T06:28:22Z

[Q-06] QA Report - $0.00

Findings Information

Labels

Awards

External Links

RTokenAsset.price() should return FIX_MAX if this is the BU price

Replace the assert() at RTokenAsset.tryPrice() with a string or custom error

STATIC_ATOKEN_IMPL might be too long for a symbol

Compound V3’s accrue account runs after the balance check

#0 - c4-judge2023-08-06T10:17:21Z

#0 - c4-judge
2023-08-05T09:31:14Z

#1 - c4-sponsor
2023-08-08T19:43:56Z

#2 - c4-judge
2023-09-05T06:13:03Z

#3 - c4-judge
2023-09-05T06:13:07Z

#4 - c4-judge
2023-09-05T13:48:06Z

#5 - c4-judge
2023-09-05T13:48:20Z

#0 - c4-judge
2023-08-05T10:20:49Z

#1 - tbrent
2023-08-09T15:44:46Z

#2 - tbrent
2023-08-09T23:24:44Z

#3 - 0xA5DF
2023-08-10T19:23:33Z

#4 - tbrent
2023-08-10T21:14:45Z

#5 - c4-sponsor
2023-08-10T21:14:50Z

#6 - 0xA5DF
2023-08-11T15:35:46Z

#7 - tbrent
2023-08-11T17:05:44Z

#8 - tbrent
2023-08-11T17:11:48Z

#9 - c4-sponsor
2023-08-11T17:11:53Z

#10 - c4-judge
2023-08-14T04:33:27Z

#11 - c4-judge
2023-09-05T06:28:18Z

#12 - c4-judge
2023-09-05T06:28:22Z

`RTokenAsset.price()` should return FIX_MAX if this is the BU price

Replace the `assert()` at `RTokenAsset.tryPrice()` with a string or custom error

`STATIC_ATOKEN_IMPL` might be too long for a symbol

#0 - c4-judge
2023-08-06T10:17:21Z