core: fix overflow and panic in priority fee estimation (#839)

The U256 priority fees were being coerced into u32, which was not big
enough for actual values. The overflow was happening but not checked.
This led to a possible divide-by-zero panic in this code as well.

This change does the math as I256 -- overkill, but it works.
This commit is contained in:
juniorbeef 2022-01-29 05:54:38 -07:00 committed by GitHub
parent 8b9a18ce01
commit 97744b87a6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 14 additions and 9 deletions

View File

@ -4,6 +4,7 @@
### Unreleased ### Unreleased
- Fix overflow and possible divide-by-zero in `estimate_priority_fee`
- Add BSC mainnet and testnet to the list of known chains - Add BSC mainnet and testnet to the list of known chains
[831](https://github.com/gakonst/ethers-rs/pull/831) [831](https://github.com/gakonst/ethers-rs/pull/831)
- Returns error on invalid type conversion instead of panicking - Returns error on invalid type conversion instead of panicking

View File

@ -25,11 +25,11 @@ pub use rlp;
/// Re-export hex /// Re-export hex
pub use hex; pub use hex;
use crate::types::{Address, Bytes, U256}; use crate::types::{Address, Bytes, I256, U256};
use elliptic_curve::sec1::ToEncodedPoint; use elliptic_curve::sec1::ToEncodedPoint;
use ethabi::ethereum_types::FromDecStrErr; use ethabi::ethereum_types::FromDecStrErr;
use k256::{ecdsa::SigningKey, PublicKey as K256PublicKey}; use k256::{ecdsa::SigningKey, PublicKey as K256PublicKey};
use std::{convert::TryInto, ops::Neg}; use std::convert::{TryFrom, TryInto};
use thiserror::Error; use thiserror::Error;
#[derive(Error, Debug)] #[derive(Error, Debug)]
@ -335,15 +335,13 @@ fn estimate_priority_fee(rewards: Vec<Vec<U256>>) -> U256 {
let mut rewards_copy = rewards.clone(); let mut rewards_copy = rewards.clone();
rewards_copy.rotate_left(1); rewards_copy.rotate_left(1);
let mut percentage_change: Vec<i64> = rewards let mut percentage_change: Vec<I256> = rewards
.iter() .iter()
.zip(rewards_copy.iter()) .zip(rewards_copy.iter())
.map(|(a, b)| { .map(|(a, b)| {
if b > a { let a = I256::try_from(*a).expect("priority fee overflow");
((b - a).low_u32() as i64 * 100) / (a.low_u32() as i64) let b = I256::try_from(*b).expect("priority fee overflow");
} else { ((b - a) * 100.into()) / a
(((a - b).low_u32() as i64 * 100) / (a.low_u32() as i64)).neg()
}
}) })
.collect(); .collect();
percentage_change.pop(); percentage_change.pop();
@ -354,7 +352,7 @@ fn estimate_priority_fee(rewards: Vec<Vec<U256>>) -> U256 {
// If we encountered a big change in fees at a certain position, then consider only // If we encountered a big change in fees at a certain position, then consider only
// the values >= it. // the values >= it.
let values = if *max_change >= EIP1559_FEE_ESTIMATION_THRESHOLD_MAX_CHANGE && let values = if *max_change >= EIP1559_FEE_ESTIMATION_THRESHOLD_MAX_CHANGE.into() &&
(max_change_index >= (rewards.len() / 2)) (max_change_index >= (rewards.len() / 2))
{ {
rewards[max_change_index..].to_vec() rewards[max_change_index..].to_vec()
@ -682,5 +680,11 @@ mod tests {
// The median should be taken because none of the changes are big enough to ignore values. // The median should be taken because none of the changes are big enough to ignore values.
assert_eq!(estimate_priority_fee(rewards), 102_000_000_000u64.into()); assert_eq!(estimate_priority_fee(rewards), 102_000_000_000u64.into());
// Ensure fee estimation doesn't panic when overflowing a u32. This had been a divide by
// zero.
let overflow = U256::from(u32::MAX) + 1;
let rewards_overflow: Vec<Vec<U256>> = vec![vec![overflow], vec![overflow]];
assert_eq!(estimate_priority_fee(rewards_overflow), overflow);
} }
} }