Bitcoin Staking - BitHive

The bithive contract designates an owner during installation, granting full control over the upgrade process and the ability to configure critical protocol parameters in bithive/src/admin.rs.

This violates the principle of separation of duties, introduces centralization risks, and grants the owner absolute control over the entire contract.

It is recommended to create low-privilege roles for executing routine tasks within the protocol to minimize the impact of potential compromises of these accounts.

RISK ACCEPTED: The BitHive team accepted the risk of this finding and stated the following:

Currently we have only one owner role that can change configurations of the smart contract. There's another relayer operator role which can  call functions like submit_deposit_tx,  queue_withdrawal, sign_withdrawal and submit_withdrawal_tx, but all these functions are public and are callable by anyone, without permission control. So I think we don't need to introduce low-privilege roles.

The ownership transfer of the bithive contract is executed in a single step, which introduces a security risk. This issue is present in the admin::change_owner function, where the change of ownership lacks a verification step before finalization. As a result, there is a risk that the owner could make a mistake when executing the ownership transfer, such as setting an incorrect address, which could lead to the contract being locked or inaccessible.

Code Location

Below is the implementation of the change_owner function:

#[payable]
pub fn change_owner(&mut self, new_owner_id: AccountId) {
    self.assert_owner();
    self.owner_id = new_owner_id;
}

It is recommended to implement a two-step ownership transfer process: one function to establish the new owner's address (propose_new_owner) and another to accept the transfer (accept_ownership). The latter function should only be executable by the new owner.

SOLVED: The BitHive team solved the issue in the following commit IDs:

• 8157df2: This commit includes the following changes:

  • Removed the change_owner function.

  • Added the pending_owner_id variable to the bithive contract struct.

  • Introduced the new propose_change_owner function, which is called by the current owner to set a new owner account as pending_owner_id.

  • Added the accept_change_owner function, which is called by the pending owner to finalize the ownership transfer by setting the current owner as the pending_owner_id.
    

• 92c33aa: In this commit, 2FA was implemented in the accept_change_owner function using assert_one_yocto(), as it is a privileged function.

To unstake BTC, users must first initiate a queue withdrawal request by signing BTC messages. This request is then processed by a relayer on the NEAR side, which invokes the queue_withdrawal function within the bithive contract.

However, the queue_withdrawal function currently lacks a verification step to ensure that the caller has attached sufficient execution gas. Consequently, if the attached gas is insufficient, the function will fail and consume all of the attached gas. This approach prevents an early revert, which could have preserved some of the gas for the user.

It is recommended to verify that the prepaid gas is sufficient to cover the execution gas requirements at the beginning of the queue_withdrawal function. This precaution will help prevent unnecessary execution attempts that could exhaust all attached gas, ensuring a more efficient use of resources.

SOLVED: The BitHive team solved the issue in the specified commit id.

Within the dry_run_deposit function, the provided deposit transaction is verified to ensure it has not already been confirmed before proceeding with the transaction verification process. If the deposit transaction is found to be already confirmed, the transaction reverts with the following message: "deposit txn verification failed". However, this message does not accurately convey the specific issue, as the transaction's failure is due to its prior confirmation, not an inherent problem with the transaction verification itself.

Code Location

Below is the implementation of the dry_run_deposit function:

pub fn dry_run_deposit(&self, tx_hex: String, embed_vout: u64) {
    self.assert_running();
    let tx = deserialize_hex::<Transaction>(&tx_hex).unwrap();
    let output_id = output_id(&tx.compute_txid().to_string().into(), embed_vout);

    require!(
        !self.confirmed_deposit_txns.contains(&output_id),
        "deposit txn verification failed"
    );
    self.verify_deposit_txn(&tx, embed_vout);
}

It is recommended to update the error message to something more descriptive and accurate, such as: "Deposit transaction already confirmed". This would clearly convey the reason for the failure, helping users better understand the issue.

SOLVED: The BitHive team solved the issue in the specified commit id.

In computer programming, an overflow occurs when an arithmetic operation attempts to create a numeric value that is outside the range that can be represented with a given number of bits – either larger than the maximum or lower than the minimum representable value. This is a good practice, since having overflow-checks enabled in the workspace would avoid overflow situations in release mode.

While the likelihood of an overflow issue arising from the identified occurrences is low, it's still best practice to handle overflow errors gracefully.

Code Location

Below is a list of identified math operations that remain unchecked:

contracts/bithive/src/account.rs
L140: self.total_deposit += value;
L155: self.queue_withdrawal_amount + amount <= self.total_deposit
L158: self.queue_withdrawal_amount += amount;
L160: self.nonce += 1;
L177: self.total_deposit -= deposit.value;

contracts/bithive/src/withdraw.rs
L324: account.queue_withdrawal_start_ts + self.withdrawal_waiting_time_ms
L351: let actual_withdraw_amount = deposit_input_sum - reinvest_amount;

contracts/bithive/src/utils/btc.rs
L55: let flag = sig[0] - 27;

It is recommended to utilize checked arithmetic operations to handle overflow errors gracefully.

ACKNOWLEDGED: The BitHive team acknowledged this issue and stated the following:

Since we already have overflow-checks = true in release build, all arithmetic operations will be protected and the contract will panic if overflow happened. The reason why we think using checked math is unnecessary is that even if it is used, what we can do in case of overflow is still abort the execution and panic, there is no way we can recover from this kind of error. So basically this would have the same effect as it is now.

The following typographical error was identified:

contracts/bithive/src/deposit.rs
L23: const ERR_BAD_PUBKER_HEX: &str = "Invalid pubkey hex";

It is recommended to fix the aforementioned typographical error as follows:

• ERR_BAD_PUBKER_HEX -> ERR_BAD_PUBKEY_HEX

SOLVED: The BitHive team solved the issue in the specified commit id.

Incorporating documentation and comments into the codebase is essential for elucidating key aspects and facilitating comprehension of the developer's intentions by others. However, the codebase contains a misleading comment in the verify_pending_sign_request_amount function, where the actual withdrawal amount is incorrectly documented as being 'less than' the requested withdrawal amount, rather than 'less than or equal to', as shown below:

Code Location

// make sure the actual amount is less than the requested withdrawal amount
require!(
    actual_withdraw_amount <= account.queue_withdrawal_amount,
    ERR_BAD_WITHDRAWAL_AMOUNT
);

It is recommended to correct the misleading comment for accuracy.

SOLVED: The BitHive team solved the issue in the specified commit id.

The bithive contract implements owner-only functions that modify critical state variables without emitting events. The affected functions are the following:

• change_owner

• set_btc_light_client_id

• set_bip322_verifier_id

• set_chain_signatures_id

• set_n_confirmation

• set_withdrawal_waiting_time

• set_min_deposit_satoshi

• set_earliest_deposit_block_height

• set_solo_withdrawal_sequence_heights

• set_paused

The absence of events prevents external systems or users from tracking critical administrative changes via event logs. This lack of transparency diminishes visibility into owner actions that impact the protocol's behavior.

It is recommended to emit an event whenever important contract state variables are updated.

PARTIALLY SOLVED: The BitHive team partially solved this issue. They chose to emit events only in the accept_change_owner and set_paused functions and stated the following:

We choose not to emit events for all these operations because based on our business logic and actual usage, in most cases users won't need to track the updates of these parameters, they just need to query the latest value before interacting with us.