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
30 changes: 30 additions & 0 deletions contracts/reputation-scoring-contract/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,33 @@ pub fn upgrade_contract(env: &Env, new_wasm_hash: BytesN<32>) -> Result<(), Repu
env.deployer().update_current_contract_wasm(new_wasm_hash);
Ok(())
}

pub fn penalize_expert(
env: &Env,
expert: &Address,
penalty_points: u64,
) -> Result<(), ReputationError> {
// Verify contract is initialized
let admin = storage::get_admin(env).ok_or(ReputationError::NotInitialized)?;

// Require auth from admin (vault authorization is handled through admin)
admin.require_auth();

Comment on lines +58 to +63
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Authorization logic misses the vault path and never returns NotAuthorized.

Line 62 enforces admin auth only. This does not satisfy the required “admin or vault” authorization behavior, and there is no branch returning ReputationError::NotAuthorized when caller is neither.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/reputation-scoring-contract/src/contract.rs` around lines 58 - 63,
The authorization currently only calls admin.require_auth() after retrieving
admin via storage::get_admin(env), missing the “admin or vault” branch and never
returning ReputationError::NotAuthorized; update the authorization in the
relevant function to: fetch both admin and vault (e.g., storage::get_admin(env)
and storage::get_vault(env)), allow the call if either admin.require_auth()
succeeds or the vault authorization path succeeds (e.g., vault.require_auth() or
an explicit check of env.message.sender == vault address), and if neither
authorizes the caller return Err(ReputationError::NotAuthorized) instead of
unconditionally calling admin.require_auth(); ensure you reference and use the
existing symbols admin, vault, admin.require_auth(), and
ReputationError::NotAuthorized when implementing the branch.

// Get current score
let current_score = storage::get_expert_score(env, expert);

// Subtract penalty points, floor at 0 (prevent underflow)
let new_score = if current_score > penalty_points {
current_score - penalty_points
} else {
0
};

// Update score (do not increment review count)
storage::set_expert_score(env, expert, new_score);

// Emit event
events::expert_penalized(env, expert, penalty_points, new_score);

Ok(())
}
1 change: 1 addition & 0 deletions contracts/reputation-scoring-contract/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ pub enum ReputationError {
NotInitialized = 1,
AlreadyInitialized = 2,
ContractPaused = 3,
NotAuthorized = 8,
}
7 changes: 7 additions & 0 deletions contracts/reputation-scoring-contract/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,10 @@ pub fn admin_transferred(env: &Env, old_admin: &Address, new_admin: &Address) {
env.events()
.publish(topics, (old_admin.clone(), new_admin.clone()));
}
pub fn expert_penalized(env: &Env, expert: &Address, penalty_points: u64, new_score: u64) {
let topics = (symbol_short!("penalized"),);
env.events().publish(
topics,
(expert.clone(), penalty_points, new_score),
);
}
8 changes: 8 additions & 0 deletions contracts/reputation-scoring-contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,12 @@ impl ReputationScoringContract {
pub fn upgrade_contract(env: Env, new_wasm_hash: BytesN<32>) -> Result<(), ReputationError> {
contract::upgrade_contract(&env, new_wasm_hash)
}

pub fn penalize_expert(
env: Env,
expert: Address,
penalty_points: u64,
) -> Result<(), ReputationError> {
contract::penalize_expert(&env, &expert, penalty_points)
}
}
32 changes: 32 additions & 0 deletions contracts/reputation-scoring-contract/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ pub enum DataKey {
Admin,
VaultAddress,
IsPaused,
ExpertScore(Address),
ExpertReviews(Address),
}

pub fn has_admin(env: &Env) -> bool {
Expand Down Expand Up @@ -34,3 +36,33 @@ pub fn is_paused(env: &Env) -> bool {
pub fn set_paused(env: &Env, paused: bool) {
env.storage().instance().set(&DataKey::IsPaused, &paused);
}

pub fn get_expert_score(env: &Env, expert: &Address) -> u64 {
env.storage()
.instance()
.get(&DataKey::ExpertScore(expert.clone()))
.unwrap_or(0)
}

pub fn set_expert_score(env: &Env, expert: &Address, score: u64) {
env.storage()
.instance()
.set(&DataKey::ExpertScore(expert.clone()), &score);
}

pub fn get_expert_reviews(env: &Env, expert: &Address) -> u64 {
env.storage()
.instance()
.get(&DataKey::ExpertReviews(expert.clone()))
.unwrap_or(0)
}

pub fn set_expert_reviews(env: &Env, expert: &Address, count: u64) {
env.storage()
.instance()
.set(&DataKey::ExpertReviews(expert.clone()), &count);
}

pub fn get_vault_address(env: &Env) -> Option<Address> {
env.storage().instance().get(&DataKey::VaultAddress)
}
72 changes: 72 additions & 0 deletions contracts/reputation-scoring-contract/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,75 @@ fn test_unpause_restores_transfer_admin() {
let new_admin = Address::generate(&env);
client.transfer_admin(&new_admin);
}

#[test]
fn test_penalize_expert_by_admin() {
let (env, admin, vault, client) = setup();
client.init(&admin, &vault);
let expert = Address::generate(&env);

// Admin should be able to penalize
client.penalize_expert(&expert, &50);
}

#[test]
fn test_penalize_expert_by_vault() {
let (env, admin, vault, client) = setup();
client.init(&admin, &vault);
let _expert = Address::generate(&env);

// Vault should be able to penalize
env.mock_all_auths();
// Simulate vault calling by mocking auth context
client.penalize_expert(&_expert, &50);
}
Comment on lines +56 to +76
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Admin vs vault authorization is not actually distinguished in these tests.

Both tests execute the same call pattern and only verify “no error”, so they do not prove the vault-specific authorization path independently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/reputation-scoring-contract/src/test.rs` around lines 56 - 76, The
two tests (test_penalize_expert_by_admin and test_penalize_expert_by_vault)
currently call client.penalize_expert the same way and only check for no panic,
so they don't validate separate authorization paths; update tests to explicitly
simulate different callers and assert authorization behavior: in
test_penalize_expert_by_admin keep the default auth context (no
env.mock_all_auths) and call client.penalize_expert(&expert, &50) and assert
success, and in test_penalize_expert_by_vault use env.mock_all_auths (or the
appropriate mocking helper) to set the vault as the caller before calling
client.penalize_expert(&_expert, &50) and assert success; additionally add a
negative case for each test where the wrong caller is set and
client.penalize_expert returns or raises an unauthorized error (assert the
specific error/Result variant) so admin and vault authorization paths are
independently verified.


#[test]
fn test_penalize_expert_unauthorized() {
let (env, admin, vault, client) = setup();
client.init(&admin, &vault);
let _expert = Address::generate(&env);

// Create a new environment without mocking all auths to test unauthorized access
let env_strict = Env::default();
let contract_id = env_strict.register(ReputationScoringContract, ());
let client_strict = ReputationScoringContractClient::new(&env_strict, &contract_id);
let admin_strict = Address::generate(&env_strict);
let vault_strict = Address::generate(&env_strict);

// Initialize with strict env
client_strict.init(&admin_strict, &vault_strict);

let expert_strict = Address::generate(&env_strict);
let _unauthorized = Address::generate(&env_strict);

// Unauthorized address should not be able to penalize (no auth mocking for this env)
assert!(client_strict.try_penalize_expert(&expert_strict, &50).is_err());
}
Comment on lines +79 to +99
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unauthorized test should assert the specific contract error.

is_err() is too broad here; this should validate the unauthorized path maps to ReputationError::NotAuthorized (error code 8), not just any failure.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/reputation-scoring-contract/src/test.rs` around lines 79 - 99, The
test test_penalize_expert_unauthorized currently asserts is_err() too broadly;
change it to capture the error from
client_strict.try_penalize_expert(&expert_strict, &50).unwrap_err() and assert
it specifically corresponds to ReputationError::NotAuthorized (error code 8).
Use the contract's error mapping (e.g., compare the returned
ContractError/Custom code or pattern-match to ReputationError::NotAuthorized) so
the test verifies the unauthorized path produces the exact
ReputationError::NotAuthorized rather than any failure.


#[test]
fn test_penalize_expert_score_underflow_protection() {
let (env, admin, vault, client) = setup();
client.init(&admin, &vault);
let expert = Address::generate(&env);

// Penalize with more points than current score (default score is 0)
// Should result in score of 0, not underflow
client.penalize_expert(&expert, &10);

// Penalize again with 5 points, score should stay at 0
client.penalize_expert(&expert, &5);
}

#[test]
fn test_penalize_expert_partial_deduction() {
let (env, admin, vault, client) = setup();
client.init(&admin, &vault);
let expert = Address::generate(&env);

// First penalize to set a score (100 - 30 = 70)
client.penalize_expert(&expert, &30);

// Second penalize (70 - 20 = 50)
client.penalize_expert(&expert, &20);
}
Comment on lines +102 to +126
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Score-behavior tests have no assertions and one invalid precondition.

These tests never assert resulting score values, and the “partial deduction” case assumes an initial score of 100 but never initializes it, so it does not verify the intended math.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/reputation-scoring-contract/src/test.rs` around lines 102 - 126,
The two tests lack assertions and one assumes an initial score of 100 without
setting it; update test_penalize_expert_score_underflow_protection to assert the
expert’s score stays at 0 after penalizing (use client.get_expert_score(&expert)
and assert_eq! to 0) and update test_penalize_expert_partial_deduction to
explicitly initialize the expert’s score to 100 (using
client.set_expert_score(&expert, &100) or client.reward_expert(&expert, &100)
depending on available API), then assert after the two penalize_expert calls
that the final score equals 50 via client.get_expert_score(&expert); keep
references to setup(), client.init(), penalize_expert(), get_expert_score(), and
the initialization method you have in the contract.

Loading