Division by zero in SwapExecution::max_price
Description
The function Dex::end_block panics as a result of the call to arbitrage, returning an error when given a set of six positions involving five assets that trigger a zero division in SwapExecution::max_price.
The relevant positions are constructed in the following test case:
#[tokio::test]
async fn zero_division_with_six_positions() {
use cnidarium_component::Component;
use penumbra_sct::{component::clock::EpochManager, epoch::Epoch};
use std::str::FromStr;
use tendermint::abci;
let asset_a = asset::Id::from_str(
"passet1mv4dg744vmefu3azks3cljxzpnkux9nt778pu9n9njdql94r6u8qe90s6q"
).unwrap();
let asset_b = asset::Id::from_str(
"passet1984fctenw8m2fpl8a9wzguzp7j34d7vravryuhft808nyt9fdggqxmanqm"
).unwrap();
let asset_c = asset::Id::from_str(
"passet1z9kse4k7jdyudph4sqhhexqhnecdfzv28rsuc00exuzujn55r5xqjlyc3p"
).unwrap();
let asset_d = asset::Id::from_str(
"passet1r4kcf2m4r92jqmdks5c9yt7v2tgnht4aj3fml4qln56x72nm8qrsm9d598"
).unwrap();
let asset_e = asset::Id::from_str(
"passet15e489q49rlsr9lk0ec76cclfh8d962lls5p072jrf42dsthhjqxq6xmlfn"
).unwrap();
let mut rng = rand_chacha::ChaChaRng::seed_from_u64(1312);
let positions = vec![
Position::new(&mut rng, DirectedTradingPair::new(asset_a, asset_b),
0,
Amount::from(149389448249173150936496u128),
Amount::from(20845039290582300130674u128),
Reserves {
r1: Amount::from(0u64),
r2: Amount::from(1674038047566237470u64), }
),
Position::new(&mut rng, DirectedTradingPair::new(asset_c, asset_b),
0,
Amount::from(11482491653881581581u128),
Amount::from(22964983307763163163u128),
Reserves {
r1: Amount::from(6428821468204u64),
r2: Amount::from(3997249009584709369u64), }
),
Position::new(&mut rng, DirectedTradingPair::new(asset_c, asset_a),
593,
Amount::from(369914451723480732836066u128),
Amount::from(253592161842809128314442u128),
Reserves {
r1: Amount::from(0u64),
r2: Amount::from(3346911693977u64), }
),
Position::new(&mut rng, DirectedTradingPair::new(asset_d, asset_a),
0,
Amount::from(461882542585266790278356u128),
Amount::from(639568646294864146743u128),
Reserves {
r1: Amount::from(0u64),
r2: Amount::from(233587891225428463u64), }
),
Position::new(&mut rng, DirectedTradingPair::new(asset_d, asset_a),
4848,
Amount::from(1445730096413720996991u128),
Amount::from(864162800629363471837189u128),
Reserves {
r1: Amount::from(323449097135647u64),
r2: Amount::from(0u64), }
),
Position::new(&mut rng, DirectedTradingPair::new(asset_d, asset_e),
0,
Amount::from(1u128),
Amount::from(1u128),
Reserves {
r1: Amount::from(0u64),
r2: Amount::from(1u64), }
),
];
let storage = TempStorage::new().await.unwrap();
let mut state = Arc::new(StateDelta::new(storage.latest_snapshot()));
let height = 1;
{
let mut state_tx = state.try_begin_transaction().unwrap();
state_tx.put_epoch_by_height(
height,
Epoch {
index: 0,
start_height: 0,
},
);
state_tx.put_block_height(height);
state_tx.apply();
}
{
let mut state_tx = state.try_begin_transaction().unwrap();
for pos in positions {
state_tx.put_position(pos).await.unwrap();
}
state_tx.apply();
}
let end_block = abci::request::EndBlock {
height: height.try_into().unwrap(),
};
crate::component::Dex::end_block(&mut state, &end_block).await;
}The relevant excerpt of the stack trace is
5: penumbra_dex::swap_execution::SwapExecution::max_price
at ./src/swap_execution.rs:27:21
6: penumbra_dex::component::router::route_and_fill::RouteAndFill::rout
e_and_fill::{{closure}}::{{closure}}
at ./src/component/router/route_and_fill.rs:277:44
7: penumbra_dex::component::router::route_and_fill::RouteAndFill::rout
e_and_fill::{{closure}}
at ./src/component/router/route_and_fill.rs:156:5
8: <core::pin::Pin<P> as core::future::future::Future>::poll
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/
future/future.rs:125:9
9: penumbra_dex::component::arb::Arbitrage::arbitrage::{{closure}}::{{
closure}}
at ./src/component/arb.rs:63:14
10: penumbra_dex::component::arb::Arbitrage::arbitrage::{{closure}}
at ./src/component/arb.rs:22:5
11: <core::pin::Pin<P> as core::future::future::Future>::poll
at /rustc/79e9716c980570bfd1f666e3b16ac583f0168962/library/core/src/
future/future.rs:125:9
12: <penumbra_dex::component::dex::Dex as cnidarium_component::compone
nt::Component>::end_block::{{closure}}::{{closure}}
at ./src/component/dex.rs:102:14Impact
Causing a panic in end_block will halt the chain, causing a denial of service. While this specific set of positions requires a high quantity of specific tokens that are whitelisted for arbitrage to trigger the panic, it is possible that the root cause is triggerable with a smaller amount of tokens, as this set of positions was extracted from proptest with a reduction strategy that reduced the values in the positions independently after reducing the number of positions, which is not guaranteed to find a global minimum value market that triggers the same panic, only to find a local minimum.
Recommendations
If the successful completion of arbitrage in Dex::end_block is not required for security, only as an optimization, Dex::end_block should log its result on error instead of panicking. Additionally, consider whether route_and_fill can handle execution.max_price() returning Err more locally (e.g., whether it makes sense to close the erroring position and backtrack here).
Remediation
This issue has been acknowledged by Penumbra Labs, and fixes were implemented in the following commits: