Coordinator Program Assessment - CrunchDAO

In the Coordinator program, any user can register as a Coordinator, but they must be approved by an admin to access platform functionalities. The approve_coordinator function allows transitioning a Coordinator from Pending to Approved, but there is no function to transition an approved Coordinator to Rejected, despite this state existing in the code.

programs/coordinator/src/instructions/approve_coordinator.rs

pub fn approve_coordinator(ctx: Context<ApproveCoordinator>) -> Result<()> {
    require!(
        ctx.accounts.signer.key() == ctx.accounts.config.coordinator_admin_authority,
        ErrorCode::OnlyAdminCanApproveCoordinator
    );
    let coordinator = &mut ctx.accounts.coordinator;
    coordinator.state = CoordinatorState::Approved;
    Ok(())
}

This means that once a Coordinator is approved, there is no built-in mechanism to revoke their access, even if they become malicious, compromised, or inactive.

A key example of this risk is found in the withdraw_usdc_from_crunch function, which allows a Coordinator to withdraw USDC from the reward_vault. This function does not check whether the Coordinator is still in an Approved state, assuming that once a Crunch reaches MarginPaidout, the associated Coordinator must be valid. However, since there is no way to revoke approval, a compromised Coordinator could continue accessing funds indefinitely.

programs/coordinator/src/instructions/approve_coordinator.rs

pub fn withdraw_usdc_from_crunch(
    ctx: Context<WithdrawUsdcFromCrunch>,
    amount: u64
) -> Result<()> {
    // Not all rewards might be payed out. Then there should be a way to withdraw the remaining rewards.
    // This should deposit back into the coodinators wallet
    let crunch = &ctx.accounts.crunch.key();
    require!(ctx.accounts.crunch.state == CrunchState::MarginPaidout, ErrorCode::CrunchMarginNotPaidout);
    require!(ctx.accounts.coordinator.key() == ctx.accounts.crunch.owner, ErrorCode::OnlyAdminCanWithdrawUsdcTheCrunch);
    let seeds = &[
        CRUNCH_AUTHORITY_SEED.as_bytes(), 
        crunch.as_ref(), 
        &[ctx.accounts.crunch.reward_vault_authority_bump]];

    let signer = &[&seeds[..]];


    let cpi_accounts = TransferChecked {
        from: ctx.accounts.reward_vault.to_account_info(),
        to: ctx.accounts.user_ata.to_account_info(), // TODO: this needs to change to the coordinator wallet
        authority: ctx.accounts.reward_vault_authority.to_account_info(),
        mint: ctx.accounts.usdc_mint.to_account_info(),
    };

    let cpi_program = ctx.accounts.token_program.to_account_info();

    let decimals = ctx.accounts.usdc_mint.decimals;

    let cpi_ctx = CpiContext::new_with_signer(cpi_program, cpi_accounts, signer);

    token::transfer_checked(cpi_ctx, amount, decimals)?;

    Ok(())
}

This lack of control poses a security risk, as an approved Coordinator retains full privileges indefinitely, including access to sensitive operations such as withdrawing funds from the reward_vault. Since functions like withdraw_usdc_from_crunch assume that an approved Coordinator remains valid, a compromised entity could continue executing privileged actions without restriction. Implementing a method to transition an Approved Coordinator to Rejected would provide a necessary safeguard to prevent unauthorized access.

Consider adding a function that allows an admin to change a Coordinator’s state to Rejected, ensuring that access can be revoked if necessary.

// Recommendation: Implement a function to revoke a coordinator's approval
#[derive(Accounts)]
pub struct RejectCoordinator<'info> {
    #[account(mut)]
    pub signer: Signer<'info>,

    #[account(
        mut,
        seeds = [COORDINATOR_SEED.as_bytes(), coordinator.owner.as_ref()],
        bump = coordinator.bump,
    )]
    pub coordinator: Account<'info, Coordinator>,

    #[account(
        seeds = [CONFIG_SEED.as_bytes()],
        bump = config.bump,
    )]
    pub config: Account<'info, CoordinatorConfig>,

    pub system_program: Program<'info, System>,
}

pub fn reject_coordinator(ctx: Context<RejectCoordinator>) -> Result<()> {
    require!(
        ctx.accounts.signer.key() == ctx.accounts.config.coordinator_admin_authority,
        ErrorCode::OnlyAdminCanRejectCoordinator
    );
    
    let coordinator = &mut ctx.accounts.coordinator;
    coordinator.state = CoordinatorState::Rejected;

    Ok(())
}

Additionally, all entry points that rely on an approved Coordinator should explicitly check the CoordinatorState before executing privileged actions to prevent unauthorized access.

For example, in the withdraw_usdc_from_crunch entry point, there is currently no validation to ensure that the Coordinator is still in the Approved state before allowing fund withdrawals. Adding this check would prevent a Rejected or unapproved Coordinator from accessing the reward_vault.

pub fn withdraw_usdc_from_crunch(ctx: Context<WithdrawUsdcFromCrunch>, amount: u64) -> Result<()> {
    require!(
        ctx.accounts.crunch.state == CrunchState::MarginPaidout,
        ErrorCode::CrunchMarginNotPaidout
    );
    
    require!(
        ctx.accounts.coordinator.key() == ctx.accounts.crunch.owner,
        ErrorCode::OnlyAdminCanWithdrawUsdcTheCrunch
    );

    // Recommendation: Add a check to ensure the Coordinator is still approved
    require!(
        ctx.accounts.coordinator.state == CoordinatorState::Approved,
        ErrorCode::CoordinatorNotApproved
    );

SOLVED: The CrunchDAO team resolved this finding by adding a new instruction reject_coordinator that sets the coordinator's state to Rejected and by ensuring all relevant instructions verify the correct coordinator state.

The update_coordinator_authority function update an admin authority directly, without a 2-step ownership transfer mechanism (e.g., requiring the new admin to accept the transfer before it is finalized). This could lead to accidental loss of control or malicious privilege escalation.

Take the snippet below as an example:

programs/coordinator/src/instructions/admin/update.rs

pub fn update_coordinator_authority(ctx: Context<UpdateCoordinatorConfig>, pub_key: Pubkey) -> Result<()> {

  let config_account = &mut ctx.accounts.config;

  // only the current admin can update the authority

  require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);

  config_account.coordinator_admin_authority = pub_key;

  Ok(())

}

Directly reassigning the authority without requiring confirmation from the new admin introduces the possibility of misconfiguration or unintended loss of access. If an incorrect public key is set, the program could become permanently inaccessible, preventing further administrative actions. Additionally, the lack of an acceptance step means that an authority change could take effect immediately without verification, increasing the risk of operational disruptions.

Implement a 2-step ownership transfer where:

1. The current admin proposes a new admin.

pub fn propose_new_coordinator_authority(ctx: Context<UpdateCoordinatorConfig>, proposed_admin: Pubkey) -> Result<()> {
    let config_account = &mut ctx.accounts.config;
    
    // Only the current admin can propose a new admin
    require!(
        *ctx.accounts.signer.key == config_account.coordinator_admin_authority,
        ErrorCode::OnlyAdminAllowed
    );

    // Store the proposed admin without finalizing the transfer
    config_account.proposed_coordinator_admin = Some(proposed_admin);

    msg!("New coordinator admin proposed: {}", proposed_admin);
    Ok(())
}

2. The new admin explicitly accepts the transfer before it takes effect.

pub fn accept_coordinator_authority(ctx: Context<UpdateCoordinatorConfig>) -> Result<()> {
    let config_account = &mut ctx.accounts.config;
    let signer_key = ctx.accounts.signer.key();

    // Ensure there is a pending admin transfer
    require!(
        config_account.proposed_coordinator_admin.is_some(),
        ErrorCode::NoPendingAdminTransfer
    );

    // Ensure the caller is the proposed new admin
    require!(
        config_account.proposed_coordinator_admin.unwrap() == *signer_key,
        ErrorCode::UnauthorizedAdminAcceptance
    );

    // Finalize the transfer and clear the proposed admin field
    config_account.coordinator_admin_authority = *signer_key;
    config_account.proposed_coordinator_admin = None;

    msg!("Coordinator admin authority transferred to {}", signer_key);
    Ok(())
}

SOLVED: The CrunchDAO team solved the issue by including a 2 step ownership transfer process.

In the Coordinator program, the init_config function initializes these values when creating a new CoordinatorConfig. Additionally, the functions update_burn_margin_percentage and update_foundation_margin_percentage allow modifying margin percentages that directly impact fund distribution in the payout_margin instruction.

However, none of these functions enforce any validation on the percentages before storing them. This means both the initialization and subsequent updates allow excessively high values, which could cause unintended behavior in payout calculations, as shown in the snippets below:

programs/coordinator/src/instructions/init_config.rs

pub fn init_config(ctx: Context<InitConfig>, config: CoordinatorConfig) -> Result<()> {
  let config_account = &mut ctx.accounts.config;
  config_account.bump = ctx.bumps.config;
  config_account.coordinator_admin_authority = config.coordinator_admin_authority;
  config_account.usdc_mint = config.usdc_mint;
  config_account.foundation_margin_percentage = config.foundation_margin_percentage;
  config_account.burn_margin_percentage = config.burn_margin_percentage;
  config_account.foundation_wallet = config.foundation_wallet;
  config_account.burn_wallet = config.burn_wallet;
  Ok(())
}

programs/coordinator/src/instructions/update_config.rs

pub fn update_burn_margin_percentage(ctx: Context<UpdateCoordinatorConfig>, burn_margin_percentage: u8) -> Result<()> {
  let config_account = &mut ctx.accounts.config;
  require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);
  config_account.burn_margin_percentage = burn_margin_percentage;
  Ok(())
}

programs/coordinator/src/instructions/update_config.rs

pub fn update_foundation_margin_percentage(ctx: Context<UpdateCoordinatorConfig>, foundation_margin_percentage: u8) -> Result<()> {
  let config_account = &mut ctx.accounts.config;
  require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);
  config_account.foundation_margin_percentage = foundation_margin_percentage;
  Ok(())
}

Since these percentages directly affect the payout calculation in payout_margin, excessively high values could lead to transferring all or more than the total available funds from the reward_vault to the burn_wallet or foundation_wallet.

Risk:

• Unbounded values during initialization and updates: The absence of validation allows setting an unreasonably high margin percentage at any time.

• Potential overflows or miscalculations: Since foundation_margin_percentage and burn_margin_percentage directly influence fund transfers, high values could result in unintended token movements.

• Fund depletion risk: The lack of constraints could allow the entire available balance in the reward_vault to be transferred, preventing other expected payouts.

Consider adding an upper limit validation in init_config, update_burn_margin_percentage and update_foundation_margin_percentage functions to ensure these percentages do not exceed a threshold, for instance, 100%. This will prevent unintended excessive transfers to the corresponding wallets:

programs/coordinator/src/instructions/init_config.rs

// Recommendation: Enforce an upper bound for burn and foundation margin percentages in initialization
pub fn init_config(ctx: Context<InitConfig>, config: CoordinatorConfig) -> Result<()> {
    let config_account = &mut ctx.accounts.config;
    config_account.bump = ctx.bumps.config;
    config_account.coordinator_admin_authority = config.coordinator_admin_authority;
    config_account.usdc_mint = config.usdc_mint;

    require!(config.foundation_margin_percentage <= 100, ErrorCode::InvalidFoundationMarginPercentage); // Recommendation: Set a reasonable max limit
    require!(config.burn_margin_percentage <= 100, ErrorCode::InvalidBurnMarginPercentage);

    config_account.foundation_margin_percentage = config.foundation_margin_percentage;
    config_account.burn_margin_percentage = config.burn_margin_percentage;
    config_account.foundation_wallet = config.foundation_wallet;
    config_account.burn_wallet = config.burn_wallet;
    Ok(())
}

programs/coordinator/src/instructions/update_config.rs

// Recommendation: Enforce an upper bound for burn and foundation margin percentages in updates
pub fn update_burn_margin_percentage(ctx: Context<UpdateCoordinatorConfig>, burn_margin_percentage: u8) -> Result<()> {
    let config_account = &mut ctx.accounts.config;
    require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);
    require!(burn_margin_percentage <= 100, ErrorCode::InvalidBurnMarginPercentage);
    config_account.burn_margin_percentage = burn_margin_percentage;
    Ok(())
}

programs/coordinator/src/instructions/update_config.rs

pub fn update_foundation_margin_percentage(ctx: Context<UpdateCoordinatorConfig>, foundation_margin_percentage: u8) -> Result<()> {
    let config_account = &mut ctx.accounts.config;
    require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);
    require!(foundation_margin_percentage <= 100, ErrorCode::InvalidFoundationMarginPercentage);
    config_account.foundation_margin_percentage = foundation_margin_percentage;
    Ok(())
}

SOLVED: The CrunchDAO team resolved this finding by implementing explicit upper limit validation for the burn and foundation margin percentages. This ensures that these values are always initialized or updated within the correct range.

The function update_coordinator_usdc_mint allows the administrator to update the USDC mint of the platform.

crunchdao-protocol/programs/coordinator/src/state/config.rs

pub fn update_coordinator_usdc_mint(ctx: Context<UpdateCoordinatorConfig>, usdc_mint: Pubkey) -> Result<()> {
  let config_account = &mut ctx.accounts.config;
  // only the current admin can update the usdc mint
  require!(*ctx.accounts.signer.key == config_account.coordinator_admin_authority, ErrorCode::OnlyAdminAllowed);
  config_account.usdc_mint = usdc_mint;
  Ok(())
}

This is problematic because several functions throughout the protocol rely on the USDC mint being a fixed, immutable value. Changing the mint address could cause unexpected failures in critical functionalities, including:

• Transfers & Payments: If the mint changes, existing USDC token accounts may become invalid, leading to failed transactions.

• Reward Distributions: The logic for distributing rewards and margin payments depends on a stable USDC mint.

• Liquidity Pools & Treasury Management: Any integration that depends on the previous USDC mint would need to be manually updated, introducing operational risks.

Allowing this function in a production environment could break core functionalities of the protocol and introduce security vulnerabilities.

Consider removing the function that updates the USDC mint.

Instead of allowing direct updates to the USDC mint, the protocol should implement a macro-based configuration system that automatically selects the appropriate mint address based on whether the environment is testnet or mainnet.

We can define a constant for the USDC mint that depends on the build configuration:

#[cfg(feature = "mainnet")]
pub const USDC_MINT: &str = "MainnetUSDCAddress456";

#[cfg(not(feature = "mainnet"))]
pub const USDC_MINT: &str = "LocalnetUSDCAddress456";

This way, the correct address is provided according to the features flag.

Then, enforce it at the account level by using the address constraint in the #[account] macro. This guarantees that only the correct USDC mint is accepted.

For example, in an entry point like withdraw_usdc_from_crunch, the usdc_mint account should be constrained to always match the predefined USDC_MINT:

programs/coordinator/src/instructions/crunch/withdrawusdc.rs

#[derive(Accounts)]
pub struct WithdrawUsdcFromCrunch<'info> {
    #[account(mut)]
    pub signer: Signer<'info>,

    #[account(
        seeds = [COORDINATOR_SEED.as_bytes(), signer.key().as_ref()],
        bump = coordinator.bump,
    )]
    pub coordinator: Account<'info, Coordinator>,

    #[account(
        seeds = [CONFIG_SEED.as_bytes()],
        bump = config.bump,
    )]
    pub config: Account<'info, CoordinatorConfig>,

    #[account( 
        mut,
        token::mint = usdc_mint, 
        token::authority = reward_vault_authority,
    )]
    pub reward_vault: Account<'info, TokenAccount>,

    #[account(seeds = [CRUNCH_AUTHORITY_SEED.as_bytes(), crunch.key().as_ref()], bump = crunch.reward_vault_authority_bump)]
    pub reward_vault_authority: AccountInfo<'info>,

    // Recommendation: Enforce the correct mint address at the account level
    #[account(address = USDC_MINT)]
    pub usdc_mint: Account<'info, Mint>,

    ////...... rest of the accounts .....
    
    pub system_program: Program<'info, System>,
    pub token_program: Program<'info, Token>,
}

To ensure the correct mint address is enforced at compile time, build the program with the appropriate feature flag:

cargo build-bpf --features mainnet --release

If no mainnet feature flag is specified, the program will default to using the testnet/localnet USDC mint address.

SOLVED: The CrunchDAO team resolved this finding by removing the instruction to update the USDC mint address and thus making the mint immutable.

The end_crunch function updates the crunch.state to CrunchState::Ended, but it does not close the crunch account. Since the account remains open, rent exemption fees are not recovered, leading to unnecessary storage costs on-chain.

The mentioned situation can be seen in the snippet below:

programs/coordinator/src/instructions/crunch/end.rs

pub fn end_crunch(ctx: Context<EndCrunch>) -> Result<()> {
    let crunch = &mut ctx.accounts.crunch;
    require!(
        crunch.state == CrunchState::Started,
        ErrorCode::InvalidCrunchStateToEnd
    );
    require!(
        crunch.owner == ctx.accounts.coordinator.key(),
        ErrorCode::OnlyAdminCanEndTheCrunch
    );

    crunch.state = CrunchState::Ended;

    emit!(CrunchEnded {
        crunch_pubkey: crunch.key()
    });

    Ok(())
}

The program marks the crunch as ended but does not reclaim the SOL locked in the rent-exempt account. Over time, keeping unused accounts open results in unnecessary costs, reducing the efficiency of the program. This can be particularly problematic in high-usage environments, where unclosed accounts accumulate and increase the storage footprint.

Consider using the #[account(close = recipient)] attribute in the Accounts struct to ensure that the Crunch account is automatically closed when the function executes, reclaiming rent exemption.

programs/coordinator/src/instructions/crunch/end.rs

#[derive(Accounts)]
pub struct EndCrunch<'info> {
    #[account(mut)]
    pub signer: Signer<'info>,

    #[account(
        seeds = [COORDINATOR_SEED.as_bytes(), signer.key().as_ref()],
        bump = coordinator.bump,
    )]
    pub coordinator: Account<'info, Coordinator>,

    #[account(mut, close = signer)] // Ensure the Crunch account is closed after execution
    pub crunch: Account<'info, Crunch>,
    
    pub system_program: Program<'info, System>,
}

ACKNOWLEDGED: The CrunchDAO team acknowledged this finding.

In the Coordinator program, the payout_checkpoint_transfer instruction includes cruncher_wallet as an account. However, upon reviewing the function, it is evident that this account is never accessed or utilized within the logic of the instruction.

Unused accounts in instructions increase unnecessary complexity by requiring clients to pass and maintain additional parameters that do not contribute to the execution of the instruction. This not only makes the program harder to interact with, but also introduces additional overhead for developers integrating with it.

programs/coordinator/src/instructions/payout_checkpoint_transfer.rs

#[account(mut)]
/// CHECK: This AccountInfo is safe, is not used
pub cruncher_wallet: AccountInfo<'info>,

Risk:

• Increased complexity: Developers interacting with the instruction must include cruncher_wallet even though it serves no functional purpose.

• Reduced maintainability: Future updates may introduce unintended behaviors or dependencies on this unused account, leading to harder-to-debug issues.

• Inefficient client interaction: External applications must unnecessarily pass this account when calling the instruction, making integration more cumbersome.

For future implementations, consider eliminating unused accounts from the instruction definition to reduce complexity and improve usability.

SOLVED: The CrunchDAO team resolved this finding by removing the unnecessary cruncher_wallet account.

In the Coordinator program, each Crunch is meant to be uniquely associated with a single Coordinator. This ensures that a Crunch cannot exist independently or be misattributed to multiple Coordinators. However, in the create_crunch function, the Crunch account is derived using only [CRUNCH_SEED.as_bytes(), name.as_bytes()] as its seeds, without including the coordinator account key.

By not using the coordinator.key() as part of the PDA derivation, there is no cryptographic enforcement of this one-to-one relationship, which introduces potential inconsistencies and security risks.

The mentioned situation can be seen in the snippet below:

programs/coordinator/src/instructions/create_crunch.rs

#[account(
    init, 
    payer = signer, 
    space = ANCHOR_DISCRIMINATOR + Crunch::INIT_SPACE,
    seeds = [CRUNCH_SEED.as_bytes(), name.as_bytes()],
    bump,
)]
pub crunch: Box<Account<'info, Crunch>>,

The code has been reviewed, and the current implementation correctly enforces the relationship between Coordinator and Crunch through state validation. However, since the Crunch account is not derived from the Coordinator account key, future implementations that interact with Crunch may introduce errors if they do not properly validate its ownership.

Without PDA derivation using the coordinator.key(), every time a Crunch account is passed to an instruction, explicit validation is required to ensure it belongs to the correct Coordinator. If a future instruction forgets to enforce this validation, it could allow unauthorized associations or unintended behavior. Using the coordinator.key() as a seed would prevent these issues by cryptographically enforcing the relationship.

Consider modifying the Crunch PDA derivation to include the coordinator.key() as a seed. This will create a direct relationship between Crunch and Coordinator, preventing the need for runtime validation checks in every instruction that interacts with a Crunch.

// Recommendation: Use the coordinator account key as a seed to enforce a one-to-one relationship
#[derive(Accounts)]
pub struct CreateCrunch<'info> {
    
    #[account(mut)]
    pub signer: Signer<'info>,

    #[account(
        seeds = [COORDINATOR_SEED.as_bytes(), signer.key().as_ref()], 
        bump = coordinator.bump,
    )]
    pub coordinator: Account<'info, Coordinator>,

    #[account(
        init, 
        payer = signer, 
        space = ANCHOR_DISCRIMINATOR + Crunch::INIT_SPACE,
        seeds = [CRUNCH_SEED.as_bytes(), coordinator.key().as_ref(), name.as_bytes()],
        bump,
    )]
    pub crunch: Box<Account<'info, Crunch>>,

    /* ... Other accounts ... */
    pub system_program: Program<'info, System>,
}

For future implementations, consider using seeds between related accounts to strengthen the relationship between them and improve security.

By deriving PDAs using the relevant account keys, ownership and associations are enforced at the protocol level, reducing the need for manual validation checks in instruction handlers.

SOLVED: The CrunchDAO team resolved this finding by modifying the Crunch PDA derivation to include the coordinator.key() as a seed and thus ensuring a Crunch account is related to a given coordinator account.

In the Coordinator program, multiple PDA accounts across different instructions are not explicitly derived using their seeds in the #[account] constraints. Instead, ownership and correctness are enforced through manual validation within the instruction logic.

One clear example of this issue appears in the payout_checkpoint_initialize entry point, where the previous_checkpoint account is a PDA but lacks explicit seed derivation.

Instead, its validity is manually checked by comparing its crunch field to ensure it belongs to the expected Crunch.

This requires additional runtime validation, increasing the risk of human error.

The mentioned situation can be seen in the snippet below:

programs/coordinator/src/instructions/payout_checkpoint_initialize.rs

#[account()]
pub previous_checkpoint: Account<'info, PayoutCheckpoint>,

This pattern is repeated in other instructions, leading to a widespread reliance on manual checks rather than leveraging PDA derivation to enforce relationships at the protocol level.

Since PDA derivation is not enforced in the context accounts, every time these accounts are passed to other instructions, additional manual validation is needed to confirm they belong to the correct entity. This increases the risk of:

• Incorrect associations, as ownership validation relies on logic within the instruction instead of cryptographic enforcement at the PDA level.

• Higher error likelihood, as developers must remember to explicitly validate ownership in every instruction that interacts with these accounts.

• Security vulnerabilities, since missing or inconsistent validation checks in future updates could allow unintended associations between accounts.

Given that this issue appears in multiple instructions, the cumulative effect is a higher chance of errors across the program. Using explicit PDA derivation in the context accounts would eliminate the need for repeated manual checks and strengthen security.

For future implementations, consider using seeds between related accounts to strengthen their relationships and improve security. By deriving PDAs using the relevant account keys, ownership and associations are enforced at the protocol level, reducing the need for manual validation checks in instruction handlers.

SOLVED: The CrunchDAO team resolved this finding by applying the Anchor seeds constraint where applicable to enforce correct account relationships.