Summary
The checkUpkeep function contains an unbounded loop that iterates over all liveMarketIds and their associated marketAssets. If the number of markets or assets is very large, the function could run out of gas, causing the transaction to fail. This is a common issue in smart contracts that process large datasets.
Vulnerability Details
function checkUpkeep(bytes calldata ) external view returns (bool upkeepNeeded, bytes memory performData) {
FeeConversionKeeperStorage memory self = _getFeeConversionKeeperStorage();
uint128[] memory liveMarketIds = self.marketMakingEngine.getLiveMarketIds();
bool distributionNeeded;
uint128[] memory marketIds = new uint128[](liveMarketIds.length * 10);
address[] memory assets = new address[](liveMarketIds.length * 10);
uint256 index = 0;
uint128 marketId;
for (uint128 i; i < liveMarketIds.length; i++) {
marketId = liveMarketIds[i];
(address[] memory marketAssets, uint256[] memory feesCollected) =
self.marketMakingEngine.getReceivedMarketFees(marketId);
for (uint128 j; j < marketAssets.length; j++) {
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
if (distributionNeeded) {
upkeepNeeded = true;
marketIds[index] = marketId;
assets[index] = marketAssets[j];
index++;
}
}
}
if (upkeepNeeded) {
performData = abi.encode(marketIds, assets);
}
}
Impact
If there are too many liveMarketIds or marketAssets, the loop could consume more gas than the block gas limit, causing the transaction to fail.
Processing a large number of markets and assets in a single transaction is inefficient and could lead to high gas costs.
An attacker could intentionally create a large number of markets or assets to make the function unusable. Denial of Service (DoS):
Tools Used
Manual Code Review
Recommendations
To address the unbounded loop issue, consider the following solutions:
Add a gas limit check to ensure the function does not consume too much gas
uint256 gasLimit = 5_000_000;
uint256 gasUsed = 0;
for (uint128 i; i < liveMarketIds.length; i++) {
if (gasUsed >= gasLimit) {
break;
}
uint128 marketId = liveMarketIds[i];
(address[] memory marketAssets, uint256[] memory feesCollected) =
self.marketMakingEngine.getReceivedMarketFees(marketId);
for (uint128 j; j < marketAssets.length; j++) {
uint256 startGas = gasleft();
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
if (distributionNeeded) {
upkeepNeeded = true;
marketIds[index] = marketId;
assets[index] = marketAssets[j];
index++;
}
gasUsed += startGas - gasleft();
if (gasUsed >= gasLimit) {
break;
}
}
}
Pagination
Process markets and assets in smaller batches.
function checkUpkeep(uint128 startMarketId, uint128 batchSize) external view returns (bool upkeepNeeded, bytes memory performData) {
FeeConversionKeeperStorage memory self = _getFeeConversionKeeperStorage();
uint128[] memory liveMarketIds = self.marketMakingEngine.getLiveMarketIds();
bool distributionNeeded;
uint128[] memory marketIds = new uint128[](batchSize * 10);
address[] memory assets = new address[](batchSize * 10);
uint256 index = 0;
for (uint128 i = startMarketId; i < startMarketId + batchSize && i < liveMarketIds.length; i++) {
uint128 marketId = liveMarketIds[i];
(address[] memory marketAssets, uint256[] memory feesCollected) =
self.marketMakingEngine.getReceivedMarketFees(marketId);
for (uint128 j; j < marketAssets.length; j++) {
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
if (distributionNeeded) {
upkeepNeeded = true;
marketIds[index] = marketId;
assets[index] = marketAssets[j];
index++;
}
}
}
if (upkeepNeeded) {
performData = abi.encode(marketIds, assets);
}
}
