Unreliable communication with Cosmos via Deep Space

[andrey-kuprianov]

Surfaced from @informalsystems audit of Althea Gravity Bridge at commit 19a4cfe

severity: Medium
type: Implementation bug
difficulty: Easy

Involved artifacts

• Orchestrator
• Deep Space library

Description

For communicating with Cosmos Orchestrator uses in multiple places the Deep Space library, also from Althea. In particular, in multiple places the Contact::send_transaction function is called; the typical usage is e.g. in send_valset_confirms():

let response = contact
    .send_transaction(msg_bytes, BroadcastMode::Sync)
    .await?;

contact.wait_for_tx(response, TIMEOUT).await

The Contact::send_transaction() function, after broadcasting the transaction, checks for possible errors in the following way:

if let Some(v) = determine_min_fees_and_gas(&response) {
    return Err(CosmosGrpcError::InsufficientFees { fee_info: v });
} else if !check_tx_response(&response) {
    return Err(CosmosGrpcError::TransactionFailed {
        tx: response,
        time: Duration::from_secs(0),
    });
}
Ok(response)

The functions determine_min_fees_and_gas() and check_tx_response() contain these problematic code:

pub fn determine_min_fees_and_gas(input: &TxResponse) -> Option<FeeInfo> {
    if input.raw_log.contains("insufficient_fees") || input.raw_log.contains("insufficient fee") {
        ...
    } else {
        None
    }
}

and

pub fn check_tx_response(input: &TxResponse) -> bool {
    let key_phrases = [
        "account sequence mismatch",
        "incorrect account sequence",
        "invalid coins",
    ];
    for key in key_phrases.iter() {
        if input.raw_log.contains(key) {
            return false;
        }
    }
    true
}

This is particularly worrying taking into account cosmos/base/abci/v1beta1/abci.proto: TxResponse:

message TxResponse {
  // The output of the application's logger (raw string). May be
  // non-deterministic.
  string raw_log = 6;
}

As can be seen, checking for errors depend on the existence of a predefined set of phrases in the transaction response logs. This method is highly unreliable, and even if it may work now, it is bound to fail with one of the next updates to the Cosmos GRPC end points.

As can be seen above, the Contact::send_transaction() function will succeed in that case, although the transaction has actually failed.

While it may seem that the case of a failing transaction will be caught by Contact::wait_for_tx(), this function also doesn't seem to handle all possible cases reliably, as e.g. demonstrated by this commit. Besides, it depends on the function get_tx_by_hash(), which also doesn't seem to follow proper error handling discipline:

// Gets a transaction using it's hash value, TODO should fail if the transaction isn't found
pub async fn get_tx_by_hash(&self, txhash: String) -> Result<GetTxResponse, CosmosGrpcError> {
    let mut txrpc = TxServiceClient::connect(self.url.clone()).await?;
    let res = txrpc
        .get_tx(GetTxRequest { hash: txhash })
        .await?
        .into_inner();
    Ok(res)
}

Problem Scenarios

There is a possibility of transactions sent to Cosmos from orchestrator to be silently failing due to an unaccounted combination of errors during GRPC requests.

Recommendation

Rework error handling in the Deep Space library, in particular make it independent from the non-deterministic output of application's logger.