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
4 changes: 2 additions & 2 deletions Scarb.lock
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,15 @@ dependencies = [
[[package]]
name = "starkware_utils"
version = "1.0.0"
source = "git+https://github.com/starkware-libs/starkware-starknet-utils?branch=main#aa8b89ae7febcd207d83f16ba3f953cf5b8e3884"
source = "git+https://github.com/starkware-libs/starkware-starknet-utils?branch=main#a8bfc260e603a2768b9c8fd94dabfd292adc69ca"
dependencies = [
"openzeppelin",
]

[[package]]
name = "starkware_utils_testing"
version = "1.0.0"
source = "git+https://github.com/starkware-libs/starkware-starknet-utils?branch=main#aa8b89ae7febcd207d83f16ba3f953cf5b8e3884"
source = "git+https://github.com/starkware-libs/starkware-starknet-utils?branch=main#a8bfc260e603a2768b9c8fd94dabfd292adc69ca"
dependencies = [
"openzeppelin",
"snforge_std",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
pub mod AssetsComponent {
use RolesComponent::InternalTrait as RolesInternalTrait;
use core::cmp::min;
use core::hash::Hash;
use core::iter::{IntoIterator, Iterator};
use core::num::traits::Zero;
use core::panic_with_felt252;
use core::pedersen::HashState;
use openzeppelin::access::accesscontrol::AccessControlComponent;
use openzeppelin::introspection::src5::SRC5Component;
use openzeppelin::token::erc20::interface::IERC20Dispatcher;
Expand All @@ -26,7 +29,7 @@ pub mod AssetsComponent {
use perpetuals::core::components::operator_nonce::OperatorNonceComponent;
use perpetuals::core::components::operator_nonce::OperatorNonceComponent::InternalTrait as NonceInternal;
use perpetuals::core::types::asset::synthetic::{
SyntheticConfig, SyntheticTimelyData, SyntheticTrait,
OptionSyntheticConfig, SyntheticConfig, SyntheticTimelyData, SyntheticTrait,
};
use perpetuals::core::types::asset::{AssetId, AssetStatus};
use perpetuals::core::types::balance::Balance;
Expand All @@ -35,19 +38,26 @@ pub mod AssetsComponent {
Price, PriceMulTrait, SignedPrice, convert_oracle_to_perps_price,
};
use perpetuals::core::types::risk_factor::{RiskFactor, RiskFactorTrait};
use starknet::ContractAddress;
use starknet::storage::{
Map, MutableVecTrait, StorageMapReadAccess, StoragePathEntry, StoragePointerReadAccess,
StoragePointerWriteAccess, Vec, VecTrait,
IntoIterRange, Map, Mutable, MutableVecTrait, StorableStoragePointerReadAccess,
StorageAsPath, StorageAsPointer, StorageMapReadAccess, StorageMapWriteAccess, StoragePath,
StoragePathEntry, StoragePathMutableConversion,
StoragePointer0Offset, StoragePointerReadAccess, StoragePointerWriteAccess, Vec, VecTrait,
};
use starknet::storage_access::{
StorageBaseAddress, Store, storage_address_from_base, storage_address_from_base_and_offset,
};
use starknet::syscalls::{storage_read_syscall, storage_write_syscall};
use starknet::{ContractAddress, SyscallResult, SyscallResultTrait};
use starkware_utils::components::pausable::PausableComponent;
use starkware_utils::components::pausable::PausableComponent::InternalTrait as PausableInternal;
use starkware_utils::components::roles::RolesComponent;
use starkware_utils::constants::{MINUTE, TWO_POW_128, TWO_POW_32, TWO_POW_40};
use starkware_utils::math::abs::Abs;
use starkware_utils::signature::stark::{PublicKey, validate_stark_signature};
use starkware_utils::storage::iterable_map::{
IterableMap, IterableMapIntoIterImpl, IterableMapReadAccessImpl, IterableMapWriteAccessImpl,
IterableMap, IterableMapIntoIterImpl, IterableMapReadAccessImpl, IterableMapTrait,
IterableMapWriteAccessImpl,
};
use starkware_utils::storage::utils::{AddToStorage, SubFromStorage};
use starkware_utils::time::time::{Time, TimeDelta, Timestamp};
Expand All @@ -65,7 +75,7 @@ pub mod AssetsComponent {
collateral_token_contract: IERC20Dispatcher,
collateral_quantum: u64,
num_of_active_synthetic_assets: usize,
pub synthetic_config: Map<AssetId, Option<SyntheticConfig>>,
pub synthetic_config: Map<AssetId, OptionSyntheticConfig>,
pub synthetic_timely_data: IterableMap<AssetId, SyntheticTimelyData>,
pub risk_factor_tiers: Map<AssetId, Vec<RiskFactor>>,
asset_oracle: Map<AssetId, Map<PublicKey, felt252>>,
Expand Down Expand Up @@ -186,7 +196,9 @@ pub mod AssetsComponent {
get_dep_component!(@self, Roles).only_app_governor();

let synthetic_entry = self.synthetic_config.entry(asset_id);
assert(synthetic_entry.read().is_none(), SYNTHETIC_ALREADY_EXISTS);
let x: Option<SyntheticConfig> = synthetic_entry.read().into();

assert(x.is_none(), SYNTHETIC_ALREADY_EXISTS);
if let Option::Some(collateral_id) = self.collateral_id.read() {
assert(collateral_id != asset_id, ASSET_REGISTERED_AS_COLLATERAL);
}
Expand All @@ -208,7 +220,7 @@ pub mod AssetsComponent {
:resolution_factor,
);

synthetic_entry.write(Option::Some(synthetic_config));
synthetic_entry.write(synthetic_config.into());

let synthetic_timely_data = SyntheticTrait::timely_data(
// These fields will be updated in the next price tick.
Expand Down Expand Up @@ -258,7 +270,7 @@ pub mod AssetsComponent {
assert(config.status == AssetStatus::ACTIVE, SYNTHETIC_NOT_ACTIVE);

config.status = AssetStatus::INACTIVE;
self.synthetic_config.entry(synthetic_id).write(Option::Some(config));
self.synthetic_config.entry(synthetic_id).write(config.into());
self.num_of_active_synthetic_assets.sub_and_write(1);

self.emit(events::SyntheticAssetDeactivated { asset_id: synthetic_id });
Expand Down Expand Up @@ -453,7 +465,7 @@ pub mod AssetsComponent {
let old_quorum = synthetic_config.quorum;
assert(old_quorum != quorum, INVALID_SAME_QUORUM);
synthetic_config.quorum = quorum;
self.synthetic_config.write(synthetic_id, Option::Some(synthetic_config));
self.synthetic_config.write(synthetic_id, synthetic_config.into());
self
.emit(
events::AssetQuorumUpdated {
Expand Down Expand Up @@ -509,11 +521,13 @@ pub mod AssetsComponent {
fn get_synthetic_price(
self: @ComponentState<TContractState>, synthetic_id: AssetId,
) -> Price {
if let Option::Some(data) = self.synthetic_timely_data.read(synthetic_id) {
data.price
} else {
panic_with_felt252(NOT_SYNTHETIC)
}
let entry = self.synthetic_timely_data.pointer(key: synthetic_id);
let price = storage_read_syscall(
0, storage_address_from_base_and_offset(entry.__storage_pointer_address__, 2),
)
.unwrap_syscall();
let price: u64 = price.try_into().unwrap();
price.into()
Copy link

Choose a reason for hiding this comment

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

Bug: Storage Access Vulnerabilities in Price and Index Retrieval

The get_synthetic_price and get_funding_index functions use hardcoded storage offsets with direct syscalls, making them brittle to SyntheticTimelyData struct layout changes. get_synthetic_price incorrectly reads last_price_update (offset 2) instead of price (offset 1), while get_funding_index reads from an invalid offset (4). Additionally, the check for non-existent assets is removed, potentially returning uninitialized data.

Additional Locations (1)

Fix in Cursor Fix in Web

}

/// Get the risk factor of a synthetic asset.
Expand All @@ -530,44 +544,46 @@ pub mod AssetsComponent {
balance: Balance,
price: Price,
) -> RiskFactor {
if let Option::Some(synthetic_config) = self.synthetic_config.read(synthetic_id) {
let asset_risk_factor_tiers = self.risk_factor_tiers.entry(synthetic_id);
let synthetic_value: u128 = price.mul(rhs: balance).abs();
let index = if synthetic_value < synthetic_config.risk_factor_first_tier_boundary {
0_u128
} else {
let tier_size = synthetic_config.risk_factor_tier_size;
let first_tier_offset = synthetic_value
- synthetic_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 entry = self.synthetic_config.entry(synthetic_id);
let risk_factor_first_tier_boundary = entry
.risk_factor_first_tier_boundary
.read(); // read 1

let asset_risk_factor_tiers = self.risk_factor_tiers.entry(synthetic_id);
let synthetic_value: u128 = price.mul(rhs: balance).abs();
let index = if synthetic_value < risk_factor_first_tier_boundary {
0_u128
} else {
panic_with_felt252(NOT_SYNTHETIC)
}
let risk_factor_tier_size = entry.risk_factor_tier_size.read(); // read 2
let first_tier_offset = synthetic_value - risk_factor_first_tier_boundary;
min(
1_u128 + (first_tier_offset / risk_factor_tier_size),
asset_risk_factor_tiers.len().into() - 1,
)
};
let con_index: u64 = index.try_into().expect('INDEX_SHOULD_NEVER_OVERFLOW');
let risk_factor_entry: StoragePath<RiskFactor> = asset_risk_factor_tiers
.update(con_index);
risk_factor_entry.read()
}

fn get_funding_index(
self: @ComponentState<TContractState>, synthetic_id: AssetId,
) -> FundingIndex {
if let Option::Some(data) = self.synthetic_timely_data.read(synthetic_id) {
data.funding_index
} else {
panic_with_felt252(NOT_SYNTHETIC)
}
let entry = self.synthetic_timely_data.pointer(key: synthetic_id);
let funding_index = storage_read_syscall(
0, storage_address_from_base_and_offset(entry.__storage_pointer_address__, 4),
)
.unwrap_syscall();
let x: i64 = funding_index.try_into().unwrap();
x.into()
Copy link

Choose a reason for hiding this comment

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

Bug: Incorrect Storage Offset and Mutable Access Violation

The get_funding_index function reads the funding_index field using a hardcoded storage offset of 4. The SyntheticTimelyData struct layout suggests offset 3 is correct, potentially returning wrong data. Also, get_synthetic_risk_factor uses .update() for a read operation on asset_risk_factor_tiers, where .at() is typically for immutable access.

Fix in Cursor Fix in Web

}


fn validate_active_asset(self: @ComponentState<TContractState>, asset_id: AssetId) {
if let Option::Some(config) = self.synthetic_config.read(asset_id) {
assert(config.status == AssetStatus::ACTIVE, SYNTHETIC_NOT_ACTIVE);
} else {
panic_with_felt252(NOT_SYNTHETIC);
}
let entry = self.synthetic_config.entry(asset_id);
let status = entry.status.read();
assert(status == AssetStatus::ACTIVE, SYNTHETIC_NOT_ACTIVE);
Copy link

Choose a reason for hiding this comment

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

Bug: Synthetic Data Handling and Storage Vulnerabilities

The get_funding_index, get_synthetic_risk_factor, and validate_synthetic_active functions now read synthetic asset data directly without validating its existence. This removes the NOT_SYNTHETIC error handling, risking uninitialized or incorrect data reads. get_funding_index further introduces fragile direct storage syscalls with a hardcoded offset, bypassing type safety.

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: Storage Access Bypasses Asset Validation

The get_synthetic_price, get_funding_index, get_synthetic_risk_factor, and validate_synthetic_active functions now directly access storage without verifying asset existence. This reads uninitialized data for non-existent assets, leading to incorrect calculations or invalid state instead of the expected NOT_SYNTHETIC panic. Furthermore, get_synthetic_price and get_funding_index use fragile hardcoded storage offsets, with get_synthetic_price specifically reading the last_price_update field instead of price and returning an unscaled value.

Additional Locations (1)

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: Incorrect Storage Access Causes Invalid Data

The get_funding_index function reads the funding_index from SyntheticTimelyData using an incorrect storage offset (4 instead of 3), leading to invalid data. Also, validate_synthetic_active directly accesses the status field of synthetic_config without first checking if the configuration exists, which can result in reading garbage data for non-existent assets instead of panicking.

Fix in Cursor Fix in Web

}

/// Validates assets integrity prerequisites:
Expand Down Expand Up @@ -598,7 +614,8 @@ pub mod AssetsComponent {
fn _get_synthetic_config(
self: @ComponentState<TContractState>, synthetic_id: AssetId,
) -> SyntheticConfig {
self.synthetic_config.read(synthetic_id).expect(SYNTHETIC_NOT_EXISTS)
let x: Option<SyntheticConfig> = self.synthetic_config.read(synthetic_id).into();
x.expect(SYNTHETIC_NOT_EXISTS)
}

fn _get_synthetic_timely_data(
Expand Down Expand Up @@ -714,7 +731,7 @@ pub mod AssetsComponent {
// Activates the synthetic asset.
synthetic_config.status = AssetStatus::ACTIVE;
self.num_of_active_synthetic_assets.add_and_write(1);
self.synthetic_config.write(asset_id, Option::Some(synthetic_config));
self.synthetic_config.write(asset_id, synthetic_config.into());
self.emit(events::AssetActivated { asset_id });
}
self.emit(events::PriceTick { asset_id, price });
Expand Down
6 changes: 5 additions & 1 deletion workspace/apps/perpetuals/contracts/src/core/core.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub mod Core {
IterableMapIntoIterImpl, IterableMapReadAccessImpl, IterableMapWriteAccessImpl,
};
use starkware_utils::time::time::{Time, TimeDelta, Timestamp, validate_expiration};
use crate::core::types::asset::synthetic::SyntheticConfig;

component!(path: AccessControlComponent, storage: accesscontrol, event: AccessControlEvent);
component!(path: OperatorNonceComponent, storage: operator_nonce, event: OperatorNonceEvent);
Expand Down Expand Up @@ -857,7 +858,10 @@ pub mod Core {
let position_b = self.positions.get_position_snapshot(position_id: position_id_b);

// Validate base asset is inactive synthetic.
if let Option::Some(config) = self.assets.synthetic_config.read(base_asset_id) {
let x = self.assets.synthetic_config.read(base_asset_id);

let y: Option<SyntheticConfig> = x.into();
if let Option::Some(config) = y {
assert(config.status == AssetStatus::INACTIVE, SYNTHETIC_IS_ACTIVE);
} else {
panic_with_felt252(NOT_SYNTHETIC);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,55 @@ pub struct SyntheticConfig {
pub resolution_factor: u64,
}

#[derive(Copy, Drop, Serde, starknet::Store)]
pub struct OptionSyntheticConfig {
pub option: felt252,
version: u8,
// Configurable
pub status: AssetStatus,
pub risk_factor_first_tier_boundary: u128,
pub risk_factor_tier_size: u128,
pub quorum: u8,
// Smallest unit of a synthetic asset in the system.
pub resolution_factor: u64,
}

pub impl OptionSyntheticConfigIntoOptionSyntheticConfig of Into<
OptionSyntheticConfig, Option<SyntheticConfig>,
> {
fn into(self: OptionSyntheticConfig) -> Option<SyntheticConfig> {
if self.option == 1 {
Option::Some(
SyntheticConfig {
version: self.version,
status: self.status,
risk_factor_first_tier_boundary: self.risk_factor_first_tier_boundary,
risk_factor_tier_size: self.risk_factor_tier_size,
quorum: self.quorum,
resolution_factor: self.resolution_factor,
},
)
} else {
Option::None
}
}
}

pub impl SyntheticConfigIntoOptionSyntheticConfig of Into<SyntheticConfig, OptionSyntheticConfig> {
fn into(self: SyntheticConfig) -> OptionSyntheticConfig {
OptionSyntheticConfig {
option: 1,
version: self.version,
status: self.status,
risk_factor_first_tier_boundary: self.risk_factor_first_tier_boundary,
risk_factor_tier_size: self.risk_factor_tier_size,
quorum: self.quorum,
resolution_factor: self.resolution_factor,
}
}
}


#[derive(Copy, Drop, Serde, starknet::Store)]
pub struct SyntheticTimelyData {
version: u8,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ impl FundingIndexIntoImpl of Into<FundingIndex, i64> {
}
}

impl i64IntoImplFundingIndex of Into<i64, FundingIndex> {
fn into(self: i64) -> FundingIndex {
FundingIndex { value: self }
}
}


/// Validates the funding rate by ensuring that the index difference is bounded by the max funding
/// rate.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ pub struct Price {
value: u64,
}

impl u64IntoImplPrice of Into<u64, Price> {
fn into(self: u64) -> Price {
Price { value: self }
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Price Conversion Bypasses Validation

The Into<u64, Price> implementation directly assigns the raw u64 value, bypassing PriceTrait::new's essential scaling by PRICE_SCALE and validation against MAX_PRICE. This can result in incorrectly scaled or invalid Price instances, potentially affecting price calculations.

Fix in Cursor Fix in Web


#[derive(Copy, Debug, Drop, Serde)]
pub struct SignedPrice {
Expand Down
Loading
Loading