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

Out-of-Bounds Vulnerability in the FeeConversionKeeper's checkUpkeep Function

rolando

Summary

This issue involves creating fixed-size arrays to store market IDs (marketIds) and their corresponding assets (assets) by multiplying the number of live markets by a factor of 10. The code assumes that no single market will hold more than 10 assets, yet this assumption is never validated. As a result, if a market exceeds 10 assets, the indexing operation can write beyond the array’s valid range, causing a revert and halting automation. This out-of-bounds write is particularly impactful because it blocks checkUpkeep execution, preventing fee conversion processes from running correctly.

Vulnerability Details

The implementation in checkUpkeep creates two fixed-size arrays marketIds and assets by multiplying the number of live markets (liveMarketIds.length) by 10. However, no check ensures that each market has at most 10 fee-collecting assets. If a market exceeds that limit, the indexing variable (index) increments beyond the allocated array size, resulting in an out-of-bounds write operation. Consequently, the contract reverts, which disrupts fee distribution automation and halts the process for all markets.

Impact

Because checkUpkeep reverts when processing any market with more than 10 assets, the entire automation process for fee conversion is blocked. This halts critical housekeeping operations and could lead to delayed or stalled distributions of collected fees. In a worst-case scenario, fees remain locked in the system indefinitely, impacting liquidity, revenue, and user trust.

Proof of Concept

This finding arises from the creation of fixed-size arrays (marketIds and assets) of length liveMarketIds.length * 10. However, there is no validation ensuring that each market has at most 10 assets. If just one market exceeds this limit, the function may attempt to write outside the array’s boundaries, causing a revert and blocking the execution of checkUpkeep.

Code Analysis

Below is a condensed version of the function where the issue resides, focusing on the relevant code section:

function checkUpkeep(bytes calldata /**/ ) external view returns (bool upkeepNeeded, bytes memory performData) {
// ... Preliminary code ...
uint128[] memory marketIds = new uint128[](liveMarketIds.length * 10);
address[] memory assets = new address[](liveMarketIds.length * 10);
uint256 index = 0;
​
// If any market has more than 10 assets, 'index' can exceed the array size
​
for (uint128 i; i < liveMarketIds.length; i++) {
// ...
for (uint128 j; j < marketAssets.length; j++) {
// ...
marketIds[index] = marketId; // (!) Fixed-size array index usage
assets[index] = marketAssets[j]; // (!) Fixed-size array index usage
index++;
}
}
​
// ... Subsequent code ...
}

( ! ) The calculation liveMarketIds.length * 10 assumes each market has a maximum of 10 assets, which is not guaranteed. This can lead to an out-of-bounds array write.

Explanation

If marketAssets.length exceeds 10 for even a single market, the index variable may increment beyond the array limits, causing an out-of-bounds revert at runtime and blocking checkUpkeep execution.

Vulnerable Scenario

Consider a market with an ID of 1 that contains 11 assets instead of 10. During the inner loop, index will reach 11, surpassing the capacity of the arrays (which only account for 10 assets per market). This triggers a runtime revert that prevents checkUpkeep from completing and halts the automated fee-conversion process for all markets.

Test and Result

This test deploys a mock engine returning 11 assets for a single market, thus exceeding the assumed capacity of liveMarketIds.length * 10. By calling checkUpkeep, expect it to revert due to an out-of-bounds array write. The successful [PASS] confirms the logical error: the keeper reverts with an “array out-of-bounds” panic (0x32), exactly as predicted.

  • Add the following test to test/FeeConversionKeeperCheckUpkeepIntegrationTest.t.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.25;
​
// Zaros dependencies
import { Base_Test } from "test/Base.t.sol";
import { FeeConversionKeeper } from "@zaros/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol";
import { ChainlinkAutomationUtils } from "script/utils/ChainlinkAutomationUtils.sol";
import { DexSwapStrategy } from "@zaros/market-making/leaves/DexSwapStrategy.sol";
​
contract FeeConversionKeeperCheckUpkeepIntegrationTest is Base_Test {
modifier givenCheckUpkeepIsCalled() {
_;
}
function testRevertsWhenMarketHasMoreThanTenAssets() external givenCheckUpkeepIsCalled {
// Deploy the mock that simulates a market returning 11 assets
MockMarketMakingEngineForFeeConversion mockEngine = new MockMarketMakingEngineForFeeConversion();
​
// Configure FeeConversionKeeper using the mock engine
changePrank({ msgSender: users.owner.account });
feeConversionKeeper = ChainlinkAutomationUtils.deployFeeConversionKeeper(
users.owner.account,
address(mockEngine),
1, // Dummy dexSwapStrategyId
1 // Dummy minFeeDistributionValueUsd
);
​
// Expect a revert upon calling checkUpkeep due to out-of-bounds array access
vm.expectRevert();
FeeConversionKeeper(feeConversionKeeper).checkUpkeep("");
}
}
​
/**
* @notice Minimal implementation of IMarketMakingEngine used by FeeConversionKeeper
* that simulates:
* - A single market (getLiveMarketIds returns [1]).
* - getReceivedMarketFees returning arrays of 11 elements, exceeding
* the assumed size of (1 * 10) in FeeConversionKeeper.checkUpkeep.
* - getAssetValue returning a high enough amount to ensure distributionNeeded = true.
* - getDexSwapStrategy is implemented to prevent the keeper initialization from reverting.
*/
contract MockMarketMakingEngineForFeeConversion {
/// @dev Returns a dummy DexSwapStrategy.Data to ensure the FeeConversionKeeper can be initialized
function getDexSwapStrategy(uint128 /*dexSwapStrategyId*/) external pure returns (DexSwapStrategy.Data memory) {
DexSwapStrategy.Data memory data;
data.dexAdapter = address(1); // Non-zero dummy value to pass validation
return data;
}
​
function getLiveMarketIds() external pure returns (uint128[] memory) {
uint128[] memory marketIds = new uint128[](1);
marketIds[0] = 1;
return marketIds;
}
​
function getReceivedMarketFees(uint128 /*marketId*/) external pure returns (address[] memory, uint256[] memory) {
// Return arrays of 11 elements to force an out-of-bounds scenario in FeeConversionKeeper
address[] memory assets = new address[](11);
uint256[] memory fees = new uint256[](11);
for (uint256 i = 0; i < 11; i++) {
assets[i] = address(uint160(i + 1)); // Dummy addresses
fees[i] = 100; // Dummy fee values
}
return (assets, fees);
}
​
function getAssetValue(address /*asset*/, uint256 /*collectedFee*/) external pure returns (uint256) {
// Return a value above the minimum to ensure distributionNeeded remains true
return 1000;
}
}
​
[PASS] testRevertsWhenMarketHasMoreThanTenAssets() (gas: 1966414)
│ │ │ └─ ← [Return] 1000
│ │ ├─ [324] MockMarketMakingEngineForFeeConversion::getAssetValue(0x000000000000000000000000000000000000000b, 100) [staticcall]
│ │ │ └─ ← [Return] 1000
│ │ └─ ← [Revert] panic: array out-of-bounds access (0x32)
│ └─ ← [Revert] panic: array out-of-bounds access (0x32)
└─ ← [Stop]

Confirmation

Using fixed-size arrays under the current logic can lead to an out-of-bounds revert when a market has more than 10 assets. This confirms the existence of the issue and highlights the potential critical impact on the automatic fee-conversion process.

Tools Used

Manual Code Review
A thorough inspection of the contract code was conducted to identify the logic flaw in array sizing and indexing. This method allowed for a detailed analysis of potential out-of-bounds writes within the checkUpkeep function.

Recommendations

  • Validate the number of assets per market to ensure it never exceeds the fixed array limit.

  • Use dynamic arrays or implement boundary checks before writing to marketIds and assets to avoid out-of-bounds writes.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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