Как я искал (и нашел!) баги в смартконтракте проекта kickico

    В августе я, неожиданно для себя, поучаствовал в bugbounty проекта Kickico. Я уже рассказал об этом на митапе Atlas Blockchain в прошлую пятницу. Статья — текстовая версия этого доклада с дополнением и небольшим пятничным конкурсом :)

    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. На гитхабе версия с багом, смотреть дифф с текущей версией неспортивно :) Описание бага прячьте пожалуйста под спойлер.
    MixBytes
    61,00
    Внедряем блокчейн, пишем смарт-контракты
    Поделиться публикацией

    Похожие публикации

    Комментарии 26

      0
      На днях kickico выпустили обновленную версию токена, в которой, кроме прочего, был исправлен эпик критикал баг, который не увидел ни я до ICO, ни сторонние аудиторы.

      Есть детали?

        0

        Если никто в комментариях не опишет, сделаю завтра апдейт поста

        +3
        И этим людям дали $28m, а они «просят незнакомцев поискать баги в их коде, ну пожалуйста».
          0

          Погуглите "google bugbounty". Уж google то точно мог бы сам у себя все проверить, так нет, просит зачем-то незнакомцев.

            +1
            Заметил, что аудит всё же есть. Аппеляция была к его отсутствию.
          +1
          Интересно…

          В функции
          А функция calculateDividends может вызыватся всеми? (а не только onlyOwner?).
          Далее в calculateDividends, при вызове с limit == addressByIndex.length не происходит обновление countComplete потому что оно в else блоке. Значит можно посчитать дивиденты еще раз.
          Да и когда limit = countComplete + limit стоит делать safeAdd чтобы точно без переполнений.

            0
            Хорошая попытка, но нет :)

            Метод специально разрешено вызывать всем, чтобы любой мог начислить дивиденды, если владелец вдруг решит не начислять.

            Обновляется currentDividendIndex и при следующем вызове будут начисляться уже следующие по очереди дивиденды.

            Safeadd добавлен в новой версии, но баг не в этом
              +1
              Это то что сразу видно в первые 15 минут.

              Аудиторы сразу должны были сказать используйте OpenZeppelin/SafeMath во всех методах которые даете пользователям рулить.

              А вот логика с calculateDividends как по мне странная, да:

              Дают пользователям доступ по начислению дивидентов, но вот гарантии что данный метод начислит дивиденты к msg.sender собственно и нету (не попал в limit). A если пользователей миллионы то расход газа и его стоимость на подсчет дивидентов по всем(или limit) ложится в стоимость транзакции от msg.sender которому собственно не гарантируется начисление дивидентов :(

              Это и не баг но я бы сказал что такую логику не ожидает пользователь который вызывает транзакцию.

              Это UX — пользователь вызывающий calculateDividends как минимум ожидает дивиденты себе любимому так как именно он оплачивает транзакцию и газ по ней.
                0
                В финальной версии появился метод по запросу дивидендов конкретно себе
                  0
                  Это уже получше.

                  Хотя тоже могут быть вопросы — например никто не хочет тратить деньги на транзакции для всех calculateDividends (а если там миллионы адресов а твой последний :) ), а будет считать только дивиденты для себя, будет ли увеличиваться как-то currentDividendIndex (который сейчас увеличивается только в calculateDividends).

                  Или сам kickico будет делать по тысяче транзакций с limit=1000 (мне вчера транзакция для ENS имени например стоила около $3.5 с лимитом газа 300K) чтобы обойти все адреса… (ну или какие параметры там будут выгодны)

                  Ну или пусть тогда каждый transfer дополнительно считает 10-100 дивидентов для других пользователей, не накладно.

                  Я бы им посоветовал продумать такие методы с оценкой вычислительной сложности от массивов как addressByIndex. Когда токен например выйдет на биржи и пользователи будут им торговать количество адресов начнет расти неплохо (можно завести на биржу, поторговать, вернуть, начислилить дивиденты и назад на биржу, добавятся еще временные адреса пользователей на биржах при всех transfer() итд)

                    0
                    Сами Kickico будут тратить деньги на просчет дивидендов всем. Это вопрос их репутации. currentDividendIndex увеличиваться не будет. Самому можно получить только текущие дивиденды.

                    И да, каждый transfer считает текущие дивиденды для отправителя и получателя.

                    >можно завести на биржу, поторговать, вернуть, начислилить дивиденты и назад на биржу
                    Как уже сказал при переводе считаются за счет переводящего
            +2
            если вызвать функцию еще раз, то дивиденды начислятся еще раз.
            Мне казалось, что функция начисления дивидендов — одна из базовых и ошибку при работе с ней легко мог найти тестировщик. Или у них нет тестировщика? Они же всё таки по-сути банковское ПО пишут. Странно просить аудиторов проверять на наличие элементарных багов. Обычная схема:
            написание кода -> автотесты -> ревью -> тестирование -> аудирование
            , а здесь:
            написание кода -> аудирование
            Или там всё гораздо лучше, а это просто я не разобрался? У них на сайте в разделе «команда» нашёл только разработчиков.
              0

              Тут ничего не могу сказать, не знаю как там внутри было организовано

              +1
              Заголовок спойлера

              В функции accountBalance небезопасно вычитается agingBalanceOf[_address][0] из balances[_address]. Если в какой-то момент agingBalanceOf[_address][0] станет больше balances[_address], произойдет переполнение и balances[_address] станет близким к 2**256.

                0

                По коду видно, что agingBalanceOf не может быть больше основного баланса.


                Хоть в новом версии и добавили везде safemath, но эпик ошибка не связана с переполнением.

                  +2
                  Заголовок спойлера

                  Тогда предположу, что функция transfer перестанет работать после 30.09.2019 09:00:00, так как в dividends не будет 25го элемента.

                    0

                    Бинго!

                      0

                      Напишите ваш адрес кошелька в эфире, вам перешлют 500 kick.


                      Лучше здесь, в комментариях. Чтобы было все прозрачно :) Можете для этого новый кошелек создать

                +2
                механизм обновления кода без смены адреса

                Идея "код это закон" полностью провалилась. Вряд ли кому интересен закон, который можно поменять вечером в пятницу и который сразу вступает в силу.


                Но как механизм автоматизации — удобно. Выплаты по расписанию и все такое.

                  +1
                  +1. А еще на фоне этого:
                  Метод специально разрешено вызывать всем, чтобы любой мог начислить дивиденды, если владелец вдруг решит не начислять.
                  если владелец вдруг захочет всех кинуть, то он просто сделает update кода. Либо обновит код, но допустит в нём труднозаметную ошибку, и сам же ей воспользуется, и никто не докажет, что это он всё украл
                    0
                    Если владелец захочет кинуть, у него есть много способов, помимо изменения кода смартконтракта :) Так что я бы не стал по этому сильно париться
                    0
                    Как показывает практика, ничто не мешает законодателю принимать и вводить действия законы «моментально», если он видит необходимость.
                      0

                      Да. И отказаться нельзя. А в случае смартконтракта-то можно его не заключать. Т.е. если нужна автоматизация — смартконтракт подойдёт. Если нужен закон — люди просто пройдут мимо изменяемого смартконтракта.

                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                  Самое читаемое