From 700794971ba6bb1d905c6de171331dbd265619e3 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Thu, 5 Oct 2023 14:27:28 +0200 Subject: [PATCH] Release 2 new detectors --- slither/detectors/all_detectors.py | 2 + .../detectors/assembly/incorrect_return.py | 91 ++++++++++++++++++ .../assembly/return_instead_of_leave.py | 64 ++++++++++++ ...tReturn_0_8_10_incorrect_return_sol__0.txt | 4 + ...OfLeave_0_8_10_incorrect_return_sol__0.txt | 2 + .../0.8.10/incorrect_return.sol | 36 +++++++ .../0.8.10/incorrect_return.sol-0.8.10.zip | Bin 0 -> 2892 bytes .../return-leave/0.8.10/incorrect_return.sol | 8 ++ .../0.8.10/incorrect_return.sol-0.8.10.zip | Bin 0 -> 1445 bytes tests/e2e/detectors/test_detectors.py | 10 ++ tests/e2e/solc_parsing/test_ast_parsing.py | 8 +- 11 files changed, 221 insertions(+), 4 deletions(-) create mode 100644 slither/detectors/assembly/incorrect_return.py create mode 100644 slither/detectors/assembly/return_instead_of_leave.py create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_IncorrectReturn_0_8_10_incorrect_return_sol__0.txt create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_ReturnInsteadOfLeave_0_8_10_incorrect_return_sol__0.txt create mode 100644 tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol create mode 100644 tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol-0.8.10.zip create mode 100644 tests/e2e/detectors/test_data/return-leave/0.8.10/incorrect_return.sol create mode 100644 tests/e2e/detectors/test_data/return-leave/0.8.10/incorrect_return.sol-0.8.10.zip diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 57f44bc56e..4a111bb64a 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -92,3 +92,5 @@ from .operations.cache_array_length import CacheArrayLength from .statements.incorrect_using_for import IncorrectUsingFor from .operations.encode_packed import EncodePackedCollision +from .assembly.incorrect_return import IncorrectReturn +from .assembly.return_instead_of_leave import ReturnInsteadOfLeave diff --git a/slither/detectors/assembly/incorrect_return.py b/slither/detectors/assembly/incorrect_return.py new file mode 100644 index 0000000000..dc1868e646 --- /dev/null +++ b/slither/detectors/assembly/incorrect_return.py @@ -0,0 +1,91 @@ +from typing import List, Optional + +from slither.core.declarations import SolidityFunction, Function +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.slithir.operations import SolidityCall +from slither.utils.output import Output + + +def _assembly_node(function: Function) -> Optional[SolidityCall]: + """ + Check if there is a node that use return in assembly + + Args: + function: + + Returns: + + """ + + for ir in function.all_slithir_operations(): + if isinstance(ir, SolidityCall) and ir.function == SolidityFunction( + "return(uint256,uint256)" + ): + return ir + return None + + +class IncorrectReturn(AbstractDetector): + """ + Check for cases where a return(a,b) is used in an assembly function + """ + + ARGUMENT = "incorrect-return" + HELP = "If a `return` is incorrectly used in assembly mode." + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-assembly-return" + + WIKI_TITLE = "Incorrect return in assembly" + WIKI_DESCRIPTION = "Detect if `return` in an assembly block halts unexpectedly the execution." + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract C { + function f() internal returns (uint a, uint b) { + assembly { + return (5, 6) + } + } + + function g() returns (bool){ + f(); + return true; + } +} +``` +The return statement in `f` will cause execution in `g` to halt. +The function will return 6 bytes starting from offset 5, instead of returning a boolean.""" + + WIKI_RECOMMENDATION = "Use the `leave` statement." + + # pylint: disable=too-many-nested-blocks + def _detect(self) -> List[Output]: + results: List[Output] = [] + for c in self.contracts: + for f in c.functions_declared: + + for node in f.nodes: + if node.sons: + for function_called in node.internal_calls: + if isinstance(function_called, Function): + found = _assembly_node(function_called) + if found: + + info: DETECTOR_INFO = [ + f, + " calls ", + function_called, + " which halt the execution ", + found.node, + "\n", + ] + json = self.generate_result(info) + + results.append(json) + + return results diff --git a/slither/detectors/assembly/return_instead_of_leave.py b/slither/detectors/assembly/return_instead_of_leave.py new file mode 100644 index 0000000000..74c377d400 --- /dev/null +++ b/slither/detectors/assembly/return_instead_of_leave.py @@ -0,0 +1,64 @@ +from typing import List + +from slither.core.declarations import SolidityFunction, Function +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.slithir.operations import SolidityCall +from slither.utils.output import Output + + +class ReturnInsteadOfLeave(AbstractDetector): + """ + Check for cases where a return(a,b) is used in an assembly function that also returns two variables + """ + + ARGUMENT = "return-leave" + HELP = "If a `return` is used instead of a `leave`." + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-assembly-return" + + WIKI_TITLE = "Return instead of leave in assembly" + WIKI_DESCRIPTION = "Detect if a `return` is used where a `leave` should be used." + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract C { + function f() internal returns (uint a, uint b) { + assembly { + return (5, 6) + } + } + +} +``` +The function will halt the execution, instead of returning a two uint.""" + + WIKI_RECOMMENDATION = "Use the `leave` statement." + + def _check_function(self, f: Function) -> List[Output]: + results: List[Output] = [] + + for node in f.nodes: + for ir in node.irs: + if isinstance(ir, SolidityCall) and ir.function == SolidityFunction( + "return(uint256,uint256)" + ): + info: DETECTOR_INFO = [f, " contains an incorrect call to return: ", node, "\n"] + json = self.generate_result(info) + + results.append(json) + return results + + def _detect(self) -> List[Output]: + results: List[Output] = [] + for c in self.contracts: + for f in c.functions_declared: + + if len(f.returns) == 2 and f.contains_assembly: + results += self._check_function(f) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_IncorrectReturn_0_8_10_incorrect_return_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_IncorrectReturn_0_8_10_incorrect_return_sol__0.txt new file mode 100644 index 0000000000..5d87cd932d --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_IncorrectReturn_0_8_10_incorrect_return_sol__0.txt @@ -0,0 +1,4 @@ +C.bad1() (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#21-24) calls C.indirect() (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#17-19) which halt the execution return(uint256,uint256)(5,6) (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#4) + +C.bad0() (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#8-11) calls C.internal_return() (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#2-6) which halt the execution return(uint256,uint256)(5,6) (tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol#4) + diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ReturnInsteadOfLeave_0_8_10_incorrect_return_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ReturnInsteadOfLeave_0_8_10_incorrect_return_sol__0.txt new file mode 100644 index 0000000000..28f579f818 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_ReturnInsteadOfLeave_0_8_10_incorrect_return_sol__0.txt @@ -0,0 +1,2 @@ +C.f() (tests/e2e/detectors/test_data/return-leave/0.8.10/incorrect_return.sol#3-7) contains an incorrect call to return: return(uint256,uint256)(5,6) (tests/e2e/detectors/test_data/return-leave/0.8.10/incorrect_return.sol#5) + diff --git a/tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol b/tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol new file mode 100644 index 0000000000..e81a747ba1 --- /dev/null +++ b/tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol @@ -0,0 +1,36 @@ +contract C { + function internal_return() internal{ + assembly { + return (5, 6) + } + } + + function bad0() public returns (bool){ + internal_return(); + return true; + } + + function indirect2() internal { + internal_return(); + } + + function indirect() internal { + indirect2(); + } + + function bad1() public returns (bool){ + indirect(); + return true; + } + + function good0() public{ + // Dont report if there is no following operation + internal_return(); + } + + function good1() public returns (uint a, uint b){ + assembly { + return (5, 6) + } + } +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol-0.8.10.zip b/tests/e2e/detectors/test_data/incorrect-return/0.8.10/incorrect_return.sol-0.8.10.zip new file mode 100644 index 0000000000000000000000000000000000000000..2491a10a6554d40be21373fc9f7eeb55ceb9f46e GIT binary patch literal 2892 zcma*p_dgU40|)R!=4FrUjH5H+IOA+dDC=w`j_i5%-p4s7oDp)VbfQklAtU25zuClD z$yTZC(HZIK`~3dAUeD+A!{?XJAMl2o(p}I4&;r;1(6Ad0?*h9wk>8aosh74l9Ko4*R0FnU!9Sp`B zdIk+C|2Wd9#rmi>$XALinx1VfsC#)jtesD3|NGM=z*qXI!$~$;!vs~PW`pxh1(c*> z?;s1U4m}vdbvk`TXbj}Lr2&DEOcW>Lo6b7PYmapJgT>MjN4JhfqASfKGTzV4bXcQ` zcoL-KedM=7T*!QONZzlK!2K5;{L#Nw%Qkd>dH0U2SS8lM^h6_6Gi}@L?6i($)J~)d z9~X48CM4^jM7?K#D=2W$8O>Q_GiodMns4S**APQ#%Y8|D+ih^GFnxRaJYkF=8u@&k zzqrI8FhY1eV%~+ht3>(!(n82N3(JHg>yU0RVF;)?_{Fi86xR2^vaDsyrdT!2PlY5Q z*A_C_Fq^`w)LQ(lNb7=1RD$znf*=j7+z#43L@+JOyT{kYJpxf3jwP1p$(+?cNhZXY ze%4of5AL%5(J6j2?t)zqZpWz^NUB+KMah*4i}gcxKM6|k_@tS(37}s2W?S22h?K7> z1d0|CxMGNkLeAf+6Sb$PEK&1y`C{wkDB1ou#X;IJR8b#iTG%Yo444%{Le|0Hp3`t4 z$oo;WZh#llR|`fC+6|;_LK*4l=-P=&R#Nh0qLFM~^Bu?cnYV(cKQz!}h`7%mFSsb3 zl2sPDx$c4%Xv{3QO)ZFTtO&KeS zxM3o(Qw4jHa%!>5Yc-|f>cMYh30(ED*wOey+2Ge*R!(iz5bE^YPnAiBS}YP2f&+y_ z#41J}i1_ZK-7*@swLOB-{ILs;byfMhGohqv5BI?P1M(_~LrN+!=!L4YKDSgvNz8N# z$2;}vsXk{a^dh6!*K*bacLQpsmXd@Xx823vJ=_sMY}{WiB20>$8&lGx!{$*N%+_$Z zz^Rhsz5RKE?o0GVtoD`PzszrRF!{ZCP5xa<3l&fk97fe-Sa zbfbbs+7);B6bZf{&izA3UXAdmv1+HnJm(&zi}dPF?R(>qCq=)AzHIXmd@Q#}-7tpJ z`cQjwi7IQal{KI2x|d9;TB~IkWO6t?Zix; zL&;=G^UI&>oF$KK?sRM|pY~wY_t}G-bOJ0SQ5kc7(ITmkeOYvB@*4(LfmoBy;4Gxe zcpZG#GGz@RF+z0KKMI~83eS0iiuC8El)Ma%kpBB;7N zrokZ_rxWGnOqD?2&KGiZQtr+wPv>L~T>c#z;q=SF+y0VBm@~gdC8A!mcOEqS=N^%v zZ(=9_kLdR1uylLKZgti>bQ8=D(XJlUA&pe$v_hX<0qJA&o8VF(*E9n*yZzAqNvqf6 ziR+p&w|nTNfcd8@2V&CFFFk*@@&F1N&3NlQM<~Vk9P!Sku<|hx?w{&bh^r(@&f=keYmik+-@%bHw-DxnU2f)Vt`FL(x;2LMobv zx(*(M#cc0J4+KznSy~rBUVU&>|(Et-5(2Y!{bTp zdwO~z*d76N6X}}fMP_BpmfIu7{6Ay14fD%)3TSk{7y&Ikn@;S&v23bnBnSdfgFPQAux}UCr{)dBde`IHYiSN{2VPrs{@y;ilo*o9stQNTMd_H zUI?#KQPPg0`+Rw?<*svA$&MK29RTg$Y>9!X!`u>(4Rr^PxM0tOBf9B>D>Zyl8S*x3 zy3NOjzmqQR-#ud5$+u7n64^0K>nYX1n4GgO#pw!6m0AW-K6eIONicoZl*d9LBO-)n{gw&B#GNBCt9XaH-*=+$bKLFE_*enk5L<>x~G43dI-~+Ut@~9RrJ-E3`{Hs5#1KfS-~q{U@!LF z6UR@N$JBpbFfuM@KeVOZ3dVH2>&9=FthVvq<*=+tOtm1>nDCYD?BnpEEd7A1IhV3o zQetEc`UUb|LBfVX6b$EX$6)K?Af#WCd2Sc_gZH6w;j)%RiVdrmQh(8(&%-QzRvrl+ z;OTfP|~aAzH%% zAajU+*%4F=v+l?0Dhr+GdE+OW!nFT8(3dePR>XZ5s+ZZdxmWRzoAPA_eYmfUvzhn! zB+h2z)Is`r?jdz7@S}R(jPfdoRf+ETHn`$jl6C;L4fo)956!COFTY2&8Bj~~dv^cY zL&a^gOE))Mb|OHG8}2QmdWu{f_`0LFNuo1I{EG2wGk z0O$2*?S!~8)7iOPqJ0WKBI5ELQxhNWSZB+s!&Z9L7KDn~;@FJcn7@$~EJV{Skygx3 z)e!nLeXK>(h4-6~DHT9tJeC2ATvWM2_M9j%Wn`;P`^@`2Ey)26hyT#vYtp;D6fc-! zF6uX$spGenx{oa=?!wVDg)~N;4pDmQjT>eJ@0p*RWu7P$*!*;_`=w_6e5PBWjuAjB z)2#X(54bfBb2+n820;n~;!=qdS+w!+P}GGl__0{8?ZCNpLmuWBbV@gWq{(gGlyM!~ zIKMfv+xZ**?WFc-i^PLT^!A6%NUlt~Mr_)=Dlb1S=kHkD5Y9YkwENSC&xu`}$$yF(jj{u0O&vKbQnpH1&RP$|G=zD( z9~DWAF@$k-x?gAwW%2|6lW_@c4os{Q9qH6hoD znuKbpE-ZRISQI+0phZV;dG0F91cSD(V4VH1La*O=W9l1?tKn{}Xb}}#Q|C41ovB;P zbj^Z>Cn#m#NVhipZ2(o6Q^{F-&|2Ys1vy~ZC7)Pv=PG{e)Fo`28DrYP(l_XNeE_}1 zVtlUcm~5U<@k%@C?n^3UOr<4I|0Sh=mRG0VtlpB9 zgb$t$XcIN}yg^hNUk!uM3z%AQ7BKl~6S|}EI%IJhDHM@OyuO-W(N*(wD}t^-zG-sD z)D1nUxT0X>S|$EB($PWmduiXl~SUHSV&HKT`QK|A-ELL5t0JI!g#S=eF9pOizNZ5UM!IHWXk z@f=(jpnn-^3t2$fG!%?Kv}sfV=8u$?>V|gO-L=*3w&rUU5CrgSPF}2&Y!q(lifAG; z`W$~=TF~-BD1}0qcO-Vl$w(T&M1}))^fGxDM=?H?=5Zo^d9;#OoKbc%o9jI~u8yKx ztprMsNFGTI85pu+n<6IQuZ}KINpakhAEJ|IS2;z6k4>S6?fxQe==UPK;tXMGfW-`h zk8&OR$YXZ<8=G}BGZy}6;s(P+t|f12Cd@}-279EUzlPXrVxB2mU#{XB5!!-Z;bdmN zRV}o}&^V5~ce$^bLM*fC$!DU^=tpK@iCxwN$)mVIF65wJIdtvG_qe2m3qOt~wz$cS zgGgoNp~uA|*PFm6ku5Ba)%o}gl`pp!j|VuR;5NG#Z$}0+YHZOep^qjS=jP17lU4@> zKg+R3%^;fliSXKJ;jVg)?Y?9-C&Vu-n<=qzqshnh=5_{ganK-hyGY@d)U=m`mAPlx zgoUxqC!(MWjW1`ZvaedzbBf)`6lreki9>DPk{aizUX1R zyL)&juy50&`^7Q(2&*6|Mn1(6!JyHGAsxN4R;hW{n>XB9J!Zy9esGr*}SAaV}wCD$xSB5X{NscBxLlf zOF!Og_#JYM%)1YrFLm5ZSuU#ebI=~b=)#t8SWW=vp1tCx8nI|(l2@wdAIJ}3C%xaI zt3IHLt*dG6G7WSgOr4o4et6BKFypJINmh_kt4D1_;Kk#gQYVGqAM!d_2;>*=N zO{7o56=dA5&B~RD#2JWJ{h6A3;nOoD{4oB2Sgm}zusthD)hkDItR3!YKfQfUy&Y>W k1~L=>Z*}h2^WRAj`-T52Qmj2#@>gGMXD{y12>dJk1CAf3f&c&j literal 0 HcmV?d00001 diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 6fc04e4e1f..9ca1bfcde9 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1654,6 +1654,16 @@ def id_test(test_item: Test): "encode_packed_collision.sol", "0.7.6", ), + Test( + all_detectors.IncorrectReturn, + "incorrect_return.sol", + "0.8.10", + ), + Test( + all_detectors.ReturnInsteadOfLeave, + "incorrect_return.sol", + "0.8.10", + ), ] GENERIC_PATH = "/GENERIC_PATH" diff --git a/tests/e2e/solc_parsing/test_ast_parsing.py b/tests/e2e/solc_parsing/test_ast_parsing.py index 2da4cf1e26..37f6d7274b 100644 --- a/tests/e2e/solc_parsing/test_ast_parsing.py +++ b/tests/e2e/solc_parsing/test_ast_parsing.py @@ -4,13 +4,13 @@ import sys from pathlib import Path from typing import List, Dict, Tuple -from packaging.version import parse as parse_version -import pytest + +from crytic_compile import CryticCompile, save_to_zip +from crytic_compile.utils.zip import load_from_zip from deepdiff import DeepDiff +from packaging.version import parse as parse_version from solc_select.solc_select import install_artifacts as install_solc_versions from solc_select.solc_select import installed_versions as get_installed_solc_versions -from crytic_compile import CryticCompile, save_to_zip -from crytic_compile.utils.zip import load_from_zip from slither import Slither from slither.printers.guidance.echidna import Echidna