Broker fees are not taken from swap amount
Note: Chainflip Labs independently discovered and promptly addressed this issue just a few days into the engagement. We commend their proactive approach in identifying and remedying this critical issue. Because the vulnerability exists in the audit version, we document it in this finding for completeness.
Description
The broker commission fee is calculated and added to the accrued fees, but it is not subtracted from the swap amount:
fn schedule_swap_from_channel(
deposit_address: ForeignChainAddress,
deposit_block_height: u64,
from: Asset,
to: Asset,
amount: AssetAmount,
destination_address: ForeignChainAddress,
broker_id: Self::AccountId,
broker_commission_bps: BasisPoints,
channel_id: ChannelId,
) {
let fee = Permill::from_parts(broker_commission_bps as u32 * BASIS_POINTS_PER_MILLION) *
amount;
! EarnedBrokerFees::<T>::mutate(&broker_id, from, |earned_fees| {
! earned_fees.saturating_accrue(fee)
! });
let encoded_destination_address =
T::AddressConverter::to_encoded_address(destination_address.clone());
let swap_origin = SwapOrigin::DepositChannel {
deposit_address: T::AddressConverter::to_encoded_address(deposit_address),
channel_id,
deposit_block_height,
};
if let Some(swap_id) = Self::schedule_swap_with_check(
from,
to,
! amount,
destination_address.clone(),
&swap_origin,
) {Impact
An attacker could abuse this behavior to completely drain the protocol. Because any user can register as a broker and choose arbitrary fees (up to 100%), any user can be an attacker.
If an attacker chooses 100% fees, then initiates a swap using a deposit channel with amount of funds, they can receive (i.e., double the output) from the following:
from the regular egress of a swap (e.g., USDC for USDC)
from the 100% broker commission rate (once the fees are accounted for the broker but not removed from the swap amount)
By repeatedly performing no-op swaps, an attacker could have enough fees accounted to them to withdraw all of the liquidity from the protocol.
Recommendations
Subtract the fee from amount.
Remediation
As noted, Chainflip Labs independently identified this issue during the assessment period and remediated it in commit .