From 9ecc66ddf81cb4e0bfe19dd53197c25634467401 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Wed, 5 Jul 2023 21:59:17 -0500 Subject: [PATCH] fix regression that caused retdata to be flagged --- .../unchecked_low_level_return_values.py | 62 ++++++++++++++++-- .../operations/unused_return_values.py | 6 +- .../0.7.6/unchecked_lowlevel.sol | 8 ++- .../0.7.6/unchecked_lowlevel.sol-0.7.6.zip | Bin 2436 -> 2753 bytes 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/slither/detectors/operations/unchecked_low_level_return_values.py b/slither/detectors/operations/unchecked_low_level_return_values.py index 0537ebbf2..c1fb1a868 100644 --- a/slither/detectors/operations/unchecked_low_level_return_values.py +++ b/slither/detectors/operations/unchecked_low_level_return_values.py @@ -1,15 +1,24 @@ """ Module detecting unused return values from low level """ -from slither.detectors.abstract_detector import DetectorClassification -from slither.detectors.operations.unused_return_values import UnusedReturnValues +from typing import List + +from slither.core.cfg.node import Node from slither.slithir.operations import LowLevelCall -from slither.slithir.operations.operation import Operation + +from slither.core.declarations.function_contract import FunctionContract +from slither.core.variables.state_variable import StateVariable +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.utils.output import Output -class UncheckedLowLevel(UnusedReturnValues): +class UncheckedLowLevel(AbstractDetector): """ - If the return value of a send is not checked, it might lead to losing ether + If the return value of a low-level call is not checked, it might lead to losing ether """ ARGUMENT = "unchecked-lowlevel" @@ -38,5 +47,44 @@ If the low level is used to prevent blocking operations, consider logging failed WIKI_RECOMMENDATION = "Ensure that the return value of a low-level call is checked or logged." - def _is_instance(self, ir: Operation) -> bool: # pylint: disable=no-self-use - return isinstance(ir, LowLevelCall) + @staticmethod + def detect_unused_return_values(f: FunctionContract) -> List[Node]: + """ + Return the nodes where the return value of a call is unused + Args: + f (Function) + Returns: + list(Node) + """ + values_returned = [] + nodes_origin = {} + for n in f.nodes: + for ir in n.irs: + if isinstance(ir, LowLevelCall): + # if a return value is stored in a state variable, it's ok + if ir.lvalue and not isinstance(ir.lvalue, StateVariable): + values_returned.append(ir.lvalue) + nodes_origin[ir.lvalue] = ir + + for read in ir.read: + if read in values_returned: + values_returned.remove(read) + + return [nodes_origin[value].node for value in values_returned] + + def _detect(self) -> List[Output]: + """Detect low level calls where the success value is not checked""" + results = [] + for c in self.compilation_unit.contracts_derived: + for f in c.functions_and_modifiers: + unused_return = UncheckedLowLevel.detect_unused_return_values(f) + if unused_return: + + for node in unused_return: + info: DETECTOR_INFO = [f, " ignores return value by ", node, "\n"] + + res = self.generate_result(info) + + results.append(res) + + return results diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index 80be98b45..783a44807 100644 --- a/slither/detectors/operations/unused_return_values.py +++ b/slither/detectors/operations/unused_return_values.py @@ -101,10 +101,8 @@ contract MyConc{ def _detect(self) -> List[Output]: """Detect high level calls which return a value that are never used""" results = [] - for c in self.compilation_unit.contracts: - for f in c.functions + c.modifiers: - if f.contract_declarer != c: - continue + for c in self.compilation_unit.contracts_derived: + for f in c.functions_and_modifiers: unused_return = self.detect_unused_return_values(f) if unused_return: diff --git a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol index 96712b077..cd6f78e98 100644 --- a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol +++ b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol @@ -4,8 +4,14 @@ contract MyConc{ } function good(address payable dst) external payable{ - (bool ret, bytes memory _) = dst.call{value:msg.value}(""); + (bool ret, ) = dst.call{value:msg.value}(""); require(ret); } + function good2(address payable dst) external payable{ + (bool ret, ) = dst.call{value:msg.value}(""); + if (!ret) { + revert(); + } + } } diff --git a/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip b/tests/e2e/detectors/test_data/unchecked-lowlevel/0.7.6/unchecked_lowlevel.sol-0.7.6.zip index 530d247c4f4af07568e1b56c088632f1a7807cb3..bccb47182ef5c2d7ed86af2a327775c317856a86 100644 GIT binary patch delta 2487 zcmV;o2}t&Y6TuZ5P)h>@KL7#%4gk@v^853C_uPWg@p2^eZx$lZnsAfiEgQGuzhG z0;s6Wb4Q5|6{dx+_(wqs`+uP#s`ynzpK30mhG7oopuo#Vt0?FH4UZg#J@raQl-JLzY+7ij7r*zq_0meMG+y zv&WfGfaxV|I&-_35FNU=sCOBtd6JO^dj1=^^45NfSlLIr9KO73gk|FdPfyVNm$^sK z)L-cL@cEAhcYKeXUENmnLgyUHlgF6}8H8MpuqZG*!?16g__)J)X~{wzL{LzB3D3ti-ws{w>)47pY*;qlFfEvnFc=Z$URN zKiVF#7P8ueJ|GOvIB`W!#kz5BgHaL|{Ntq3I~lnP%XMtu?L!L4l0K@ChR)cfHpC4E z&ynn~poWuED)bXs_M4!OwIek`kFibhG{_6VlXI$SOf11qgH5QXG~pAPyau7SZ4E_PlvFRnEf2vHy3N%ae9X*E8T{a8+;|29FFa6iQX%=8T2-~#wx=Vo2ooBF z$qjFQFZstE-aUV9$#m5$?4DCzDWvG_=k7r~G}#a--W1WPshis!z&Q)30&v`q21wra zNvZT%;SaZTn4dZfo!ha47EyhulySKh&>q*FV^qxQv@WJ|dqTjmg!>v(;>AxJh8T8^ zx|)r@`93t!bJt?F%O`AMX96{0x^h?`;Jw=BbL@*8@xgz+b&9~=(hNUOP!G+2dmbAT zz)VI9AVAYb0eDzHiUnxMR6n)OUR2hSu8=qy47q3{cV-eD<5ioBy1nAW0$2pT^L4xx zu68a6*tt_+cC5I}-zEA%sh<_n&M2-X+YO~IcOi`U*&KJ$jxhPCq~+{30=erZ6dJe4 z6Uyo)R{MYZMZ@qVT9oVN1{gDkdgkE-GLL-q`66FoEUJ{@$x*a6x8R0c9(yEk0Tmz2 zQ=H+2uHT!!db;)C`Zn?09IxlJ9PvW%0nM5(C_0VUA-kJK?=5DAlT1iQ>-e>x_kd5h z{+&V#2zI$aR96Py`%od_@a=WW7=zL-`(|Rb-)Mib4rKDo01k?(lr+!#Pf_{}rt}&~ zq5n2J0=cz_DQLf^rDN^2hMX#Wt@b-uVlO2nPGu^%i-wm)xxz?D+PW%b*e@1nxS)SO zUT2zlM)Ilp@@r1B9tX5NXtP~-bZ+Z@g|Ij|tg}yV#sWD{e3aRlvK28Gw-Q2agUP(x zw9F`!lO2H9`rG1FI*hPxM>1-0nbstU)X;OW7Sh4%)iT9m%G0FdO-*jayeDv65=0@hCd*z|UQIW6zdS5$tR55E*;o&6Pu z=KspaZIf9rl}^RSoZOhukEc@&;$nd%!~2%}>BVNtW8n;Nh)_oi#(4Kvz`o`aH#qQQx#jmBk#T>OT>!hj z%$2bJ0D;iVIX2Ht!({YSc>ggeTWIk@E9i9bG$i}az|Xwq*}8eK(HPaRoUsNVA8unj z=M_r?;YneX2_&5T4b}?wdJEF%R@VaY6= zcC)T^_D5V#nQVjP5#0L;NSK_C+N1a2$3;J9$j7gonhT3d8(wCI(@}r>ip#benwPq2 z1U)r*v!J_S{h0l!-B>{zCG{DOokWxdDi^u7X*?P=V|x8v8lwp#c6y^Vob{l6F`xkf z61eq09E!Yp#p`0!IG^kZ+AM)MksILddj@(*ttS?lucvhedTx^mFrlh+_K`>08a)dS z*$T~3Vt~sY>ab9`cp!g(bT6Du-`fO^*!pC1jSeB8UqLOenEbBXxDG=I7IllviBG(t z8pRO&sb+4)IYky}h?eE1JG7pbZ;%g&9Q>M{o|C?$QB||uCA5}Q-{=0ho8I0R)=;ZY zaX*p%@f}J^A1~Vci8!iba~*#H*{*E7P&q6YfKJ_w$)5%_eWjlB$Tmhl}MHXz?o4!Q9`_Ec_!6yP@llb}s zfRD6dS~i>>7HiQfqub_6p>M(=6tFIMoH#s$UPij%9Ub=&MYB7OjM9`~{EB>qWMP?V3o=@(a8KRqeonL zVMB@UFPbm^hBonRP)h*@KL7#%4gfKYcUGFpfDg_H002Qb001hJehMv-NVI=b7peVa zwt;1nI}UGEJxzOrRjzXc??Q=x-B`h&jvnzoHUb}CiNC&$e(vmYbyd2E)$Hr@neyC> zo$4uC<4gDDbJY9!?!5tS2BfKiUPVG4$b=MghEXV29>DIxl&cQo00~7Sk z4-(v6GbyEfjglB?Y6gsLsEvQ&>#gue=7vaQbMDBBlADp)`0o+}eWlw-1e$i$r%fca zwTiXeK0xeKaN}$mj6u=cyu|-)Kw|33%$CAy{#&Z{U znpn*)q|6H)P*+(`c*7V^#jF0ojwf7o8J+a0K2m0DO|8@OYW)}bE0v#t147iG25l^0 zZQPHXa>->vnSgQS_Ah@{ZC778$P1KCW_sKH^cimp9SBAh2lzsQVqe6WKB#?Cia>*Y z@(wYtRm*u07fyL{DI&2{G{1T!3t~PTv_RR9RytbKE(JPpHD|6x({9VU%8nbQHA0ea zP<*4Y3|cG1oMxO2plO%Pe}@O#tdBhvee3*XubU#i-D*JTz8Qb)fCSM;M=qHFZelr# zT#J7cb`H4>pht-ao4t+K#$X>K_bH0!E7QGw>*c&5c9RkrWGX!f)bx&%Vx_oPwh3Q6 z@}?*cxZ0L$P|Nkc2s<;#BGq0B9u1IO^Cl$N@!7(K0Uz1y<23afdvPx9#RERq6*s1f zr-rK=6_3K_?@NEYNG1PDVadx)VL^x+9!fvnM%IqZ z$+du6_q2cI(uDDCd`YH&mw6$Tp;qL`2pI$Rg!+*mAJbbyQD)h^%A_9@sj%<6*8+WA zIPqk%b*O$5zU1&yScCvZ5F*Tf@V~LtSNj@2qQFvMLZ|$SniTj>KJ!fA%xB?*b=)Jc zi*RqUb2*M7RFEE%P9a&zy}*ue%#k8&D<_QT)S`dq8`;+#Y;+p3danQhU6DXQNutQ% zNi~Af>~kjyx^`JOHpoJQ-a1teV+^!tqXnUx+5Hkbdejx+7kKIf`VMfP8c?D1ionAk zMrY9nBHweFi^0Of%zwwo#Kfaxz@^cM50$Yfyl~G4Va>GGZ3a#*`Sk4c{fx~>1_vV? z8!dlrXj8i;;`@dW@O7UU)oT` zECH@acO=t*ND5uTf7`h|25`D zy|`6aAmbE)`Bt1lDZ(zg3^+y`(NE3LR3x&qHGJz|s*K2KZYHZ^Q9CuNo)1|x)(L;= z3bgNpAhh}q=2)ec4FL0*KaRHBZDh)(u%%ET5ng5LhDNC>u zGxx!mc{UpI&HX~(v;8I(+BkozITm-GoT02G5p8v)Wn+7eNL7bisQrda*^&a?Sg$@* zbmZ^tTgd+n=C{RX+*g4EXB$c0vF|Vt-VA_huE~R8C)oo6PGb-u2-lAmpm#A<4-!WC zi-p5X&D7xsDv`cB^@oP^^*BbT6$Tz=9^~aluocaw1&ROtV)OmQZB&1Tw|k-48jWc* zdm8qNpQF+T0=c_UnbX`^bKwL%rS4RvQh#AT1{3$3bHdQ?q0hlm(5@k5Z2*7gO)Xuk z)FZDK9BbB^fvysHuBbcJ69O6ZizXFDJERd7nffXId`adkJ_|S0+x=Elo@4CC t|M?kqp-@W!0zU&k00ICG05OesR+`Fy56%bx06{sEZ3<2X76||V004VRDhU7p