Wrong ERC20 decimals computation in SendToCosmos

[hannydevelop]

Original Issue

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

severity: Medium
type: Implementation bug
difficulty: Low

Involved artifacts

• Orchestrator: gbt/utils.rs

Description

When sending tokens from Ethereum to Cosmos using gbt client, the function fraction_to_exponent() computes amounts of tokens to send from the ERC20 representation in the following way:

/// TODO revisit this for higher precision while
/// still representing the number to the user as a float
/// this takes a number like 0.37 eth and turns it into wei
/// or any erc20 with arbitrary decimals
pub fn fraction_to_exponent(num: f64, exponent: u8) - Uint256 {
    let mut res = num;
    // in order to avoid floating point rounding issues we
    // multiply only by 10 each time. this reduces the rounding
    // errors enough to be ignored
    for _ in 0..exponent {
        res *= 10f64
    }
    (res as u128).into()
}

The arguments to this function are transferred from the command-line arguments, where num represents the amount of tokens as displayed to the user, which then needs to be multiplied to the 10 to the power of exponent (ERC20 decimals). The problem with this code is that it:

• uses f64 type to represent intermediate results, which looses precision already around 15-16 decimal digits

• uses u128 as the end result, which has only half the bitrange of u256.

As can be seen from this Rust playground, with the num equal to 1, the function saturates to u128::MAX already at the exponent of 39.

37        - 9999999999999998358171037484709838848
38        - 99999999999999978859343891977453174784
39        - 340282366920938463463374607431768211455
40        - 340282366920938463463374607431768211455
41        - 340282366920938463463374607431768211455
u128::MAX - 340282366920938463463374607431768211455

The fraction_to_exponent() function is used in eth_to_cosmos() as follows:

    let res = web3
        .get_erc20_decimals(erc20_address, ethereum_public_key)
        .await
        .expect("Failed to query ERC20 contract");
    let decimals: u8 = res.to_string().parse().unwrap();
    let amount = fraction_to_exponent(amount, decimals);

    let erc20_balance = web3
        .get_erc20_balance(erc20_address, ethereum_public_key)
        .await
        .expect("Failed to get balance, check ERC20 contract address");


    if erc20_balance == 0u8.into() {
        error!(
            "You have zero {} tokens, please double check your sender and erc20 addresses!",
            erc20_address
        );
        exit(1);
    } else if amount.clone()  erc20_balance {
        error!("Insufficient balance {}  {}", amount, erc20_balance);
        exit(1);
    }

As can be seen, the incorrectly computed, saturated to u128::MAX amount may pass the check, but the user won't actually have the funds to complete the requested operation. Alternatively, the user may have the funds, but the computed amount will differ from the requested one. The transfer from Ethereum to Cosmos will then proceed successfully, but the actually transferred amount may be much smaller than the requested one.

Problem Scenarios

• a user intends to send funds from Ethereum to Cosmos using an ERC20 contract with moderately large decimals (around 30-40);

• the operation will succeed, but the actual transferred amount will be much smaller than requested;

• the user will not discover the error until later, when the funds transferred to Cosmos will be smaller than requested;

• the Ethereum gas will be effectively lost without user benefit;

• this may also cause later transaction failures on the Cosmos side due to insufficient funds.

Recommendation

Rework the function fraction_to_exponent(), such that:

• the function maxes out at 256 bits, not at 128 bits;

• a warning or error is issued to the user in case the function approaches or reaches the saturation.

[EricBolten]

fraction_to_exponent is not present in our fork, and the input it was processing is parsed directly as a Uint256 here:

gravity-bridge/orchestrator/gorc/src/commands/eth_to_cosmos.rs

Lines 49 to 50 in eca0feb

	let init_amount = self.args.get(4).expect("amount is required");
	let amount: Uint256 = init_amount.parse().unwrap();

We should be able to simply close this issue out.

[cbrit]

👍

[hannydevelop]

@EricBolten I think we have a solution for this in our eth_to_cosmos.rs command. I'll discuss this too with Jack.