Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub mod AssetsComponent {
};
use starkware_utils::storage::utils::{AddToStorage, SubFromStorage};
use starkware_utils::time::time::{Time, TimeDelta, Timestamp};
use crate::core::types::asset::RiskConfig;

#[storage]
pub struct Storage {
Expand All @@ -70,6 +71,8 @@ pub mod AssetsComponent {
num_of_active_synthetic_assets: usize,
#[rename("synthetic_config")]
pub asset_config: Map<AssetId, Option<AssetConfig>>,
pub risk_config: Map<AssetId, RiskConfig>,
pub risk_factor_tiers_opt: Map<AssetId, Map<u64, RiskFactor>>,
#[rename("synthetic_timely_data")]
pub asset_timely_data: IterableMap<AssetId, AssetTimelyData>,
pub risk_factor_tiers: Map<AssetId, Vec<RiskFactor>>,
Expand Down Expand Up @@ -151,6 +154,33 @@ pub mod AssetsComponent {
self.emit(events::OracleAdded { asset_id, asset_name, oracle_public_key, oracle_name });
}

fn migrate_risk(ref self: ComponentState<TContractState>) {
for (asset_id, _) in self.asset_timely_data {
// pub risk_factor_first_tier_boundary: u128,
// /// - `risk_factor_tier_size` — 92-bit field stored in a `u128`.
// pub risk_factor_tier_size: u128,
// pub len: u32,
let x = self.asset_config.read(asset_id).unwrap();

self
.risk_config
.write(
asset_id,
RiskConfig {
risk_factor_first_tier_boundary: x.risk_factor_first_tier_boundary,
risk_factor_tier_size: x.risk_factor_tier_size,
len: self.risk_factor_tiers.entry(asset_id).len().try_into().unwrap(),
},
);
let vec = self.risk_factor_tiers.entry(asset_id);
for i in 0..vec.len() {
let value = vec.at(i.into()).read();
self.risk_factor_tiers_opt.entry(asset_id).write(i.into(), value);
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unprotected Migration Overwrites Current Risk Configurations

The migrate_risk function is a public, unprotected migration function. Anyone can call it, allowing current risk configurations to be overwritten with initial configuration data from the old storage structures.

Fix in Cursor Fix in Web



/// Add asset is called by the app governer to add a new synthetic asset.
///
/// Validations:
Expand Down Expand Up @@ -548,26 +578,22 @@ pub mod AssetsComponent {
balance: Balance,
price: Price,
) -> RiskFactor {
if let Option::Some(asset_config) = self.asset_config.read(asset_id) {
let asset_risk_factor_tiers = self.risk_factor_tiers.entry(asset_id);
let synthetic_value: u128 = price.mul(rhs: balance).abs();
let index = if synthetic_value < asset_config.risk_factor_first_tier_boundary {
0_u128
} else {
let tier_size = asset_config.risk_factor_tier_size;
let first_tier_offset = synthetic_value
- asset_config.risk_factor_first_tier_boundary;
min(
1_u128 + (first_tier_offset / tier_size),
asset_risk_factor_tiers.len().into() - 1,
)
};
asset_risk_factor_tiers
.at(index.try_into().expect('INDEX_SHOULD_NEVER_OVERFLOW'))
.read()
let risk_config = self.risk_config.read(asset_id);
let synthetic_value: u128 = price.mul(rhs: balance).abs();
let index = if synthetic_value < risk_config.risk_factor_first_tier_boundary {
0_u128
} else {
panic_with_felt252(ASSET_NOT_EXISTS)
}
let first_tier_offset = synthetic_value
- risk_config.risk_factor_first_tier_boundary;
min(
1_u128 + (first_tier_offset / risk_config.risk_factor_tier_size),
risk_config.len.into() - 1,
)
};
self
.risk_factor_tiers_opt
.entry(asset_id)
.read(index.try_into().expect('INDEX_SHOULD_NEVER_OVERFLOW'))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Asset Risk Factor Calculation Fails Silently

The get_asset_risk_factor function no longer checks for asset existence. It now directly reads risk_config and risk_factor_tiers_opt, which can return default values for non-existent assets or if migration data is missing. This results in incorrect risk factor calculations instead of the expected panic for invalid assets.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: New Assets Lack Risk Config Initialization

get_asset_risk_factor reads from risk_config and risk_factor_tiers_opt storage fields, but the _add_asset function (which adds new assets) does not populate these fields. Only the migrate_risk function populates them. This means newly added assets will have default/zero values for risk configuration, causing incorrect risk factor calculations until migrate_risk is manually called. The _add_asset function should populate these new storage fields when adding an asset.

Fix in Cursor Fix in Web

}

fn get_funding_index(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ pub trait IAssets<TContractState> {
);
fn update_asset_quorum(ref self: TContractState, asset_id: AssetId, quorum: u8);

fn migrate_risk(ref self: TContractState);

// View functions.
fn get_collateral_token_contract(self: @TContractState) -> IERC20Dispatcher;
fn get_collateral_quantum(self: @TContractState) -> u64;
Expand Down
33 changes: 28 additions & 5 deletions workspace/apps/perpetuals/contracts/src/core/core.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,22 @@ pub mod Core {
fulfillment_entry.write(total_amount);
}

fn _validate_order(ref self: ContractState, order: Order) {
fn _validate_order(
ref self: ContractState,
order: Order,
now: Option<Timestamp>,
collateral_id: Option<AssetId>,
) -> (Timestamp, AssetId) {
let now = if now.is_none() {
Time::now()
} else {
now.unwrap()
};
let collateral_id = if collateral_id.is_none() {
self.assets.get_collateral_id()
} else {
collateral_id.unwrap()
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Redundant Time::now() Call Breaks Timestamp Consistency

Duplicate Time::now() call in _validate_order function. The now variable is already computed at lines 1108-1112, but line 1127 calls Time::now() again, overwriting the parameter value. This defeats the purpose of passing now as an optional parameter for reuse between order_a and order_b validation, potentially causing inconsistent timestamp checks.

Fix in Cursor Fix in Web

// Verify that position is not fee position.
assert(order.position_id != FEE_POSITION, CANT_TRADE_WITH_FEE_POSITION);
// This is to make sure that the fee is relative to the quote amount.
Expand All @@ -1119,9 +1134,9 @@ pub mod Core {
assert(!have_same_sign(order.quote_amount, order.base_amount), INVALID_AMOUNT_SIGN);

// Validate asset ids.
let collateral_id = self.assets.get_collateral_id();
assert(order.quote_asset_id == collateral_id, ASSET_ID_NOT_COLLATERAL);
assert(order.fee_asset_id == collateral_id, ASSET_ID_NOT_COLLATERAL);
(now, collateral_id)
}

fn _validate_trade(
Expand All @@ -1135,12 +1150,20 @@ pub mod Core {
) {
// Base asset check.
assert(order_a.base_asset_id == order_b.base_asset_id, DIFFERENT_BASE_ASSET_IDS);
self.assets.validate_active_asset(asset_id: order_a.base_asset_id);
// self.assets.validate_active_asset(asset_id: order_a.base_asset_id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Active Asset Validation Bypass

In _validate_trade, commenting out self.assets.validate_active_asset removes the check ensuring trades use active base assets. This allows trading with inactive assets, which violates business logic and could lead to unintended system behavior.

Fix in Cursor Fix in Web


assert(order_a.position_id != order_b.position_id, INVALID_SAME_POSITIONS);

self._validate_order(order: order_a);
self._validate_order(order: order_b);
let (now, collateral_id) = self
._validate_order(
order: order_a, now: Default::default(), collateral_id: Default::default(),
);
self
._validate_order(
order: order_b,
now: Option::Some(now),
collateral_id: Option::Some(collateral_id),
);

// Non-zero actual amount check.
assert(actual_amount_base_a.is_non_zero(), INVALID_ZERO_AMOUNT);
Expand Down
41 changes: 40 additions & 1 deletion workspace/apps/perpetuals/contracts/src/core/types/asset.cairo
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
use core::num::traits::Pow;
use core::num::traits::zero::Zero;
use perpetuals::core::types::balance::Balance;
use perpetuals::core::types::funding::FundingIndex;
use perpetuals::core::types::price::Price;
use perpetuals::core::types::risk_factor::RiskFactor;
use starknet::storage::StoragePointer0Offset;
use starknet::storage_access::storage_address_from_base_and_offset;
use starknet::storage_access::{StorePacking, storage_address_from_base_and_offset};
use starknet::syscalls::storage_read_syscall;
use starknet::{ContractAddress, SyscallResultTrait};
use starkware_utils::time::time::Timestamp;
const TWO_POW_92: u128 = 2_u128.pow(92);
const MASK_92: u128 = TWO_POW_92 - 1;

#[derive(Copy, Debug, Default, Drop, Hash, PartialEq, Serde, starknet::Store)]
pub struct AssetId {
Expand Down Expand Up @@ -101,6 +104,42 @@ pub struct AssetConfig {
pub asset_type: AssetType // V2
}


#[derive(Copy, Drop, Serde)]
pub struct RiskConfig {
/// - `risk_factor_first_tier_boundary` — 92-bit field stored in a `u128`.
pub risk_factor_first_tier_boundary: u128,
/// - `risk_factor_tier_size` — 92-bit field stored in a `u128`.
pub risk_factor_tier_size: u128,
pub len: u32,
}

/// ┌──────────────────────────────────────────────────────────────┐
/// │ 252-bit felt (interpreted as uint256) │
/// ├─────────────────────────┬─────────────────┬──────────────────┤
/// │ bitadd s 1–92 │ bits 93–124 │ bits 129–220 │
/// │ risk_factor_first_tier_boundary (92 bits) │ len (32 bits) │
/// │ risk_factor_tier_size (92 bits) │ │
/// └─────────────────────────┴─────────────────┴──────────────────┘
impl StorePackingRiskConfig of StorePacking<RiskConfig, felt252> {
fn pack(value: RiskConfig) -> felt252 {
let low = value.risk_factor_first_tier_boundary + (value.len.into()) * TWO_POW_92;
let high = value.risk_factor_tier_size;
let result = u256 { low, high };
result.try_into().unwrap()
}

/// Unpacks a storage representation back into the original type.
fn unpack(value: felt252) -> RiskConfig {
let u256 { low, high } = value.into();
let risk_factor_first_tier_boundary = low & MASK_92;
let risk_factor_tier_size = high;
let len: u32 = (low / TWO_POW_92).try_into().unwrap();
RiskConfig { risk_factor_first_tier_boundary, risk_factor_tier_size, len }
}
}


#[derive(Copy, Drop, Serde, starknet::Store)]
pub struct AssetTimelyData {
version: u8,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use perpetuals::core::components::assets::interface::{IAssetsDispatcher, IAssetsDispatcherTrait};
use perpetuals::core::core::Core::{InternalCoreFunctions, SNIP12MetadataImpl};
use perpetuals::core::interface::{ICoreDispatcher, ICoreDispatcherTrait, Settlement};
use perpetuals::core::types::order::Order;
Expand All @@ -13,6 +14,7 @@ use starkware_utils::storage::iterable_map::*;
use starkware_utils::time::time::Timestamp;
use starkware_utils_testing::test_utils::cheat_caller_address_once;


// Performance test for Core contract multi-trade execution.
//
// This test file is designed to measure the gas usage of executing multiple
Expand Down Expand Up @@ -630,6 +632,9 @@ fn test_performance() {
let dispatcher = ICoreDispatcher { contract_address: CONTRACT_ADDRESS };
let trades = settlements();

let assetdispatcher = IAssetsDispatcher { contract_address: CONTRACT_ADDRESS };
assetdispatcher.migrate_risk();

cheat_caller_address_once(contract_address: CONTRACT_ADDRESS, caller_address: OPERATOR_ADDRESS);
dispatcher.multi_trade(operator_nonce: CURRENT_OPERATOR_NONCE, :trades);
}
Loading