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

FeeConversionKeeper.performUpkeep will fail due to empty vaules in array

glightspeed2

Summary

Incorrect array lengths returned from FeeConversionKeeper.checkUpkeep will cause revert in FeeConversionKeeper.performUpkeep

Vulnerability Details

Root Cause

FeeConversionKeeper.checkUpkeep returns ABI-encoded bytes of marketIds and assets that requires distribution. These marketIds and assets have extended length liveMarketIds.length * 10 to cover all possible combination of marketIds and assets .

However, these lengths are usually more than enough, the rest of the array items will be filled with default values (address(0), uint256(0))

This will cause revert in FeeConversionKeeper.performUpkeep.

POC

The following is a modified version of performUpkeep.t.sol. Instead of using hard-coded performData, POC will use returned value from checkUpkeep

pragma solidity 0.8.25;
​
import { Base_Test } from "test/Base.t.sol";
import { Errors } from "@zaros/utils/Errors.sol";
import { FeeConversionKeeper } from "@zaros/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol";
import { FeeDistributionBranch } from "@zaros/market-making/branches/FeeDistributionBranch.sol";
​
contract FeeConversionKeeper_PerformUpkeep_Integration_Test is Base_Test {
function setUp() public virtual override {
Base_Test.setUp();
changePrank({ msgSender: address(users.owner.account) });
createVaults(marketMakingEngine, INITIAL_VAULT_ID, FINAL_VAULT_ID, true, address(perpsEngine));
configureMarkets();
}
​
modifier givenInitializeContract() {
_;
}
​
function testFuzz_GivenCallPerformUpkeepFunction(
uint256 marketId,
uint256 amount,
uint256 minFeeDistributionValueUsd
)
external
givenInitializeContract
{
PerpMarketCreditConfig memory fuzzPerpMarketCreditConfig = getFuzzPerpMarketCreditConfig(marketId);
​
minFeeDistributionValueUsd = bound({
x: minFeeDistributionValueUsd,
min: 1,
max: convertUd60x18ToTokenAmount(address(usdc), USDC_DEPOSIT_CAP_X18)
});
​
amount = bound({
x: amount,
min: minFeeDistributionValueUsd,
max: convertUd60x18ToTokenAmount(address(usdc), USDC_DEPOSIT_CAP_X18)
});
deal({ token: address(usdc), to: address(fuzzPerpMarketCreditConfig.engine), give: amount });
​
changePrank({ msgSender: users.owner.account });
​
configureFeeConversionKeeper(1, uint128(minFeeDistributionValueUsd));
​
FeeConversionKeeper(feeConversionKeeper).setForwarder(users.keepersForwarder.account);
​
changePrank({ msgSender: address(fuzzPerpMarketCreditConfig.engine) });
​
marketMakingEngine.receiveMarketFee(fuzzPerpMarketCreditConfig.marketId, address(usdc), amount);
​
changePrank({ msgSender: users.keepersForwarder.account });
// use performData from checkUpkeep function, instead of using hard-coded value
(bool upkeepNeeded, bytes memory performData) = FeeConversionKeeper(feeConversionKeeper).checkUpkeep("");
(uint128[] memory marketIds, address[] memory assets) = abi.decode(performData, (uint128[], address[]));
// sanity check
assertEq(upkeepNeeded, true);
assertEq(assets[0], address(usdc));
assertEq(marketIds[0], fuzzPerpMarketCreditConfig.marketId);
// revert due to empty item in the array
vm.expectRevert(abi.encodeWithSelector(Errors.CollateralDisabled.selector, address(0)));
FeeConversionKeeper(feeConversionKeeper).performUpkeep(performData);
}
}
​

Impact

  • FeeConversionKeeper.performUpkeep will fail for most of the times

  • Non-WETH received fees won't be distributed to stakers

Tools Used

Foundry

Recommendations

Shrink marketIds and assets to appropriate length like the following:

diff --git a/src/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol b/src/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol
index 533908a..0511739 100644
--- a/src/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol
+++ b/src/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol
@@ -102,6 +102,11 @@ contract FeeConversionKeeper is IAutomationCompatible, BaseKeeper {
}
if (upkeepNeeded) {
+ assembly {
+ mstore(marketIds, index)
+ mstore(assets, index)
+ }
+
performData = abi.encode(marketIds, assets);
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

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