Skip to content

feat(benchmark): add gas and footprint profiling for critical admin operations #277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
OlufunbiIK merged 2 commits into from
Mar 24, 2026
Merged

feat(benchmark): add gas and footprint profiling for critical admin operations #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1631a06
feat(security): implement modular access control system for admin aut…
lishmanTech Mar 24, 2026
9d7e812
feat(benchmark): add gas and footprint profiling for critical admin o…
lishmanTech Mar 24, 2026
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
24 changes: 24 additions & 0 deletions contracts/access_control/admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
use soroban_sdk::{Env, Address};
use super::storage::get_admin;

/// 🔒 Require admin authentication
pub fn require_admin(env: &Env) -> Address {
let admin = get_admin(env);

// Enforce authentication
admin.require_auth();

admin
}

/// 🔄 Optional: transfer admin ownership
pub fn transfer_admin(env: &Env, new_admin: Address) {
let current_admin = require_admin(env);

// Prevent accidental self-transfer (optional safety)
if current_admin == new_admin {
panic!("New admin must be different");
}

super::storage::set_admin(env, &new_admin);
Comment on lines +15 to +23
Copy link

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Consider requiring new_admin.require_auth() for two-sided authorization.

The transfer_admin function only authenticates the current admin, not the new admin. This allows transferring ownership to any address without the recipient's consent, which could lead to:

  • Accidental transfer to an incorrect or non-existent address
  • No confirmation that the new admin can actually control that address

Per the relevant code snippets, sibling contracts (staking, multisig) use a two-sided pattern where the admin authenticates themselves during initialization. Consider requiring new_admin.require_auth() to ensure the recipient can control the address before accepting ownership.

🔒 Proposed fix for two-sided authorization 
 pub fn transfer_admin(env: &Env, new_admin: Address) {
     let current_admin = require_admin(env);
 
     // Prevent accidental self-transfer (optional safety)
     if current_admin == new_admin {
         panic!("New admin must be different");
     }
 
+    // Require new admin to consent to ownership transfer
+    new_admin.require_auth();
+
     super::storage::set_admin(env, &new_admin);
 }
📝 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 transfer_admin(env: &Env, new_admin: Address) {
let current_admin = require_admin(env);
// Prevent accidental self-transfer (optional safety)
if current_admin == new_admin {
panic!("New admin must be different");
}
super::storage::set_admin(env, &new_admin);
pub fn transfer_admin(env: &Env, new_admin: Address) {
let current_admin = require_admin(env);
// Prevent accidental self-transfer (optional safety)
if current_admin == new_admin {
panic!("New admin must be different");
}
// Require new admin to consent to ownership transfer
new_admin.require_auth();
super::storage::set_admin(env, &new_admin);
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@contracts/access_control/admin.rs` around lines 15 - 23, The transfer_admin
function currently only checks the caller via require_admin(env) and sets the
new admin without confirming the recipient can authenticate; update
transfer_admin to also call new_admin.require_auth() (or equivalent) before
calling super::storage::set_admin to enforce two-sided authorization so the new
admin consents/controls the address (keep existing require_admin(env) check and
the self-transfer panic).

}
6 changes: 6 additions & 0 deletions contracts/access_control/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pub mod admin;
pub mod storage;

// Re-export for easy use
pub use admin::{require_admin, transfer_admin};
pub use storage::{set_admin, get_admin};
14 changes: 14 additions & 0 deletions contracts/access_control/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
use soroban_sdk::{Env, Address, Symbol};

const ADMIN_KEY: Symbol = Symbol::short("ADMIN");

pub fn set_admin(env: &Env, admin: &Address) {
env.storage().instance().set(&ADMIN_KEY, admin);
}

pub fn get_admin(env: &Env) -> Address {
env.storage()
.instance()
.get(&ADMIN_KEY)
.expect("Admin not initialized")
}
15 changes: 15 additions & 0 deletions contracts/benchmark/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
pub mod profiler;

pub use profiler::{GasProfile};
use soroban_sdk::Env;

/// Measure gas/footprint for a closure
pub fn profile<F>(env: &Env, f: F) -> GasProfile
where
F: FnOnce(),
{
let start = GasProfile::new(env);
f();
let end = GasProfile::new(env);
start.delta(&end)
}
21 changes: 21 additions & 0 deletions contracts/benchmark/profiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use soroban_sdk::{Env, BytesN};
Copy link

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove unused import BytesN.

BytesN is imported but never used in this file.

🧹 Proposed fix 
-use soroban_sdk::{Env, BytesN};
+use soroban_sdk::Env;
📝 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
use soroban_sdk::{Env, BytesN};
use soroban_sdk::Env;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@contracts/benchmark/profiler.rs` at line 1, The import list includes an
unused symbol BytesN which should be removed to clean up warnings: in the
module's use declaration (currently "use soroban_sdk::{Env, BytesN};") delete
BytesN so only Env is imported; ensure no other references to BytesN (e.g.,
variables or functions named BytesN) remain in profiler.rs before committing.


pub struct GasProfile {
pub cpu_insns: u64,
pub mem_bytes: u64,
}

impl GasProfile {
pub fn new(env: &Env) -> Self {
let cpu_insns = env.budget().cpu_insns();
let mem_bytes = env.budget().mem_bytes();
Self { cpu_insns, mem_bytes }
}

pub fn delta(&self, after: &GasProfile) -> GasProfile {
GasProfile {
cpu_insns: after.cpu_insns - self.cpu_insns,
mem_bytes: after.mem_bytes - self.mem_bytes,
}
}
Comment on lines +15 to +20
Copy link

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unchecked subtraction can panic or silently wrap on underflow.

The delta() method uses unchecked u64 subtraction. If after values are unexpectedly less than self values (e.g., due to budget reset, test misconfiguration, or SDK behavior changes), this will panic in debug builds or silently wrap around in release builds, producing misleading benchmark results.

Consider using saturating_sub() for safer arithmetic that clamps to zero instead of panicking/wrapping.

🛡️ Proposed fix using saturating subtraction 
     pub fn delta(&self, after: &GasProfile) -> GasProfile {
         GasProfile {
-            cpu_insns: after.cpu_insns - self.cpu_insns,
-            mem_bytes: after.mem_bytes - self.mem_bytes,
+            cpu_insns: after.cpu_insns.saturating_sub(self.cpu_insns),
+            mem_bytes: after.mem_bytes.saturating_sub(self.mem_bytes),
         }
     }
📝 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 delta(&self, after: &GasProfile) -> GasProfile {
GasProfile {
cpu_insns: after.cpu_insns - self.cpu_insns,
mem_bytes: after.mem_bytes - self.mem_bytes,
}
}
pub fn delta(&self, after: &GasProfile) -> GasProfile {
GasProfile {
cpu_insns: after.cpu_insns.saturating_sub(self.cpu_insns),
mem_bytes: after.mem_bytes.saturating_sub(self.mem_bytes),
}
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@contracts/benchmark/profiler.rs` around lines 15 - 20, The delta method in
GasProfile uses plain u64 subtraction and can panic or wrap on underflow; update
GasProfile::delta to use saturating_sub for cpu_insns and mem_bytes so any
negative delta clamps to zero (e.g., replace after.cpu_insns - self.cpu_insns
and after.mem_bytes - self.mem_bytes with saturating_sub calls on those fields),
keeping the function signature and returned GasProfile unchanged.

}
Loading