Description

BasketToken inherits from ERC20PluginsUpgradeable, which is a modified version of 1inch's ERC20Plugins as an upgradable contract. However, ERC20Plugins has a potential reentrancy vulnerability when the from or to address is an arbitrary address. An attacker could add a plug-in to BasketToken to trigger a reentrancy attack.

function proRataRedeem(uint256 shares, address to, address from) public {
    // Effects
    uint16 feeBps = BasketManager(basketManager).managementFee(address(this));
    address feeCollector = BasketManager(basketManager).feeCollector();
    _harvestManagementFee(feeBps, feeCollector);
    if (msg.sender != from) {
        _spendAllowance(from, msg.sender, shares);
    }
    uint256 totalSupplyBefore = totalSupply();
    _burn(from, shares); // reentrancy point with `from`s plugin.
    // Interactions
    BasketManager(basketManager).proRataRedeem(totalSupplyBefore, shares, to);
}

When calling proRataRedeem(), BasketToken.totalAssets() could be affected by read-only reentrancy. 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.

Here is an attack scenario. Suppose a third party, such as a lending protocol, uses the Basket LP share tokens as collateral.

1. An attacker deposits a significant amount of assets into the basket.

2. The rebalance ends, and the attacker claims the basket tokens.

3. The attacker deposits the basket tokens into the lending protocol.

4. The attacker adds a malicious plug-in to trigger reentrancy.

5. The attacker calls proRataRedeem().

6. It burns the basket tokens and calls the malicious plug-in contract in ERC20PluginUpgradeable._updateBalances().

7. The total supply of the basket token has already decreased since the tokens were burned by calling super._update() in ERC20PluginsUpgradeable._update().

8. The asset balances have not yet decreased before calling BasketManager.proRataRedeem().

9. The LP-token price increases because the LP-token pricing calculation is based on asset_values / total_supply_of_basket_i (as described in the Cove Finance RFC documentation↗).

10. Finally, the attacker borrows other tokens (excluding the basket token, which has its own reentrancy guard) by exploiting the manipulated basket-token price as collateral.

Impact

An attacker can manipulate the price of Basket LP tokens through read-only reentrancy during proRataRedeem(), potentially exploiting third-party protocols that rely on LP-token pricing for collateral, leading to the drainage of funds of third-party protocols.

Recommendations

To adhere to the check-effects-interactions pattern, call _burn() after calling BasketManager.proRataRedeem() and do not support any tokens that can cause reentrancy (resolved in another reentrancy issue; see Finding ). Additionally, the documentation should warn third parties who use the price of BasketToken about the risk of read-only reentrancy to ensure third parties check for reentrancy.

Remediation

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