Lines of code

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

Vulnerability details

Impact

Unexpected loss of funds by the protocol or user may occur due to a missing of the the return value check of the quote() function of the BasketHandler contract in the issueTo() and the redeemTo() function of the RToken contract.

Malicious users can issue RTokens for free by using issueTo() function.
Users can lose RTokens by redeemTo() function.

Proof of Concept

The RToken.sol#redeemCustom() function performs a check to prevent loss of funds if BasketHandler.sol#quoteCustomRedemption() returns an empty redeemtion.

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

        // === Get basket redemption amounts ===

286:    (address[] memory erc20s, uint256[] memory amounts) = basketHandler.quoteCustomRedemption(
            basketNonces,
            portions,
            baskets
        );

        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
        }

        // === Interactions ===

        // Distribute tokens; revert if empty redemption
        {
320:        bool allZero = true;
            for (uint256 i = 0; i < erc20s.length; ++i) {
                if (amounts[i] == 0) continue; // unregistered ERC20s will have 0 amount
323:            if (allZero) allZero = false;

                // Send withdrawal
                // slither-disable-next-line arbitrary-send-erc20
                IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                    address(backingManager),
                    recipient,
                    amounts[i]
                );
            }
333:        if (allZero) revert("empty redemption");
        }

        // === 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");
        }
    }

As you can see, if the returned token array returns an empty array, allZero in #333 will remain true, so the transaction will be reverted and the user's RToken will not be burned.

This means that users will keep their RTokens and will not suffer any losses.

However, in the redeemTo() and issueTo() functions, this user asset protection feature is broken.

RToken.sol#redeemTo():

    function redeemTo(address recipient, uint256 amount) public notFrozen {
        SNIP...
        // {BU}
203:    uint192 baskets = _scaleDown(caller, amount);
        emit Redemption(caller, recipient, amount, baskets);

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

        // === Interactions ===

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

            // Send withdrawal
            // slither-disable-next-line arbitrary-send-erc20
218:        IERC20Upgradeable(erc20s[i]).safeTransferFrom(
219:            address(backingManager),
220:            recipient,
221:            amounts[i]
222:        );
223:    }
    }

As you can see, in #L203 the caller's RToken is burned.

In the following #L206, the BasketHandler.sol#quote() function is used to return an array of tokens that need to be redeemed based on baskets.

As you can see in #L215, the amounts[] array is expected to return an empty array and the safeTransferFrom() function is not performed.

However, since the return value check is not performed accurately as in the redeemCustom() function, if the safeTransferFrom() function is not called for the entire token array, the user cannot redeem the funds and the transaction is not reversed.

As a result, the user will lose RToken.

RToken.sol#issueTo():

This loss of funds due to the loophole in validation is also reflected in the issueTo() function.

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

        // 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 ==
146:    _scaleUp(recipient, amtBaskets, supply);

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

Users can issue RTokens for free without depositing underlying assets.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add the return value check mechanism to issueTo() and redeemTo() function as follows:

    {
        bool allZero = true;
        for (uint256 i = 0; i < erc20s.length; ++i) {
            if (amounts[i] == 0) continue; // unregistered ERC20s will have 0 amount
            if (allZero) allZero = false;

            // Send withdrawal
            // slither-disable-next-line arbitrary-send-erc20
            IERC20Upgradeable(erc20s[i]).safeTransferFrom(
                ... ....
            );
        }
        if (allZero) revert("empty array is invalid");
    }

Assessed type

Invalid Validation

code-423n4 / 2024-07-reserve-validation

Unexpected loss of funds due to missing of the return value check #119