From 538539ba9e9eb97a3ffbba87e181cb8118d40df7 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Wed, 11 Oct 2023 10:32:42 +0200 Subject: [PATCH] Add return-bomb detector --- slither/detectors/all_detectors.py | 1 + slither/detectors/statements/return_bomb.py | 123 ++++++++++++++++++ ...r_ReturnBomb_0_8_20_return_bomb_sol__0.txt | 5 + ...tologicalCompare_0_8_20_compare_sol__0.txt | 3 + .../return-bomb/0.8.20/return_bomb.sol | 57 ++++++++ .../0.8.20/return_bomb.sol-0.8.20.zip | Bin 0 -> 7966 bytes tests/e2e/detectors/test_detectors.py | 5 + 7 files changed, 194 insertions(+) create mode 100644 slither/detectors/statements/return_bomb.py create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt create mode 100644 tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol create mode 100644 tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 97622f8878..fab9562d20 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -96,3 +96,4 @@ from .assembly.return_instead_of_leave import ReturnInsteadOfLeave from .operations.incorrect_exp import IncorrectOperatorExponentiation from .statements.tautological_compare import TautologicalCompare +from .statements.return_bomb import ReturnBomb diff --git a/slither/detectors/statements/return_bomb.py b/slither/detectors/statements/return_bomb.py new file mode 100644 index 0000000000..8b6cd07a29 --- /dev/null +++ b/slither/detectors/statements/return_bomb.py @@ -0,0 +1,123 @@ +from typing import List + +from slither.core.cfg.node import Node +from slither.core.declarations import Contract +from slither.core.declarations.function import Function +from slither.core.solidity_types import Type +from slither.detectors.abstract_detector import ( + AbstractDetector, + DetectorClassification, + DETECTOR_INFO, +) +from slither.slithir.operations import LowLevelCall, HighLevelCall +from slither.analyses.data_dependency.data_dependency import is_tainted +from slither.utils.output import Output + + +class ReturnBomb(AbstractDetector): + + ARGUMENT = "return-bomb" + HELP = "A low level callee may consume all callers gas unexpectedly." + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#return-bomb" + + WIKI_TITLE = "Return Bomb" + WIKI_DESCRIPTION = "A low level callee may consume all callers gas unexpectedly." + WIKI_EXPLOIT_SCENARIO = """ +```solidity +//Modified from https://github.com/nomad-xyz/ExcessivelySafeCall +contract BadGuy { + function youveActivateMyTrapCard() external pure returns (bytes memory) { + assembly{ + revert(0, 1000000) + } + } +} + +contract Mark { + function oops(address badGuy) public{ + bool success; + bytes memory ret; + + // Mark pays a lot of gas for this copy + //(success, ret) = badGuy.call{gas:10000}( + (success, ret) = badGuy.call( + abi.encodeWithSelector( + BadGuy.youveActivateMyTrapCard.selector + ) + ); + + // Mark may OOG here, preventing local state changes + //importantCleanup(); + } +} + +``` +After Mark calls BadGuy bytes are copied from returndata to memory, the memory expansion cost is paid. This means that when using a standard solidity call, the callee can "returnbomb" the caller, imposing an arbitrary gas cost. +Callee unexpectedly makes the caller OOG. +""" + + WIKI_RECOMMENDATION = "Avoid unlimited implicit decoding of returndata." + + @staticmethod + def is_dynamic_type(ty: Type) -> bool: + # ty.is_dynamic ? + name = str(ty) + if "[]" in name or name in ("bytes", "string"): + return True + return False + + def get_nodes_for_function(self, function: Function, contract: Contract) -> List[Node]: + nodes = [] + for node in function.nodes: + for ir in node.irs: + if isinstance(ir, (HighLevelCall, LowLevelCall)): + if not is_tainted(ir.destination, contract): # type:ignore + # Only interested if the target address is controlled/tainted + continue + + if isinstance(ir, HighLevelCall) and isinstance(ir.function, Function): + # in normal highlevel calls return bombs are _possible_ + # if the return type is dynamic and the caller tries to copy and decode large data + has_dyn = False + if ir.function.return_type: + has_dyn = any( + self.is_dynamic_type(ty) for ty in ir.function.return_type + ) + + if not has_dyn: + continue + + # If a gas budget was specified then the + # user may not know about the return bomb + if ir.call_gas is None: + # if a gas budget was NOT specified then the caller + # may already suspect the call may spend all gas? + continue + + nodes.append(node) + # TODO: check that there is some state change after the call + + return nodes + + def _detect(self) -> List[Output]: + results = [] + + for contract in self.compilation_unit.contracts: + for function in contract.functions_declared: + nodes = self.get_nodes_for_function(function, contract) + if nodes: + info: DETECTOR_INFO = [ + function, + " tries to limit the gas of an external call that controls implicit decoding\n", + ] + + for node in sorted(nodes, key=lambda x: x.node_id): + info += ["\t", node, "\n"] + + res = self.generate_result(info) + results.append(res) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt new file mode 100644 index 0000000000..f53b5233fd --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt @@ -0,0 +1,5 @@ +Mark.oops(address) (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#31-55) tries to limit the gas of an external call that controls implicit decoding + ret1 = BadGuy(badGuy).fbad{gas: 2000}() (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#42) + (x,str) = BadGuy(badGuy).fbad1{gas: 2000}() (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#44) + (success,ret) = badGuy.call{gas: 10000}(abi.encodeWithSelector(BadGuy.llbad.selector)) (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#47-51) + diff --git a/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt new file mode 100644 index 0000000000..76685043ce --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt @@ -0,0 +1,3 @@ +A.check(uint256) (tests/e2e/detectors/test_data/tautological-compare/0.8.20/compare.sol#3-5) compares a variable to itself: + (a >= a) (tests/e2e/detectors/test_data/tautological-compare/0.8.20/compare.sol#4) + diff --git a/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol new file mode 100644 index 0000000000..76413fdcaa --- /dev/null +++ b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol @@ -0,0 +1,57 @@ +contract BadGuy { + function llbad() external pure returns (bytes memory) { + assembly{ + revert(0, 1000000) + } + } + + function fgood() external payable returns (uint){ + assembly{ + return(0, 1000000) + } + } + + function fbad() external payable returns (uint[] memory){ + assembly{ + return(0, 1000000) + } + } + + function fbad1() external payable returns (uint, string memory){ + assembly{ + return(0, 1000000) + } + } + + +} + +contract Mark { + + function oops(address badGuy) public{ + bool success; + string memory str; + bytes memory ret; + uint x; + uint[] memory ret1; + + x = BadGuy(badGuy).fgood{gas:2000}(); + + ret1 = BadGuy(badGuy).fbad(); //good (no gas specified) + + ret1 = BadGuy(badGuy).fbad{gas:2000}(); + + (x, str) = BadGuy(badGuy).fbad1{gas:2000}(); + + // Mark pays a lot of gas for this copy 😬😬😬 + (success, ret) = badGuy.call{gas:10000}( + abi.encodeWithSelector( + BadGuy.llbad.selector + ) + ); + + // Mark may OOG here, preventing local state changes + //importantCleanup(); + } +} + diff --git a/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip new file mode 100644 index 0000000000000000000000000000000000000000..4c10ea6fea0dfc804d3fef44769721e1be8d5765 GIT binary patch literal 7966 zcma*sQ(GkfqXpod?RK_pdskCUwr$((Cfl~{nrwS&GAFxfvU$Gm+@I$;YhA3H^$%WU zIVfmR03-kv@LNqzOCO~u!IK04Fq#1XC;zUK(@gQ~g8 z!^+S0#TsnlnD!aeMYZ@WfC{tA^Y1sYmiuN^|p4x<5Tz{bd`2Wr>zk zK1ms)$QGx85!gtPREbFCvh9Y%-IOi6#AARy_$xEp02d!(Hb9B*y7F$-@4U9ds%MZR zwRi=u6@x(@Xe{$k8WWAEX$k|q|1K|NQY3>66h44I(kE@fR9xQ6Nm@VZbPtp{6??%l zq3Z$934f?S*z}^0VIG0n6Fb5^g6CkIMthIyQF9z%9pKF#!kUK&Izo$t!`0Z=31hct70vJE-ZX zTVTb%X>mDzu^2GJ^JmzC@458{k21utN>6rboeV)b192en0`81lj}cm0a&b#&Piss` zg__evwa=Bb=5DZEab}?16F+T_o{ ziPxm-KdEQYPt}XVgqC)-%d7pla!I`@SGlPg?DQokGnAG<*7C$p46qhTK{ z#_sCLi`l(?)%?VqOhz&qodC_TC57uKS~hC_d|ClbTs4MH%kflDUYI%94d@+n2f9~^ zfmi?%TJ^N*IrY@lc`4eZQHA;7G5~I30%T zpz42m?QD>KHeC(=JKmSe=_lJR$%?J z-e@{x&qPPy#Xo$WPL(i~9sDv+-v`F}st!|Myzb3@`=GlZe;!w}S-+gxnl>3t*G-cx zhey(}28JS;ugMbSiqlvgHBB_t(HXJ8D0G*|Sy|Ozi$;qQREp((i4I;QCvZeLBp0)l zk_fOhzrEBKo8+8B;^;+L@?-=7P9!J7EvTr4de(aPW-;F$8QgN*b-GskygY_Wl|DP* z*vRdHG4V*jKU;doQhunSC<4dXHO^j2oT?T`H#qIplAWdnysb46v;X#VgaDG#+1pv1 z5=F}_O&P8ij8)|LMhu29wbztC_)0~%gWHBr!D_`WcUd`Uh$9rrZ~`(9XeC~0 zlw#0^6zRW`WQ^>4Iua8vF_!-dW%6RUi;)59A2MBmOyZaPvK2dBBEHKDjf7lPgkW9r z<$9*OxQc#jTPI33Ag|Y@){0tGya-}J;#lDQY8R;7^7q39$lBvPx^KphYMVUt0E6dk z;I6stq@&Va91~-*7p}%jRFU_|4+d)vWMht>tbv2ur1F&7ILGde^!h9C@ZbIV-qws* z1{c&Gz5758p;~sltED6!BC4if{!T}nDuT-Q@#=RIOxIVgG+o1y)(diGp}$dY?6bMg znLVnzG`oXb>;9SV2*0cW4%4{rnhmObPBYkbqV235!r#4n#3dcMr->^OB8^*oXU^drM^y2L^M#aq0woFuH@T zooRe))xun8^-raXroBT-^9IMm_m2I(`w*pV^xz(v*z_i!@SusB^C~bD?WlA9>5|6X z%@<5EgyBeC=os70NB=I;oz{Xecp&e;F>o|t_Q09T+RHNx3G~7*U)kMWS%<`JCmYRr zK5%wWN@jH9+~lE);y6juu7-M)%@?gOgb2D=r$}AuFV1%xDlQ;<`!bgBb0X5UE{8BG z3Cz}MnsVZv6XzuMrQktQLAAm`Y&g_1Q9o3pN&aqv3#j-VuXuEuu&njl1PrHaQfYzn z0v9^=Km~!@hJL$xDswGU@ooP zSU8N^%u{PINytOksbf#Sqy^_We2g12iJ2+)W+1J3WjGROF+*DZIpsP+R`NM?Q>zgVjlbN+@>~Q|uU6>ALmz z*t>IH1DPtcVTa1HcpDRHM*JF$MElrPDND(cb%jQ|MBvuEmoEHyS*ujPR|~e=jl59T z1>GbuFu+>P4!lr+h|3iV+bTtEvEvT;xPz1z_|1zTo9O~h$+>78oj8aNTf0Q-t9q0% zJ=3lS&nm94>xE8Q{ul1za*^pqqa!EF?)A5?aM&fKij(LDvh9k!qrx9t-di>H5(2D zaXc4V^AA^z8`mZ$+S!vEN)iVLGJCH=wB^9i5UvXo5=D$-!mk@C1$gGUs|J`G>Zdi^ z!chMz8Bnhs7o4?ro)`cQgKptLk)V86kTmnDX$C0+XP^$P+addvr^p&WL`hB~sp~c$ z#a}1R1UuM9V^K>dVh(^A!SIY9nx{+jdL%rUYg^RH? zb8|5wFzQ`Np-XrdrB-l~P#H+$%qv+o?Qd*qN5HSKwXq6Dd&-ULvMB5VoKe+mV{_Cu z>0^F3a$e*{OxuDrGYX}~U!v=3qG^?^eag&A10<5r`JLy54K+5e>}0|R%p3$qsoL9D zKF2}1$#s(P;<}IdFO&L{-oE!^p;yz1RY(rBIM_$iOdS=pBM(>4TE4h47Y?0EDa1L` z+oOHPK|}8y&}}Pwuiw_~zYzwYzrEtaet|c=+(B8-c7$D@S5h@K>W7f-PQ1M5xz@LX(9mq9tRk(>o0 z474slS|UF9R9$L*DZLbSx(4^teNw4eEkIC z#Ywj+vLLwHC{x5t~jsW3@>=P`E;h?IZ%E`gGxv_y;a86 zLcKsP2A2z|JxAff^k6PE+;!s?rcUVKD70asnpu#wC!hcVmE;RRw2%hQ}2V;v(7)}W{@58LN z>2br}O*$rhy-tf4!Tcg~YfwsIqvrGxzsdon{-gaJ;%zx&oYQCBB|dj+bylO+5dR;_ zYS6Z!C$z6Dfw)k5$a5 z%XbBg)mESxun96#Goue1*|oZyaj~#9iw@L|IqLb} zK+5?DUOTRiM-w2&Cln9e`0G5lHlJ+_T5%yz5SH~9XMK#uLS6O*L@CXFO_|An{XF9w zU);X1n_q&`=uXZ1FZcP=AQoJnts@zMh+#H}TlqX;P&1%*brBshYmH`@rpuS=Jk)Oi z0gK)5*^eQ?XCft1J$lcY%45JpJD~?!{6{sA3Bq@y!$~pAh3y)L_5tk#G}TJM5i|P> zWRDe$UfH4!{}!%v2=t@;8vcuQzE=jn%sjv( zC=C7C50cx1{FvM^_0dcE-NYE6TbdQEiwj)+mJQ^a@tc==lZH^}nKRnKthH{C(o-lB z6(3K*fT~W;-Xl9g!gk_yoHijJs87b?({CZ%-O%rh2TDP zN2gGp#nN$|;9z4Lv?M?U8S#(fyTPEY8mkt+PkVbK?L>)W`gEu5NWG!on+eC!&Y_ zv1QYSr)1TG=xA0!1^T!61q8c4f(A5Nj$eA;yy#cUE>FBKuPU9wawo$EIcspz6n^)? z)9Q8kd5qgH=c7N7yC4SPQ7yQ>BO6FZd-YV!-(y#!3DsPGu)T(R%D;K)ORneP+_73Y z3vt1}b7>rg$n2k7sDZMR118EOHJxDV-;E zhb^4xdsR;Et}KPV?{}_%Q-UfyL#HHN@e?^FmZJ@_FlrQ_!e|>0+Qw0&MeY}h!*1m5 zGCPFboju~bN7ehD%4H*8$2(m4{#~o$E*W*~=b3{)&7`Rd+rV=ccGhuCDB96Q#HAuU>+nk5OLfz*H8zF5N=o7WeVxY| z?6g#Q^=}9wMqPNcCvUxy~x5cQ&@*6a>u>sy60^*d=j{?5aiLObr$eV5tg}*cG(Ujj_b`EB|~` zUNBF3wdeWvFdxfjY0tm;QCh{4DbO;1Ba1D2pLC1k41w>i&J&xW(8i^p+HiI^Iz4F; zO~SANE%kNDUbI2l@NXAiKCEp7$H^RCq6nQhr?620wb&=0fC!q{i;QPEIc2dg(F;>J z@C5qQdk`WzunHZ7`F%;7LvhW(U5X_{jSf=1kOGi+3Jca+FxR)SiPtbNkI^qZNU*UY zPjcp3nb6qGx9Kof`}cLjD=vgWA0Q$(_<^&^6my zS;eT|r}+$D!@Zv+v2**~_GWkT`rWk)Ej|a6e+bQ3RI`LL5elCoLv6O4^S4KHd^^~| zFlhNLRjovH!E|BfQ6ECNuES)QxFZ%S!g7OIlJ?Ii7oV@KW_+$gsNg1skq&~Bs1cvH zsr-)R5YfJ6dl1ab9;C(DV;+)NQsG4g;64xD|4GrN1ClvDhGLj?h@|0Fh&A(@A8!Q| zT(}PZUvURrF}y<<9!TvK=^X`3_j#(7T(t)xjio-&h6qpmY5r3Pb+j~g$=j;*ZFJd@ z6%q~cqGMmwPIA0mSdVi_x)Hw5JRualo8+ie&&Cah>CE(^)HR~=`#-eAxoJqqhr#4p zRPV_di$YU9IrKmr?~xBak8KKduGiXCY*)w9i8FJ-DcYT z#|{Ng4OT44)|H3zN^$H;a=8g6Vm~l$lR%~jp~m?!ZnauIMp)B7*u>N!0WpEF#)12N z{^nlRP-5mB_k{t~=FsQkIxR!Ofd1}h1mD-C)5Ri^OBU-nC2c|bekz){cNuK5oYGnXaHW0_dI z)6TJY5{H6Ju=woa&$A7Ng=Km3xP$H>MF<3XKH(Gs#-#&qrt@qyA2B4*zhw z;Me1*K=R&r)*xo;P>8Q<+byzd{;RG##Nqt2sx!Pm-b$S-Q%+6R^hVG)ku_m@u(;>u z!nWlImuy`41I4%X4e7;d>Ap;#iXK7h03dxX=`-O@D~Z)&ll#XwDF)EPLAI5TYF$Ck z%rbx$CT5Izziej+vOcbEv&S#pRkWm?0+jqi#?{Hgg$|-l5@LX8z#IJmK1Oe0!-N9h zOY)5iQD}%MrFEs~RA{!j_H}NLu++@M&r?VAAo_iZ%s?CJ_o4;+(|h9IxYLsYshSDWdBLn|)`G}8M?PyFoo z0l+@s{&QB8yVBP%bT1Jr^TEQd|1N&@QR+ZJ89Ou7BZT+NB;%T@4>tSL`#oL=6 zwPeZTYrJwx(4`A$s!QcKkEYdZSY%1^2+<8)FI~HmBx79w=3HXLQJOHxMU5x(3pLBJ znufvb$~sWQXj(#vVzBUg7zxj-ENS0|AMm+=#E0LMKWUB=*DKtGiL8NGcw=W@FEFw^ zIzm;WdSO@6I5nH{58tNa$|U*e1I>=Ed&Azy%KK6 zF&6oAJ+mbv>@39Ub|;}cN(Rua4&^;Y6ptyHDUS=DsHfa zo6tzQW>@=E_QSsEL(E=xr_~b;rnA2>9kjLj*Xsq~;@Xe|D1oPUffI|vbKMzI@aam2si1a^iT5~ilnJgTfJ(v-+C4BQT+M}@McAr zf}>H)YT@|t9cy|01tee%Sz9Ws;TPGco49YLx>RH1DToL#i))(y!Kf7B`Q!$9A#=7D zS`&v*(1B%kCPPzzA>R6nK55tA#HuK3v_5)CcM*RI%MgYIaZa}p81V&>C3WIl?ZTWK zTcFI^4HG|rHBGELXh^*W?WW|b3q^EEui1&ni|UYcCE_9CB*3%-2boBo+hJP=?(k!G z!-!d$it1f{yP-SF#7(BNVAF{x{;aFcMZ!{&za!aJ%79nv(+}wv^2Q)|L95|wr`kIs z_##zH@%~0N8=MVC85NU@#&CWBMlgpneo&D3*eb!nsGXfH31G~A1h-{m;OAK~Eosq) zOm0c#4?B)hZI-pNV|EH-j&P33BpGMysyljQUEEidO+9?O_J%nLdY*7Ybgti?&{4iG zxOI#AB~?+Ly|j96NP{489r?}JDuOH7dgEUNEE}KZ>{t+w`?ObiM=O^D3w~naZ?GCN zDhFk`?J?uz^+=zn|F#Rde00`=DAB?D6;D*O71HFX@3dv7kc3`mW&DVk>^ui&5P+HA zHNJ>?53yl8_Y4c_-QN z`;_!HtP9vdF8{+vS|($gY8_=-aacu-e;Bhzy>y9)Lt|Bs6TZ+=3mV-;aE-4W{nv46 z9`o__xVGS`EC&H83iW@(^?&U8e-jAt-}!$qwz3=y?0>Hi|EbD