From 69d3c257b4285c0935c797ff99f52cb44448564e Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Wed, 28 Feb 2024 17:36:49 -0600 Subject: [PATCH] feat: add out-of-order-retryable ticket detector --- slither/detectors/all_detectors.py | 3 + .../functions/out_of_order_retryable.py | 143 ++++++++++++++++++ ...e_0_8_20_out_of_order_retryable_sol__0.txt | 8 + .../0.8.20/out_of_order_retryable.sol | 82 ++++++++++ .../out_of_order_retryable.sol-0.8.20.zip | Bin 0 -> 5285 bytes tests/e2e/detectors/test_detectors.py | 5 + 6 files changed, 241 insertions(+) create mode 100644 slither/detectors/functions/out_of_order_retryable.py create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt create mode 100644 tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol create mode 100644 tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index fab9562d2..fc33ddfd3 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -97,3 +97,6 @@ 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 + + +from .functions.out_of_order_retryable import OutOfOrderRetryable diff --git a/slither/detectors/functions/out_of_order_retryable.py b/slither/detectors/functions/out_of_order_retryable.py new file mode 100644 index 000000000..b273052fe --- /dev/null +++ b/slither/detectors/functions/out_of_order_retryable.py @@ -0,0 +1,143 @@ +from typing import List + +from slither.core.cfg.node import Node +from slither.core.declarations import Function, FunctionContract +from slither.slithir.operations import HighLevelCall +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.utils.output import Output + + +class OutOfOrderRetryable(AbstractDetector): + + ARGUMENT = "out-of-order-retryable" + HELP = "Out-of-order retryable transactions" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#out-of-order-retryable-transactions" + + WIKI_TITLE = "Out-of-order retryable transactions" + WIKI_DESCRIPTION = "Out-of-order retryable transactions" + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract L1 { + function doStuffOnL2() external { + // Retryable A + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gasLimit, + maxFeePerGas: maxFeePerGas, + data: abi.encodeCall(l2contract.claim_rewards, ()) + }); + // Retryable B + IInbox(inbox).createRetryableTicket({ + to: l2contract, + l2CallValue: 0, + maxSubmissionCost: maxSubmissionCost, + excessFeeRefundAddress: msg.sender, + callValueRefundAddress: msg.sender, + gasLimit: gas, + maxFeePerGas: maxFeePerGas, + data: abi.encodeCall(l2contract.unstake, ()) + }); + } +} + +contract L2 { + function claim_rewards() public { + // rewards is computed based on balance and staking period + uint unclaimed_rewards = _compute_and_update_rewards(); + token.safeTransfer(msg.sender, unclaimed_rewards); + } + + // Call claim_rewards before unstaking, otherwise you lose your rewards + function unstake() public { + _free_rewards(); // clean up rewards related variables + balance = balance[msg.sender]; + balance[msg.sender] = 0; + staked_token.safeTransfer(msg.sender, balance); + } +} +``` +Bob calls `doStuffOnL2` but the first retryable ticket calling `claim_rewards` fails. The second retryable ticket calling `unstake` is executed successfully. As a result, Bob loses his rewards.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Do not rely on the order or successful execution of retryable tickets." + + key = "OUTOFORDERRETRYABLE" + + def _detect_multiple_tickets( + self, function: FunctionContract, node: Node, visited: List[Node] + ) -> None: + if node in visited: + return + + visited = visited + [node] + + fathers_context = [] + + for father in node.fathers: + if self.key in father.context: + fathers_context += father.context[self.key] + + # Exclude path that dont bring further information + if node in self.visited_all_paths: + if all(f_c in self.visited_all_paths[node] for f_c in fathers_context): + return + else: + self.visited_all_paths[node] = [] + + self.visited_all_paths[node] = self.visited_all_paths[node] + fathers_context + + if self.key not in node.context: + node.context[self.key] = fathers_context + + # analyze node + for ir in node.irs: + if ( + isinstance(ir, HighLevelCall) + and isinstance(ir.function, Function) + and ir.function.name == "createRetryableTicket" + ): + node.context[self.key].append(node) + + if len(node.context[self.key]) > 1: + self.results.append(node.context[self.key]) + return + + for son in node.sons: + self._detect_multiple_tickets(function, son, visited) + + def _detect(self) -> List[Output]: + results = [] + + # pylint: disable=attribute-defined-outside-init + self.results = [] + self.visited_all_paths = {} + + for contract in self.compilation_unit.contracts: + for function in contract.functions: + if ( + function.is_implemented + and function.contract_declarer == contract + and function.entry_point + ): + function.entry_point.context[self.key] = [] + self._detect_multiple_tickets(function, function.entry_point, []) + + for multiple_tickets in self.results: + info = ["Multiple retryable tickets created in the same function:\n"] + + for x in multiple_tickets: + info += ["\t -", x, "\n"] + + json = self.generate_result(info) + results.append(json) + + return results diff --git a/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt new file mode 100644 index 000000000..4f07601b1 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_OutOfOrderRetryable_0_8_20_out_of_order_retryable_sol__0.txt @@ -0,0 +1,8 @@ +Multiple retryable tickets created in the same function: + -Y(msg.sender).createRetryableTicket(address(1),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#62-70) + -Y(msg.sender).createRetryableTicket(address(2),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#72-80) + +Multiple retryable tickets created in the same function: + -Y(msg.sender).createRetryableTicket(address(1),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#40-48) + -Y(msg.sender).createRetryableTicket(address(2),0,0,address(0),address(0),0,0,) (tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol#50-58) + diff --git a/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol new file mode 100644 index 000000000..afd4d274b --- /dev/null +++ b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol @@ -0,0 +1,82 @@ +interface Y { + function createRetryableTicket( + address to, + uint256 l2CallValue, + uint256 maxSubmissionCost, + address excessFeeRefundAddress, + address callValueRefundAddress, + uint256 gasLimit, + uint256 maxFeePerGas, + bytes calldata data + ) external payable returns (uint256); +} + +contract X { +function good() external { + if (true) { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } else { + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } +} +function bad1() external { + if (true) { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + } + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + +} +function bad2() external { + Y(msg.sender).createRetryableTicket( + address(1), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); + + Y(msg.sender).createRetryableTicket( + address(2), + 0, + 0, + address(0), + address(0), + 0, + 0, + ""); +} +} \ No newline at end of file diff --git a/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip b/tests/e2e/detectors/test_data/out-of-order-retryable/0.8.20/out_of_order_retryable.sol-0.8.20.zip new file mode 100644 index 0000000000000000000000000000000000000000..1baadba84d201e0a4cecf23dae4f0c2af3f8445c GIT binary patch literal 5285 zcmb7|)n5|;qlO2J7~M#BZFCAqNFxG6Vjx`;=`N*17)W=AGzdybP8i+YjR-hEx+Ko` zo%?ff-uL49y%+C4@IW=OfbsxL05O0&&eTZV-$Y}N0sx4@#sCNa0038SFAG<=g{z0H zori^ootKBdm9>+dfTydIg`J<9tB03`m4}V}dmlRi2TxZQ5D*(+1^|Qr0J721??mrK z-HWEjn`MY{`&{00BAFsTzoC<3UhwRs7Cu!Yi7ZVshb?>nkh=e++Rq_HmF_BpQ*w8x zOm{vCs+Mo1n_~wQhmW?9`osrTXQ|_pl9J!6G2MIs{B->jOe&i{8p}rDXN_fe)h?2PJ&wp*b z7;;;?x(!?@=e_^&y_Pn^R8-(^q+&iS?E_w#XV`JGzEkGV>}zQM)v zUr@~FpZj3%eK8znN@o9Cawbpf<&{oCeRZe$Re{zxLGRax-BTe*JEe>fHakqxYEf{# zr>2JY*Aeo$MAN;0mQNPibgW{ngr^rFh1Zts$1;q&Ea-wQ4ZXk-_sYtVxfFPT>#R$~ zD@pVoo_m~ozCu+y@^xvCPA8vRtht4;WL;}&YDFrdmX3y_Knh};UYf~l9h0J%ZwydX zr=`)6?|bY!ouGIm$;y+kVGeMGuXIgoftE2 zyE2HR?el-HgVq;-`(IH?l;)1@C~cWcONd*HWB8B+5uhzy+A8o_w7irq2-LJ?>l_X^ zefJAH4N6UcXRzx(HISGJ|)P~OH8W89L8^-bOU@D*0GQhnCtrP&gJ z$Qigm@YMGvB2K(ltKZjda>QAVSE;*FS{t0f3T!Q)BG(WDR>tuH&@;CLo1kiOna?qc(UXgBC^6(F4~#9k;kOJpj&->0`wf{F{F+f-Q_fvKgv(NEFB2=NDVeN$iyqL9{U@D)ZFYFg{AvgzyGJLANm-ta3ca-Sp ziF3@4np;ys^y#&+Wbx>K1(Dy>WJE0bew<&rA~tJwrBNI>vPX*vC-dJ>mezu*V=oPR z|9UYy$cxiEmF0;&k@YB2<7Cv$uwskzDdYtX3?49TA9Js8V54(}oV^Xie5&k*Sqdi5 z<<%5KcFabd5enEM#^qU|A>3jFEdDeg7!dWO|JIE( z9zEo|K3sZ4s4L)D#1eub)uR3*y?-D=#SL%_$Ki4bI|c8JZnMJT(-=9VMUVBdy4jE2 z?@P13V-l9IjQS*J$f-6qtmG62iL;j_Jxldjyoe%L1;j!6bSZcFSZA z%_fA#DT~riu_{LIJddr5OujC(hhek8Sbm6xrAcmGMQ`w%oyN?7WZp%~Qf{sse%!-Z zLTG#Q6O~rEQ`g~#PFVWsT^130v{mrGY6=tbP08on&sQCl$MI3_Y50Ldx4#ANGY6_K zy%~kaQs-u-4=Q5@7p4i&w~e)MV_+o>bdFD+GqvVy2w(C`sOu%$8rrDX#pjI-{p->T zj~nEv_A7EzrhX31vivKex89L2k)tY~?~@kGYhP2&c*}haHk(^t_A#eM&J_|+9s_X5 z%1H|e??MDAXE;bH<%Zslmf3kLq4+#%5_p&{85}k$R_zF}G{2)^>p09Gg)nK#tiE|9 z7rak3`o#pOfjV~gvIZCX%d;g($KZ-`42HfOhktJlqnDI?cv1QnkWS3~z~WIENv~HB z(u~PuLx4mdIfj(5b%4_nB*#DRNK-bO&sGfa@ z`CJ>U-t9(2sO6y?baM6KRhao?hhvb8@@!cS;p|6N~xv^EqNsB4W9A+Q&dtf|R)nKNPjpJ%h z5#oj(*b=zE8KhT{%7%C^HdX3oyl;`QWaL4}bvj58JdE207!1X=#J3xXgPTQxwzC9Z zPr@Ngosxuv{l5Kr)R~-^uTv(pRj3*+iw(aeOSW(BWRO&f%7sTaGp48l0n3GB-YO%IG_O#dHxD~RGX`A<&17_uCq-2udFGz+%GpGf8+Uu- z`mdhQQP#8Zu;sSvDtm|4`&hYpTeJ5v$60a>ix@phj#2MCmfE1^gsVzc*dwKUZ#G6N z9shtPD55bDLO)rErpW$z7I{6kdeH_G&4npEXgEw4Zb^Rk&~r(uRgCm8adGX^0nCIZ z<_BQRJ)Fw)$MbHCN5*Z7?jVkJAv^5T{A@kPC~l~gzkUxV&O%*a60V^cQP9o;OeTE;ZYJ(&rLTX!{u zoYa<76Pz~ih&7Z%(7*tesR65&o0LD&oyu5{CMM{2PLgnX*T6mT7ANGQ-wqpeNvRgm zR#Vr}$v9kB{Y6f&Vc+M>ur?=*OXt%R8yTYd_Z&g|+S$m&m zWM5w6X}B{IxPebC3zdB*_vMv40ens`(pGJe2Fo zZ6~sAPqv<;*UjIabc8~FkzErae)N4JO*xpDq01MvT7hSAt3GYfYBLlC^Y#20Z!i;2 z&5))kQlY)7X&FIMuY|0tSC=E=o;NH3J;I9;`aSWM<+}K8EWlr5sW5aZ7D91}XNMa% z+cB?Rk;ZoZ_SwAhbsJlXXPAg{01Dwpbwi3Stqo)pL2t6~APOp^aOuy!xZa%x17}>S zRZK^J{xwmpOKImmizn54DREM>;nIONboh0`LRm-%L(=-HntqMbRO99t!{zw9Z<)f+ zmp?CVJ?F>l7vu=Md3w?r4OElN-?I%UJaT$lYz2;Zn6YfsjUOX0K!5J0YwpOtcL*+z zDC%VmwW{SnczufKzx|z3!cYTx;nf1X*Rs)T_r%%j+xF3*!L1sg4oXw2urObas6d#I zlg*t7F9|j`HQWaY>G>^w@u%X@bp2{eI;Y$O4kH*AlfvdVR^rJXyQ?1{fAfT4;kO7= zU&y3`?ug_xCY#gB8J7PkW@VBxp4vNXT@(@8Zc%TN2j^k_**&6lX$xU8vawMYelfK- zXv8~k50&q_7O$U##Pp-G!`)Z}aB%nq2DA1ibgwViHa5(HdqK@wt#Ie?{(JSypU&dc zX(gXCq?6N~ z4CG88AIZggP)VV1>&$c3^MebvQIq@PUUy#)9R5Zq?qutj_}VD3Ey;6X*vT40a6`gsU&JE=*gsT_aXb6a zMs?CGm3cx;e3fh`uK+ZaTuD{2C~UGp11hAUwrv1MocEWO-e7{?yn(aSb_Q)5U1o{c zS_qhG!D$&;51P0`F8XB%Vn2%|EEDuttgecL>wX2!@)VPw4f)_e()B9>V!fL4HLZAM zU6`ILt8A#ppw3ZK(w{EJe~f!3G`wu{b{%Swh#XxxTS%wt7r-r*(e@bf6Lb`-qDD8s z2tm@B1Neo*gNgki^2~5LjfB`Rtk-w_s`nRJSb=@_sfg{W`*n>{F-O_`2oP_%_ABi) zl(Ct-eH8Aan)C}Lhx&KvK8o+o=Rn=ODEZKersnGawjfIusPs4S`obTZa*aQlL`g=F zmbesq(Z>0v3c}JbUj^GK#PI^h@`qN>ir>ONs|da%z_5*%3zuO;h}8*lMnU# zqFj-MS~2oZP-_%pm!Yj#JNP)UEM;dbdOmBTAJM z#qn%zMSd-ELU`|4EY+6rX|5W3d)@6~4~#^ObCoC2YB^ksvdkzryTw~`GIS`ac6Brj zcFJs|j>wxm&u-NunDX1)?BxN#rW zfpJL}xA||K9soEsc{$CEQitnGP$)doAQQr?M=;a;vR&AeK;s+CX~_Ww5p%g){~sh z+&3^E1);_Kq^`7Zs6%UvW9M(%`nlsXUZ|Yho8e!Zm@nB6f$20TYiLE9FDcsZmp-k9 zH}jr}NuDoxq2y(EhmxY_dW$gq?r$3(i8^}stUQzDItt%&Ssc%Qwp)hXN$g!WN)E6H zef4BiT_#c_sP%#U^5ap*%4}bB-eD^OHbM!EAKXuZp|9Hj@z-}TUH?*;@F(c9VlZ37 zg>O_Uyz2$A%BF^y5*9Ryj8n+1=X_GINH;)^>lu6L&^X#2(PPOAoU)y}8EIafOigT} zH-{CiZHlI}epVyh*98eFfq5?>vQr7UVu}s1hl_?^TPV<3!sW{j{8GZnkl`$b_+C9< zruA9%fG+G&JbudOZ2x`_;kgH7(umsRNm28l=UeltsxSY>#~iu1SP%#&mOWpCd)aIJ z4wr7uC_NBN&!--@N-)oF5H#89eXETu(chk}dMh$A(iPQ2vymUD^1VHNm;BF$5A7^{ z#K3qE0<8cluKD?h;<;(D%$zpP&QXhK47lid{b7=r02Z7zWjwcg}D<151s z30jvFptvUIZBxe}vjJ|=;N>wpX93hB52#7l1&6V)`M92)@>8|D2xTLIqLl%u2vmNS z{bi>4c13h$3U}!>C@?E@zctAWI+<#6-;nvCm>`3}x?_JX~n}0(dW(ZpY}x;_oJG&u4`>ZBhHa-u1wMMi3fp=0T`YG)A5%mvuGGUKTjA6d<^gfOmVPmGB~yrQ4c3F+=kI+)t%2dEcjy@wEK9(_F>AYVFP*Qp#b_<53? zj{8MN@@QGxYTBMeKi!#tXClEDzXX_E#)Az44b{ZJl*jtNrQv_P;r}KC