Description

When calling proRataRedeem(), BasketToken.totalAssets() could be affected by read-only reentrancy.

// Interactions
for (uint256 i = 0; i < assetsLength;) {
    address asset = assets[i];
    // nosemgrep: solidity.performance.state-variable-read-in-a-loop.state-variable-read-in-a-loop
    uint256 balance = self.basketBalanceOf[basket][asset];
    // Rounding direction: down
    // Division-by-zero is not possible: totalSupplyBefore is greater than 0
    uint256 amountToWithdraw = FixedPointMathLib.fullMulDiv(burnedShares, balance, totalSupplyBefore);
    if (amountToWithdraw > 0) {
        // nosemgrep: solidity.performance.state-variable-read-in-a-loop.state-variable-read-in-a-loop
        self.basketBalanceOf[basket][asset] = balance - amountToWithdraw;
        // Asset is an allowlisted ERC20 with no reentrancy problem in transfer
        // slither-disable-next-line reentrancy-no-eth
        IERC20(asset).safeTransfer(to, amountToWithdraw);
    }
    unchecked {
        // Overflow not possible: i is less than assetsLength
        ++i;
    }
}

If the basket supports tokens as assets that allow callbacks, such as ERC-777 tokens, an attacker can hijack the control flow using the receiver hook during the asset transfer in proRataRedeem(). Although BasketManager has a reentrancy guard in place, BasketToken.totalAssets() can still be called.

If a third party uses the totalAssets() function for value or price calculations, they may end up calculating an imbalanced asset value by calling bm.basketBalanceOf() while the assets have not yet been fully redeemed in the loop.

Impact

Third parties that utilize totalAssets() could receive incorrect values. If these values are used for price calculations, it may lead to asset losses for the third parties.

Recommendations

To adhere to the checks-effects-interactions pattern, separate the balance effects and asset transfer into two separate loops. Ensure that tokens that can cause reentrancy, such as ERC-777 tokens, are not supported as assets.

Remediation

Storm Labs will not support any tokens that can cause reentrancy.

This issue has been acknowledged by Storm Labs, and a fix was implemented in commit ae3f3050↗.