Canopy - SCA #2 - Canopy

The router exhibits a major architectural inconsistency in how it handles deposits for the Meridian strategy. The Meridian strategy requires a CoinType parameter for deposits as it operates with wrapped staking coins, but the router incorrectly calls meridian_rewards::vault_deposit_fa<CoinType> from the deposit_coin (coin path) instead of deposit_fa (FA path).

This creates a fundamental mismatch between the router's expected flow and the Meridian strategy's actual requirements:

1. Router expects: Users to deposit wrapper coins through deposit_coin, which are then unwrapped to extract fungible assets

2. Meridian reality: Users cannot hold wrapped coins directly, and the strategy expects fungible assets to be deposited and then wrapped internally

Since deposits are broken, the withdrawal functionality is irrelevant as users cannot deposit into the strategy in the first place. However, it's worth noting that the withdrawal side does have proper support through withdraw_fa_with_coin_type for strategies like Meridian that require coin type parameters, while the deposit side lacks an equivalent deposit_fa_with_coin_type function.

Code Location

Below is the implementation of the deposit_coin function:

public(friend) fun deposit_coin<CoinType>(
    vault: Object<Vault>,
    strategy: Object<BaseStrategy>,
    packet: Option<vector<u8>>,
    amount: u64
) {
    let borrowed_router_ref = auth::borrow_router_ref();
    let vault_signer = vault::get_signer_for_router(vault, auth::borrowed_ref(&borrowed_router_ref));
    auth::return_router_ref(borrowed_router_ref);

    let concrete_address = base_strategy::concrete_address(strategy);
    if (concrete_address == @echelon_simple) {
        echelon_simple::vault_deposit_coin<CoinType>(&vault_signer, strategy, amount);
    } else if (concrete_address == @moveposition_simple) {
        let packet = option::destroy_some(packet);
        moveposition_simple::vault_deposit_coin<CoinType>(&vault_signer, strategy, packet, amount);
    } else if (concrete_address == @meridian_rewards) {
        meridian_rewards::vault_deposit_fa<CoinType>(&vault_signer, strategy, amount);
    }
}

Below is the implementation of the deposit_fa function:

public(friend) fun deposit_fa(
    vault: Object<Vault>,
    strategy: Object<BaseStrategy>,
    packet: Option<vector<u8>>,
    amount: u64
) {
    let borrowed_router_ref = auth::borrow_router_ref();
    let vault_signer = vault::get_signer_for_router(vault, auth::borrowed_ref(&borrowed_router_ref));
    auth::return_router_ref(borrowed_router_ref);

    let concrete_address = base_strategy::concrete_address(strategy);
    if (concrete_address == @echelon_simple) {
        echelon_simple::vault_deposit_fa(&vault_signer, strategy, amount);
    } else if (concrete_address == @moveposition_simple) {
        let packet = option::destroy_some(packet);
        moveposition_simple::vault_deposit_fa(&vault_signer, strategy, packet, amount);
    } else if (concrete_address == @layerbank_simple) {
        layerbank_simple::vault_deposit_fa(&vault_signer, strategy, amount);
    };
}

It is recommended to create a deposit_fa_with_coin_type function that mirrors the withdrawal pattern, allowing Meridian strategy deposits to be handled through the appropriate FA path while supporting the required CoinType parameter. This will ensure consistent architectural patterns between deposit and withdrawal operations for strategies that require coin type parameters.

SOLVED: The Canopy team solved this issue in the specified pull request.

Several strategy implementations fail to check whether reward assets are different from the strategy's base asset before notifying rewards. This can lead to mistakenly notifying rewards in the base asset, which should be skipped.

This affects the following functions:

1. Meridian Strategy:

   • notify_rewards

2. LayerBank Strategy:

   • notify_rewards

   • notify_all_rewards

3. Echelon Strategy:

   • notify_rewards_coin

Without proper validation, these functions may incorrectly notify rewards in the base asset, potentially leading to unexpected or incorrect reward distributions.

It is recommended to add base asset validation in the aforementioned notify functions to ensure they only process rewards that are different from the strategy's base asset.

SOLVED: The Canopy team solved this issue in the specified pull request.

The claim_behalf function in the LayerBank airdrop block contains a logic error in its reward calculation. When checking balances before and after claiming, it queries the signer's account balance instead of the on_behalf_of address that actually receives the rewards. As a result, when these addresses differ, the function will incorrectly report zero rewards claimed even though rewards were successfully transferred to the beneficiary account. This creates a discrepancy between the actual rewards claimed and the amount reported by the function.

Currently, this risk is limited since claim_behalf is only invoked internally by the claim function and no strategies call it directly. However, the risk could increase if future strategies invoke claim_behalf directly or if the protocol expands to support claiming rewards on behalf of other addresses.

Code Location

Below is the implementation of the claim_behalf function:

public fun claim_behalf(account: &signer, on_behalf_of: address): u64 {
    let pre_balance = primary_store_balance(signer::address_of(account), reward_metadata());
    simple_airdrop::claim_behalf(account, on_behalf_of);
    let post_balance = primary_store_balance(signer::address_of(account), reward_metadata());
    post_balance - pre_balance
}

Below is the implementation of the claim function:

public fun claim(account: &signer): u64 {
    claim_behalf(account, signer::address_of(account))
}

It is recommended to modify the claim_behalf function to use the on_behalf_of address when querying balances, rather than the signer's address.

SOLVED: The Canopy team solved this issue in the specified pull request.

The three strategy implementations (Echelon, Layerbank, and Meridian) have inconsistent error code values for shared error types. For example:

• EINVALID_FEE_RECIPIENT: Echelon (11) vs Layerbank (9)

• ENO_REWARDS_POOL_ADDRESS: Echelon (12) vs Layerbank (10) vs Meridian (11)

• EUPKEEP_NOT_READY: Echelon (13) vs Layerbank (11) vs Meridian (13)

• EINVALID_REWARDS_POOL_ADDRESS: Echelon (14) vs Layerbank (12) vs Meridian (10)

While this does not affect core functionality since error handling happens within the protocol, it makes the code harder to maintain and debug. External systems that integrate with multiple strategies need to handle different error codes for the same error conditions, which increases complexity and the chance of bugs.

It is recommended to create a shared error constants module that all strategies can import to standardize error codes across the protocol and ensure consistent error handling.

RISK ACCEPTED: The Canopy team accepted the risk of this finding. They stated the following:

Acknowledged. We decided to maintain independent error code lists. There is no impact on functionality.

The LayerBank and Echelon strategy implementations lack the ability to update the upkeep interval after strategy creation, unlike the Meridian strategy which includes a set_upkeep_interval function. Both LayerBank and Echelon strategies use a hardcoded DEFAULT_UPKEEP_INTERVAL of 24 hours (60*60*24 seconds) that cannot be modified once the strategy is deployed.

This creates a permanent operational limitation where strategy managers cannot adjust the frequency of reward distribution after strategy creation. The fixed 24-hour interval cannot be modified without deploying new strategy contracts, which affects the protocol's ability to adapt to changing market conditions and user preferences. This limitation is irreversible and creates inconsistency across strategy implementations.

It is recommended to add a set_upkeep_interval function to both LayerBank and Echelon strategies, following the same pattern as the Meridian strategy implementation. Additionally, consider adding validation to ensure the new interval falls within reasonable bounds (e.g., minimum 1 hour, maximum 7 days) to prevent abuse and ensure operational efficiency.

SOLVED: The Canopy team solved this issue in the specified pull request.

The Meridian strategy's create and set_rewards_pool_address functions do not validate whether the provided rewards_pool_address (when not None) actually contains a StakingPool resource. Additionally, in both the LayerBank and Echelon strategies, the set_rewards_pool_address function exhibits inconsistent validation logic—if the strategy integrates a rewards system, it assigns the new address without verifying that it references a StakingPool resource, whereas strategies without rewards perform this validation.

This inconsistency allows strategies to be configured with invalid rewards pool addresses, which can lead to runtime failures when the strategy attempts to utilize the rewards functionality.

This issue occurs because the get_staking_pool function calls object::address_to_object<StakingPool>(option::destroy_some(rewards_pool_address)), which reverts with ERESOURCE_DOES_NOT_EXIST (error code 7) if a StakingPool resource does not exist at the specified address.

This vulnerability impacts multiple critical functions, including notify_rewards, notify_all_rewards, and perform_upkeep, thereby preventing proper reward distribution and automated upkeep mechanisms from functioning correctly.

Code Location

Below is the implementation of the get_staking_pool function:

fun get_staking_pool(strategy: Object<BaseStrategy>): Object<StakingPool> acquires RewardsPoolDetails {
    let rewards_pool_address = rewards_pool_address(strategy);
    assert!(option::is_some(&rewards_pool_address), ENO_REWARDS_POOL_ADDRESS);
    object::address_to_object<StakingPool>(option::destroy_some(rewards_pool_address))
}

Below is the implementation of the notify_reward_amount_internal function:

fun notify_reward_amount_internal(
    strategy_signer: &signer,
    strategy: Object<BaseStrategy>,
    reward_metadata: Object<Metadata>,
    reward_amount: u64
) acquires RewardsPoolDetails {
    // If a rewards pool address is set, notify the rewards module
    let rewards_pool_address = rewards_pool_address(strategy);
    if (option::is_some(&rewards_pool_address)) {
        let staking_pool = get_staking_pool(strategy);
        multi_rewards::notify_reward_amount(strategy_signer, staking_pool, reward_metadata, reward_amount);
    }
}

Below is a code snippet of the notify_all_rewards_internal function:

fun notify_all_rewards_internal<WrapperStakeCoin>(
    strategy: Object<BaseStrategy>
) acquires MeridianRewardsStrategy, RewardsPoolDetails {
    let strategy_ref = borrow_strategy<WrapperStakeCoin>(strategy);
    let strategy_signer = &base_strategy::get_signer(&strategy_ref.auth_ref);

    let staking_pool = get_staking_pool(strategy);
    // rest of the code
}

It is recommended to add validation in both the create and set_rewards_pool_address functions for the Meridian strategy, and standardize the validation in set_rewards_pool_address for LayerBank and Echelon strategies, to ensure that the provided rewards_pool_address holds a valid StakingPool resource before storing it. This will prevent strategies from being created or configured with invalid rewards pool addresses that would cause runtime failures.

SOLVED: The Canopy team solved this issue in the specified pull request.

The set_upkeep_interval function in the Meridian strategy lacks validation to ensure that the new upkeep interval falls within appropriate lower and upper thresholds. Currently, the function only validates that the caller is the strategy manager but does not check if the provided upkeep_interval value is reasonable.

This allows strategy managers to set extremely short intervals (e.g., 1 second) which could lead to excessive gas consumption and potential DoS attacks through rapid upkeep calls, or extremely long intervals (e.g., 1 year) which could prevent timely reward distribution and maintenance operations.

The function should enforce minimum and maximum bounds for the upkeep interval to ensure:

• Minimum interval prevents excessive gas consumption and spam

• Maximum interval ensures regular maintenance and reward distribution

• Reasonable defaults align with protocol expectations

Code Location

Below is the implementation of the set_upkeep_interval function:

public entry fun set_upkeep_interval(
    account: &signer, strategy: Object<BaseStrategy>, upkeep_interval: u64
) acquires RewardsPoolDetails {
    assert!(
        signer::address_of(account) == base_strategy::manager(strategy),
        ENOT_AUTHORIZED
    );

    let rewards_pool_details_ref = borrow_mut_rewards_pool_details(strategy);
    rewards_pool_details_ref.upkeep_interval = upkeep_interval;
}

It is recommended to add validation in the set_upkeep_interval function to ensure the new interval falls within acceptable bounds.

SOLVED: The Canopy team solved this issue in the specified pull request by enforcing the upkeep interval to be between 1 hour and 7 days.

The set_rewards_pool_address function in both LayerBank and Echelon strategies contains a redundant state update when initializing rewards pool details for strategies created before the multi-rewards package was integrated.

The issue occurs in the following logic flow:

1. If RewardsPoolDetails does not exist, the function invokes initialize_reward_pool_details() to create the resource with the specified rewards_pool_address.

2. Immediately afterward, the function unconditionally updates the rewards_pool_address field again, despite it having just been set during initialization.

This redundant update results in unnecessary gas consumption by performing the same state write operation twice. The second update is entirely superfluous, as it writes the same value that was just assigned during initialization.

Code Location

Below is the implementation of the set_rewards_pool_address function:

public entry fun set_rewards_pool_address(
    account: &signer, strategy: Object<BaseStrategy>, rewards_pool_address: Option<address>
) acquires RewardsPoolDetails, EchelonStrategy {
    assert!(
        signer::address_of(account) == base_strategy::manager(strategy),
        ENOT_AUTHORIZED
    );

    // Strategies created before the multirewards package was integrated will not have a rewards pool address set.
    // In this case, we initialize the rewards pool details.
    let strategy_ref = borrow_strategy(strategy);
    if (!exists<RewardsPoolDetails>(object::object_address(&strategy))) {
        initialize_reward_pool_details(strategy_ref, rewards_pool_address);
    };

    let rewards_pool_details_ref = borrow_mut_rewards_pool_details(strategy);
    rewards_pool_details_ref.rewards_pool_address = rewards_pool_address;
}

Below is the implementation of the initialize_reward_pool_details function:

fun initialize_reward_pool_details(
    strategy_ref: &EchelonStrategy, rewards_pool_address: Option<address>
) {
    let strategy_signer = base_strategy::get_signer(&strategy_ref.auth_ref);
    if (option::is_some(&rewards_pool_address)) {
        assert!(
            object::object_exists<StakingPool>(*option::borrow(&rewards_pool_address)),
            EINVALID_REWARDS_POOL_ADDRESS
        );
    };

    move_to(
        &strategy_signer,
        RewardsPoolDetails {
            rewards_pool_address,
            last_upkeep_timestamp: 0,
            upkeep_interval: DEFAULT_UPKEEP_INTERVAL
        }
    );
}

It is recommended to place the rewards pool address assignment logic inside an else clause to ensure the address is only updated when a change is actually required, avoiding the redundant state update during initial resource creation.

SOLVED: The Canopy team solved this issue in the specified pull request.

The Echelon strategy’s vault_deposit_coin and vault_deposit_fa functions lack validation to ensure the deposit amount is greater than zero. Although the documentation comments specify that these functions should revert with EINVALID_AMOUNT if the amount is zero, the actual implementation does not perform this check.

This discrepancy between the documented behavior and the implementation allows for the possibility of processing zero amounts through the vault debt creation mechanism, resulting in potentially unnecessary or wasted operations.

It is recommended to add the zero amount validation check at the start of both vault_deposit_coin and vault_deposit_fa functions in the Echelon strategy.

SOLVED: The Canopy team solved this issue in the specified pull request. The team stated the following:

The zero-amount validation is done on the vault level. Since the vault_deposit_fa and vault_deposit_coin functions can only be called by the vault, the check is not needed in these functions. We have updated the comment, which caused this confusion.

Multiple strategy implementations contain incorrect documentation in their comments:

1. Protocol Name Inconsistencies:

   • LayerBank Strategy:

     // this is the latest(i.e. now) base asset value of the strategy in the underlying echelon market

     The comment incorrectly references "echelon market" instead of "layerbank market".

   
   

   • Meridian Strategy:

     // we get the idle assets in the strategy and deploy it to LayerBank

     The comment incorrectly references "LayerBank" instead of "Meridian".

   
   

2. Incorrect Error Description:

   • Echelon Strategy:

     /// @reverts EINSUFFICIENT_BALANCE if the account does not have enough balance to deposit.

     The comment incorrectly describes the error condition. In vault deposit functions, the signer's account is not depositing funds - the vault provides assets from its idle asset pool.

These documentation inconsistencies could confuse developers reading the code about which protocols the strategies interact with and how error conditions work.

Correct the comments to accurately reflect the underlying protocols and error conditions:

• LayerBank strategy: Change "echelon market" to "layerbank market".

• Meridian strategy: Change "LayerBank" to "Meridian".

• Echelon strategy: Update the error description to reference EVAULT_INSUFFICIENT_ASSETS when the vault lacks sufficient idle assets.

SOLVED: The Canopy team solved this issue in the specified pull request.

Several modules within the codebase contain unused code elements that should be removed to enhance code clarity and facilitate ongoing maintenance:

• Unused Import - LayerBank Strategy:

  use std::vector;

  
  

• Unused Import - LayerBank Airdrop Block:

  use aptos_framework::aptos_coin::{Self, AptosCoin};

  
  

• Unused Variable - LayerBank Strategy:

  let rewards_pool_address = *option::borrow(&pool_details_ref.rewards_pool_address);

  
  

• Unused Variable - LayerBank Strategy:

  let strategy_signer = &base_strategy::get_signer(&strategy_ref.auth_ref);

The detected issues involve imports that are declared but not utilized within their respective modules, as well as variables that are assigned values but not referenced thereafter.

It is recommended to remove unused code segments to improve code maintainability and clarity:

• Remove use std::vector; from the LayerBank strategy implementation.

• Remove Self from the aptos_coin import statement in the LayerBank airdrop block module.

• Remove the unused rewards_pool_address variable assignment within the harvest function.

• Remove the unused strategy_signer variable assignment within the notify_all_rewards_internal function.

SOLVED: The Canopy team solved this issue in the specified pull request.

Multiple unnecessary variable assignments were identified in the strategy implementations, where function parameters are redundantly assigned to local variables with the same name. These assignments do not perform any transformation, modification, or provide any benefit.

Code Location

The following redundant assignments were found across multiple strategies:

• LayerBank Strategy

  let strategy = strategy;

  
  

• Echelon Strategy

  let strategy = strategy;

  let strategy = strategy;

  
  

• Meridian Strategy

  let strategy = strategy;

It is recommended to remove the aforementioned unnecessary variable assignments. The parameters can be used directly without the redundant assignment, which will improve code clarity and reduce unnecessary lines of code.

SOLVED: The Canopy team solved this issue in the specified pull request.

The Echelon strategy's set_rewards_pool_address function borrows the strategy global resource before checking if RewardsPoolDetails exists, but only uses the borrow inside the conditional block when the resource does not exist. This creates unnecessary gas costs from borrowing the reference outside where it's needed.

Code Location

Below is a code snippet of the set_rewards_pool_address function:

let strategy_ref = borrow_strategy(strategy);
if (!exists<RewardsPoolDetails>(object::object_address(&strategy))) {
    initialize_reward_pool_details(strategy_ref, rewards_pool_address);
};

It is recommended to relocate the borrow_strategy operation within the conditional block to enhance gas efficiency and prevent unnecessary resource borrowing.

SOLVED: The Canopy team solved this issue in the specified pull request.