Skip to content
Merged
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
16 changes: 9 additions & 7 deletions .github/workflows/contracts-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,16 @@ jobs:
- name: Install asdf
uses: asdf-vm/actions/setup@v2

- name: Install plugins
- name: Install Dojo using dojoup
run: |
curl -L https://install.dojoengine.org | bash
. "$HOME/.config/.dojo/env"
dojoup install 1.5.0
echo "$HOME/.config/.dojo/bin" >> $GITHUB_PATH
shell: bash

- name: Install Starknet Foundry
run: |
asdf plugin add scarb
asdf install scarb 2.10.1
asdf global scarb 2.10.1
asdf plugin add dojo https://github.com/dojoengine/asdf-dojo
asdf install dojo 1.5.0
asdf global dojo 1.5.0
asdf plugin add starknet-foundry
asdf install starknet-foundry 0.35.0
asdf global starknet-foundry 0.35.0
Expand Down
273 changes: 273 additions & 0 deletions src/helpers/security.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
use starknet::{ContractAddress, get_caller_address, get_block_timestamp};
use dojo::model::ModelStorage;
use dojo::event::EventStorage;
use dojo::world::WorldStorage;
use coa::models::core::Contract;
use coa::models::security::{
RateLimit, SecurityConfig, AdminRole, PlayerSecurityStatus, SecurityEvent, RateLimitExceeded,
ContractPaused, ContractUnpaused,
};
use coa::models::session::SessionKey;
use core::num::traits::Zero;
use core::poseidon::poseidon_hash_span;

// Security constants
pub const SUPER_ADMIN: felt252 = 'SUPER_ADMIN';
pub const GAME_ADMIN: felt252 = 'GAME_ADMIN';
pub const MODERATOR: felt252 = 'MODERATOR';

// Operation types as numbers for rate limiting
pub const CREATE_SESSION_OP: u32 = 1;
pub const SPAWN_ITEMS_OP: u32 = 2;
pub const ADMIN_ACTION_OP: u32 = 3;

pub const SECURITY_CONFIG_ID: felt252 = 'SECURITY_CONFIG';
pub const COA_CONTRACTS: felt252 = 'COA_CONTRACTS';

// Basic admin validation function (simplified)
pub fn validate_admin_access(world: WorldStorage, _required_role: felt252) {
let caller = get_caller_address();

// Validate caller is not zero address
assert(!caller.is_zero(), 'ZERO_ADDRESS');

// Read contract state
let contract: Contract = world.read_model(COA_CONTRACTS);

// Validate contract state
assert(!contract.admin.is_zero(), 'INVALID_CONTRACT_STATE');
assert(!contract.paused, 'CONTRACT_PAUSED');

// Basic admin check (simplified for now)
assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
}
Comment on lines +28 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Simplified admin validation has limitations.

The basic validate_admin_access function only checks if the caller is the contract admin, ignoring the _required_role parameter. This creates an inconsistency with the function signature.

Either rename the parameter to indicate it's unused or implement role checking:

-pub fn validate_admin_access(world: WorldStorage, _required_role: felt252) {
+pub fn validate_admin_access(world: WorldStorage, required_role: felt252) {
     let caller = get_caller_address();
     
     // Validate caller is not zero address
     assert(!caller.is_zero(), 'ZERO_ADDRESS');
     
     // Read contract state
     let contract: Contract = world.read_model(COA_CONTRACTS);
     
     // Validate contract state
     assert(!contract.admin.is_zero(), 'INVALID_CONTRACT_STATE');
     assert(!contract.paused, 'CONTRACT_PAUSED');
     
-    // Basic admin check (simplified for now)
-    assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
+    // For now, only support SUPER_ADMIN role through this simplified version
+    assert(required_role == SUPER_ADMIN, 'USE_FULL_VERSION_FOR_OTHER_ROLES');
+    assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub fn validate_admin_access(world: WorldStorage, _required_role: felt252) {
let caller = get_caller_address();
// Validate caller is not zero address
assert(!caller.is_zero(), 'ZERO_ADDRESS');
// Read contract state
let contract: Contract = world.read_model(COA_CONTRACTS);
// Validate contract state
assert(!contract.admin.is_zero(), 'INVALID_CONTRACT_STATE');
assert(!contract.paused, 'CONTRACT_PAUSED');
// Basic admin check (simplified for now)
assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
}
pub fn validate_admin_access(world: WorldStorage, required_role: felt252) {
let caller = get_caller_address();
// Validate caller is not zero address
assert(!caller.is_zero(), 'ZERO_ADDRESS');
// Read contract state
let contract: Contract = world.read_model(COA_CONTRACTS);
// Validate contract state
assert(!contract.admin.is_zero(), 'INVALID_CONTRACT_STATE');
assert(!contract.paused, 'CONTRACT_PAUSED');
// For now, only support SUPER_ADMIN role through this simplified version
assert(required_role == SUPER_ADMIN, 'USE_FULL_VERSION_FOR_OTHER_ROLES');
assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
}
🤖 Prompt for AI Agents
In src/helpers/security.cairo around lines 28 to 43, the function
validate_admin_access currently accepts a _required_role parameter but ignores
it and only checks that the caller equals contract.admin; update the function to
either (A) implement role-based validation by reading the caller's role from
WorldStorage (or whatever roles mapping exists), compare it to required_role and
assert on mismatch while preserving the existing admin and paused checks, or (B)
if no role system is available, remove or rename the parameter to indicate it is
unused (e.g., omit _required_role from the signature or rename to
_unused_required_role) and update all call sites and docs accordingly so the
signature matches behavior.


// Full admin validation function
pub fn validate_admin_access_full(world: WorldStorage, required_role: felt252) {
let caller = get_caller_address();

// Validate caller is not zero address
assert(!caller.is_zero(), 'ZERO_ADDRESS');

// Read contract state
let contract: Contract = world.read_model(COA_CONTRACTS);

// Validate contract state
assert(!contract.admin.is_zero(), 'INVALID_CONTRACT_STATE');
assert(!contract.paused, 'CONTRACT_PAUSED');

// Check if caller is super admin (contract admin)
if caller == contract.admin {
return;
}

// Check role-based access
let admin_role: AdminRole = world.read_model(caller);
assert(admin_role.is_active, 'INSUFFICIENT_PERMISSIONS');

// Validate role hierarchy using if-else
if required_role == SUPER_ADMIN {
assert(caller == contract.admin, 'SUPER_ADMIN_REQUIRED');
} else if required_role == GAME_ADMIN {
assert(
caller == contract.admin
|| admin_role.role_type == GAME_ADMIN
|| admin_role.role_type == SUPER_ADMIN,
'GAME_ADMIN_REQUIRED',
);
} else if required_role == MODERATOR {
assert(
caller == contract.admin
|| admin_role.role_type == SUPER_ADMIN
|| admin_role.role_type == GAME_ADMIN
|| admin_role.role_type == MODERATOR,
'MODERATOR_REQUIRED',
);
} else {
assert(false, 'INVALID_ROLE');
}
}

pub fn validate_player_access(world: WorldStorage, player_id: ContractAddress) {
let caller = get_caller_address();

// Validate caller is not zero address
assert(!caller.is_zero(), 'ZERO_ADDRESS');

// Validate player ID matches caller (unless admin)
let contract: Contract = world.read_model(COA_CONTRACTS);
if caller != contract.admin {
assert(caller == player_id, 'UNAUTHORIZED_PLAYER');
}

// Check if player is banned
let security_status: PlayerSecurityStatus = world.read_model(player_id);
if security_status.is_banned {
let current_time = get_block_timestamp();
if security_status.ban_expires_at > current_time {
assert(false, 'PLAYER_BANNED');
}
}
}

pub fn check_rate_limit(
mut world: WorldStorage, user: ContractAddress, operation_type: u32,
) -> bool {
let current_time = get_block_timestamp();
let time_window = current_time / 3600; // 1 hour windows
let operation_felt: felt252 = operation_type.into();
let rate_limit_key = (user, operation_felt, time_window);

let mut rate_limit: RateLimit = world.read_model(rate_limit_key);
let security_config: SecurityConfig = world.read_model(SECURITY_CONFIG_ID);

// Use if-else for operation type checking
let limit = if operation_type == CREATE_SESSION_OP {
security_config.max_sessions_per_hour
} else if operation_type == SPAWN_ITEMS_OP {
security_config.max_spawns_per_hour
} else if operation_type == ADMIN_ACTION_OP {
50 // Default admin action limit
} else {
100 // Default limit
};

if rate_limit.count >= limit {
// Emit rate limit exceeded event
let event = RateLimitExceeded {
user,
operation: operation_felt,
current_count: rate_limit.count,
limit,
timestamp: current_time,
};
world.emit_event(@event);

// Log security event
let security_event = SecurityEvent {
event_type: 'RATE_LIMIT_EXCEEDED',
user,
timestamp: current_time,
details: operation_felt,
};
world.emit_event(@security_event);

return false;
}

rate_limit.count += 1;
world.write_model(@rate_limit);
true
}

pub fn sanitize_input(input: felt252) -> felt252 {
// Basic input sanitization - in a real implementation,
// you might want to check for malicious patterns
input
}
Comment on lines +163 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Placeholder sanitize_input needs implementation.

The current implementation is a no-op that just returns the input unchanged. Given it is crucial to validate parameters, especially those provided by users, this should be properly implemented.

The function needs actual sanitization logic. Would you like me to implement proper input sanitization that checks for malicious patterns, validates felt252 bounds, and ensures safe characters?


pub fn validate_faction(faction: felt252) -> bool {
faction == 'CHAOS_MERCENARIES' || faction == 'SUPREME_LAW' || faction == 'REBEL_TECHNOMANCERS'
}

pub fn validate_session_duration(duration: u64) -> bool {
duration >= 3600 && duration <= 86400 // 1 hour to 24 hours
}

pub fn generate_secure_session_id(player: ContractAddress) -> felt252 {
// Use multiple sources of entropy for security
let tx_hash: felt252 = starknet::get_tx_info().unbox().transaction_hash;
let block_timestamp: felt252 = get_block_timestamp().into();
let player_address: felt252 = player.into();

// Combine entropy sources
let mut hash_data = array![tx_hash, block_timestamp, player_address];
poseidon_hash_span(hash_data.span())
}

pub fn validate_contract_not_paused(world: WorldStorage) {
let contract: Contract = world.read_model(COA_CONTRACTS);
assert(!contract.paused, 'CONTRACT_PAUSED');
}

pub fn log_security_event(
mut world: WorldStorage, event_type: felt252, user: ContractAddress, details: felt252,
) {
let event = SecurityEvent { event_type, user, timestamp: get_block_timestamp(), details };
world.emit_event(@event);
}

// Emergency functions
pub fn pause_contract(mut world: WorldStorage, reason: felt252) {
validate_admin_access(world, SUPER_ADMIN);

let mut contract: Contract = world.read_model(COA_CONTRACTS);
contract.paused = true;
world.write_model(@contract);

let event = ContractPaused {
paused_by: get_caller_address(), timestamp: get_block_timestamp(), reason,
};
world.emit_event(@event);
}
Comment on lines +201 to +212
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Recursive call in pause_contract function.

The pause_contract helper function calls validate_admin_access which checks if the contract is paused, but we're in the process of pausing it. This could cause issues if the contract is already paused.

Fix the logic to avoid the recursive validation issue:

 pub fn pause_contract(mut world: WorldStorage, reason: felt252) {
-    validate_admin_access(world, SUPER_ADMIN);
+    let caller = get_caller_address();
+    assert(!caller.is_zero(), 'ZERO_ADDRESS');
+    
+    let contract: Contract = world.read_model(COA_CONTRACTS);
+    assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
     
     let mut contract: Contract = world.read_model(COA_CONTRACTS);
     contract.paused = true;
     world.write_model(@contract);
     
     let event = ContractPaused {
-        paused_by: get_caller_address(), timestamp: get_block_timestamp(), reason,
+        paused_by: caller, timestamp: get_block_timestamp(), reason,
     };
     world.emit_event(@event);
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/helpers/security.cairo around lines 201 to 212, the pause_contract
function currently calls validate_admin_access which itself enforces a "not
paused" check causing a recursive/blocked flow when attempting to pause an
already-paused contract; change the call to use an admin-validation that does
not enforce the paused-state check (either by adding a boolean parameter to
validate_admin_access like skip_paused_check=true or by adding a separate helper
validate_admin_access_ignore_paused), call that lighter admin-only validator at
the start of pause_contract, then proceed to set contract.paused, write the
model and emit the event as before.


pub fn unpause_contract(mut world: WorldStorage) {
validate_admin_access(world, SUPER_ADMIN);

let mut contract: Contract = world.read_model(COA_CONTRACTS);
contract.paused = false;
world.write_model(@contract);

let event = ContractUnpaused {
unpaused_by: get_caller_address(), timestamp: get_block_timestamp(),
};
world.emit_event(@event);
}
Comment on lines +214 to +225
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Similar issue in unpause_contract function.

The unpause_contract function has the same issue - it validates that the contract is not paused while trying to unpause it.

Fix to allow unpausing a paused contract:

 pub fn unpause_contract(mut world: WorldStorage) {
-    validate_admin_access(world, SUPER_ADMIN);
+    let caller = get_caller_address();
+    assert(!caller.is_zero(), 'ZERO_ADDRESS');
+    
+    let contract: Contract = world.read_model(COA_CONTRACTS);
+    assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
     
     let mut contract: Contract = world.read_model(COA_CONTRACTS);
     contract.paused = false;
     world.write_model(@contract);
     
     let event = ContractUnpaused {
-        unpaused_by: get_caller_address(), timestamp: get_block_timestamp(),
+        unpaused_by: caller, timestamp: get_block_timestamp(),
     };
     world.emit_event(@event);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub fn unpause_contract(mut world: WorldStorage) {
validate_admin_access(world, SUPER_ADMIN);
let mut contract: Contract = world.read_model(COA_CONTRACTS);
contract.paused = false;
world.write_model(@contract);
let event = ContractUnpaused {
unpaused_by: get_caller_address(), timestamp: get_block_timestamp(),
};
world.emit_event(@event);
}
pub fn unpause_contract(mut world: WorldStorage) {
let caller = get_caller_address();
assert(!caller.is_zero(), 'ZERO_ADDRESS');
let contract: Contract = world.read_model(COA_CONTRACTS);
assert(caller == contract.admin, 'INSUFFICIENT_PERMISSIONS');
let mut contract: Contract = world.read_model(COA_CONTRACTS);
contract.paused = false;
world.write_model(@contract);
let event = ContractUnpaused {
unpaused_by: caller, timestamp: get_block_timestamp(),
};
world.emit_event(@event);
}
🤖 Prompt for AI Agents
In src/helpers/security.cairo around lines 214 to 225, the unpause_contract
function currently validates that the contract is not paused before unpausing,
which prevents unpausing a paused contract; change the validation so it requires
the contract to be paused (or remove the inverse check) while keeping the admin
access check, i.e., replace the "must not be paused" check with a "must be
paused" check so the function only proceeds when contract.paused == true, then
set paused to false, write the model, and emit the event as-is.


// Session security helpers
pub fn create_secure_session(
mut world: WorldStorage, session_duration: u64, max_transactions: u32,
) -> felt252 {
let caller = get_caller_address();
let current_time = get_block_timestamp();

// Validate inputs
assert(validate_session_duration(session_duration), 'INVALID_DURATION');
assert(max_transactions > 0 && max_transactions <= 1000, 'INVALID_TRANSACTIONS');

// Check rate limiting
assert(check_rate_limit(world, caller, CREATE_SESSION_OP), 'RATE_LIMIT_EXCEEDED');

// Generate secure session ID
let session_id = generate_secure_session_id(caller);

// Create session with security measures
let session_key = SessionKey {
session_id,
player_address: caller,
session_key_address: caller,
created_at: current_time,
expires_at: current_time + session_duration,
last_used: current_time,
status: 0, // Active
max_transactions,
used_transactions: 0,
is_valid: true,
};

world.write_model(@session_key);
session_id
}

// Input validation helpers
pub fn validate_item_id(item_id: u256) -> bool {
item_id > 0
}

pub fn validate_quantity(quantity: u256) -> bool {
quantity > 0 && quantity <= 1000000 // Reasonable upper limit
}

pub fn validate_address_not_zero(address: ContractAddress) -> bool {
!address.is_zero()
}
3 changes: 3 additions & 0 deletions src/lib.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub mod models {
pub mod pet_stats;
pub mod tournament;
pub mod session;
pub mod security;
pub mod weapon {
pub mod blunt;
pub mod bow;
Expand Down Expand Up @@ -65,6 +66,7 @@ pub mod helpers {
pub mod gear;
pub mod body;
pub mod session_validation;
pub mod security;
}

pub mod types {
Expand All @@ -87,6 +89,7 @@ pub mod test {
pub mod upgrade_gear_test;
pub mod gear_read_test;
pub mod model_test_player;
pub mod security_test;
}

pub mod traits {
Expand Down
Loading
Loading