Гайд по автоматическому аудиту смарт-контрактов. Часть 2: Slither +8


Анализатор: Slither
Описание: Open-source static analysis framework for Solidity
githib: https://github.com/trailofbits/slither


Это статический анализатор кода, написанный на python. Он умеет следить за переменными, вызовами, и детектирует вот такой список уязвимостей: https://github.com/trailofbits/slither#detectors. У каждой уязвимости есть ссылка с описанием, и, если вы новичок в Solidity, вам имеет смысл ознакомиться со всеми.


Slither может работать, как модуль python и предоставлять программисту интерфес, для аудита по собственному плану. Простой и показательный пример того, что умеет делать slither можно увидеть тут: https://github.com/trailofbits/slither/blob/master/examples/scripts/functions_writing.py


Мы еще вернемся к сценариям анализа в конце статьи, а пока запустим Slither:


git clone https://github.com/trailofbits/slither.git
cd slither
docker build -t slither .

и попробуем проанализировать наш контракт.


Заходим в каталог с constructor-eth-booking и пытаемся запустить slither из докера:


$ docker run -v $(pwd)/contracts:/slither/contracts slither contracts/flattened.sol

получаем ошибку “Source file requires different compiler version”, и теперь нам надо засунуть версию solc=0.4.20 в докер slither-а. Для этого правим Dockerfile самого Slither, как было указано в п.2 во введении, т.е. где нибудь в конце Dockerfile (https://github.com/trailofbits/slither/blob/master/Dockerfile) добавляем строку:


COPY --from=ethereum/solc:0.4.20 /usr/bin/solc /usr/bin

, пересобираем образ, запускаем, ура, все компилируется.


Видим вывод, warning-и про разные “pragma” и про неверные названия переменных типа Parameter '_price' of Booking.Booking (flattened.sol#73) is not in mixedCase. Анализаторы выдают много предупреждений, но мы ищем реальные баги, и не будем обращать внимание на мелочь. Отфильтруем все сообщения про mixedCase, сейчас не до стиля:


$ docker run -v $(pwd)/contracts:/slither/contracts slither contracts/flattened.sol 2>&1 | fgrep -v 'mixedCase'

Истинные программисты пропускают все зелененькое, смотрят все красненькое, и вот что помимо false-positives нашел в контракте Slither:


Booking.refundWithoutCancellationFee (flattened.sol#243-250) sends eth to arbirary user
        Dangerous calls:
        - client.transfer(address(this).balance) (flattened.sol#249)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations
INFO:Detectors:
Booking.refundWithCancellationFee (flattened.sol#252-259) sends eth to arbirary user
        Dangerous calls:
        - owner.transfer(m_cancellationFee) (flattened.sol#257)
        - client.transfer(address(this).balance) (flattened.sol#258)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations

теперь смотрим что в контракте не так с этим функциями:


    /************************** PRIVATE **********************/

    function refundWithoutCancellationFee() private  {
        address client = m_client;
        m_client = address(0);
        changeState(State.OFFER);

        client.transfer(address(this).balance);
    }

    function refundWithCancellationFee() private {
        address client = m_client;
        m_client = address(0);
        changeState(State.CANCELED);

        owner.transfer(m_cancellationFee);
        client.transfer(address(this).balance);
    }

при этом, например, функция refundWithoutCancellationFee() вызывается вот так:


    function rejectPayment() external onlyOwner onlyState(State.PAID) {
        refundWithoutCancellationFee();
    }

    function refund() external onlyClient onlyState(State.PAID) {
        refundWithoutCancellationFee();
    }

Хм, формально ошибок нет: вызовы защищены всякими onlyOwner, но Slither ругается, что, мол, внутри refundWithoutCancellationFee() без всяких проверок шлется эфир. И он прав, сама функция действительно не имеет почти никаких ограничений. Пускай она private, и вызывается из wrapper-ов “rejectPayment()” “refund()” c нужными ограничениями, но в таком виде, если дорабатывать контракт — велик риск забыть про ограничения и воткнуть вызов refundWithoutCancellationFee() в какое нибудь другое место, доступное атакующему. Так, что, пусть формально уязвимости нет, информация оказалась полезной — это как минимум “warning” level, если по заданию планируется развивать код контракта дальше. В данном случае, две функции от разных участников используют один код, и, такое решение было принято для экономии газа — контракт является одноразовым, и стоимость его выкладки является важным фактором.


Я перепроверил, не просто ли так Slither ругается на любую отправку эфира, и перенес тело функции прямо в вышеуказанные “rejectPayment()” и “refund()”, предупреждение исчезло, т.е. Slither понял, что теперь эфир не высылается без проверок адресов. Отличное начало!


Теперь проверим как Slither следит за инициализацией переменных, для этого закомментируем две инициализации:


- m_fileHash = _fileHash;
+ // m_fileHash = _fileHash;
- m_price = _price;
+ // m_price = _price;

первая — не сильно важная, в плане дыр, кроме траты ресурсов, ибо m_fileHash не используется нигде, он просто сохраняется в блокчейне при создании контракта. А вот m_price используется, и Slither правильно ругается на то, что m_price нигде не инициализируется, хотя и используется:


Booking.m_price (flattened.sol#128) is never initialized. It is used in:
        - fallback (flattened.sol#144-156)

Ну это простой трюк, как и ожидалось, всё сработало нормально.


Теперь внесём в контракт столь полюбившуюся всем reentrancу: будем изменять state контракта после внешнего вызова. Внесем такие изменения:


    function refundWithoutCancellationFee() private  {
        address client = m_client;
-         m_client = address(0);
-         changeState(State.OFFER);
-         client.transfer(address(this).balance);
+        client.call.value(address(this).balance)();
+        m_client = address(0);
+        changeState(State.OFFER);
    }

Пришлось заменить transfer на call, т.к. на вариант с transfer Slither не ругается из за того, что transfer отправляет вызов с минимумом газа, и обратный вызов невозможен (хотя при переходе на форк Constantinople в Ethereum, была изменена цена газа, и это заново “включило” reentrancy атаку с использованием transfer (https://github.com/ChainSecurity/constantinople-reentrancy).


Результат поиска reentrancy:


Reentrancy in Booking.refundWithoutCancellationFee (flattened.sol#243-253):
        External calls:
        - client.call.value(address(this).balance)() (flattened.sol#245)
        State variables written after the call(s):
        - m_client (flattened.sol#246)

Прекрасно, как минимум изменять state variables после внешних вызовов он не даст, и это очень хорошо.


Если двигаться по списку, то остальные уязвимости в списке либо представляют собой просто поиск конкретных методов в коде, либо известные паттерны, которые при наличии доступа к разметке кода, выполненной python-ом, конечно же работают, и достаточно надежно. Т.е. well-knows паттерны Slither не пропустит.


Теперь, я внесу изменения, которые отлично показывают специфику работы статических анализаторов:


-         client.transfer(address(this).balance
+         for (uint i=0; i < 1; i++) {                                                                                                                                     
+             client.transfer(address(this).balance - 999999999999999999);                                                                                                 
+         }  

и результат:


Booking.refundWithoutCancellationFee has external calls inside a loop:
        - client.transfer(address(this).balance - 999999999999999999) (flattened.sol#252)
Reference: https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop

Цикл выполняется единожды, и является вырожденным — поэтому выданное предупреждение — false positive, а отсутствие предупреждения об опасной арифметике — false negative. Анализ типов, результатов операций, подсчет вызовов — задачи не для статических анализаторов. Поэтому чётко понимайте, какие ошибки найдет Slither, а какие необходимо искать с помощью других инструментов.


Мы обещали упомянуть о возможности написания собственных сценариев для тестирования, и вывода всякой интересной информации о контракте с помощью ключа --print. С этой точки зрения, Slither представляет собой отличный инcтрумент для CI. Разработчики большой системы контрактов знают названия критических для безопасности переменных: балансов, размеров комиссий, флагов, и могут написать сценарий тестирования, который будет блокировать любые изменения в коде, которые, к примеру, перезаписывают важную переменную, или меняют state переменные после внешнего вызова, и его отлично предсказуемый анализ является отличным инструментом для использования в hook-ах.
Задача Slither — избавить вас от глупых багов, найти хорошо знакомые опасные паттерны и предупредить разработчика. В этом варианте он хорош и как инструмент начинающего разработчика на Solidity, сразу подсказывая как правильно писать код на Solidity.


Итоги


В моем личном тестировании я бы поставил Slither четверку, за универсальность, простоту и удобство использования, а также за простые и понятные сценарии тестирования и приспособленность к CI.
Slither уверенно нашел реальный WARNING, связанный с использованием функции, отправляющей эфир, обнаружил все внесенные баги. Он не справился лишь с динамическим анализом, который формально и не должен делать, иначе придется пожертвовать универсальностью, предсказуемостью и простотой использования.


В следующей статье мы займемся анализатором Mythril, а вот все оглавление статей, которые готовы или планируются к написанию:
Часть 1. Введение. Компиляция, flattening, версии Solidity
Часть 2. Slither (эта статья)
Часть 3. Mythril (в процессе написания)
Часть 4. Manticore (в процессе написания)
Часть 5. Echidna (в процессе написания)




К сожалению, не доступен сервер mySQL