Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Incorrect Array Initialization in FeeConversionKeeper::checkUpkeep Function

josh4324

Summary

The checkUpkeep function contains a critical vulnerability due to improper initialization of the marketIds and assets arrays. These arrays are initialized with a fixed size of liveMarketIds.length * 10, which does not account for the actual number of assets requiring fee distribution. This approach leads to buffer overflow/underflow risks and data truncation.

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;
​
// Iterate over markets by id
for (uint128 i; i < liveMarketIds.length; i++) {
marketId = liveMarketIds[i];
​
(address[] memory marketAssets, uint256[] memory feesCollected) =
self.marketMakingEngine.getReceivedMarketFees(marketId);
​
// Iterate over receivedMarketFees
for (uint128 j; j < marketAssets.length; j++) {
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
​
if (distributionNeeded) {
// set upkeepNeeded = true
upkeepNeeded = true;
​
// set marketId, asset
marketIds[index] = marketId;
assets[index] = marketAssets[j];
​
index++;
}
}
}
​
if (upkeepNeeded) {
performData = abi.encode(marketIds, assets);
}
}
​

The arrays are initialized to the size of liveMarketIds.length * 10, assuming each market has at most liveMarketIds.length * 10 assets. This is an arbitrary assumption and not enforced by the protocol.

Suppose a market has more than liveMarketIds.length * 10 assets, the loop will attempt to write beyond the array’s bounds, causing a runtime error.

Suppose a market has fewer than liveMarketIds.length * 10 assets, the arrays will contain unused slots, wasting gas.

The inner loop iterates over marketAssets, which can have an arbitrary number of assets per market. There is no limit on how many assets a market can have, making the fixed-size arrays unsafe.

Impact

  • Denial-of-Service (DoS): The function will revert if any market has more assets than the fixed array allows, preventing all fee distributions.

  • Incomplete Distributions: Valid fee distribution requests will be not be processed if the total exceeds the fixed array size, violating protocol guarantees.

  • Gas Waste: Unused array slots increase gas costs unnecessarily.

Tools Used

Manual Review

Recommendations

Replace fixed-size arrays with dynamic arrays to accommodate an arbitrary number of entries:

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

FeeConversionKeeper performUpkeep iterates over entire oversized arrays instead of just populated entries, causing reverts on address(0) slots

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.