c4-bot-2 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L183-L225 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L342 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L409-L413

Vulnerability details

Impact

rToken::redeemCustom has slippage protection due to portions and pro rata accounting which can be volatile during under-collateralized times. But during proper collateralized times, the exchange rate between total supply of rToken and backing needed can be volatile and user might incur slippage loss when redeeming the rTokens and there's no validation to check the minimum erc20 token amounts transferred after redemption.

Rebalancing can happen if the rToken is not frozen during trade settlement or when tradeEnd < block.timestamp, and redemption can be done by users if rToken isn't frozen. 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 is over > 0.5%, it 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");

Proof of Concept

A user wants to redeem x rTokens. But the amount of erc20 tokens transferred depends on exchange rate between basketsNeeded and totalSupply 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

And if any redemption is submitted by a user, and suddenly the rToken is rebalanced to move the exchange rate between total supply of rToken and backing needed for > 2%, then slippage loss to the user The slippage loss can be allowed to move between low >= MIN_EXCHANGE_RATE && high <= MAX_EXCHANGE_RATE, 1e9 < rate < 1e27. So the loss of funds is a severe issue. Adding minTokensOut is a good slippage protection.

rToken::redeemTo and rToken::_scaleDown

    function redeemTo(address recipient, uint256 amount) public notFrozen {

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

    @@  uint192 baskets = _scaleDown(caller, amount);
        emit Redemption(caller, recipient, amount, baskets);

        (address[] memory erc20s, uint256[] memory amounts) = basketHandler.quote(
            baskets,
            false,
            FLOOR
        );

        for (uint256 i = 0; i < erc20s.length; ++i) {
            if (amounts[i] == 0) continue; 

            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                address(backingManager),
                recipient,
                amounts[i]
            );
        }
    }

    function _scaleDown(address account, uint256 amtRToken) private returns (uint192 amtBaskets) {
  @@    amtBaskets = basketsNeeded.muluDivu(amtRToken, totalSupply()); 
        emit BasketsNeededChanged(basketsNeeded, basketsNeeded - amtBaskets);
        basketsNeeded -= amtBaskets;

        _burn(account, amtRToken);
    }

Tools Used

Vs code

Recommended Mitigation Steps

Modify rToken::redeemTo just like rToken::redeemCustom

-   function redeemTo(address recipient, uint256 amount) public notFrozen {    
+   function redeemTo(address recipient, uint256 amount, uint256[] memory minAmounts) public notFrozen {
---------------

+       uint256[] memory pastBals = new uint256[](expectedERC20sOut.length);
+       for (uint256 i = 0; i < expectedERC20sOut.length; ++i) {
+           pastBals[i] = IERC20(expectedERC20sOut[i]).balanceOf(recipient);
+       }

        for (uint256 i = 0; i < erc20s.length; ++i) {
            if (amounts[i] == 0) continue; 

            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                address(backingManager),
                recipient,
                amounts[i]
            );
        }

+       for (uint256 i = 0; i < expectedERC20sOut.length; ++i) {
+           uint256 bal = IERC20(expectedERC20sOut[i]).balanceOf(recipient);
+           require(bal - pastBals[i] >= minAmounts[i], "redemption below minimum");
        }
    }

Assessed type

MEV

nuthan2x commented 1 month ago

@thereksfour I think this issue is wrongly grouped. The primary issue 74 and others in group 48 talks about slippage loss in rToken issuance. The issue was known and judged accordingly here

But this issue talks about slippage loss during rToken REDEMPTION. Please re-judge as primary issue and not a dup of #74.

thereksfour commented 1 month ago

The group label is not accurate, this is automated and we do not rely on it. redeem is only allowed when fully collateralized, when there is sufficient collateral and no slippage loss.

nuthan2x commented 1 month ago

redeem is only allowed when fully collateralized, when there is sufficient collateral and no slippage loss.

Okay if users wants redeem only when collateralized. But redeem is still not protected during the processing of rToken revenue. Redemption with get affected by slippage loss during settle auction trade (either re-collateralization or processing RToken revenue).

Quoting from first 3 lines from Vulnerability details

rToken::redeemCustom has slippage protection due to portions and pro rata accounting which can be volatile during under-collateralized times. But during proper collateralized times, the exchange rate between total supply of rToken and backing needed can be volatile