search

SoyFinance / smart-contracts

11 stars 9 forks source link

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

Open Upaut1 opened 3 years ago

Upaut1 commented 3 years ago

У функций:

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 commented 3 years ago

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

Upaut1 commented 3 years ago

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

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

yuriy77k commented 3 years ago

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