Underflow when calculating basket balances
Description
The _processInternalTrades function handles the internal trades between baskets. It calculates the initial buy amount and the net buy amount for each trade. The function then checks if the net buy amount is within the min and max amount limits. It then checks whether the calculated net buy and trade sell amounts are within the current basket balances' limits. It then updates the balances for each basket that requires a trade.
function _processInternalTrades(
BasketManagerStorage storage self,
InternalTrade[] calldata internalTrades,
address[] calldata baskets,
uint256[][] memory basketBalances
)
private
{
uint256 swapFee = self.swapFee; // Fetch swapFee once for gas optimization
uint256 internalTradesLength = internalTrades.length;
for (uint256 i = 0; i < internalTradesLength;) {
InternalTrade memory trade = internalTrades[i];
// ...
uint256 initialBuyAmount = self.eulerRouter.getQuote(
self.eulerRouter.getQuote(info.netSellAmount, trade.sellToken, _USD_ISO_4217_CODE),
_USD_ISO_4217_CODE,
trade.buyToken
);
if (swapFee > 0) {
info.feeOnBuy = FixedPointMathLib.fullMulDiv(initialBuyAmount, swapFee, 20_000);
self.collectedSwapFees[trade.buyToken] += info.feeOnBuy;
emit SwapFeeCharged(trade.buyToken, info.feeOnBuy);
}
info.netBuyAmount = initialBuyAmount - info.feeOnBuy;
if (info.netBuyAmount < trade.minAmount || trade.maxAmount < info.netBuyAmount) {
revert InternalTradeMinMaxAmountNotReached();
}
if (trade.sellAmount > basketBalances[info.fromBasketIndex][info.sellTokenAssetIndex]) {
revert IncorrectTradeTokenAmount();
}
if (info.netBuyAmount > basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex]) {
revert IncorrectTradeTokenAmount();
}
unchecked {
self.basketBalanceOf[trade.fromBasket][trade.sellToken] =
basketBalances[info.fromBasketIndex][info.sellTokenAssetIndex] -= trade.sellAmount; // nosemgrep
self.basketBalanceOf[trade.fromBasket][trade.buyToken] =
basketBalances[info.fromBasketIndex][info.buyTokenAssetIndex] += info.netBuyAmount; // nosemgrep
self.basketBalanceOf[trade.toBasket][trade.buyToken] =
basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex] -= initialBuyAmount; // nosemgrep
self.basketBalanceOf[trade.toBasket][trade.sellToken] =
basketBalances[info.toBasketIndex][info.toBasketSellTokenIndex] += info.netSellAmount; // nosemgrep
++i;
}
}
}The issue is that, even though the if case for the buy amount checks whether info.netBuyAmount is greater than the basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex], the subtraction operation is performed with initialBuyAmount, which will always be greater than info.netBuyAmount. This can lead to an underflow when calculating the new basket balance. Normally, in Solidity versions 0.8.0 and above, the compiler will throw an error when an underflow is detected. However, in this case, the underflow is not detected by the compiler because the subtraction operation is performed within an unchecked block.
Impact
The underflow would lead to the basket balances being set to a very large number, potentially blocking the usability of the particular basket. This in turn could lead to loss of funds for the users.
Recommendations
We recommend ensuring that the if case checks the correct variable to prevent underflows. Additionally, we recommend removing the unchecked block to allow the compiler to detect underflows, as the gas optimization gained from using unchecked is minimal compared to the potential risks.
function _processInternalTrades(
BasketManagerStorage storage self,
InternalTrade[] calldata internalTrades,
address[] calldata baskets,
uint256[][] memory basketBalances
)
private
{
uint256 swapFee = self.swapFee; // Fetch swapFee once for gas optimization
uint256 internalTradesLength = internalTrades.length;
for (uint256 i = 0; i < internalTradesLength;) {
// ...
- if (info.netBuyAmount > basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex]) {
+ if (initialBuyAmount > basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex]) {
revert IncorrectTradeTokenAmount();
}
- unchecked {
self.basketBalanceOf[trade.fromBasket][trade.sellToken] =
basketBalances[info.fromBasketIndex][info.sellTokenAssetIndex] -= trade.sellAmount;
self.basketBalanceOf[trade.fromBasket][trade.buyToken] =
basketBalances[info.fromBasketIndex][info.buyTokenAssetIndex] += info.netBuyAmount;
self.basketBalanceOf[trade.toBasket][trade.buyToken] =
basketBalances[info.toBasketIndex][info.toBasketBuyTokenIndex] -= initialBuyAmount;
self.basketBalanceOf[trade.toBasket][trade.sellToken] =
basketBalances[info.toBasketIndex][info.toBasketSellTokenIndex] += info.netSellAmount;
++i;
- }
}
}Remediation
This issue has been acknowledged by Storm Labs, and a fix was implemented in commit 8ae19aea↗.