Lack of Slippage Protection in issueTo() functions of the RToken contract

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/RToken.sol#L105-L155

Vulnerability details

Impact

The issueTo() function lacks the slippage control that allows the users to revert if the amount of the underlying tokens they deposits is bigger than the amount they expected. Therefore, Assets for the affected users may be lose.

Proof of Concept

During issuance, the user deposits the underlying tokens into the RToken contract, and the RToken contract uses the issueTo() function to issue RToken to the user.

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

        // == Refresh ==

110:    assetRegistry.refresh();
        ...
        uint256 supply = totalSupply();

        SNIP...

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

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

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

        for (uint256 i = 0; i < erc20s.length; ++i) {
            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                issuer,
                address(backingManager),
                deposits[i]
            );
        }
    }

As you can see, slippage control is not implemented in the issueTo() function.

However, slippage can occur in the issueTo() function due to various reasons.

• Calculation of the BU change based on totalSupply() and basketsNeeded

  The BU change is calculated as follows:

  amtBaskets = basketsNeeded * amount / totalSupply()

  However, totalSupply() and basketsNeeded can increase or decrease at any time depending on the current on-chain conditions when the transaction is executed.

  • _scaleUp

  function _scaleUp(
      address recipient,
      uint192 amtBaskets,
      uint256 totalSupply
  ) 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);
  }

  • _scaleDown

  function _scaleDown(address account, uint256 amtRToken) private returns (uint192 amtBaskets) {
      // D18{BU} = D18{BU} * {qRTok} / {qRTok}
      amtBaskets = basketsNeeded.muluDivu(amtRToken, totalSupply()); // FLOOR
      emit BasketsNeededChanged(basketsNeeded, basketsNeeded - amtBaskets);
      basketsNeeded -= amtBaskets;
  
      // Burn RToken from account; reverts if not enough balance
      _burn(account, amtRToken);
  }

• Calculating deposits[] based on coll.refPerTok()

  The issueTo() function calculates deposits[] using the basketHandler.quote() function based on the calculated BU change in #L139.

  The quote() function determines deposits[] based on coll.refPerTok().

      function quote(
          uint192 amount,
          bool applyIssuancePremium,
          RoundingMode rounding
      ) public view returns (address[] memory erc20s, uint256[] memory quantities) {
          uint256 length = basket.erc20s.length;
          erc20s = new address[](length);
          quantities = new uint256[](length);
  
          for (uint256 i = 0; i < length; ++i) {
              erc20s[i] = address(basket.erc20s[i]);
              ICollateral coll = assetRegistry.toColl(IERC20(erc20s[i]));
  
              // {tok} = {tok/BU} * {BU}
  -->         uint192 q = _quantity(basket.erc20s[i], coll, rounding).safeMul(amount, rounding);
  
              SNIP...
  
              // {qTok} = {tok} * {qTok/tok}
  -->         quantities[i] = q.shiftl_toUint(
                  int8(IERC20Metadata(address(basket.erc20s[i])).decimals()),
                  rounding
              );
          }
      }

      function _quantity(
          ...
      ) internal view returns (uint192) {
  -->     uint192 refPerTok = coll.refPerTok();
          if (refPerTok == 0) return FIX_MAX;
  
          // {tok/BU} = {ref/BU} / {ref/tok}
          return basket.refAmts[erc20].div(refPerTok, rounding);
      }

  coll.refPerTok() It depends on the token, but it can also increase or decrease at any time depending on the current on-chain conditions when the transaction is executed.

In summary, the issueTo() function lacks the slippage control that allows the users to revert if the amount of the underlying tokens they deposits is bigger than the amount they expected.

To prevent this, the redeemCustom() function implements slippage control as follows.

    function redeemCustom(
        ...
    ) external notFrozen {
        SNIP...

        // === Save initial recipient balances ===

        uint256[] memory pastBals = new uint256[](expectedERC20sOut.length);
        for (uint256 i = 0; i < expectedERC20sOut.length; ++i) {
            pastBals[i] = IERC20(expectedERC20sOut[i]).balanceOf(recipient);
            // we haven't verified this ERC20 is registered but this is always a staticcall
        }

        SNIP...

        // === Post-checks ===

        // Check post-balances
        for (uint256 i = 0; i < expectedERC20sOut.length; ++i) {
            uint256 bal = IERC20(expectedERC20sOut[i]).balanceOf(recipient);
            // we haven't verified this ERC20 is registered but this is always a staticcall
            require(bal - pastBals[i] >= minAmounts[i], "redemption below minimum");
        }
    }

Considering this, I think this issue could be a valid medium.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a slippage control that allows the users to revert if the amount of the underlying tokens they deposits is bigger than the amount they expected.

Assessed type

MEV