[transfer-fee] calculate_inverse_fee logic error when transfer_fee_basis_points is MAX_FEE_BASIS_POINTS about solana-program-library HOT 6 CLOSED

Incorrect Function Return Values for Two Specific Scenarios

Issue Description

The function in question is returning 0 for two distinct scenarios where this output is not expected or correct. The scenarios and the suggested corrections are as follows:

1. Scenario 1: post_fee_amount Greater Than 0

   When post_fee_amount is greater than 0, it implies that transfer_fee must have been equal to max_fee_amount. To rectify the returned value, max_fee_amount should be added back to obtain the pre_fee_amount. This adjustment will yield the correct output and allow the function to proceed as intended.

2. Scenario 2: post_fee_amount Equals 0

   In this scenario, the amount could have ranged anywhere between 0 and max_fee_amount. To address this, it is proposed that the function should always return max_fee_amount. This approach will ensure a more accurate representation of the possible fee range when post_fee_amount is 0.

Suggested Changes

• For Scenario 1: Add max_fee_amount back to the output when post_fee_amount > 0.
• For Scenario 2: Return max_fee_amount when post_fee_amount is 0.

from solana-program-library.

This appears to be intentional behavior. When calculating the inverse_fee, the pre_fee_amount resolves to 0 automatically when transfer_fee_basis_points is MAX_FEE_BASIS_POINTS.

Then of course that resolved 0 will yield another 0 from calculate_fee.

Are you sure calculate_inverse_fee is what you want here, and not calculate_fee?

from solana-program-library.

It's still wrong because it cannot be the case that the fee is always 0 for ONE_IN_BASIS_POINTS.

Imagine a token that has the rate set as ONE_IN_BASIS_POINTS and also has max_fee set, the inverse_fee should be max_fee in this case.

from solana-program-library.

@metamania01 It's not that the fee is 0, it's that it can't be calculated by calculate_inverse_fee if the basis points are 100%.

MAX_BASIS_POINTS == ONE_IN_BASIS_POINTS == 100%.

You'd end up dividing by zero for the pre fee:

It seems you're suggesting if transfer_fee_basis_points == ONE_IN_BASIS_POINTS it should in fact return Some(maximum_fee as u128)?

cc @joncinque @samkim-crypto

from solana-program-library.

This is a good point, thanks for bringing it up! we were a bit lazy in bailing out if the fee is 100%.

You're definitely right, so let's go through the situations:

• if post_fee_amount is greater than 0, and the fee is 100%, then the inverse fee needs to be the maximum fee
• if post_fee_amount is 0, and the fee is 100%, what do we say? The fee could have been anything between 1 and maximum fee. Should we return an error?

Keep in mind that the maximum fee is important here, however. If it's set to u64::MAX - 1, and the post_fee_amount is 100, then we'll need to return an error since there's no solution. I believe that should be covered by the existing code.

What do you think about all this?

from solana-program-library.

I think generally, these functions are not perfect because the solution is not really well-defined in some edge cases.

• if post_fee_amount is 0, and the fee is 100%, what do we say? The fee could have been anything between 1 and maximum fee. Should we return an error?

It seems like the fee could have also been 0 in this case since the pre_fee_amount could have been 0, independent of the maximum fee?

I think the question is what to do when there are multiple solutions possible. For calculate_pre_fee_amount, we chose to always return the smallest possible pre_fee_amount. If I want to transfer an amount to a destination account so that the destination account receives a certain amount after fees are deducted, then I can use this function to calculate the exact amount I need to transfer from the perspective of the sender. In this case, I do not want to overpay, so it makes sense to choose the smallest amount.

So if we add this special case to calculate_inverse_fee:

• if post_fee_amount is greater than 0, and the fee is 100%, then the inverse fee needs to be the maximum fee

then things are consistent in that both calculate_pre_fee_amount and calculate_inverse_fee returns the smallest possible number. If we update calculate_inverse_fee to return an error when post_fee_amount is 0 and the fee is 100%, then it seems like we should also update calculate_pre_fee_amount to return errors on undetermined outputs for consistency.

from solana-program-library.