Skip to content
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.

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

BugBounty: Проблема с функциями обмена поддерживающих комиссию за передачу токенов #8

Open
Upaut1 opened this issue Sep 15, 2021 · 3 comments

Comments

@Upaut1
Copy link

Upaut1 commented Sep 15, 2021

У функций:

swapExactTokensForTokensSupportingFeeOnTransferTokens
swapExactETHForTokensSupportingFeeOnTransferTokens
swapExactTokensForETHSupportingFeeOnTransferTokens

располагающихся в файле SoyFinanceRouter.sol, отсутствует какая-либо проверка длины торгового маршрута (параметр path).
Это приводит к возможности вызывать данные функции с пустым массивом в качестве path, либо с одним элементом массива в качестве path. Что в свою очередь ведет к полной (100%) потери Gas Limit, который пользователь выделил на проведение транзакции.
Сами транзакции не будут проведены (вызов функций таким образом приведет к ошибке "Error: invalid opcode: opcode 0xfe not defined"), но весь газ, по лимиту газа, будет потрачен.

Для ясности картины, я сделал пару таких транзакций:
Первая транзакция вызывает функцию swapExactTokensForTokensSupportingFeeOnTransferTokens с одним элементом массива path.
Вторая транзакция вызывает функцию swapExactTokensForTokensSupportingFeeOnTransferTokens с пустым массивом path.

Я предлагаю добавить проверку длины маршрута в начала тел этих функций:
require(path.length >= 2, 'SoyFinanceLibrary: INVALID_PATH');

Это как минимум убережет пользователей от полной потери газа.

@yuriy77k
Copy link
Member

yuriy77k commented Oct 5, 2021

Если пользователь сильно хочет себе навредить - то он всегда сможет это сделать указав неверные параметры (например в параметре to он может указать чужой или несуществующий адрес). Контракт не может защитить пользователя от умышленной само-деструкции.
SoyFinance предназначен для обмена как минимум 2-х токенов (что логично), а если пользователь хочет поменять 1 токен на "ничто" то вполне логично что транзакция вызовет ошибку. При этом пользователь не потеряет свои токены.
Не имеет смысла добавлять дополнительную проверку (и заставлять всех пользователей платить за это дополнительный газ) из-за того что некий пользователь умышленно сделает ошибку и потеряет газ за транзакцию.

@Upaut1
Copy link
Author

Upaut1 commented Oct 6, 2021

Любой может создать сайт (веб морду) для торговли токенами с использованием ваших контрактов, которым в последствии, будут пользоваться другие люди. Эти люди могут потерять газ из-за недобросовестного создателя, а не из-за того что они хотели навредить себе.

Я по прежнему считаю что проверка длинны маршрута нужна, тем более газ за такую проверку вообще мизерный.
Кстати,
Функции getAmountsIn и getAmountsOut тоже имеют проверку длинны маршрута require(path.length >= 2, 'SoyFinanceLibrary: INVALID_PATH');
Давайте тогда уберем эти бессмысленные проверки (ведь SoyFinance предназначен для обмена как минимум 2-х токенов (что логично)) и не будем заставлять всех пользователей платить за эти проверки дополнительный газ.

@yuriy77k
Copy link
Member

yuriy77k commented Oct 6, 2021

Согласен. Такая проверка не потребует много газа. при том что эти функции используются крайне редко.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants