Skip to content

audit-fix: Rounding Logic Flaws in Division Operations #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Hazim-om wants to merge 1 commit into
base: main
Choose a base branch
from
Open

audit-fix: Rounding Logic Flaws in Division Operations #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 29 additions & 18 deletions programs/omnipair/src/instructions/futarchy/distribute_tokens.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use anchor_spl::token_interface::Mint;
use crate::state::futarchy_authority::FutarchyAuthority;
use crate::constants::{FUTARCHY_AUTHORITY_SEED_PREFIX, BPS_DENOMINATOR};
use crate::errors::ErrorCode;
use crate::utils::math::ceil_div;
use crate::generate_futarchy_authority_seeds;

#[derive(AnchorSerialize, AnchorDeserialize, Clone)]
Expand Down Expand Up @@ -80,24 +81,34 @@ impl<'info> DistributeTokens<'info> {
// Get total balance to distribute
let total_balance = source_token_account.amount as u128;

// Calculate amounts for each recipient using stored percentages
let amount1 = total_balance
.checked_mul(futarchy_authority.revenue_distribution.futarchy_treasury_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

let amount2 = total_balance
.checked_mul(futarchy_authority.revenue_distribution.buybacks_vault_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

let amount3 = total_balance
.checked_mul(futarchy_authority.revenue_distribution.team_treasury_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;
// Calculate amounts for each recipient using ceiling division to ensure no funds are lost
// Use ceiling for first two, then calculate third as remainder to ensure exact distribution
let amount1 = ceil_div(
total_balance
.checked_mul(futarchy_authority.revenue_distribution.futarchy_treasury_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

let amount2 = ceil_div(
total_balance
.checked_mul(futarchy_authority.revenue_distribution.buybacks_vault_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

// This ensures all funds are distributed exactly
let amount1_plus_amount2 = (amount1 as u128)
.checked_add(amount2 as u128)
.ok_or(ErrorCode::FeeMathOverflow)?;
let amount3 = if amount1_plus_amount2 <= total_balance {
(total_balance - amount1_plus_amount2) as u64
} else {
// This should never happen if percentages are valid, but handle gracefully
return Err(ErrorCode::FeeMathOverflow.into());
};

// Generate PDA seeds for signing
let seeds = generate_futarchy_authority_seeds!(futarchy_authority);
Expand Down
27 changes: 16 additions & 11 deletions programs/omnipair/src/instructions/lending/flashloan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::{
errors::ErrorCode,
events::*,
utils::token::transfer_from_pool_vault_to_user,
utils::math::ceil_div,
generate_gamm_pair_seeds,
};

Expand Down Expand Up @@ -157,18 +158,22 @@ impl<'info> Flashloan<'info> {

let FlashloanArgs { amount0, amount1, data } = args;

// Calculate fees (5 bps = 0.05%)
let fee0 = (amount0 as u128)
.checked_mul(FLASHLOAN_FEE_BPS as u128)
.unwrap()
.checked_div(BPS_DENOMINATOR as u128)
.unwrap() as u64;
// Calculate fees (5 bps = 0.05%) using ceiling division to ensure fees are never zero
let fee0 = ceil_div(
(amount0 as u128)
.checked_mul(FLASHLOAN_FEE_BPS as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

let fee1 = (amount1 as u128)
.checked_mul(FLASHLOAN_FEE_BPS as u128)
.unwrap()
.checked_div(BPS_DENOMINATOR as u128)
.unwrap() as u64;
let fee1 = ceil_div(
(amount1 as u128)
.checked_mul(FLASHLOAN_FEE_BPS as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

// Record balances before the flash loan
token0_vault.reload()?;
Expand Down
15 changes: 12 additions & 3 deletions programs/omnipair/src/instructions/lending/liquidate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::{
events::{UserPositionLiquidatedEvent, EventMetadata},
state::user_position::UserPosition,
utils::token::transfer_from_pool_vault_to_user,
utils::math::ceil_div,
generate_gamm_pair_seeds,
};

Expand Down Expand Up @@ -197,9 +198,17 @@ impl<'info> Liquidate<'info> {
if is_collateral_token0 { user_position.collateral0 } else { user_position.collateral1 }
);

let caller_incentive = (collateral_final as u128)
.checked_mul(LIQUIDATION_INCENTIVE_BPS as u128).ok_or(ErrorCode::DebtMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128).ok_or(ErrorCode::DebtMathOverflow)?.try_into().map_err(|_| ErrorCode::DebtMathOverflow)?;
// Use ceiling division to ensure liquidators always get at least some incentive
// This prevents zero incentives for small liquidations which would disincentivize liquidators
let caller_incentive = ceil_div(
(collateral_final as u128)
.checked_mul(LIQUIDATION_INCENTIVE_BPS as u128)
.ok_or(ErrorCode::DebtMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::DebtMathOverflow)?
.try_into()
.map_err(|_| ErrorCode::DebtMathOverflow)?;

// Remaining collateral goes to reserves (after incentive)
let collateral_to_reserves = collateral_final
Expand Down
29 changes: 17 additions & 12 deletions programs/omnipair/src/instructions/spot/swap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::{
errors::ErrorCode,
events::*,
utils::token::{transfer_from_user_to_pool_vault, transfer_from_pool_vault_to_user},
utils::math::ceil_div,
generate_gamm_pair_seeds,
};

Expand Down Expand Up @@ -159,19 +160,23 @@ impl<'info> Swap<'info> {
let last_k = (pair.reserve0 as u128).checked_mul(pair.reserve1 as u128).ok_or(ErrorCode::InvariantOverflow)?;
let is_token0_in = user_token_in_account.mint == pair.token0;

// Calculate total fee amount
let total_fee = (amount_in as u128)
.checked_mul(pair.swap_fee_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;
// Calculate total fee amount using ceiling division to ensure fees are never zero
let total_fee = ceil_div(
(amount_in as u128)
.checked_mul(pair.swap_fee_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

// Calculate futarchy fee portion of the total fee
let futarchy_fee = (total_fee as u128)
.checked_mul(futarchy_authority.revenue_share.swap_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?
.checked_div(BPS_DENOMINATOR as u128)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;
// Calculate futarchy fee portion of the total fee using ceiling division
let futarchy_fee = ceil_div(
(total_fee as u128)
.checked_mul(futarchy_authority.revenue_share.swap_bps as u128)
.ok_or(ErrorCode::FeeMathOverflow)?,
BPS_DENOMINATOR as u128
)
.ok_or(ErrorCode::FeeMathOverflow)? as u64;

// Transfer futarchy fee to authority immediately if non-zero
if futarchy_fee > 0 {
Expand Down
33 changes: 19 additions & 14 deletions programs/omnipair/src/state/user_position.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use anchor_lang::prelude::*;
use crate::constants::*;
use crate::errors::ErrorCode;
use crate::utils::math::ceil_div;
use super::Pair;
use std::cmp::max;

Expand Down Expand Up @@ -59,13 +60,15 @@ impl UserPosition {
pair.total_debt0_shares = amount;
self.debt0_shares = amount;
} else {
let shares = (amount as u128)
.checked_mul(pair.total_debt0_shares as u128)
.ok_or(ErrorCode::DebtShareMathOverflow)?
.checked_div(pair.total_debt0 as u128)
.ok_or(ErrorCode::DebtShareDivisionOverflow)?
.try_into()
.map_err(|_| ErrorCode::DebtShareDivisionOverflow)?;
let shares = ceil_div(
(amount as u128)
.checked_mul(pair.total_debt0_shares as u128)
.ok_or(ErrorCode::DebtShareMathOverflow)?,
pair.total_debt0 as u128
)
.ok_or(ErrorCode::DebtShareDivisionOverflow)?
.try_into()
.map_err(|_| ErrorCode::DebtShareDivisionOverflow)?;
pair.total_debt0_shares = pair.total_debt0_shares.saturating_add(shares);
self.debt0_shares = self.debt0_shares.saturating_add(shares);
}
Expand All @@ -76,13 +79,15 @@ impl UserPosition {
pair.total_debt1_shares = amount;
self.debt1_shares = amount;
} else {
let shares = (amount as u128)
.checked_mul(pair.total_debt1_shares as u128)
.ok_or(ErrorCode::DebtShareMathOverflow)?
.checked_div(pair.total_debt1 as u128)
.ok_or(ErrorCode::DebtShareDivisionOverflow)?
.try_into()
.map_err(|_| ErrorCode::DebtShareDivisionOverflow)?;
let shares = ceil_div(
(amount as u128)
.checked_mul(pair.total_debt1_shares as u128)
.ok_or(ErrorCode::DebtShareMathOverflow)?,
pair.total_debt1 as u128
)
.ok_or(ErrorCode::DebtShareDivisionOverflow)?
.try_into()
.map_err(|_| ErrorCode::DebtShareDivisionOverflow)?;
pair.total_debt1_shares = pair.total_debt1_shares.saturating_add(shares);
self.debt1_shares = self.debt1_shares.saturating_add(shares);
}
Expand Down
10 changes: 10 additions & 0 deletions programs/omnipair/src/utils/math.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,14 @@ pub fn normalize_two_values_to_nad(
normalize_two_values_to_scale(a, a_decimals, b, NAD_DECIMALS)
}

/// Ceiling division: rounds up to the nearest integer
/// Formula: ceil(a / b) = (a + b - 1) / b
/// Returns None on overflow
pub fn ceil_div(a: u128, b: u128) -> Option<u128> {
if b == 0 {
return None;
}
a.checked_add(b - 1)?.checked_div(b)
}