Audit of Demox Labs - Aleo Standard Programs

Description. In Pondo oracle, the validator can transfer Aleo credits to the protocol to boost the yield. The Pondo oracle then ranks the validator by the sum of native staking yield (tracked by the reference delegator) and boost yield of the last epoch. However, in the code the boost yeild was added twice and the unnormalized boost_amount is used instead of normalized_boost_amount. This will lead to a incorrect validator rank.

  async function finalize_update_data(
    public delegator: address
  ) {
    ...
    let normalized_boost_amount: u128 = boost_amount * PRECISION / current_pondo_tvl as u128;

    // Ensure the last update was in the previous epoch, otherwise set the yield to zero
    // The attack here is to prevent a validator from keeping many reference delegators and then choosing the most favorable range.
    let previous_update_epoch: u32 = existing_validator_datum.block_height / BLOCKS_PER_EPOCH;
    let did_update_last_epoch: bool = (previous_update_epoch + 1u32) == current_epoch;
    let new_microcredits_yield_per_epoch: u128 = did_update_last_epoch ? yield_per_epoch + normalized_boost_amount : 0u128;

    // Construct and save the new validator_datum for the delegator
    let new_validator_datum: validator_datum = validator_datum {
      delegator: delegator,
      validator: existing_validator_datum.validator,
      block_height: block.height,
      bonded_microcredits: bonded.microcredits,
      microcredits_yield_per_epoch: new_microcredits_yield_per_epoch,
      commission: validator_committee_state.commission,
      boost: boost_amount
    };
    validator_data.set(delegator, new_validator_datum);
    ...
    // Perform swaps and drop the last element
    // The order of the swap_validator_data is subtle but very important.
    let swap_result_0: (validator_datum, validator_datum, bool) = swap_validator_data(new_validator_datum, datum_0, epoch_start_height, false, allocations[0u8]);
    ...
    // Set the new top 10
    top_validators.set(0u8, new_top_10);
  }

In the code above, the microcredits_yield_per_epoch field of new_validator_datum is set as the sum of native staking yield and boost yield. The boost field is set as the unnormalized boost_amount. Then new_validator_datum is then passed into swap_validator_data function for ranking. However, in the swap_validator_data function below, the boost yield is added again. This introduces the double counting issues. Moreover, unnormalized boost amount is used in the adding. The unit of the microcredits_yield_per_epoch and the unnormalized boost_amount mismatch.

  // Swap the positions of each datum given:
  // 1. If auto swap bit is on, always swap
  // 2. If one is outdated (if both are outdated, preference no swap)
  // 3. If one yield is 0 (if both are 0, preference no swap)
  // 4. The higher yield or the lower yield if they reference the same validator
  inline swap_validator_data(
    datum_0: validator_datum,
    datum_1: validator_datum,
    epoch_start_block: u32,
    auto_swap: bool,
    boost_multiple: u128,
  ) -> (validator_datum, validator_datum, bool) {
    ...

    // Calculate the yields
    // Note: the boost multiple depends on the % of the pondo tvl that the spot would get
    // If the boost multiple is too high, it would be more profitable to boost than to decrease commission ie protocol loses money
    // If the boost multiple is too low, it would be more profitable to decrease commission than to boost ie no one would ever boost unless commissions were 0s
    let first_yield: u128 = datum_0.microcredits_yield_per_epoch + datum_0.boost * BOOST_PRECISION / boost_multiple;
    let second_yield: u128 = datum_1.microcredits_yield_per_epoch + datum_1.boost * BOOST_PRECISION / boost_multiple;

    ...
  }

Recommendation. It is recommended to set the microcredits_yield_per_epoch field of new_validator_datum as the native staking field and set the boost field as normalized_boost_amount. In addition, consider adding tests with synthetic data to make sure the ranking works as expected.

Client Response. This issue was fixed by changing boost_amount to normalized_boost_amount and removing the addition of boost to new_microcredits_yield_per_epoch.

  async function finalize_update_data(
    public delegator: address
  ) {
    ...
    let normalized_boost_amount: u128 = boost_amount * PRECISION / current_pondo_tvl as u128;
    // Ensure the last update was in the previous epoch, otherwise set the yield to zero
    // The attack here is to prevent a validator from keeping many reference delegators and then choosing the most favorable range.
    let previous_update_epoch: u32 = existing_validator_datum.block_height / BLOCKS_PER_EPOCH;
    let did_update_last_epoch: bool = (previous_update_epoch + 1u32) == current_epoch;
    let new_microcredits_yield_per_epoch: u128 = did_update_last_epoch ? yield_per_epoch : 0u128;

    // Construct and save the new validator_datum for the delegator
    let new_validator_datum: validator_datum = validator_datum {
      delegator: delegator,
      validator: existing_validator_datum.validator,
      block_height: block.height,
      bonded_microcredits: bonded.microcredits,
      microcredits_yield_per_epoch: new_microcredits_yield_per_epoch,
      commission: validator_committee_state.commission,
      boost: normalized_boost_amount
    };
    ...
  }

Description. The initialize function of pondo_oracle.aleo initializes the control address, delegator allocation, and the top validator list. Unfortunately, the function can be repeatedly called without limitation.

  async function finalize_initialize() {
    // Set the control addresses
    control_addresses.set(INITIAL_DELEGATOR_APPROVER_ADDRESS, true);
    control_addresses.set(pondo_delegator1.aleo, false);
    control_addresses.set(pondo_delegator2.aleo, false);
    control_addresses.set(pondo_delegator3.aleo, false);
    control_addresses.set(pondo_delegator4.aleo, false);
    control_addresses.set(pondo_delegator5.aleo, false);

    delegator_allocation.set(0u8, [
      ...
    ]);
    top_validators.set(0u8,
      [
        ...
      ]
    );
  }

If the protocol is already running, the new call will reset the top_validators to dead addresses. Then the protocol won’t earn any staking rewards. The new call will also reset the control addresses to the default. If the Pondo oracle has changed control addresses, it may lead to a loss of control.

Recommendation. It is recommended to prohibit calling the initialize function if it has already been called.

Client Response. This issue was fixed by adding a guard in the initialize function to avoid duplicate calls:

  async function finalize_initialize() {
    // Assert the protocol hasn't been initialized yet
    assert_eq(delegator_allocation.contains(0u8), false);
    assert_eq(top_validators.contains(0u8), false);
    ...
  }

Description. In pondo_token.aleo, the PONDO token holder can burn PONDO tokens to get the same share of the pALEO tokens in the pool (earned from commission). All of the PONDO tokens are minted at the beginning and won’t increase. The pALEO tokens are accumulated from the Pondo core protocol and won’t decrease (except for withdrawal by burn). Therefore, the same unit of PONDO tokens is expected to be worth more and more pALEO tokens.

In the burn_public function, it calculates the ratio of PONDO tokens to pALEO tokens as PONDO token supply divided by pALEO balance. Then it compares the withdrawal ratio with the post-burn ratio to restrict the amount of withdrawn pALEO. Unfortunately, the ratio assertion is flipped. This will lead to anyone with PONDO tokens being able to withdraw almost all of the pALEO tokens.

  async function finalize_burn_public(f0: Future, f1: Future, amount: u128, paleo_amount: u128) {
    ...
    // Calculate the pondo to paleo ratio
    let pondo_paleo_ratio: u128 = pondo_supply_after.supply * PRECISION / paleo_balance_after.balance;
    let withdrawal_ratio: u128 = amount * PRECISION / paleo_amount;

    // Ensure that the pondo to paleo ratio is greater than the withdrawal ratio
    let valid_withdrawal: bool = pondo_paleo_ratio >= withdrawal_ratio;
    assert(valid_withdrawal);
  }

Actually, after the burn, the PONDO tokens should be worth more pALEO tokens (if not equal). This means the pondo_paleo_ratio should be less than or equal to the withdrawal_ratio.

Recommendation. It is recommended to change the assert to pondo_paleo_ratio <= withdrawal_ratio and add more tests for it.

Client Response. Demox Labs changed the assertion to pondo_paleo_ratio <= withdrawal_ratio:

  async function finalize_burn_public(f0: Future, f1: Future, amount: u128, paleo_amount: u128) {
    ...
    // Calculate the pondo to paleo ratio
    let pondo_paleo_ratio: u128 = pondo_supply_after.supply * PRECISION / paleo_balance_after.balance;
    let withdrawal_ratio: u128 = amount * PRECISION / paleo_amount;

-   // Ensure that the pondo to paleo ratio is greater than the withdrawal ratio
-   let valid_withdrawal: bool = pondo_paleo_ratio >= withdrawal_ratio;
+   // Ensure that the pondo to paleo ratio is less than the withdrawal ratio
+   // A lower pondo / paleo implies each pondo represents more paleo
+   let valid_withdrawal: bool = pondo_paleo_ratio <= withdrawal_ratio;
    assert(valid_withdrawal);
  }

Description. In the Multi Token Support Program, the authorized_until field the marks the expiration block number of the authorization. In the transfer_private transition, it needs to check if the authorization is expired in the finalize_transfer_private function.

  async transition transfer_private(
    recipient: address,
    amount: u128,
    input_record: Token
  ) -> (Token, Token, Future) {
    ...
    return (updated_record, transfer_record, finalize_transfer_private(external_authorization_required, input_record.authorized_until));
  }

  async function finalize_transfer_private(
    external_authorization_required: bool,
    input_token_authorized_until: u32
  ) {
    assert(block.height <= input_token_authorized_until || !external_authorization_required);
  }

As it’s a function executed on-chain, the input_token_authorized_until field will be public. This may leak information about the record. If some records have distinct authorized_until values, others may be able to know exactly which record is being transferred. Note this applies to both restricted and unrestricted tokens.

Recommendation. It is recommended to add documentation to notify users and admins that the transfer_private function may leak information with authorized_until.

Description. The Pondo core protocol is entitled to commission fee in pALEO token and reserves it in the owed_commission map. The commission token can be minted to the pondo_token.aleo in the rebalance_retrieve_credits function.

  async transition rebalance_retrieve_credits(
    public transfer_amounts: [u64; 5],
    public commission_mint: u64
  ) -> Future {
    ...
    let f5: Future = pondo_staked_aleo_token.aleo/mint_public(commission_mint, pondo_token.aleo);

    return finalize_rebalance_retrieve_credits(f0, f1, f2, f3, f4, f5, transfer_amounts, commission_mint);
  }

  async function finalize_rebalance_retrieve_credits(
    ...
    commission_mint: u64
  ) {
    ...
    let new_commission_paleo: u64 = calculate_new_paleo(current_balance as u128, deposit_pool as u128, new_commission as u128, total_paleo_minted);
    // New owed commission is whatever commission is left after the new commission mint, plus what we may have earned between calling the function and now
    owed_commission.set(0u8, current_owed_commission + new_commission_paleo - commission_mint);
    ...
  }

In the rebalance_retrieve_credits function, there is no lower bound on the amount of mint (commission_mint). Then an arbitrary amount of pALEO can be minted provided it is less than the reserved amount.

If the rebalance_retrieve_credits function is called with commission_mint=0 for many consecutive epochs, there will be a large amount of pALEO still reserved in the owed_commission map. Considering that the rebalance_retrieve_credits function is permissionless and can only be called once in every epoch, someone with motivation might be able to frontrun transactions to achieve that. In such circumstances, if others burn the PONDO tokens to get pALEO in pondo_token.aleo, they may receive fewer tokens than their real value. This may lead to an unfair result.

Recommendation. It is recommended to enforce the transfer of most of the owed_commission in the finalize_prep_rebalance function.

Client Response. This issue was fixed by enforcing that the commission_mint be at least 98% of the total owed commission:

  async function finalize_rebalance_retrieve_credits(...) {
    ...
    assert(commission_mint >= (current_owed_commission * 98u64) / 100u64); // Assert that the minted commission is at least 98% of the owed commission
    // New owed commission is whatever commission is left after the new commission mint, plus what we may have earned between calling the function and now
    owed_commission.set(0u8, current_owed_commission + new_commission_paleo - commission_mint);
    // Update total balance
    balances.set(DELEGATED_BALANCE, current_balance + new_commission);

    // Update protocol state
    let current_state: u8 = protocol_state.get(PROTOCOL_STATE_KEY);
    assert(current_state == PREP_REBALANCE_STATE); // Assert that the protocol is in a normal state
    protocol_state.set(PROTOCOL_STATE_KEY, REBALANCING_STATE);
  }

Description. The Pondo protocol is entitled to 10% of the staking rewards as a commission fee. The protocol tracks the staking rewards by subtracting the last recorded balance from the current Aleo credit balance. The pondo_core_protocol.aleo stores the last recorded balance into balances[DELEGATED_BALANCE]. Whenever the commission fee is charged, the program needs to update the last recorded balance.

Unfortunately, in the instant_withdraw_public function, balances[DELEGATED_BALANCE] is not updated after the commission fee is charged. The next time the commission fee is charged, it will use the old Aleo credit balance. This will lead to double charging commission fee.

  async function finalize_instant_withdraw_public(
    f0: Future,
    f1: Future,
    paleo_burn_amount: u64,
    withdrawal_credits: u64,
    caller: address
  ) {
    ...
    // Update owed commission balance
    let new_commission_paleo: u64 = calculate_new_paleo(currently_delegated as u128, deposit_pool as u128, new_commission as u128, total_paleo_minted);
    current_owed_commission += new_commission_paleo;
    total_paleo_minted += new_commission_paleo as u128;
    currently_delegated += new_commission;

    // Calculate full pool size
    let full_pool: u128 = currently_delegated as u128 + deposit_pool as u128;

    // Calculate credits value of burned pALEO
    let withdrawal_fee: u64 = calculate_withdraw_fee(paleo_burn_amount);
    let net_burn_amount: u64 = paleo_burn_amount - withdrawal_fee;
    let withdrawal_calculation: u128 = (net_burn_amount as u128 * full_pool as u128) / total_paleo_minted as u128;
    assert(withdrawal_credits <= withdrawal_calculation as u64); // Assert that the withdrawal amount was at most the calculated amount

    // Update owed commission to reflect withdrawal fee
    owed_commission.set(0u8, current_owed_commission + withdrawal_fee);
  }

Recommendation. It is recommended to update balances[DELEGATED_BALANCE] after charging the commission fee in the instant_withdraw_public function.

Client Response. This issue was fixed by updating the balances[DELEGATED_BALANCE] in the instant_withdraw_public function:

  async function finalize_instant_withdraw_public(...) {
    ...
    // Update total balance
    balances.set(DELEGATED_BALANCE, currently_delegated);
    ...
  }

Description. Users of the Pondo protocol need to wait several days to withdraw (for non-instant withdrawals). The Pondo core protocol stores the total withdrawal amount of the current epoch in the withdrawal_batches[epoch] map and reserves the corresponding Aleo credits in the next epoch. However, if the next epoch is skipped (not able to cycle through these states in one epoch), the reservations for the withdrawal of the current epoch will be missed.

  async function finalize_rebalance_retrieve_credits(...) {
    ...
    // Move bonded withdrawals to available to claim
    let current_epoch: u32 = block.height / BLOCKS_PER_EPOCH;
    //  Process withdrawals from the previous epoch
    let current_withdrawal_batch: u64 = withdrawal_batches.get_or_use(current_epoch - 1u32, 0u64);
    balances.set(CLAIMABLE_WITHDRAWALS, reserved_for_withdrawal + current_withdrawal_batch);
    ...
  }

In the rebalance_retrieve_credits function above, it updates balances[CLAIMABLE_WITHDRAWALS] by only adding withdrawal_batches of the last epoch. If the function is not triggered in some epochs, some withdrawal_batches may be missed forever. This will lead to a lower Aleo credit balance reserved in pondo_core_protocol.aleo. Then some users may not be able to withdraw their stake after the lock period.

Note that ensuring the rebalance_retrieve_credits function is called in every epoch is not an easy task: the Aleo network might be in congestion; the execution bot may be accidentally down; prep_rebalance function can only be called on the first day of the Epoch so there is only a one-day interval to execute; the block interval is not a constant 5s, so it is easier to miss. Therefore, the protocol should handle the case that some epochs are missed.

Recommendation. It is recommended to correctly reserve Aleo credits for all the withdrawal_batches in previous epochs.

Client Response. Demox Labs now moves all of the BONDED_WITHDRAWALS to CLAIMABLE during a rebalance:

  async function finalize_rebalance_retrieve_credits(...) {
    ...
    // Move bonded withdrawals to available to claim
    balances.set(CLAIMABLE_WITHDRAWALS, reserved_for_withdrawal + bonded_withdrawals);
    // Reset bonded withdrawals
    balances.set(BONDED_WITHDRAWALS, 0u64);
    // Calculate deposit pool
    let deposit_pool: u64 = core_protocol_account - transfers_total - reserved_for_withdrawal;
    ...
  }

Description. The Pondo oracle ranks validators according to the staking yield and boost in the last epoch. The staking yield is largely decided by the commission rate of the validator. A malicious validator not in the top 5 can set its commission rate to 0% and supply a small amount of boost to rank at the top. Then, in the next epoch, it will be allocated some portion of the deposits. However, it can change the commission to 50% without being caught. In this way, it can gain 50% of the commission, which is probably more profitable than other honest validators.

The malicious validator acts in this way:

1. The validator is not in the top 5. Keep the commission at 0% for nearly one epoch to make a high yield.
2. Add a small amount of boost to itself before the update period (optional).
3. Just before the update period, quit and rejoin the committee to update the commission rate to 50%. The validator needs to wait about a 1-hour unbonding period. However, it won’t be caught because the validator is not in the top 5.
4. In the update period, trigger the update_data function to get a top rank with high microcredits_yield_per_epoch and 50% commission.
5. Gain the 50% commission in the next epoch and do not boost. Thus, it will probably get a very low rank in the next epoch. But this is OK because it has already earned enough and can repeat this cycle by transferring the funds to new validators.

In this way, the validator will gain a 50% commission for one epoch in every two epochs. The expected earnings are a 25% commission per epoch minus the one-hour stake earnings (the loss of quitting and rejoining the committee).

Recommendation. The root cause of this issue is that a validator not in the top 5 can change their commission without being caught. It is recommended to allow banning the validator before the update period in the ban_validator function. Consider prohibiting the execution of the update_data function if the commission of the validator increases too much in consecutive epochs.

Client Response. Demox Labs fixed this issue by allowing the banning of the validator before the update period:

  // Anyone can ban a validator if the validator in the update window if:
  // 1. The validator has a commission greater than MAX_COMMISSION
  // 2. The validator leaves the committee
  async transition ban_validator(
    public reference_delegator: address,
  ) -> Future {
    return finalize_ban_validator(reference_delegator);
  }
  async function finalize_ban_validator(
    public reference_delegator: address
  ) {
    // Get the validator address
    let validator: address = delegator_to_validator.get(reference_delegator);

    // Check if the height is within the update window
    let epoch_blocks: u32 = block.height % BLOCKS_PER_EPOCH;
-   let is_update_period: bool = epoch_blocks >= UPDATE_BLOCKS_DISALLOWED;
-   assert(is_update_period);
+   let is_rebalance_period: bool = epoch_blocks >= REBALANCE_PERIOD;
+   assert(is_rebalance_period);
    ...
  }

It is possible that some validators get banned accidentally. For example, a validator may not be aware of being tracked by the Pondo protocol. It quits and rejoins the committee to change the commission. Then it can be banned forever by others calling the ban_validator function. In this case, Demox Labs provides unban_validator for the admin multisig to unban the validator manually.

Description. In the Multi Token Support Program, the prehook_public function is used to authorize the locked balance to be an authorized balance. In the prehook_public function, if the owner already has a balance in the authorized_balances map, the authorized_until field will be overwritten by the input. This may accidentally extend or shorten the authorized duration of the existing balance.

  async function finalize_prehook_public(
    owner: TokenOwner,
    amount: u128,
    authorized_until: u32,
    caller: address
  ) {
    ...
    let new_locked_balance: u128 = actual_locked_balance - amount;
    let new_authorized_balance: u128 = actual_authorized_balance + amount;

    // Update the authorized balance with the incremented amount and new expiration
    let new_authorized_balance_struct: Balance = Balance {
      token_id: owner.token_id,
      account: owner.account,
      balance: new_authorized_balance,
      authorized_until: authorized_until
    };
    authorized_balances.set(balance_key, new_authorized_balance_struct);

    // Update the locked balance
    let new_locked_balance_struct: Balance = Balance {
      token_id: owner.token_id,
      account: owner.account,
      balance: new_locked_balance,
      authorized_until: locked_balance.authorized_until
    };
    balances.set(balance_key, new_locked_balance_struct);
  }

Recommendation. We recommend adding documentation to notify the executor that the authorized_until field could be overwritten.

Description. In the Multi Token Support Program, the prehook_public and prehook_private functions are used to authorize the locked balance when the token is restricted (external_authorization_required=true). For unrestricted token (external_authorization_required=false) the functions are useless.

However, the external_authorization_party is able to execute prehook_public (with amount=0) and prehook_private with unrestricted token. This may introduce unintuitive behavior.

For example, calling prehook_public function (with amount=0) will create an element in the balances[] map. This is unintuitive because the unrestricted token should have never touched the balances[] map. By calling prehook_private, it can split the private record into two parts with different authorized_until values. This is unintuitive because the authorized_until should always be 0 for unrestricted token.

Recommendation. It is recommended to prohibit calling these functions with a token that has external_authorization_required=false.

Client Response. This issue was fixed by adding assert(token.external_authorization_required) in the prehook_public function:

  async function finalize_prehook_public(
    owner: TokenOwner,
    amount: u128,
    authorized_until: u32,
    caller: address,
    balance_key: field
  ) {
    let token: TokenMetadata = registered_tokens.get(owner.token_id);
    // Check that the token requires external authorization
    assert(token.external_authorization_required);
    // Check that the caller has permission to authorize
    ...
  }

Description. The Pondo core protocol splits the Aleo credits among the 5 delegators according to specific allocation. In the rebalance_redistribute function, it is required that the validator_portion should be close to the expected allocation portion.

  async function finalize_rebalance_redistribute(...) {
    ...
    let validator1_portion: u128 = (transfer_amounts[0u8] as u128 * PRECISION_UNSIGNED) / total_credits_128;
    let validator2_portion: u128 = (transfer_amounts[1u8] as u128 * PRECISION_UNSIGNED) / total_credits_128;
    let validator3_portion: u128 = (transfer_amounts[2u8] as u128 * PRECISION_UNSIGNED) / total_credits_128;
    let validator4_portion: u128 = (transfer_amounts[3u8] as u128 * PRECISION_UNSIGNED) / total_credits_128;
    let validator5_portion: u128 = (transfer_amounts[4u8] as u128 * PRECISION_UNSIGNED) / total_credits_128;
    assert(delegator_allocation[0u8] - validator1_portion <= 2u128); // ensure that the validator portion is close to the expected portion
    assert(delegator_allocation[1u8] - validator2_portion <= 2u128); // ensure that the validator portion is close to the expected portion
    assert(delegator_allocation[2u8] - validator3_portion <= 2u128); // ensure that the validator portion is close to the expected portion
    assert(delegator_allocation[3u8] - validator4_portion <= 2u128); // ensure that the validator portion is close to the expected portion
    assert(delegator_allocation[4u8] - validator5_portion <= 2u128); // ensure that the validator portion is close to the expected portion
    ...
  }

The max deviation of the allocated amount is limited to 0.02%. This is a strict limit. If the total Aleo credits is a small number (like 501), it will be impossible to find a valid split with the constraints. Then such function cannot be executed and the protocol will be stuck.

This is not a big issue though. The protocol operator can always add more credits to the protocol to ensure the credits amount is splittable.

Recommendation. It is recommended to monitor the balance of the protocol and ensure it is splittable.

Client Response. This issue was mitigated by requiring a deposit of at least 100 credits to initialize the protocol.

  async transition initialize(transfer_amount: u64) -> Future {
    assert(transfer_amount >= 100_000_000u64); // Assert that the transfer amount is at least 100 credits, to ensure there is no division by zero, and that there is enough for the liquidity pool

    // Transfer ALEO to the protocol
    let f0: Future = credits.aleo/transfer_public_as_signer(self.address, transfer_amount);

    // Initialize pALEO and PNDO tokens
    let f1: Future = pondo_staked_aleo_token.aleo/register_token();
    let f2: Future = multi_token_support_program.aleo/mint_public(PALEO_TOKEN_ID, self.address, transfer_amount as u128, 4294967295u32);
    let f3: Future = pondo_token.aleo/initialize_token();

    // Initialize delegators
    let f4: Future = pondo_delegator1.aleo/initialize();
    let f5: Future = pondo_delegator2.aleo/initialize();
    let f6: Future = pondo_delegator3.aleo/initialize();
    let f7: Future = pondo_delegator4.aleo/initialize();
    let f8: Future = pondo_delegator5.aleo/initialize();

    return finalize_initialize(f0, f1, f2, f3, f4, f5, f6, f7, f8, transfer_amount);
  }

Description. In the Pondo oracle, the admin can be updated with the update_admin function. In the function, it first sets the new admin and then removes the old admin. Unfortunately, if the current admin calls update_admin with new_admin still as its own address, the admin will be accidentally removed, and no admin will be set. The update_admin function follows a dangerous pattern.

  async function finalize_update_admin(
    public new_admin: address,
    public caller: address
  ) {
    // Ensure the caller is an admin
    let is_admin: bool = control_addresses.get(caller);
    assert(is_admin);

    // Set the new admin
    control_addresses.set(new_admin, true);
    // Remove the old admin
    control_addresses.remove(caller);
  }

Recommendation. It is recommended to remove the old admin before adding the new admin. Another general way to avoid this pattern is splitting update_admin into add_admin and remove_admin functions.

Client Response. Demox Labs moved the remove action to before the set action:

  async function finalize_update_admin(...) {
    ...
    control_addresses.remove(old_admin);

    // Set the new admin
    control_addresses.set(new_admin, true);
  }

Description. The Pondo oracle can ban a validator if it misbehaves. The pondo_ban_validator function allows the delegator to ban the validator if the misbehavior is caught.

  // Pondo delegators can ban a validator
  // This is to prevent a validator from keeping reference delegators while forcibly unbonding pondo delegators
  // or closing the validator to delegators when the pondo delegators try to bond
  async transition pondo_ban_validator(
    public validator: address
  ) -> Future {
    // Check that 0Group addresses cannot be banned
    assert_neq(validator, self.address);

    return finalize_pondo_ban_validator(validator, self.caller);
  }

  async function finalize_pondo_ban_validator(
    public validator: address,
    public caller: address
  ) {
    // Check if the caller is a control address
    let is_control_address: bool = control_addresses.contains(caller);
    assert(is_control_address);

    banned_validators.set(validator, true);
  }

In the code above, it requires the caller to be one of the control addresses. However, control_addresses contains not only the Pondo delegator but also the admin. Thus, a single admin can manually ban the validator. This may hurt the neutrality of the protocol.

Recommendation. It is recommended to forbid a single admin address from manually banning the validator.

Client Response. Demox Labs fixed this issue by only allowing non-admin address to ban the validator:

  async function finalize_pondo_ban_validator(
    public validator: address,
    public caller: address
  ) {
    // Check if the caller is an admin control address
    let is_non_admin_control_address: bool = control_addresses.get(caller);
    assert(!is_non_admin_control_address);

    banned_validators.set(validator, true);
  }

Description. The initialize function in the multi_token_support_program_v1.aleo program can be repeatedly called multiple times. The function initializes the wrapped Aleo credits token metadata, which is static. Thus, it will not introduce a vulnerability but is less intuitive.

  async transition initialize() -> Future {
    return finalize_initialize();
  }

  async function finalize_initialize() {
    // Initialize the CREDITS_RESERVED_TOKEN_ID token
    let credits_reserved_token: TokenMetadata = TokenMetadata {
      token_id: CREDITS_RESERVED_TOKEN_ID,
      name: 1095517519u128,
      symbol: 1095517519u128,
      decimals: 6u8,
      supply: 1_500_000_000_000_000u128,
      max_supply: 1_500_000_000_000_000u128,
      admin: self.address,
      external_authorization_required: false,
      external_authorization_party: self.address
    };

    registered_tokens.set(CREDITS_RESERVED_TOKEN_ID, credits_reserved_token);
  }

Recommendation. It is recommended to prohibit calling the initialize function if it has already been called.

Client Response. This issue was fixed by adding a guard in the initialize function to avoid duplicate calls:

  async function finalize_initialize() {
    // Check if the CREDITS_RESERVED_TOKEN_ID token has already been initialized
    let already_initialized: bool = registered_tokens.contains(CREDITS_RESERVED_TOKEN_ID);
    assert_eq(already_initialized, false);
    ...
  }

Description. The Multi Token Support Program supports private token transfers using the record model on Aleo. However, there are no split and join methods for the Token record.

This is inconvenient. For example, if a user has two Token records of the same token, there is no direct way to merge the two records. This will lead to inefficiencies and potential complications in managing token balances. Users may find it challenging to consolidate their holdings, which can result in fragmented token records and increased complexity in tracking and utilizing their assets.

Furthermore, the absence of a split method means that users cannot easily divide a single Token record into smaller portions. This limitation can hinder the flexibility and usability of the token system, especially in scenarios where precise token amounts are required for transactions or other operations.

Recommendation. To enhance the functionality and flexibility of the Token record, it is recommended to implement split and join functions.

Client Response. Demox Labs added the split and join functions to MTSP:

  transition join(
    private token_1: Token,
    private token_2: Token
  ) -> Token {
    // Check that the tokens are the same
    assert(token_1.token_id == token_2.token_id);
    let new_amount: u128 = token_1.amount + token_2.amount;
    // Take the smaller of the two authorized_until values
    let authorized_until: u32 = token_1.authorized_until < token_2.authorized_until
      ? token_1.authorized_until
      : token_2.authorized_until;
    let new_token: Token = Token {
      owner: token_1.owner,
      amount: new_amount,
      token_id: token_1.token_id,
      external_authorization_required: token_1.external_authorization_required,
      authorized_until: authorized_until
    };

    return new_token;
  }

  transition split(
    private token: Token,
    private amount: u128
  ) -> (Token, Token) {
    assert(token.amount >= amount);

    let new_token_1: Token = Token {
      owner: token.owner,
      amount: amount,
      token_id: token.token_id,
      external_authorization_required: token.external_authorization_required,
      authorized_until: token.authorized_until
    };
    let new_token_2: Token = Token {
      owner: token.owner,
      amount: token.amount - amount,
      token_id: token.token_id,
      external_authorization_required: token.external_authorization_required,
      authorized_until: token.authorized_until
    };

    return (new_token_1, new_token_2);
  }

Description. The Multi Token Support Program separates the roles to manage the token. In the set_role function, it does not check if the input role is one of MINTER_ROLE, BURNER_ROLE, or SUPPLY_MANAGER_ROLE. It is possible that the admin sets invalid roles into the map in this function.

  async transition set_role(
    public token_id: field,
    public account: address,
    public role: u8
  ) -> Future {
    assert(token_id != CREDITS_RESERVED_TOKEN_ID);
    return finalize_set_role(token_id, account, role, self.caller);
  }

  async function finalize_set_role(
    token_id: field,
    account: address,
    role: u8,
    caller: address
  ) {
    let token: TokenMetadata = registered_tokens.get(token_id);
    assert_eq(caller, token.admin);

    let role_owner: TokenOwner = TokenOwner {
      account: account,
      token_id: token_id
    };
    let role_owner_hash: field = BHP256::hash_to_field(role_owner);

    roles.set(role_owner_hash, role);
  }

Recommendation. It is recommended to check if the input role is valid in the set_role function.

Client Response. This issue was fixed by checking if the input role is one of the three valid roles.

  async transition set_role(
    public token_id: field,
    public account: address,
    public role: u8
  ) -> Future {
    assert(token_id != CREDITS_RESERVED_TOKEN_ID);
    assert(role == MINTER_ROLE || role == BURNER_ROLE || role == SUPPLY_MANAGER_ROLE);

    let role_owner: TokenOwner = TokenOwner {
      account: account,
      token_id: token_id
    };
    let role_owner_hash: field = BHP256::hash_to_field(role_owner);

    return finalize_set_role(token_id, role, self.caller, role_owner_hash);
  }

Description. The Multi Token Support Program supports wrapping Aleo credits. The metadata of the warpped Aleo credits is initialized in the initialize function. In the metadata, the supply and max_supply fields are fixed numbers (1_500_000_000_000_000u128) and don’t reflect the true supply. Moreover, the numbers are just the initial supply of Aleo credits, which is not fixed.

  async function finalize_initialize() {
    // Initialize the CREDITS_RESERVED_TOKEN_ID token
    let credits_reserved_token: TokenMetadata = TokenMetadata {
      token_id: CREDITS_RESERVED_TOKEN_ID,
      name: 1095517519u128,
      symbol: 1095517519u128,
      decimals: 6u8,
      supply: 1_500_000_000_000_000u128,
      max_supply: 1_500_000_000_000_000u128,
      admin: self.address,
      external_authorization_required: false,
      external_authorization_party: self.address
    };

    registered_tokens.set(CREDITS_RESERVED_TOKEN_ID, credits_reserved_token);
  }

Recommendation. We recommend adding documentation to note that these values are only placeholders. It would be better to set these numbers to 0 since 1_500_000_000_000_000u128 is Aleo’s initial supply and it is not fixed.

Client Response. Demox Labs now manages the supply via mtsp_credits.aleo program and sets the max supply to 10B credits as an upper bound.

Description. The Pondo protocol charges 10% of staking rewards as a commission fee. Currently, the protocol calculates and updates the commission fee whenever users add deposits (deposit_public_as_signer and deposit_public functions), withdraw (withdraw_public and instant_withdraw_public functions), and trigger rebalance (rebalance_redistribute function). The logic of charging the commission fee adds a lot of complexity to these functions.

  async function finalize_deposit_public(
    public f0: Future,
    public f1: Future,
    public f2: Future,
    public credits_deposit: u64,
    public expected_paleo_mint: u64
  ) {
    ...
    let currently_delegated: u64 = balances.get(DELEGATED_BALANCE);
    let current_owed_commission: u64 = owed_commission.get(0u8);
    let total_paleo_pool: u128 = multi_token_support_program_v1.aleo/registered_tokens.get(PALEO_TOKEN_ID).supply + current_owed_commission as u128 - expected_paleo_mint as u128;

    let rewards: i64 = total_delegated > currently_delegated as i64 ? total_delegated - currently_delegated as i64 : 0i64;
    let new_commission: u64 = get_commission(rewards as u128, PROTOCOL_FEE);
    currently_delegated += rewards as u64 - new_commission;

    let core_protocol_account: u64 = credits.aleo/account.get_or_use(self.address, 0u64);
    let reserved_for_withdrawal: u64 = balances.get(CLAIMABLE_WITHDRAWALS);
    let current_state: u8 = protocol_state.get(PROTOCOL_STATE_KEY);
    let deposit_pool: u64 = current_state != REBALANCING_STATE
     ? core_protocol_account - credits_deposit - reserved_for_withdrawal
     : core_protocol_account - currently_delegated - credits_deposit - reserved_for_withdrawal; // if the protocol is rebalancing, the full balance is in the account
    let new_commission_paleo: u64 = calculate_new_paleo(currently_delegated as u128, deposit_pool as u128, new_commission as u128, total_paleo_pool);
    owed_commission.set(0u8, current_owed_commission + new_commission_paleo);

    total_paleo_pool += new_commission_paleo as u128;
    currently_delegated += new_commission;
    // Update bonded pool balance with latest rewards
    balances.set(DELEGATED_BALANCE, currently_delegated);
    ...
  }

Recommendation. It seems that it is possible to charge the commission only in the rebalance_retrieve_credits function. The idea is that the protocol calculates the staking rewards as: the total credits retrieved sub the total credits distributed to delegators. In this way we only need to update balances[DELEGATED_BALANCE] whenever the core protocol distributes credits to the delegators (in distribute_deposits and rebalance_redistribute function). Then in rebalance_retrieve_credits we sub the retrieved credits with balances[DELEGATED_BALANCE] to get the staking rewards of the last epoch. Such logic should be simpler and cost less gas fee for users.

Description. Aleo uses the Narwhal-Bullshark consensus protocol. It does not guarantee constant block time. Currently, the program assumes a 5s block time to get the expected epoch time. In reality, the block time may vary depending on the network conditions and the number of validators.

This will have a direct impact on the epoch time. For example, if the average block time grows to 10s, the epoch will be two weeks. This will also have a direct impact on the user withdrawal lock duration.

Recommendation. It is recommended to add documentation to the code and product to notify that the epoch time and withdrawal time may vary.