soroswap / aggregator

Soroswap Aggregator and Adapters Smart Contracts. Includes Soroswap.Finance and Phoenix Adapters
https://soroswap.finance
Apache License 2.0
2 stars 1 forks source link

2 [B4] The Aggregator may try to trade negligible amounts #51

Closed ACassimiro closed 2 months ago

ACassimiro commented 3 months ago

On the calculation of the last amount added to the list of swap_amounts (in lines 104 to 112 of the Aggregator contract), the amount to be traded in a specific protocol can be negligible, leading to possible denial of transactions and weakening the purpose of validating that the parts of the distribution are different from 0 (line 83 of the Aggregator contract).

When calculating how much should be exchanged in each platform, all but the last value are calculated as amount = (total_amount * part)/total_parts. If the ratio of part to total_parts is too low, so will the amount.

Similarly, for the last element of the distributions, since all parts are greater than 0 and total_parts is the sum of all parts, part will never be equal to total_parts, meaning that there will always be a remaining value to be exchanged in the last distribution. This means that even though the parts for the last distribution will not effectively be used in calculating the amount to trade, the amount traded at the last distribution will always be greater or equal to 1.

What may emerge as a complication of this is how the AMMs will handle such low amounts. Two possibilities come to mind: (1) the transaction is refused as the input amount is too low, and (2) loss by division in the calculation of amounts to be exchanged may yield no return for the user.

As in issue #48, this may be a consequence of how the transaction and its parameters are built (either by the user or the front end), but a nice way to avoid such scenarios is to, instead of validating if the distribution.part is not zero, validate that the amount to be exchanged is above a minimum amount.

esteblock commented 3 months ago

[B4] The Aggregator May Try to Trade Negligible Amounts Severity: Informative Recommended Action: Fix Design

Description On the calculation of the last amount added to the list of swap_amounts (in lines 104 to 112 of the Aggregator contract), the amount to be traded in a specific protocol can be negligible, leading to possible denial of transactions and weakening the purpose of validating that the parts of the distribution are different from 0 (line 83 of the Aggregator contract). When calculating how much should be exchanged in each platform, all but the last value are calculated as amount = (total_amount * part)/total_parts . If the ratio of part to total_parts is too low, so will the amount. Similarly, for the last element of the distributions, since all part s are greater than 0 and total_parts is the sum of all parts, part will never be equal to total_parts , meaning that there will always be a remaining value to be exchanged in the last distribution. This means that even though the parts for the last distribution will not effectively be used in calculating the amount to trade, the amount traded at the last distribution will always be greater or equal to 1. What may emerge as a complication of this is how the AMMs will handle such low amounts. Two possibilities come to mind: (1) the transaction is refused as the input amount is too low, and (2) loss by division in the calculation of amounts to be exchanged may yield no return for the user.

Recommendation As in finding A2, this may be a consequence of how the transaction and its parameters are built (either by the user or the front end), but an approach to avoid such scenarios is to, instead of validating if the distribution.part is not zero, validate that the amount to be exchanged is above a minimum amount.