Kickico — краудфандинговая платформа, kickstarter с приемом криптовалют.
О проекте я уже знал. Где-то, возможно в чате, увидел ссылку на гитхаб с исходниками контракта и мне стало интересно взглянуть на код, который ворочает миллионами.
Тогда я еще ничего не знал о смартконтрактах и solidity, как и о том, что загруженный контракт просто так не обновить, и сразу создал ишью на то, что были использованы magic numbers c предложением заменить на dividents.length.
Также при первом чтении мне бросилась в глаза функция calculateDividends, подразумевалась, что при вызове с заданием порядкового номера из плана начисления дивидендов, она будет начислять проценты на счета держателей токенов. Вот только после начисления не деактивируется очередной этап и если вызвать функцию еще раз, то дивиденды начислятся еще раз.
Еще в рамках первого ревью я увидел, что не вызывается событие Approval, совсем не баг, и, как я понял, сейчас вообще ни на что не влияет, но все равно отправил ишью.
После этого я скинул AntiDanilevski ссылки на ишьюс, чтобы не потерялись в суматохе preico, получил обещание проверить и забыл на некоторое время о Kickico. Через 2 недели мне пришло уведомление с гитхаба о новом коммите, все мои ишьюсы были исправлены. Я написал Анти и получил свою награду в токенах.
Еще через 10 дней Анти написал мне уже сам, сообщил о большом обновлении контрактов и предложил поискать еще багов. Та часть в контракте токена, что отвечала за дивиденты и заморозку токенов была хорошо переработана, предотвращена потенциальная уязвимость, когда на начисление дивидентов могло не хватить эфира.
Посидев пару часиков я отправил еще 2 ишьюса. Первый, хоть и критичный, но был сразу заметен. Не совпадали проценты дивидендов с Whitepaper. Видимо, планировали изменить, но потом переделали.
Второй ишью опять был связан с возможностью повторно начислять дивиденты, но уже не всем, а особо хитрым, которые нашли бы этот баг. Этот баг — совокупность факторов: возможность вызывать начисление дивидендов любому пользователю, введение возможности начислять дивиденды не на все счета сразу, а на часть (из-за лимита газа).
Эти баги исправили в то же день и я получил еще вознаграждение.
В тот же день команда kickico закоммитила поддержку протокола bankor и Анти еще раз мне предложил поискать баги. Сразу я увидел только то, что не реализован вызов события NewSmartToken, но опять же это не критично.
Следующим вечером я еще раз взглянул на код и увидел, что в методе issue, не хватало добавления адреса, для которого выпускались токены, в список на начисление дивидентов. Я сразу же сообщил об этом Анти, но выяснилось, что об этом баге уже сообщили нанятые за деньги аудиторы.
Что было сделано хорошо?
- Багбаунти. Мало какие проекты проводят багбаунти.
- Выложили код на гитхабе. Я, например, совсем не собирался участвовать в багбаунти, зашел посмотреть код из любопытства, а вот как получилось :)
- Работа с участниками багбаунти. Анти сообщал мне о всех обновлениях и просил искать еще.
- Найм сторонних аудиторов.
-----Бонусная часть-----
На днях kickico выпустили обновленную версию токена, в которой, кроме прочего, был исправлен эпик критикал баг, который не увидел ни я до ICO, ни сторонние аудиторы. Что подтверждает тот факт, что если у вас более менее сложный контракт, то никто не сможет дать гарантии отсутствия в нем багов. Всегда нужно иметь план Б на этот случай. Хорошим вариантом для этого может быть механизм обновления кода без смены адреса.
Ну и собственно конкурс: кто первым опишет этот баг, получит 500 KICK от проекта kickico. На гитхабе версия с багом, смотреть дифф с текущей версией неспортивно :) Описание бага прячьте пожалуйста под спойлер.
Комментарии (25)
evnuh
29.09.2017 12:46+3И этим людям дали $28m, а они «просят незнакомцев поискать баги в их коде, ну пожалуйста».
yshurik
29.09.2017 15:27+1Интересно…
В функцииА функция calculateDividends может вызыватся всеми? (а не только onlyOwner?).
Далее в calculateDividends, при вызове с limit == addressByIndex.length не происходит обновление countComplete потому что оно в else блоке. Значит можно посчитать дивиденты еще раз.
Да и когда limit = countComplete + limit стоит делать safeAdd чтобы точно без переполнений.
quantum Автор
29.09.2017 15:55Хорошая попытка, но нет :)
Метод специально разрешено вызывать всем, чтобы любой мог начислить дивиденды, если владелец вдруг решит не начислять.
Обновляется currentDividendIndex и при следующем вызове будут начисляться уже следующие по очереди дивиденды.
Safeadd добавлен в новой версии, но баг не в этомyshurik
30.09.2017 18:21+1Это то что сразу видно в первые 15 минут.
Аудиторы сразу должны были сказать используйте OpenZeppelin/SafeMath во всех методах которые даете пользователям рулить.
А вот логика с calculateDividends как по мне странная, да:
Дают пользователям доступ по начислению дивидентов, но вот гарантии что данный метод начислит дивиденты к msg.sender собственно и нету (не попал в limit). A если пользователей миллионы то расход газа и его стоимость на подсчет дивидентов по всем(или limit) ложится в стоимость транзакции от msg.sender которому собственно не гарантируется начисление дивидентов :(
Это и не баг но я бы сказал что такую логику не ожидает пользователь который вызывает транзакцию.
Это UX — пользователь вызывающий calculateDividends как минимум ожидает дивиденты себе любимому так как именно он оплачивает транзакцию и газ по ней.quantum Автор
30.09.2017 19:14В финальной версии появился метод по запросу дивидендов конкретно себе
yshurik
30.09.2017 23:59Это уже получше.
Хотя тоже могут быть вопросы — например никто не хочет тратить деньги на транзакции для всех calculateDividends (а если там миллионы адресов а твой последний :) ), а будет считать только дивиденты для себя, будет ли увеличиваться как-то currentDividendIndex (который сейчас увеличивается только в calculateDividends).
Или сам kickico будет делать по тысяче транзакций с limit=1000 (мне вчера транзакция для ENS имени например стоила около $3.5 с лимитом газа 300K) чтобы обойти все адреса… (ну или какие параметры там будут выгодны)
Ну или пусть тогда каждый transfer дополнительно считает 10-100 дивидентов для других пользователей, не накладно.
Я бы им посоветовал продумать такие методы с оценкой вычислительной сложности от массивов как addressByIndex. Когда токен например выйдет на биржи и пользователи будут им торговать количество адресов начнет расти неплохо (можно завести на биржу, поторговать, вернуть, начислилить дивиденты и назад на биржу, добавятся еще временные адреса пользователей на биржах при всех transfer() итд)
quantum Автор
01.10.2017 20:30Сами Kickico будут тратить деньги на просчет дивидендов всем. Это вопрос их репутации. currentDividendIndex увеличиваться не будет. Самому можно получить только текущие дивиденды.
И да, каждый transfer считает текущие дивиденды для отправителя и получателя.
>можно завести на биржу, поторговать, вернуть, начислилить дивиденты и назад на биржу
Как уже сказал при переводе считаются за счет переводящего
morozovsk
29.09.2017 16:22+2если вызвать функцию еще раз, то дивиденды начислятся еще раз.
Мне казалось, что функция начисления дивидендов — одна из базовых и ошибку при работе с ней легко мог найти тестировщик. Или у них нет тестировщика? Они же всё таки по-сути банковское ПО пишут. Странно просить аудиторов проверять на наличие элементарных багов. Обычная схема:
написание кода -> автотесты -> ревью -> тестирование -> аудирование
, а здесь:
написание кода -> аудирование
Или там всё гораздо лучше, а это просто я не разобрался? У них на сайте в разделе «команда» нашёл только разработчиков.
Raz0r
29.09.2017 16:38+1Заголовок спойлераВ функции
accountBalanceнебезопасно вычитаетсяagingBalanceOf[_address][0]изbalances[_address]. Если в какой-то моментagingBalanceOf[_address][0]станет большеbalances[_address], произойдет переполнение иbalances[_address]станет близким к 2**256.quantum Автор
29.09.2017 16:57По коду видно, что agingBalanceOf не может быть больше основного баланса.
Хоть в новом версии и добавили везде safemath, но эпик ошибка не связана с переполнением.
Raz0r
29.09.2017 17:08+2Заголовок спойлераТогда предположу, что функция transfer перестанет работать после 30.09.2019 09:00:00, так как в dividends не будет 25го элемента.
quantum Автор
29.09.2017 17:43Напишите ваш адрес кошелька в эфире, вам перешлют 500 kick.
Лучше здесь, в комментариях. Чтобы было все прозрачно :) Можете для этого новый кошелек создать
Raz0r
29.09.2017 19:15+1Спасибо!
0x3dA04a48CF4647c1916A9C9b1cB87D83A5118dA7
quantum Автор
29.09.2017 20:32Добавили в общую таблицу на выплату баунти. В раздел code audit.
Выплаты ожидаются на следующей неделе. https://docs.google.com/spreadsheets/d/1KaRoVNYeNcKky_fy8lGTf_Idh-P8BloPOpQFT3lAUoQ/htmlview#
Hardcoin
29.09.2017 21:26+2механизм обновления кода без смены адреса
Идея "код это закон" полностью провалилась. Вряд ли кому интересен закон, который можно поменять вечером в пятницу и который сразу вступает в силу.
Но как механизм автоматизации — удобно. Выплаты по расписанию и все такое.
TimsTims
30.09.2017 00:47+1+1. А еще на фоне этого:
Метод специально разрешено вызывать всем, чтобы любой мог начислить дивиденды, если владелец вдруг решит не начислять.
если владелец вдруг захочет всех кинуть, то он просто сделает update кода. Либо обновит код, но допустит в нём труднозаметную ошибку, и сам же ей воспользуется, и никто не докажет, что это он всё укралquantum Автор
30.09.2017 10:00Если владелец захочет кинуть, у него есть много способов, помимо изменения кода смартконтракта :) Так что я бы не стал по этому сильно париться
VolCh
01.10.2017 11:31Как показывает практика, ничто не мешает законодателю принимать и вводить действия законы «моментально», если он видит необходимость.
Hardcoin
01.10.2017 11:44Да. И отказаться нельзя. А в случае смартконтракта-то можно его не заключать. Т.е. если нужна автоматизация — смартконтракт подойдёт. Если нужен закон — люди просто пройдут мимо изменяемого смартконтракта.
Raz0r
Есть детали?
quantum Автор
Если никто в комментариях не опишет, сделаю завтра апдейт поста