sherlock-audit / 2024-08-woofi-solana-deployment-judging

2 stars 2 forks source link

0xeix - swap_fee is incorrectly calculated for the quote_amount #2

Open sherlock-admin4 opened 1 month ago

sherlock-admin4 commented 1 month ago

0xeix

Medium

swap_fee is incorrectly calculated for the quote_amount

Summary

swap_fee parameter is calculated using quote_amount, fee_rate. However, due to incorrect divisor, incorrect amount of fees would be calculated.

Vulnerability Detail

In the current implementation of the handler() function in the swap.rs, the swap_fee parameter is calculated the following way:

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L151

let swap_fee = checked_mul_div_round_up(quote_amount, fee_rate as u128, ONE_E5_U128)?;

As you can see here, divisor is set to ONE_E5_U128 from the constants:

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/constants.rs#L5

pub const ONE_E5_U128: u128 = 100_000;

Now imagine the following scenario:

  1. We have the pool SOL / USDT where USDT is the quote token (6 decimal places)
  2. After swapping SOL to the quote token, we got 200 USDT (200e6).
  3. Fee is set to 2.5% - 25e3 in the current implementation:

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/constants.rs#L12-14

// unit: 0.1 bps (1e6 = 100%, 25 = 2.5 bps)
// Fee amount = total_amount * fee_rate / 100_000.
// Max fee rate supported is u16::MAX around 65.5%.

The issue is that the protocol assumes that 100_000 is 1e6 when in fact it's 1e5. So, when we multiply our 200 USDT amount by 25e3 and then divide by 100_000, we get the following amount:

200e6 * 25e3 / 100_000 = 50

So the fee is taken as 25% instead of set 2.5%.

Impact

The users will pay more fees than expected due to incorrect assumption about decimal places of 100%.

Code Snippet

Tool used

Manual review.

Manual Review

Recommendation

Change 100% value on 1_000_000 instead of 100_000.

toprince commented 1 month ago

it's a typo in comment. 1bps = 0.01% the unit is 0.1 bps, means 0.001%, which equals 1e-5. So below code is correct, but need change "1e6" to 1e5 in comment.

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/woonetwork/WOOFi_Solana/pull/37

gjaldon commented 3 weeks ago

The comment has been updated with the correct decimal precision.