Volatile exchange rates cause slippage loss to users during issuance and rebalancing the collateral

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L409-L413 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L483-L496 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L105

Vulnerability details

Impact

During rebalancing when trade settlement, the exchange rate between total supply of rToken and backing needed will be volatile and user can incur slippage loss and there's no validation to check the minimum rToken minted.

While issuance maybe paused during these times, but there will still be user's issue transaction losing to the slippage, because the first rebalancing can even happen before owner pausing issuance. Rebalancing can happen if the rToken is not frozen during trade settlement or when tradeEnd < block.timestamp.

Proof of Concept

A user approves the token amounts and gets minted x rTokens. But the amount of rTokens minted depends on basketsNeeded and totalSupply an the input parameter amount.
The basketsNeeded is volatile irrespective or independent of totalSupply in the following flow of rebalancing the backing manager. BackingManagerP1::settleTrade >> BackingManagerP1::rebalance >> BackingManagerP1::compromiseBasketsNeeded >> rToken.setBasketsNeeded

So, there are flows where exchange rate between total supply of rToken and backing needed, it can be allowed to move between low >= MIN_EXCHANGE_RATE && high <= MAX_EXCHANGE_RATE, 1e9 < rate < 1e27

And if this sudden slippage change of over > 0.5% is a concern to the user who is trying to mint at the current exchange rate. And to protect his slippage, there should be a minimum rToken minted validation during these rebalancing times.

rToken.setBasketsNeeded

        uint256 low = (FIX_ONE_256 * basketsNeeded_) / supply; // D18{BU/rTok}
        uint256 high = (FIX_ONE_256 * basketsNeeded_ + (supply - 1)) / supply; // D18{BU/rTok}

        // here we take advantage of an implicit upcast from uint192 exchange rates
        require(low >= MIN_EXCHANGE_RATE && high <= MAX_EXCHANGE_RATE, "BU rate out of range");

rToken::issueTo and rToken::_scaleUp

    function issueTo(address recipient, uint256 amount) public notIssuancePausedOrFrozen {

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

        uint192 amtBaskets = supply != 0
            ? basketsNeeded.muluDivu(amount, supply, CEIL) 
            : _safeWrap(amount);
        emit Issuance(issuer, recipient, amount, amtBaskets);

        // Get quote from BasketHandler including issuance premium
        (address[] memory erc20s, uint256[] memory deposits) = basketHandler.quote(
            amtBaskets,
            true,
            CEIL
        );

        // == Interactions: Create RToken + transfer tokens to BackingManager ==
        _scaleUp(recipient, amtBaskets, supply);

---------------
    }

    function _scaleUp( address recipient, uint192 amtBaskets, uint256 totalSupply) private {
  @@    uint256 amtRToken = totalSupply != 0
            ? amtBaskets.muluDivu(totalSupply, basketsNeeded) // {rTok} = {BU} * {qRTok} * {qRTok}
            : amtBaskets; // {rTok}
        emit BasketsNeededChanged(basketsNeeded, basketsNeeded + amtBaskets);
        basketsNeeded += amtBaskets;

        _mint(recipient, amtRToken);
    }

Tools Used

Vs code

Recommended Mitigation Steps

Modify rToken::issueTo and rToken::_scaleUp

- function issueTo(address recipient, uint256 amount) public notIssuancePausedOrFrozen {
+ function issueTo(address recipient, uint256 amount, uint256 min_rTokenOut) public notIssuancePausedOrFrozen {
        require(amount != 0, "Cannot issue zero");

        assetRegistry.refresh();

        address issuer = _msgSender(); // OK to save: it can't be changed in reentrant runs

        // Ensure basket is ready, SOUND and not in warmup period
        require(basketHandler.isReady(), "basket not ready");
        uint256 supply = totalSupply();

        // Revert if issuance exceeds either supply throttle
        issuanceThrottle.useAvailable(supply, int256(amount)); // reverts on over-issuance
        redemptionThrottle.useAvailable(supply, -int256(amount)); // shouldn't revert 

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

-       _scaleUp(recipient, amtBaskets, supply);
+       _scaleUp(recipient, amtBaskets, supply, min_rTokenOut);

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

    }

    function _scaleUp(
        address recipient,
        uint192 amtBaskets,
        uint256 totalSupply,
+       uint256 min_rTokenOut
    ) private {
        // take advantage of 18 decimals during casting
        uint256 amtRToken = totalSupply != 0
            ? amtBaskets.muluDivu(totalSupply, basketsNeeded) // {rTok} = {BU} * {qRTok} * {qRTok}
            : amtBaskets; // {rTok}
        emit BasketsNeededChanged(basketsNeeded, basketsNeeded + amtBaskets);
        basketsNeeded += amtBaskets;

        // Mint RToken to recipient
        _mint(recipient, amtRToken);

+       require(amtRToken >= min_rTokenOut);
    }

Assessed type

MEV