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

Potential denial of service in FeeConversionKeeper's checkUpkeep Function

zubyoz

Summary

The FeeConversionKeeper contract's checkUpkeep function is vulnerable to a Denial of Service (DoS) attack due to unbounded iteration over markets and their associated fees. An attacker could create multiple markets with small fees just above the minimum threshold, causing the function to consume excessive gas and potentially making the keeper operations unfeasible.

Vulnerability Details

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol#L69-L107

The vulnerability exists in the checkUpkeep function where there's no limit on the number of markets and fees that can be processed:

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[]();
address[] memory assets = new address[]();
// Unbounded iteration over markets
for (uint128 i; i < liveMarketIds.length; i++) {
marketId = liveMarketIds[i];
// Nested unbounded iteration over fees
for (uint128 j; j < marketAssets.length; j++) {
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
// ...
}
}
}

The root cause is:

  1. No limit on the number of markets that can be processed

  2. No batch size restriction for fee processing

  3. Nested loops that can lead to quadratic complexity

  4. Array initialization with potentially large sizes (liveMarketIds.length * 10)

Impact

The vulnerability could lead to:
Failed keeper operations due to excessive gas consumption
Delayed or stuck fee conversions
System becoming unusable as the number of markets and small fees grows
Potential DoS of the entire fee conversion mechanism

Proof of Concept

function test_DenialOfServiceInCheckUpkeep() external givenCheckUpkeepIsCalled {
// Setup initial conditions with minimal threshold
uint128 minFeeDistributionValueUsd = 1;
configureFeeConversionKeeper(1, minFeeDistributionValueUsd);
​
// Number of markets to create for DoS demonstration
uint256 NUM_MARKETS = 50;
uint256 smallFeeAmount = minFeeDistributionValueUsd + 1; // Just above minFeeDistributionValueUsd
​
// Create and configure markets with small fees
for(uint256 i = 0; i < NUM_MARKETS; i++) {
// Get market configuration
PerpMarketCreditConfig memory marketConfig = getFuzzPerpMarketCreditConfig(i);
// Deal USDC to the engine
deal(address(usdc), address(marketConfig.engine), smallFeeAmount * 5);
vm.startPrank(address(marketConfig.engine));
usdc.approve(address(marketMakingEngine), type(uint256).max);
// Register multiple small fees for each market
for(uint256 j = 0; j < 5; j++) {
marketMakingEngine.receiveMarketFee(
marketConfig.marketId,
address(usdc),
smallFeeAmount
);
}
vm.stopPrank();
}
​
// Measure gas consumption for checkUpkeep
uint256 gasStart = gasleft();
(bool upkeepNeeded, bytes memory performData) = FeeConversionKeeper(feeConversionKeeper).checkUpkeep("");
uint256 gasUsed = gasStart - gasleft();
​
// Decode perform data
(uint128[] memory marketIds, address[] memory assets) = abi.decode(performData, (uint128[], address[]));
​
// Log the results
console.log("Gas Used:", gasUsed);
console.log("Number of Markets to Process:", marketIds.length);
​
// Assertions
assertTrue(upkeepNeeded, "Upkeep should be needed with many small fees");
assertGt(gasUsed, 200000, "Should consume significant gas");
assertGt(marketIds.length, 0, "Should have markets to process");
assertEq(marketIds.length, assets.length, "Arrays should be equal length");
​
// Use the keeper's owner address for the test
address keeperOwner;
(keeperOwner,,) = FeeConversionKeeper(feeConversionKeeper).getConfig();
// Test performUpkeep with large dataset
vm.startPrank(keeperOwner);
vm.expectRevert();
FeeConversionKeeper(feeConversionKeeper).performUpkeep(performData);
vm.stopPrank();
}
}

Test Results:

Gas Used: 278902
Number of Markets to Process: 100

Tools Used

  • Manual code review

Recommendations

  1. Implement a maximum batch size for processing:

uint256 public constant MAX_BATCH_SIZE = 20;

  1. Add pagination support:

function checkUpkeep(bytes calldata data) external view returns (bool upkeepNeeded, bytes memory performData) {
uint256 startIndex = data.length > 0 ? abi.decode(data, (uint256)) : 0;
// ... process MAX_BATCH_SIZE items starting from startIndex
}

  1. Implement minimum thresholds per asset to prevent spam with small amounts

  2. Add a maximum limit on the number of assets per market

  3. Consider implementing a queue system for fee processing to ensure fair ordering

  4. Add emergency functions to handle edge cases where the system becomes congested

here is a link to a similar issue

https://solodit.cyfrin.io/issues/an-attacker-can-flood-the-protocol-with-false-offers-to-cause-dos-cyfrin-none-cyfrin-tunnl-v20-markdown

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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