Skip to content
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.

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

solana: timelock preloading ops authority #474

Merged
merged 34 commits into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
576b2f1
fix: timelock preload op auth to proposer
jadepark-dev Jan 18, 2025
39b5025
chore: tests mods & preload with shared mcm root
jadepark-dev Jan 18, 2025
e2f8c40
chore: preload ops with mcm::execute
jadepark-dev Jan 19, 2025
b48b290
chore: another preloading with mcm execute
jadepark-dev Jan 20, 2025
a71263d
chore: remove unused import
jadepark-dev Jan 20, 2025
1b5da06
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 20, 2025
14f8e08
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 20, 2025
babff69
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 20, 2025
3c62d54
chore: clean up
jadepark-dev Jan 21, 2025
24fad7a
fix: access control macro allowing multiple roles
jadepark-dev Jan 21, 2025
64f0fde
chore: preload op ac
jadepark-dev Jan 21, 2025
624af26
fix: access control macro
jadepark-dev Jan 21, 2025
3566aa0
chore: remove example for lint
jadepark-dev Jan 22, 2025
0d437cb
chore: clean up
jadepark-dev Jan 22, 2025
ba2078c
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 22, 2025
00d5ab4
fix: assert closed op account after execution
jadepark-dev Jan 22, 2025
279665f
chore: update macro names
jadepark-dev Jan 22, 2025
adbc31f
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 23, 2025
46d203e
fix: bypasser execute condition, clean up
jadepark-dev Jan 23, 2025
813ee27
Merge branch 'solana/timelock-preload-op-auth' of github.com:smartcon…
jadepark-dev Jan 23, 2025
d025072
chore: auth tests for preloading operation
jadepark-dev Jan 23, 2025
d921aa7
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 23, 2025
0c9bc62
feat: bypasser operation PDA separation
jadepark-dev Jan 24, 2025
8b5919c
fix: separated tests for bypasser op account
jadepark-dev Jan 24, 2025
411ae32
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 24, 2025
191b969
fix: remove done check from bypasser_execute
jadepark-dev Jan 24, 2025
55eab56
chore: doc clean up
jadepark-dev Jan 24, 2025
3ab0e03
chore: testing ci doc lint
jadepark-dev Jan 24, 2025
7da5bf8
chore: ci test
jadepark-dev Jan 24, 2025
ebb168b
chore: restore module doc
jadepark-dev Jan 24, 2025
90317ed
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 24, 2025
735340a
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 24, 2025
e1907bf
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 25, 2025
ca4be96
Merge branch 'main' into solana/timelock-preload-op-auth
jadepark-dev Jan 28, 2025
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
74 changes: 52 additions & 22 deletions chains/solana/contracts/programs/timelock/src/access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,56 +5,86 @@ use access_controller::AccessController;
use crate::error::{AuthError, TimelockError};
use crate::state::{Config, Role};

// NOTE: This macro is used to check if the authority is the owner
// or account that has access to the role
/// A macro that checks if the authority has admin privileges OR has at least one of the specified roles.
/// This is used as an attribute macro with #[access_control] to guard program instructions.
///
/// # Arguments
/// * `$ctx` - The context containing program accounts
/// * `$role` - One or more roles that are allowed to execute the instruction
///
#[macro_export]
macro_rules! only_role_or_admin_role {
($ctx:expr, $role:expr) => {
only_role_or_admin_role(
macro_rules! require_role_or_admin {
($ctx:expr, $($role:expr),+) => {{
only_role_or_admin(
&$ctx.accounts.config,
&$ctx.accounts.role_access_controller,
&$ctx.accounts.authority,
$role,
&[$($role),+]
)
};
}};
}

pub fn only_role_or_admin_role(
/// Helper function to verify if an authority has admin rights or any of the specified roles.
/// Returns Ok(()) if the authority is either the admin or has at least one of the roles.
///
/// # Arguments
/// * `config` - Account containing program configuration including owner
/// * `role_controller` - Account managing role-based access control
/// * `authority` - The signer attempting to execute the instruction
/// * `roles` - Array of roles being checked for authorization
///
/// # Returns
/// * `Result<()>` - Ok if authorized
/// * `Err(TimelockError::InvalidAccessController)` - If provided controller isn't configured for any roles
/// * `Err(AuthError::Unauthorized)` - If authority lacks admin rights and required roles

pub fn only_role_or_admin(
config: &Account<Config>,
role_controller: &AccountLoader<AccessController>,
authority: &Signer,
role: Role,
roles: &[Role],
) -> Result<()> {
// early authorization check if the authority is the owner
if authority.key() == config.owner {
return Ok(());
}

// check the provided access controller is the correct one
require_keys_eq!(
config.get_role_controller(&role),
role_controller.key(),
require!(
roles
.iter()
.any(|role| config.get_role_controller(role) == role_controller.key()),
TimelockError::InvalidAccessController
);

// check if the authority has access to the role
require!(
access_controller::has_access(role_controller, &authority.key())?,
AuthError::Unauthorized
);
let has_valid_role = roles.iter().any(|role| {
config.get_role_controller(role) == role_controller.key()
&& access_controller::has_access(role_controller, &authority.key()).unwrap_or(false)
});

require!(has_valid_role, AuthError::Unauthorized);
Ok(())
}

// NOTE: This macro is used to check if the authority is the owner
// this can be in account contexts, but added also for explicit consistency with other role check
/// A macro that checks if the authority is the admin (owner) of the program.
/// This provides a simpler check when only admin access is required.
///
/// # Arguments
/// * `$ctx` - The context containing program accounts
#[macro_export]
macro_rules! only_admin {
macro_rules! require_only_admin {
($ctx:expr) => {
only_admin(&$ctx.accounts.config, &$ctx.accounts.authority)
};
}

/// Helper function to verify if an authority is the admin (owner).
/// Returns Ok(()) if the authority is the admin, Err otherwise.
///
/// # Arguments
/// * `config` - Account containing program configuration including owner
/// * `authority` - The signer attempting to execute the instruction
///
/// # Returns
/// * `Result<()>` - Ok if authorized, Err with AuthError::Unauthorized otherwise
pub fn only_admin(config: &Account<Config>, authority: &Signer) -> Result<()> {
require_keys_eq!(authority.key(), config.owner, AuthError::Unauthorized);
Ok(())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/// PDA seeds
pub const TIMELOCK_CONFIG_SEED: &[u8] = b"timelock_config";
pub const TIMELOCK_OPERATION_SEED: &[u8] = b"timelock_operation";
pub const TIMELOCK_BYPASSER_OPERATION_SEED: &[u8] = b"timelock_bypasser_operation";
pub const TIMELOCK_SIGNER_SEED: &[u8] = b"timelock_signer";
pub const TIMELOCK_BLOCKED_FUNCITON_SELECTOR_SEED: &[u8] = b"timelock_blocked_function_selector";

Expand Down
26 changes: 14 additions & 12 deletions chains/solana/contracts/programs/timelock/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,44 +23,46 @@ pub enum TimelockError {
#[msg("Provided ID is invalid")]
InvalidId,

#[msg("RBACTimelock: operation not finalized")]
#[msg("operation not finalized")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: clean up error messages but subject to change on the off-chain requirement

OperationNotFinalized,

#[msg("RBACTimelock: operation is already finalized")]
#[msg("operation is already finalized")]
OperationAlreadyFinalized,

#[msg("RBACTimelock: too many instructions in the operation")]
#[msg("too many instructions in the operation")]
TooManyInstructions,

// on attempt to create PDA with the same seed(existing operation)
#[msg("RBACTimelock: operation already scheduled")]
#[msg("operation already scheduled")]
OperationAlreadyScheduled,

#[msg("RBACTimelock: insufficient delay")]
#[msg("insufficient delay")]
DelayInsufficient,

// cancel
#[msg("RBACTimelock: operation cannot be cancelled")]
#[msg("operation cannot be cancelled")]
OperationNotCancellable,

#[msg("operation is not ready")]
OperationNotReady,

#[msg("operation is already executed")]
OperationAlreadyExecuted,

#[msg("Predecessor operation is not found")]
MissingDependency,

#[msg("RBACTimelock: Provided access controller is invalid")]
#[msg("Provided access controller is invalid")]
InvalidAccessController,

#[msg("RBACTimelock: selector is blocked")]
#[msg("selector is blocked")]
BlockedSelector,

#[msg("RBACTimelock: selector is already blocked")]
#[msg("selector is already blocked")]
AlreadyBlocked,

#[msg("RBACTimelock: selector not found")]
#[msg("selector not found")]
SelectorNotFound,

#[msg("RBACTimelock: maximum capacity reached for function blocker")]
#[msg("maximum capacity reached for function blocker")]
MaxCapacityReached,
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use anchor_lang::prelude::*;
use anchor_lang::solana_program::keccak::HASH_BYTES;

use access_controller::AccessController;

Expand All @@ -11,21 +12,21 @@ use crate::state::{Config, Operation};
pub fn cancel<'info>(
_ctx: Context<'_, '_, '_, 'info, Cancel<'info>>,
_timelock_id: [u8; TIMELOCK_ID_PADDED],
id: [u8; 32],
id: [u8; HASH_BYTES],
) -> Result<()> {
emit!(Cancelled { id });
// NOTE: PDA is closed - is handled by anchor on exit due to the `close` attribute
Ok(())
}

#[derive(Accounts)]
#[instruction(timelock_id: [u8; TIMELOCK_ID_PADDED], id: [u8; 32])]
#[instruction(timelock_id: [u8; TIMELOCK_ID_PADDED], id: [u8; HASH_BYTES])]
pub struct Cancel<'info> {
#[account(
mut,
seeds = [TIMELOCK_OPERATION_SEED, timelock_id.as_ref(), id.as_ref()],
bump,
close = authority, // todo: check if we send fund back to the signer, otherwise, we'll have additional destination account
close = authority,
constraint = operation.id == id @ TimelockError::InvalidId,
constraint = operation.is_pending() @ TimelockError::OperationNotCancellable,
)]
Expand All @@ -34,7 +35,7 @@ pub struct Cancel<'info> {
#[account( seeds = [TIMELOCK_CONFIG_SEED, timelock_id.as_ref()], bump)]
pub config: Account<'info, Config>,

// NOTE: access controller check happens in only_role_or_admin_role macro
// NOTE: access controller check happens in require_role_or_admin macro
pub role_access_controller: AccountLoader<'info, AccessController>,

#[account(mut)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use access_controller::AccessController;
use bytemuck::Zeroable;

use crate::constants::{
EMPTY_PREDECESSOR, TIMELOCK_CONFIG_SEED, TIMELOCK_ID_PADDED, TIMELOCK_OPERATION_SEED,
TIMELOCK_SIGNER_SEED,
EMPTY_PREDECESSOR, TIMELOCK_BYPASSER_OPERATION_SEED, TIMELOCK_CONFIG_SEED, TIMELOCK_ID_PADDED,
TIMELOCK_OPERATION_SEED, TIMELOCK_SIGNER_SEED,
};
use crate::error::TimelockError;
use crate::event::*;
Expand All @@ -19,7 +19,7 @@ use crate::state::{Config, InstructionData, Operation};
pub fn execute_batch<'info>(
ctx: Context<'_, '_, '_, 'info, ExecuteBatch<'info>>,
timelock_id: [u8; TIMELOCK_ID_PADDED],
_id: [u8; 32],
_id: [u8; HASH_BYTES],
) -> Result<()> {
let op = &mut ctx.accounts.operation;

Expand All @@ -38,8 +38,9 @@ pub fn execute_batch<'info>(
ctx.program_id,
);

require!(
ctx.accounts.predecessor_operation.key() == expected_address,
require_keys_eq!(
ctx.accounts.predecessor_operation.key(),
expected_address,
TimelockError::InvalidInput
);

Expand All @@ -49,8 +50,9 @@ pub fn execute_batch<'info>(

require!(predecessor_acc.is_done(), TimelockError::MissingDependency);
} else {
require!(
ctx.accounts.predecessor_operation.key() == Pubkey::zeroed(),
require_keys_eq!(
ctx.accounts.predecessor_operation.key(),
Pubkey::zeroed(),
TimelockError::InvalidInput
);
}
Expand Down Expand Up @@ -78,8 +80,6 @@ pub fn execute_batch<'info>(
});
}

require!(op.is_ready(current_time), TimelockError::OperationNotReady);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: In the EVM-based version, we do a second is_ready check after executing all calls to guard against reentrancy mid-transaction, which might invalidate the operation. On Solana, however, each instruction is atomic and there is no reentrancy model that would allow an external actor to modify our operation state during its execution. Therefore, the second check is unnecessary here.


// all executed, update the timestamp
op.mark_done();

Expand All @@ -88,10 +88,11 @@ pub fn execute_batch<'info>(

/// execute operation(instructions) w/o checking predecessors and readiness
/// bypasser_execute also need the operation to be uploaded formerly
/// NOTE: operation should be closed after execution
pub fn bypasser_execute_batch<'info>(
ctx: Context<'_, '_, '_, 'info, BypasserExecuteBatch<'info>>,
timelock_id: [u8; TIMELOCK_ID_PADDED],
_id: [u8; 32],
_id: [u8; HASH_BYTES],
) -> Result<()> {
let op = &mut ctx.accounts.operation;

Expand Down Expand Up @@ -166,21 +167,22 @@ pub struct ExecuteBatch<'info> {
)]
pub timelock_signer: UncheckedAccount<'info>,

// NOTE: access controller check happens in only_role_or_admin_role macro
// NOTE: access controller check happens in require_role_or_admin macro
pub role_access_controller: AccountLoader<'info, AccessController>,

#[account(mut)]
pub authority: Signer<'info>,
}

#[derive(Accounts)]
#[instruction(timelock_id: [u8; 32], id: [u8; 32])]
#[instruction(timelock_id: [u8; TIMELOCK_ID_PADDED], id: [u8; HASH_BYTES])]
pub struct BypasserExecuteBatch<'info> {
#[account(
mut,
seeds = [TIMELOCK_OPERATION_SEED, timelock_id.as_ref(), id.as_ref()],
seeds = [TIMELOCK_BYPASSER_OPERATION_SEED, timelock_id.as_ref(), id.as_ref()],
bump,
constraint = operation.is_finalized @ TimelockError::OperationNotFinalized,
close = authority, // close the bypasser operation after execution
)]
pub operation: Account<'info, Operation>,

Expand All @@ -194,7 +196,7 @@ pub struct BypasserExecuteBatch<'info> {
)]
pub timelock_signer: UncheckedAccount<'info>,

// NOTE: access controller check happens in only_role_or_admin_role macro
// NOTE: access controller check happens in require_role_or_admin macro
pub role_access_controller: AccountLoader<'info, AccessController>,

#[account(mut)]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
pub mod cancel;
pub mod execute;
pub mod initialize;
pub mod operation;
pub mod schedule;

pub use cancel::*;
pub use execute::*;
pub use initialize::*;
pub use operation::*;
pub use schedule::*;
Loading
Loading