The Loss of Fund of the protocol due to Incorrect Boundary Check of Loop

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BasketHandler.sol#L521-L600

Vulnerability details

Impact

Protocol funds can be lost due to incorrect bounds checking on the loop variable in the BasketHandler.sol#quoteCustomRedemption() function.

Proof of Concept

The BasketHandler.sol#quoteCustomRedemption() function returns the redemption value of amount BUs for a linear combination of historical baskets.

    function quoteCustomRedemption(
        uint48[] memory basketNonces,
        uint192[] memory portions,
        uint192 amount
    ) external view returns (address[] memory erc20s, uint256[] memory quantities) {
        SNIP...

532:    uint256 len; // length of return arrays

        // Calculate the linear combination basket
        for (uint48 i = 0; i < basketNonces.length; ++i) {
            SNIP...

            // Add-in refAmts contribution from historical basket
            Basket storage b = basketHistory[basketNonces[i]];
            for (uint256 j = 0; j < b.erc20s.length; ++j) {
                // untestable:
                //     previous baskets erc20s do not contain the zero address
                if (address(b.erc20s[j]) == address(0)) continue;

                // Search through erc20sAll
553:            uint256 erc20Index = type(uint256).max;
554:            for (uint256 k = 0; k < len; ++k) {
555:                if (b.erc20s[j] == erc20sAll[k]) {
556:                    erc20Index = k;
557:                    break;
558:                }
559:            }

                // Add new ERC20 entry if not found
                uint192 amt = portions[i].mul(b.refAmts[b.erc20s[j]], FLOOR);
563:            if (erc20Index == type(uint256).max) {
                    // New entry found

                    try assetRegistry.toAsset(b.erc20s[j]) returns (IAsset asset) {
                        if (!asset.isCollateral()) continue; // skip token if not collateral

                        erc20sAll[len] = b.erc20s[j];
                        collsAll[len] = ICollateral(address(asset));

                        // {ref} = {1} * {ref}
                        refAmtsAll[len] = amt;
574:                    ++len;
                    } catch (bytes memory errData) {
                        // untested:
                        //     OOG pattern tested in other contracts, cost to test here is high
                        // see: docs/solidity-style.md#Catching-Empty-Data
                        if (errData.length == 0) revert(); // solhint-disable-line reason-string
                        // skip token if no longer registered or other non-gas issue
                    }
                } else {
                    // {ref} = {1} * {ref}
                    refAmtsAll[erc20Index] += amt;
                }
            }
        }

589:    erc20s = new address[](len);
590:    quantities = new uint256[](len);

        // Calculate quantities
593:    for (uint256 i = 0; i < len; ++i) {
594:        erc20s[i] = address(erc20sAll[i]);

            // {tok} = {BU} * {ref/BU} / {ref/tok}
597:        quantities[i] = amount
598:        .safeMulDiv(refAmtsAll[i], collsAll[i].refPerTok(), FLOOR)
599:        .shiftl_toUint(int8(collsAll[i].erc20Decimals()), FLOOR);
600:    }
    }

As you can see, the variable len is not initialized in #L532, so its value is 0.

The root cause of this problem is that the loop statement is not executed because len is 0 in #L554 to #L559. So erc20Index will keep type(uint256).max.

In other words, #L563 will be executed regardless of whether b.erc20s[j] == erc20sAll[k] in line #L555, and len will increase.

As a result, if initially b.erc20s[j] == erc20sAll[k] is true, the token will be incorrectly added to the erc20s array. As a result, the user will receive unfairly more funds than expected.

Example:

• Initial Assumtion:

  • Simply, let's assume that basketNonces.length = 1, so portions[0] = 1.
  • Next, assume that b.erc20s = {USDC, WETH}, erc20sAll = {USDC, WBTC, DAI}.
  • As a result, erc20s becomes {USDC} because len is 1 since WETH does not exist in erc20sAll.

  • Therefore, the user has to be USDC amount as follows:

    USDC_Aomunt = amount 1 b.refAmts[WETH] * collsAll[USDC].refPerTok() // b.erc20s[1] = WETH, erc20sAll[0] = USDC

• However:

  • At first, len = 0.
  • Therefore, when j = 0, the loop statement is not executed and erc20Index will keep type(uint256).max.

  • And then, len increased to 1 in #L574 even though b.erc20s[0] == erc20sAl[0] is true.

    b.erc20s[0] = USDC

    erc20sAl[0] = USDC

  • Next, when j = 1, the loop statement is executed because len = 1, but WETH does not exist in erc20sAll, so erc20Index will keep `type(uint256) still.

  • As a result, len is increased to 2, USDC and WBTC will be added to the erc20s array. In other words, the user will be received unfairly more funds than expected.

    USDC_Amount = amount b.refAmts[USDC] collsAll[USDC].refPerTok() // j = 0

    WBTC_Amount = amount b.refAmts[WETH] collsAll[WBTC].refPerTok() // j = 1

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to modify loop statement as follows:

--- for (uint256 k = 0; k < len; ++k) {
--- for (uint256 k = 0; k <= len; ++k) {
        if (b.erc20s[j] == erc20sAll[k]) {
            erc20Index = k;
            break;
        }
    }

Assessed type

Loop