Shortfall might be calculated incorrectly if a price value for one collateral isn't fetched correctly

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L449

Vulnerability details

Impact

Function price() of an asset doesn't revert. It returns values (0, FIX_MAX) for low, high values of price in case there's a problem with fetching it. Code that calls price() is able to validate returned values to detect that returned price is incorrect.

Inside function collateralShortfall() of RecollateralizationLibP1 collateral price isn't checked for correctness. As a result incorrect value of shortfall might be calculated if there are difficulties to fetch a price for one of the collaterals.

Proof of Concept

• https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L449

Recommended Mitigation Steps

Check that price is correctly fetched for a collateral.

[0xean]

Mitigation here is a little challenging to understand considering checking a price is hard on chain and hence the concern.

I think this issue is a bit too general, but would like further comments.

[c4-judge]

0xean marked the issue as satisfactory

[tbrent]

I think this issue is real, but it happens in a super-corner-case that I doubt the warden is thinking about.

Some related statements:

• prepareRecollateralizationTrade checks that all collateral in the basket is SOUND before calling collateralShortfall
• From docs/collateral.md: "Should return (0, FIX_MAX) if pricing data is unavailable or stale."

collateralShortfall should never reach a collateral with FIX_MAX high price in the normal flow of things.

But, it is possible for one RToken system instance to have an RTokenAsset registered for a 2nd RToken. In this case, it could be that RToken 2 contains a collateral plugin that is now connected to a broken oracle, but RToken 2 may not have recognized this yet. When RToken 1 calls RTokenAsset.price(), it could end up reverting because of overflow in this line from collateralShortfall:

shortfall = shortfall.plus(needed.minus(held).mul(priceHigh, CEIL));

So I think it's a real issue, and I would even leave it as Medium severity.

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged