Unbounded number of bonusTokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L629
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L270

Vulnerability details

Impact

Each additional bonus token added to the BathToken.bonusTokens[] array will cause the gas fee of BathToken.withdraw() calls to be more expensive.

The BathToken does not impose any upper limit on the BathToken.bonusTokens[] array. It also does not implement any function to reduce the size of the BathToken.bonusTokens[] array.

As such, it is possible to append new bonus token to the BathToken.bonusTokens[] array until a point where an "Out of Gas" error or a "Block Gas Limit" error happens when BathToken.withdraw() is called. At this point, none of the BathToken LPs will be able to withdraw their funds from the affected BathToken pools.

Proof-of-Concept

The following shows that the distributeBonusTokenRewards function, which is called by withdraw function, looping through the BathToken.bonusTokens[] array and execute the code for the rewards/bonus distribution in each iteration.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L629

function distributeBonusTokenRewards(
    address receiver,
    uint256 sharesWithdrawn,
    uint256 initialTotalSupply
) internal {
    if (bonusTokens.length > 0) {
        for (uint256 index = 0; index < bonusTokens.length; index++) {
            IERC20 token = IERC20(bonusTokens[index]);
            // Note: Shares already burned in Bath Token _withdraw

            // Pair each bonus token with a lightly adapted OZ Vesting wallet. Each time a user withdraws, they
            //  are released their relative share of this pool, of vested BathBuddy rewards
            // The BathBuddy pool should accrue ERC-20 rewards just like OZ VestingWallet and simply just release the withdrawer's relative share of releaseable() tokens
            if (rewardsVestingWallet != IBathBuddy(0)) {
                rewardsVestingWallet.release(
                    (token),
                    receiver,
                    sharesWithdrawn,
                    initialTotalSupply,
                    feeBPS
                );
            }
        }
    }
}

The following shows the new bonus token address appends to the BathToken.bonusTokens[] array without checking for the upper limit.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L270

/// @notice Admin-only function to add a bonus token to bonusTokens for pool incentives
function setBonusToken(address newBonusERC20) external onlyBathHouse {
    bonusTokens.push(newBonusERC20);
}

Recommended Mitigation Steps

Define a max number of bonus tokens in a pool, and have the array's length checked. Additionally, implement function to reduce the array size so that it is possible to remove some of the bonus token from the array if needed.

[bghughes]

See #45

[HickupHH3]

Valid concern; protocol availability is potentially affected

[HickupHH3]

Duplicate of #249, last example.