Summary:
The update function within the MarketConfiguration library fails to update the priceFeedHeartbeatSeconds variable, which is essential for the getIndexPrice function to operate correctly. This oversight causes the getIndexPrice function to always revert due to an uninitialized heartbeat check.
Vulnerability Detail:
In the MarketConfiguration library, the update function is designed to update various market configuration parameters. However, it neglects to update the priceFeedHeartbeatSeconds variable. Consequently, this variable remains uninitialized, defaulting to zero.
The getIndexPrice function relies on the priceFeedHeartbeatSeconds variable to validate the timeliness of the price feed data from Chainlink oracles. When it checks if (block.timestamp - updatedAt > priceFeedHeartbeatSeconds), the comparison always results in true (since priceFeedHeartbeatSeconds is zero), causing the function to revert every time it is called.
Impact:
This vulnerability significantly impacts the functionality of the protocol by making the price checking mechanism always revert. As a result, it effectively halts the correct operation of any process relying on the getIndexPrice function. Specifically, it renders the entire price validation mechanism inoperative, potentially disrupting market operations.
These are the list of functions that got affected by this vulnerability includes:
PerpMarket::getIndexPrice
TradingAccountBranch::getAccountEquityUsd
TradingAccountBranch::getAccountMarginBreakdown
TradingAccountBranch::getAccountTotalUnrealizedPnl
TradingAccountBranch::getAccountLeverage
TradingAccountBranch::withdrawMargin
SettlementBranch::fillMarketOrder
SettlementBranch::fillOffchainOrders
OrderBranch::simulateTrade
OrderBranch::getMarginRequirementForTrade
LiquidationBranch::liquidateAccount
LiquidationBranch::checkLiquidatableAccounts
Code Snippet:
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/MarketConfiguration.sol#L37-L47
struct Data {
string name;
string symbol;
address priceAdapter;
uint128 initialMarginRateX18;
uint128 maintenanceMarginRateX18;
uint128 maxOpenInterest;
uint128 maxSkew;
uint128 maxFundingVelocity;
uint128 minTradeSizeX18;
uint256 skewScale;
OrderFees.Data orderFees;
uint32 priceFeedHeartbeatSeconds;
}
function update(Data storage self, Data memory params) internal {
self.name = params.name;
self.symbol = params.symbol;
self.priceAdapter = params.priceAdapter;
self.initialMarginRateX18 = params.initialMarginRateX18;
self.maintenanceMarginRateX18 = params.maintenanceMarginRateX18;
self.maxOpenInterest = params.maxOpenInterest;
self.maxSkew = params.maxSkew;
self.maxFundingVelocity = params.maxFundingVelocity;
self.minTradeSizeX18 = params.minTradeSizeX18;
self.skewScale = params.skewScale;
self.orderFees = params.orderFees;
@>
}
Proof Of Concept:
While creating a new market configuration, the priceFeedHeartbeatSeconds param is given a non-zero value.
The update function is called to update the market configuration.
The update function never initializes the priceFeedHeartbeatSeconds variable, causing it to default to zero.
When the getIndexPrice function is called, that invokes the Chainlink oracle to fetch the price data.
The function getPrice reverts due to the uninitialized priceFeedHeartbeatSeconds variable.
Here is the commands to test:
[rpc_endpoints]
arbitrum_sepolia = "https://arb-sepolia.g.alchemy.com/v2/${API_KEY_ALCHEMY}
To test the POC, run the following Forge test:
forge test --mt test_FortisAudits_IndexPriceGetsDOS -vvvv
Proof Of Code:
Add the following code to the Base.t.sol file:
function createPerpMarketsFork() internal {
createPerpMarkets(
users.owner.account,
perpsEngine,
INITIAL_MARKET_ID,
FINAL_MARKET_ID,
IVerifierProxy(mockChainlinkVerifier),
false
);
for (uint256 i = INITIAL_MARKET_ID; i <= FINAL_MARKET_ID; i++) {
vm.label({ account: marketOrderKeepers[i], newLabel: "Market Order Keeper Fork" });
}
}
POC:
pragma solidity ^0.8.25;
import { Base_Test } from "./Base.t.sol";
import { UD60x18, ud60x18 } from "@prb-math/UD60x18.sol";
import { SD59x18 } from "@prb-math/SD59x18.sol";
import { Errors } from "@zaros/utils/Errors.sol";
contract BugTest is Base_Test {
function setUp() public override {
uint256 forkId = vm.createFork("https://sepolia-rollup.arbitrum.io/rpc");
vm.selectFork(forkId);
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarketsFork();
}
function test_FortisAudits_IndexPriceGetsDOS() public {
address ETH_USD = address(0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165);
deal({ token: address(usdc), to: users.naruto.account, give: 1000e6 });
changePrank({ msgSender: users.naruto.account });
createAccountAndDeposit(1000e6, address(usdc));
vm.expectRevert(abi.encodeWithSelector(Errors.OraclePriceFeedHeartbeat.selector, ETH_USD));
perpsEngine.exposed_getIndexPrice(2);
}
}
Forge Test stack trace:
├─ [36319] Perps Engine::exposed_getIndexPrice(2) [staticcall]
│ ├─ [31069] PerpMarketHarness::exposed_getIndexPrice(2) [delegatecall]
│ │ ├─ [27534] PerpMarket::a75e8fff(f2a828f98ebacfcc9ead142282f1b781df57bae404a8d1fa83ab68dfbae58f25) [delegatecall]
│ │ │ ├─ [5595] 0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165::decimals() [staticcall]
│ │ │ │ ├─ [253] 0xf3138B59cAcbA1a4d7d24fA7b184c20B3941433e::decimals() [staticcall]
│ │ │ │ │ └─ ← [Return] 8
│ │ │ │ └─ ← [Return] 8
│ │ │ ├─ [11235] 0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165::latestRoundData() [staticcall]
│ │ │ │ ├─ [7502] 0xf3138B59cAcbA1a4d7d24fA7b184c20B3941433e::latestRoundData() [staticcall]
│ │ │ │ │ └─ ← [Return] 330001 [3.3e5], 314332890000 [3.143e11], 1721921867 [1.721e9], 1721921867 [1.721e9], 330001 [3.3e5]
│ │ │ │ └─ ← [Return] 18446744073709881617 [1.844e19], 314332890000 [3.143e11], 1721921867 [1.721e9], 1721921867 [1.721e9], 18446744073709881617 [1.844e19]
│ │ │ └─ ← [Revert] OraclePriceFeedHeartbeat(0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165)
│ │ └─ ← [Revert] OraclePriceFeedHeartbeat(0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165)
│ └─ ← [Revert] OraclePriceFeedHeartbeat(0xd30e2101a97dcbAeBCBC04F14C3f624E67A35165)
└─ ← [Stop]
Recommendations
To resolve this issue, ensure that the priceFeedHeartbeatSeconds variable is appropriately updated within the update function. Here is the recommended mitigation:
struct Data {
string name;
string symbol;
address priceAdapter;
uint128 initialMarginRateX18;
uint128 maintenanceMarginRateX18;
uint128 maxOpenInterest;
uint128 maxSkew;
uint128 maxFundingVelocity;
uint128 minTradeSizeX18;
uint256 skewScale;
OrderFees.Data orderFees;
uint32 priceFeedHeartbeatSeconds;
}
/// @notice Updates the given market configuration.
/// @dev See {MarketConfiguration.Data} for parameter details.
function update(Data storage self, Data memory params) internal {
self.name = params.name;
self.symbol = params.symbol;
self.priceAdapter = params.priceAdapter;
self.initialMarginRateX18 = params.initialMarginRateX18;
self.maintenanceMarginRateX18 = params.maintenanceMarginRateX18;
self.maxOpenInterest = params.maxOpenInterest;
self.maxSkew = params.maxSkew;
self.maxFundingVelocity = params.maxFundingVelocity;
self.minTradeSizeX18 = params.minTradeSizeX18;
self.skewScale = params.skewScale;
self.orderFees = params.orderFees;
+ self.priceFeedHeartbeatSeconds = params.priceFeedHeartbeatSeconds;
}
By including this update, the priceFeedHeartbeatSeconds variable will hold the correct value, allowing the getIndexPrice function to perform as intended without unnecessary reverts.
