Submission Details
Severity:
FeeConversionKeeper.performUpkeep always reverts because of incorrect array management in checkUpkeep
Summary
checkUpkeep() function in FeeConversionKeeper.sol creates marketIds and assets arrays with excessive size, so uninitialized elements set to uint128(0) and address(0). This causes a revert in performUpkeep().
Vulnerability Details
marketIds and assets arrays length is too big in FeeConversionKeeper.sol:checkUpkeep():
function checkUpkeep(bytes calldata /**/ ) external view returns (bool upkeepNeeded, bytes memory performData) {
FeeConversionKeeperStorage memory self = _getFeeConversionKeeperStorage();
//@audit for example liveMarketIds.length = 2
uint128[] memory liveMarketIds = self.marketMakingEngine.getLiveMarketIds();
bool distributionNeeded;
//@audit marketIds.length = liveMarketIds.length * 10 = 2 * 10 = 20
uint128[] memory marketIds = new uint128[]();
//@audit assets.length = liveMarketIds.length * 10 = 2 * 10 = 20
address[] memory assets = new address[]();
uint256 index = 0;
uint128 marketId;
// Iterate over markets by id
for (uint128 i; i < liveMarketIds.length; i++) {
marketId = liveMarketIds[i];
//@audit for example every market has 2 assets
(address[] memory marketAssets, uint256[] memory feesCollected) =
self.marketMakingEngine.getReceivedMarketFees(marketId);
// Iterate over receivedMarketFees
for (uint128 j; j < marketAssets.length; j++) {
//@audit for tests set distributionNeeded always = true
distributionNeeded = checkFeeDistributionNeeded(marketAssets[j], feesCollected[j]);
if (distributionNeeded) {
// set upkeepNeeded = true
upkeepNeeded = true;
//@audit marketIds[0] = uint128(someRealNumber)
//@audit assets[0] = address(someRealAddress)
//@audit marketIds[1] = uint128(someRealNumber)
//@audit assets[1] = address(someRealAddress)
//@audit marketIds[2] = uint128(someRealNumber)
//@audit assets[2] = address(someRealAddress)
//@audit marketIds[3] = uint128(someRealNumber)
//@audit assets[3] = address(someRealAddress)
//@audit so in this example max length of marketIds and assets = 4
//@audit because liveMarketIds.length * marketAssets.length = 2 * 2 = 4
// set marketId, asset
marketIds[index] = marketId;
assets[index] = marketAssets[j];
index++;
}
}
}
//@audit adding marketIds and assets arrays with uninitialized values
if (upkeepNeeded) {
performData = abi.encode(marketIds, assets);
}
}
FeeConversionKeeper.sol:performUpkeep():
// call FeeDistributionBranch::convertAccumulatedFeesToWeth
function performUpkeep(bytes calldata performData) external override onlyForwarder {
FeeConversionKeeperStorage memory self = _getFeeConversionKeeperStorage();
IMarketMakingEngine marketMakingEngine = self.marketMakingEngine;
// decode performData
(uint128[] memory marketIds, address[] memory assets) = abi.decode(performData, (uint128[], address[]));
// convert accumulated fees to weth for decoded markets and assets
//@audit using all marketIds and assets, not only initialized ones leads to revert
for (uint256 i; i < marketIds.length; i++) {
marketMakingEngine.convertAccumulatedFeesToWeth(marketIds[i], assets[i], self.dexSwapStrategyId, "");
}
}
PoC
Create test file in test/integration/external/chainlink/keepers/fee-conversion/performUpkeep/ folder with name performUpkeepRevert.t.sol:
// SPDX-License-Identifier: MIT
// Zaros dependencies
import { Base_Test } from "test/Base.t.sol";
import { FeeConversionKeeper } from "@zaros/external/chainlink/keepers/fee-conversion-keeper/FeeConversionKeeper.sol";
contract FeeConversionKeeper_RevertPerformUpkeep_Integration_Test is Base_Test {
function setUp() public virtual override {
Base_Test.setUp();
changePrank({ msgSender: address(users.owner.account) });
configureMarkets();
changePrank({ msgSender: address(users.naruto.account) });
}
modifier givenCheckUpkeepIsCalled() {
_;
}
function test_FeeConversionKeeperRevert(
uint256 marketId,
uint256 amount,
uint256 minFeeDistributionValueUsd
)
external
givenCheckUpkeepIsCalled
{
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, 1);
FeeConversionKeeper(feeConversionKeeper).setForwarder(users.keepersForwarder.account);
changePrank({ msgSender: address(fuzzPerpMarketCreditConfig.engine) });
marketMakingEngine.receiveMarketFee(fuzzPerpMarketCreditConfig.marketId, address(usdc), amount);
//get upkeepNeeded and data
(bool upkeepNeeded, bytes memory performData) = FeeConversionKeeper(feeConversionKeeper).checkUpkeep("");
(uint128[] memory marketIds, address[] memory assets) = abi.decode(performData, (uint128[], address[]));
// it should return true
assertTrue(upkeepNeeded);
//check that first asset and market is ok
assertEq(assets[0], address(usdc));
assertEq(marketIds[0], fuzzPerpMarketCreditConfig.marketId);
//set up new arrays to mock call perform upkeep
//it's the same array as before, but without usdc
//it doesn't really matter, just to scip usdc and show that it will revert with address(0)
for (uint256 i = 1; i < marketIds.length; i++) {
marketIds[i - 1] = marketIds[i];
assets[i - 1] = assets[i];
}
performData = abi.encode(marketIds, assets);
changePrank({ msgSender: users.keepersForwarder.account });
//revert cause contract loads data for marketIds = 0 and assets = address(0)
vm.expectRevert();
FeeConversionKeeper(feeConversionKeeper).performUpkeep(performData);
}
}
In cmd run this command:
forge test -vvvvv --mt test_FeeConversionKeeperRevert
Output:
│ │ ├─ [8743] MarketMakingEngine::convertAccumulatedFeesToWeth(0, 0x0000000000000000000000000000000000000000, 1, 0x)
│ │ │ ├─ [5981] FeeDistributionBranch::convertAccumulatedFeesToWeth(0, 0x0000000000000000000000000000000000000000, 1, 0x) [delegatecall]
│ │ │ │ └─ ← [Revert] CollateralDisabled(0x0000000000000000000000000000000000000000)
│ │ │ └─ ← [Revert] CollateralDisabled(0x0000000000000000000000000000000000000000)
│ │ └─ ← [Revert] CollateralDisabled(0x0000000000000000000000000000000000000000)
│ └─ ← [Revert] CollateralDisabled(0x0000000000000000000000000000000000000000)
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.05s (3.01s CPU time)
Impact
It is impossible to convert accumulated fees to WETH through FeeConversionKeeper.performUpkeep()
Tools Used
Manual Review
Recommendations
marketIds and assets arrays length should be equal to all markets assets combined (if distributionNeeded = true for asset).
Updates
Lead Judging Commences
inallhonesty 6 months ago
Submission Judgement Published Assigned finding tags:
FeeConversionKeeper performUpkeep iterates over entire oversized arrays instead of just populated entries, causing reverts on address(0) slots
Prize pool breakdown
Total prize
70,000 USDC
nSLOC
3,25821 USDC / LOC
66,500
3,500
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.