`BasketHandler::price` returns the wrong lower end of the price estimate

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/libraries/Fixed.sol#L503 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L429-L431

Vulnerability details

Impact

And the users/Integrators using BasketHandler::price to get the price of a BU will read the wrong lower end of the price estimate of teh rToken.

Proof of Concept

Fixed::safeMul should return FIX_MAX if eitehr a r b == FIX_MAX. And it does, but when b == 0, a will be FIX_MAX says the comment. And if b == 0 the previous line itself will return 0.

So the code logic to return FIX_MAX if a == FIX_MAX, b == 0 will never happen. Instead flip the lines, so that the first validation of a,b is done with FIX_MAX and not 0.

BasketHandler::price has a flow where lowP == 0, highP == FIX_MAX and it calls low256 += qty.safeMul(lowP, FLOOR);. So here a == qty, b = lowP. And in a case where qty = FIX_MAX, lowP = 0, the safeMul should return FIX_MAX but instead it returns 0. So, the BasketHandler::price calcualtion of final lowP is malicious.

Fixed::safeMul

    function safeMul(
        uint192 a,
        uint192 b,
 RoundingMode rounding
 ) internal pure returns (uint192) {
        // untestable:
        //      a will never = 0 here because of the check in _price()
        if (a == 0 || b == 0) return 0;
        // untestable:
 @@   //      a = FIX_MAX iff b = 0 
        if (a == FIX_MAX || b == FIX_MAX) return FIX_MAX;

----------------------

 }

BasketHandler::price

    function price(bool applyIssuancePremium) public view returns (uint192 low, uint192 high) {
        uint256 low256;
        uint256 high256;

        uint256 len = basket.erc20s.length;
        for (uint256 i = 0; i < len; ++i) {
            try assetRegistry.toColl(basket.erc20s[i]) returns (ICollateral coll) {
                uint192 qty = _quantity(basket.erc20s[i], coll, CEIL);
                if (qty == 0) continue;

 @@          (uint192 lowP, uint192 highP) = coll.price();

 @@          low256 += qty.safeMul(lowP, FLOOR);

Tools Used

Vs code

Recommended Mitigation Steps

Modify Fixed::safeMul

 function safeMul(
 uint192 a,
 uint192 b,
 RoundingMode rounding
 ) internal pure returns (uint192) {
 // untestable:
 //      a will never = 0 here because of the check in _price()
-       if (a == 0 || b == 0) return 0;
+       if (a == FIX_MAX || b == FIX_MAX) return FIX_MAX;
 // untestable:
 //      a = FIX_MAX iff b = 0 
-       if (a == FIX_MAX || b == FIX_MAX) return FIX_MAX;
+       if (a == 0 || b == 0) return 0;

----------------------

 }

Assessed type

Invalid Validation

[tbrent]

And in a case where qty = FIX_MAX, lowP = 0, the safeMul should return FIX_MAX

There is no strict notion of "should" here -- it is not clearly defined how the safe* functions should operate (eg, 0/0 is technically undefined).

That said: FIX_MAX * 0 = FIX_MAX is not an interpretation that makes very much sense mathematical sense to me. It makes much less sense to me than FIX_MAX * 0 = 0.

If the warden believes there is an issue here I would think that issue would be with the usage of the safeMul within the price() function, not with safeMul itself.

However, there as well, it seems correct for price().low to return a value that excludes the 0-low-price collateral, and wrong for it to return FIX_MAX: The consequence of returning FIX_MAX would be to indicate to components built on top of price() that in the worst-case the BU is worth +inf value, clearly wrong.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid