Added Missing Security Measures and Access Control #160
Conversation
WalkthroughAdds a new security model and helper module, integrates access control, rate limiting, session hardening, and pause/unpause controls across core, player, and session systems. Exports modules in lib, adds unit tests, and updates CI to install Dojo via dojoup. Introduces related events, configs, and validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Core as Core System
participant Sec as Security Helpers
participant World as World/Storage
Admin->>Core: pause_contract(reason)
Core->>Sec: validate_admin_access_full(SUPER_ADMIN)
Core->>Sec: validate_contract_not_paused
Core->>World: Update contract.paused = true
Core->>World: Emit ContractPaused + SecurityEvent
Admin-->>Core: ok
Admin->>Core: unpause_contract()
Core->>Sec: validate_admin_access_full(SUPER_ADMIN)
Core->>World: Update contract.paused = false
Core->>World: Emit ContractUnpaused + SecurityEvent
Admin-->>Core: ok
sequenceDiagram
autonumber
actor Admin
participant Core as Core.spawn_items
participant Sec as Security Helpers
participant World as World/Storage
Admin->>Core: spawn_items(gear_details)
Core->>Sec: validate_admin_access_full(GAME_ADMIN or SUPER_ADMIN)
Core->>Sec: validate_contract_not_paused
Core->>Sec: check_rate_limit(Admin, SPAWN_ITEMS_OP)
Core->>Sec: sanitize_input / validate inputs
Core->>World: Create items, emit events
Admin-->>Core: items spawned
sequenceDiagram
autonumber
actor Player
participant Session as Session.create_session_key
participant Sec as Security Helpers
participant World as World/Storage
Player->>Session: create_session_key(duration, max_tx)
Session->>Sec: validate_contract_not_paused
Session->>Sec: validate_player_access(player)
Session->>Sec: validate_session_duration(duration)
Session->>Sec: check_rate_limit(player, CREATE_SESSION_OP)
Session->>Sec: generate_secure_session_id(player)
Session->>World: Store session, emit event
Player-->>Session: session_id
sequenceDiagram
autonumber
actor Player
participant PlayerSys as PlayerActions.new
participant Sec as Security Helpers
Player->>PlayerSys: new(faction, ...)
PlayerSys->>Sec: validate_contract_not_paused
PlayerSys->>Sec: validate_player_access(caller)
PlayerSys->>Sec: validate_faction(faction)
Player-->>PlayerSys: constructed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/systems/player.cairo (1)
100-106: Player initialization guard is inverted.
newreturns whenmax_hp == 0, which prevents initializing brand-new players. Likely intended to return if already initialized.Fix:
- if player.max_hp == 0 { - return; - } - player.init(faction); + if player.max_hp != 0 { + // Already initialized + return; + } + player.init(faction);
🧹 Nitpick comments (8)
SECURITY_IMPROVEMENTS.md (1)
106-113: Avoid recommending IP-based rate limiting for on-chain contracts.On-chain contracts can’t see client IPs. Suggest reframing as address-based or off-chain/middleware enforcement.
Apply this wording tweak:
- 2. **Add IP-based rate limiting** for additional protection + 2. **Add address-based rate limiting** on-chain; consider IP-based enforcement in off-chain middlewaresrc/systems/player.cairo (1)
144-161: Solid input validation for targets; minor polish.Length, non-empty, cap at 20, and target-type checks look good. Optional: extract target type validation into a small helper for reuse and readability.
src/systems/session.cairo (2)
28-31: Centralized duration validation improves consistency.Using
validate_session_duration()is good. Consider reusing it inrenew_sessionfor consistency.- assert(new_duration >= MIN_SESSION_DURATION, 'DURATION_TOO_SHORT'); - assert(new_duration <= MAX_SESSION_DURATION, 'DURATION_TOO_LONG'); + assert( + coa::helpers::security::validate_session_duration(new_duration), + 'INVALID_DURATION' + );
34-40: Rate-limit check added; consider event emission on violation.If
check_rate_limitdoesn’t already emit aRateLimitExceededevent internally, consider logging before reverting to aid ops/audit.Confirm behavior of
check_rate_limit(world, player, CREATE_SESSION_OP)regarding event emission.src/models/security.cairo (2)
17-27: SecurityConfig parameters need validation bounds.The SecurityConfig structure is well-designed, but consider that Cairo 1.0 introduces built-in support for secure integer types, such as u128 and u256, which automatically handle overflows and underflows. By utilizing these types, transactions will be rolled back if an overflow is detected.
Ensure the numeric limits are properly bounded to prevent configuration errors:
pub struct SecurityConfig { #[key] pub id: felt252, - pub max_sessions_per_hour: u32, - pub max_spawns_per_hour: u32, - pub max_transactions_per_session: u32, + pub max_sessions_per_hour: u32, // Consider adding a MAX_SESSIONS_LIMIT constant + pub max_spawns_per_hour: u32, // Consider adding a MAX_SPAWNS_LIMIT constant + pub max_transactions_per_session: u32, // Consider adding a MAX_TXS_LIMIT constant pub session_renewal_threshold: u64, // seconds pub emergency_pause_enabled: bool, }
126-131: Verify Default trait implementation placement.The
ContractAddressDefaultimplementation appears to be a utility that might be better placed in a shared utilities module rather than in the security models file. Additionally, consider an L1-to-L2 bridge contract that allows depositing tokens from L1 to L2. If the to address parameter is not properly validated, it could result in tokens being sent to an unintended address on L2. To mitigate this, it is crucial to validate parameters.Consider moving this to a shared utilities module:
// In src/helpers/utils.cairo or similar pub impl ContractAddressDefault of Default<ContractAddress> { #[inline(always)] fn default() -> ContractAddress { Zero::zero() } }src/helpers/security.cairo (2)
169-171: Faction validation uses hardcoded values.Consider moving faction names to constants for maintainability:
+pub const FACTION_CHAOS: felt252 = 'CHAOS_MERCENARIES'; +pub const FACTION_LAW: felt252 = 'SUPREME_LAW'; +pub const FACTION_REBEL: felt252 = 'REBEL_TECHNOMANCERS'; pub fn validate_faction(faction: felt252) -> bool { - faction == 'CHAOS_MERCENARIES' || faction == 'SUPREME_LAW' || faction == 'REBEL_TECHNOMANCERS' + faction == FACTION_CHAOS || faction == FACTION_LAW || faction == FACTION_REBEL }
263-273: Basic validation helpers need bounds checking.The validation helpers are minimal. Cairo 1.0 introduces built-in support for secure integer types which automatically handle overflows and underflows, but explicit bounds checking is still good practice.
Consider more robust validation:
pub fn validate_item_id(item_id: u256) -> bool { - item_id > 0 + item_id > 0 && item_id.high < u128::MAX && item_id.low < u128::MAX } pub fn validate_quantity(quantity: u256) -> bool { - quantity > 0 && quantity <= 1000000 // Reasonable upper limit + quantity > 0 && quantity <= 1000000 && quantity.high == 0 // Ensure fits in reasonable range }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
SECURITY_IMPROVEMENTS.md(1 hunks)src/helpers/security.cairo(1 hunks)src/lib.cairo(3 hunks)src/models/security.cairo(1 hunks)src/systems/core.cairo(9 hunks)src/systems/player.cairo(3 hunks)src/systems/session.cairo(1 hunks)src/test/security_test.cairo(1 hunks)
🔇 Additional comments (28)
SECURITY_IMPROVEMENTS.md (1)
31-36: Align documentation with implementation for session ID generation.Doc states “Uses Poseidon hash,” but the code calls
generate_secure_session_id(player). Either confirm that helper uses Poseidon or generalize the wording to avoid naming a specific hash.Suggested edit:
- **Secure ID Generation**: Uses Poseidon hash with multiple entropy sources + **Secure ID Generation**: Uses generate_secure_session_id() derived from multiple entropy sources and a cryptographic hash (implementation-defined)src/lib.cairo (1)
33-33: LGTM: new security modules exported.Public module additions for models, helpers, and tests look consistent with the rest of lib exports.
Also applies to: 69-69, 93-93
src/systems/player.cairo (2)
90-97: Good: added pause and player-access checks + faction validation.Early security validation in
newis appropriate and ordered correctly.Confirm
validate_player_accessenforces expected rules for first-time players (creation path) vs. existing players.
748-757: Good fix: read ERC1155 from config and validate non-zero; verifyis_zero()availability.Pattern aligns with avoiding zero-address usage. Ensure
ContractAddress::is_zero()exists in your Starknet version; otherwise, compare tocontract_address_const::<0>().If needed:
- assert(!contract.erc1155.is_zero(), 'ERC1155_NOT_CONFIGURED'); + assert(contract.erc1155 != contract_address_const::<0>(), 'ERC1155_NOT_CONFIGURED');src/systems/session.cairo (2)
21-26: Good: early pause and player-access checks.Validating paused state and player access before any work is the right order.
42-44: LGTM: secure session ID generation via helper.Good move away from ad-hoc hashing.
src/systems/core.cairo (9)
23-24: Well-designed security interface additions.The addition of
pause_contractandunpause_contractto theICoreinterface is a crucial security enhancement that allows for emergency response capabilities.
45-45: Zero value trait import is necessary for validation checks.Good addition of the
Zerotrait import, which is essential for the zero-address validation checks throughout the security measures.
61-64: Critical validation for initialization parameters.Excellent implementation of zero-address validation for critical contract addresses during initialization. This prevents misconfiguration that could lock the contract.
83-92: Comprehensive security configuration initialization.Great addition of security configuration with sensible default rate limits and emergency controls. The values appear reasonable for production use.
101-115: Robust security validation stack for spawn_items.Excellent implementation of the defense-in-depth approach with admin validation, contract pause check, and rate limiting. The order of checks is optimal - authorization first, then state, then rate limits.
183-196: Well-implemented pause_contract with proper controls.The implementation correctly validates super admin access, updates state, and logs security events with appropriate context. The reason parameter is valuable for audit trails.
198-211: Correct unpause_contract implementation.Properly mirrors the pause functionality with appropriate admin checks and event logging. Using
0as the reason for unpause events is a reasonable convention.
218-222: Proper security checks for random_gear_generator.Good addition of admin validation and pause state check for the random gear generator function.
252-258: Input validation for pick_items is well-designed.The validation checks for empty arrays and spam prevention (50 item limit) are appropriate. The order of security checks (pause, player access, then input) follows best practices.
src/models/security.cairo (5)
1-3: Essential imports for security models.Appropriate imports for ContractAddress and Zero trait which are fundamental for the security models.
5-15: Well-structured RateLimit model.The composite key structure (user, operation, time_window) is well-designed for tracking rate limits per operation per user in hourly windows. The u32 count is sufficient for rate limiting purposes.
29-38: Robust AdminRole model design.The AdminRole structure properly tracks role assignments with appropriate metadata for audit trails. The role_type as felt252 allows flexibility while the is_active flag enables role revocation without deletion.
40-50: Comprehensive PlayerSecurityStatus tracking.Excellent design for tracking player security violations with graduated responses (warnings to bans), time-based expiration, and audit trail via last_violation timestamp.
53-124: Comprehensive security event definitions.Well-structured event models covering all critical security operations. The use of composite keys in events ensures proper indexing and queryability.
src/helpers/security.cairo (8)
1-13: Comprehensive imports for security helpers.All necessary imports for security functionality are properly included.
15-25: Well-defined security constants.Good separation of role types and operation identifiers. The numeric operation codes enable efficient rate limiting lookups.
46-89: Well-structured role hierarchy validation.The full admin validation function properly implements role hierarchy with SUPER_ADMIN > GAME_ADMIN > MODERATOR. The early return for contract admin is efficient.
91-111: Player access validation logic is sound.Good implementation with admin bypass and ban expiration checking. The ban check properly compares timestamps to allow expired bans to pass.
113-161: Rate limiting implementation with proper event emission.Excellent implementation with time window calculation, configurable limits per operation type, and comprehensive event logging when limits are exceeded. The hourly window approach is standard and effective.
173-175: Session duration validation with reasonable bounds.The 1-24 hour session duration limits are appropriate for preventing both too-short sessions and indefinite sessions.
177-186: Secure session ID generation with multiple entropy sources.Good implementation using transaction hash, timestamp, and player address as entropy sources. The use of Poseidon hash is appropriate for StarkNet/Cairo.
228-260: Secure session creation with comprehensive validation.Well-implemented session creation with input validation, rate limiting, and secure ID generation. The session key structure includes all necessary security fields.
| 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'); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| pub fn sanitize_input(input: felt252) -> felt252 { | ||
| // Basic input sanitization - in a real implementation, | ||
| // you might want to check for malicious patterns | ||
| input | ||
| } |
There was a problem hiding this comment.
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 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); | ||
| } |
There was a problem hiding this comment.
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); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| /// Spawn tournamemnts and side quests here, if necessary. | ||
|
|
||
| use coa::models::gear::{Gear, GearDetails}; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Unused import detected in the file.
The import of GearDetails from coa::models::gear was added but appears to be redundant since GearDetails is already used through the existing import structure.
🤖 Prompt for AI Agents
In src/systems/core.cairo around line 7, remove the redundant import of
GearDetails from coa::models::gear since it’s unused and duplicated by the
existing imports; update the import block to eliminate that specific entry and
run the build/linter to confirm no other references remain.
| // Input validation and sanitization | ||
| assert(details.validate(), 'Invalid gear details'); | ||
| assert(details.total_count > 0, 'INVALID_TOTAL_COUNT'); | ||
| assert(details.total_count <= 1000000, 'COUNT_TOO_HIGH'); | ||
| assert(details.max_upgrade_level <= 100, 'UPGRADE_LEVEL_TOO_HIGH'); |
There was a problem hiding this comment.
🧹 Nitpick
Input validation boundaries need refinement.
While the input validation is good, the felt type in Cairo represents an integer whose value lies in the range from 0 to P-1, where P is 2^251 + 17 * 2^192 + 1 bits, and comparing arbitrary limits might cause issues.
Consider defining these limits as constants for better maintainability and ensuring they fit within Cairo's felt constraints:
+const MAX_TOTAL_COUNT: u32 = 1000000;
+const MAX_UPGRADE_LEVEL: u8 = 100;
// Input validation and sanitization
assert(details.validate(), 'Invalid gear details');
assert(details.total_count > 0, 'INVALID_TOTAL_COUNT');
-assert(details.total_count <= 1000000, 'COUNT_TOO_HIGH');
-assert(details.max_upgrade_level <= 100, 'UPGRADE_LEVEL_TOO_HIGH');
+assert(details.total_count <= MAX_TOTAL_COUNT, 'COUNT_TOO_HIGH');
+assert(details.max_upgrade_level <= MAX_UPGRADE_LEVEL, 'UPGRADE_LEVEL_TOO_HIGH');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/systems/core.cairo around lines 134 to 138, the input validation uses
magic numeric literals that may be unsafe against Cairo's felt range; replace
those literals with named constants (e.g., MAX_TOTAL_COUNT, MAX_UPGRADE_LEVEL,
MAX_ALLOWED_FELT) defined at the top of the file, ensure those constants are
declared as felt and are strictly less than Cairo's prime field modulus P, and
update the asserts to use those constants (and consider using unsigned types or
Uint256 if you need values that approach the field size). Ensure constant names
are descriptive and used throughout so the limits are maintainable and clearly
within Cairo's felt constraints.
| assert(validate_faction('CHAOS_MERCENARIES'), 'Should accept CHAOS_MERCENARIES'); | ||
| assert(validate_faction('SUPREME_LAW'), 'Should accept SUPREME_LAW'); | ||
| assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept REBEL_TECHNOMANCERS"); | ||
|
|
||
| // Invalid faction | ||
| assert(!validate_faction('INVALID_FACTION'), 'Should reject invalid faction'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix invalid assert! macro usage (Cairo).
Use assert(cond, 'msg') consistently. The assert! macro form will not compile here.
Apply:
- assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept REBEL_TECHNOMANCERS");
+ assert(validate_faction('REBEL_TECHNOMANCERS'), 'Should accept REBEL_TECHNOMANCERS');Also consider removing unused imports (e.g., ContractAddress, contract_address_const, validate_admin_access, check_rate_limit, COA_CONTRACTS, SECURITY_CONFIG_ID, CREATE_SESSION_OP) to keep tests warning-free.
Quick scan for any lingering assert!:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\bassert!\s*\('Length of output: 12354
Fix invalid assert! macro usage (Cairo) — replace with assert(cond, 'msg') across the repo.
- Replace the line in src/test/security_test.cairo:
- assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept REBEL_TECHNOMANCERS");
+ assert(validate_faction('REBEL_TECHNOMANCERS'), 'Should accept REBEL_TECHNOMANCERS');- rg found many additional
assert!(...)occurrences in tests/models (examples: src/models/gear.cairo, src/test/test_unequip.cairo, src/test/combat_session_test.cairo, src/test/gear_read_test.cairo). Runrg -nP '\bassert!\s*\('and replace withassert(expr, 'msg')everywhere; theassert!form will not compile for these files. - Also consider removing unused imports (ContractAddress, contract_address_const, validate_admin_access, check_rate_limit, COA_CONTRACTS, SECURITY_CONFIG_ID, CREATE_SESSION_OP) to avoid warnings.
📝 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.
| assert(validate_faction('CHAOS_MERCENARIES'), 'Should accept CHAOS_MERCENARIES'); | |
| assert(validate_faction('SUPREME_LAW'), 'Should accept SUPREME_LAW'); | |
| assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept REBEL_TECHNOMANCERS"); | |
| // Invalid faction | |
| assert(!validate_faction('INVALID_FACTION'), 'Should reject invalid faction'); | |
| } | |
| assert(validate_faction('CHAOS_MERCENARIES'), 'Should accept CHAOS_MERCENARIES'); | |
| assert(validate_faction('SUPREME_LAW'), 'Should accept SUPREME_LAW'); | |
| assert(validate_faction('REBEL_TECHNOMANCERS'), 'Should accept REBEL_TECHNOMANCERS'); | |
| // Invalid faction | |
| assert(!validate_faction('INVALID_FACTION'), 'Should reject invalid faction'); | |
| } |
🤖 Prompt for AI Agents
In src/test/security_test.cairo around lines 27 to 33, the test uses the
Rust-style assert! macro which Cairo doesn't support; replace
assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept
REBEL_TECHNOMANCERS"); with the Cairo form
assert(validate_faction('REBEL_TECHNOMANCERS'), 'Should accept
REBEL_TECHNOMANCERS'); and similarly update all other occurrences across the
repo (run rg -nP '\bassert!\s*\(' to find them) converting assert!(expr, "msg")
to assert(expr, 'msg'); additionally remove any unused imports mentioned
(ContractAddress, contract_address_const, validate_admin_access,
check_rate_limit, COA_CONTRACTS, SECURITY_CONFIG_ID, CREATE_SESSION_OP) from
files that no longer reference them to silence warnings.
There was a problem hiding this comment.
that is bigger than what can be in a felt or short string, so " is good
There was a problem hiding this comment.
@truthixify Thank you for the clarification! You're absolutely right - in Cairo, longer strings that exceed the felt252 capacity should use double quotes, while shorter strings use single quotes. I appreciate you correcting my understanding of Cairo's string literal syntax.
The assert!(validate_faction('REBEL_TECHNOMANCERS'), "Should accept REBEL_TECHNOMANCERS"); line should indeed keep the double quotes for the message since "Should accept REBEL_TECHNOMANCERS" is too long for a felt252.
✏ Learnings added
Learnt from: truthixify
PR: SunsetLabs-Game/COA-Contracts#160
File: src/test/security_test.cairo:27-33
Timestamp: 2025-09-23T20:27:17.417Z
Learning: In Cairo, string literals have different syntax based on length: short strings that fit in felt252 use single quotes, while longer strings use double quotes. Faction names like "REBEL_TECHNOMANCERS" and longer assertion messages should use double quotes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/contracts-ci.yml (1)
47-51: Install Scarb 2.10.1+ in CI before runningscarb fmt --checkDojo 1.5.0 requires Scarb 2.10.1 or later; the formatting step will fail if Scarb isn't installed. Add an asdf-based install/pin for scarb (e.g., install and set global scarb 2.10.1) in .github/workflows/contracts-ci.yml immediately before the "Check formatting" step.
🧹 Nitpick comments (4)
.github/workflows/contracts-ci.yml (4)
31-36: Consider removing Starknet Foundry install if unusedYou install starknet-foundry but don’t run
snforgeor related tools (onlysozo/scarbare used). Drop this to speed up CI unless you plan to re-enable the ERC721 block.- - name: Install Starknet Foundry - run: | - asdf plugin add starknet-foundry - asdf install starknet-foundry 0.35.0 - asdf global starknet-foundry 0.35.0
18-18: Update checkout to v4Use the latest major for performance/security updates and Node 20 runtime.
- uses: actions/checkout@v3 + uses: actions/checkout@v4
11-16: Optional: switch Rust toolchain action
actions-rs/toolchain@v1is community-maintained. If you only need Rust for installing local tools, considerdtolnay/rust-toolchainoractions/setup-rustfor a slimmer, maintained setup.Example:
- - name: Install Rust - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - override: true + - name: Install Rust + uses: dtolnay/rust-toolchain@stable
37-46: Add explicit tool version logging for diagnosticsPrint versions before build/test to catch mismatches early (Dojo/Scarb/Cairo).
- name: Build contracts run: | cd src + sozo --version + scarb --version || true sozo buildConfirm that
scarb --versionsucceeds after adding the Scarb install step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/contracts-ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
.github/workflows/contracts-ci.yml (1)
23-29: Harden dojoup install: pin/verify installer and add sanity checksFile: .github/workflows/contracts-ci.yml (lines 23–29)
Avoid piping an unpinned remote script into bash; add set -euo pipefail, prefer pin+checksum verification for the installer, and include a quick sanity check that the installed tools are available.
- - 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 Dojo using dojoup + shell: bash + run: | + set -euo pipefail + # Option A: pin and verify (recommended) — replace EXPECTED_SHA256 with the published value + # curl -L https://install.dojoengine.org -o /tmp/dojoup.sh + # echo "EXPECTED_SHA256 /tmp/dojoup.sh" | sha256sum -c - + # bash /tmp/dojoup.sh + # Option B: current behavior (unpinned) + curl -L https://install.dojoengine.org | bash + . "$HOME/.config/.dojo/env" + dojoup install 1.5.0 + echo "$HOME/.config/.dojo/bin" >> "$GITHUB_PATH" + # Sanity checks + command -v sozo + sozo --version
2 participants
This PR added missing security measures and access control.
Closes #149
Summary by CodeRabbit