From 24d50cc958abe502ca481f452dbb1693770ac194 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Fri, 29 May 2026 21:53:30 +0800 Subject: [PATCH 1/2] Fix divide-before-multiply false positive when divided variable is reassigned The `_explore` worklist in `slither/detectors/statements/divide_before_multiply.py` tracks the set of currently-division-tainted IR variables in a `divisions` dict. The Assignment handler only propagates taint from rvalue to lvalue when the rvalue is already in `divisions`. There is no else branch, so when the rvalue is a clean (non-divided) value, the lvalue's previous taint is never cleared. A variable that was previously assigned a division result and is then fully overwritten with an unrelated value keeps its stale taint, and the next multiplication using it fires: uint256 a = x / y; // divisions[a] = [node15] a = z; // rvalue clean; divisions[a] never cleared return a * 100; // FP: `a` here is `z`, not x/y Clear the stale taint in an `else` branch on Assignment so the dataflow kill matches the join semantics of full overwrite. Adds tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol covering two FP shapes (direct reassign, reassign through a temporary) plus a tripwire `genuineDivThenMul` that must still fire. After this change the snapshot contains only the tripwire; the two FPs are gone. All four existing DivideBeforeMultiply fixtures still pass with identical output. The change does not touch the multiplication-check itself, so the intentional-precision-loss firings (`a / b * c` in fixed-point math) are unchanged. --- .../statements/divide_before_multiply.py | 5 +++ .../divide_before_multiply_reassignment.sol | 36 +++++++++++++++++++ tests/e2e/detectors/test_detectors.py | 5 +++ 3 files changed, 46 insertions(+) create mode 100644 tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol diff --git a/slither/detectors/statements/divide_before_multiply.py b/slither/detectors/statements/divide_before_multiply.py index 5280f926a7..eed9d23184 100644 --- a/slither/detectors/statements/divide_before_multiply.py +++ b/slither/detectors/statements/divide_before_multiply.py @@ -84,6 +84,11 @@ def _explore( divisions[ir.lvalue] = divisions[ir.rvalue] + [node] # type: ignore else: divisions[ir.lvalue] = divisions[ir.rvalue] # type: ignore + else: + # rvalue is not division-tainted, so the lvalue is being + # fully overwritten. Clear any stale taint left over from + # a previous division assignment to the same lvalue. + divisions.pop(ir.lvalue, None) # type: ignore if is_division(ir): divisions[ir.lvalue] = [node] # type: ignore diff --git a/tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol b/tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol new file mode 100644 index 0000000000..c5478593eb --- /dev/null +++ b/tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.20; + +/// Regression coverage for the `divide-before-multiply` reassignment-kill fix. +/// `_explore`'s Assignment handler in `divide_before_multiply.py` propagates +/// taint from `rvalue` to `lvalue` when `rvalue` is itself division-tainted, +/// but never CLEARS `divisions[lvalue]` when the rvalue is a clean value. +/// So a variable that was previously the result of a division and is then +/// fully overwritten with an unrelated value still keeps its division taint, +/// and the next multiplication using it fires. +/// +/// Expected output after fix: exactly one finding, for `genuineDivThenMul` +/// (the tripwire confirming the detector still catches the real bug). + +contract DivideBeforeMultiplyTest { + // FP class (fix): variable reassigned to a clean value before being multiplied. + function safeReassign(uint256 x, uint256 y, uint256 z) public pure returns (uint256) { + uint256 a = x / y; + a = z; // `a` no longer holds x/y + return a * 100; // multiplies z, not a division result + } + + // FP class (fix): chained reassignment through a temporary. + function safeChainedReassign(uint256 x, uint256 y, uint256 z) public pure returns (uint256) { + uint256 a = x / y; + uint256 t = z; + a = t; // taint should not propagate from a clean rvalue + return a * 100; + } + + // Tripwire — the real divide-then-multiply pattern. Must still fire. + function genuineDivThenMul(uint256 x, uint256 y, uint256 z) public pure returns (uint256) { + uint256 a = x / y; + return a * z; // genuine precision loss path + } +} diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 7bd6e34600..4373b44e38 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1447,6 +1447,11 @@ def id_test(test_item: Test): "divide_before_multiply.sol", "0.7.6", ), + Test( + all_detectors.DivideBeforeMultiply, + "divide_before_multiply_reassignment.sol", + "0.8.20", + ), Test( all_detectors.TypeBasedTautology, "type_based_tautology.sol", From b9484807ff40d31d5cbb4f9ca6e6309514900c75 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Fri, 29 May 2026 21:53:30 +0800 Subject: [PATCH 2/2] Add test snapshot and compile archive for divide_before_multiply_reassignment Generated via: python tests/e2e/detectors/test_detectors.py --compile pytest -k DivideBeforeMultiply-0.8.20-divide_before_multiply_reassignment --insta update All 5 DivideBeforeMultiply test cases pass with the fix applied. --- ...ivide_before_multiply_reassignment_sol__0.txt | 4 ++++ ...e_before_multiply_reassignment.sol-0.8.20.zip | Bin 0 -> 4194 bytes 2 files changed, 4 insertions(+) create mode 100644 tests/e2e/detectors/snapshots/detectors__detector_DivideBeforeMultiply_0_8_20_divide_before_multiply_reassignment_sol__0.txt create mode 100644 tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol-0.8.20.zip diff --git a/tests/e2e/detectors/snapshots/detectors__detector_DivideBeforeMultiply_0_8_20_divide_before_multiply_reassignment_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_DivideBeforeMultiply_0_8_20_divide_before_multiply_reassignment_sol__0.txt new file mode 100644 index 0000000000..f31ad2a4f9 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_DivideBeforeMultiply_0_8_20_divide_before_multiply_reassignment_sol__0.txt @@ -0,0 +1,4 @@ +DivideBeforeMultiplyTest.genuineDivThenMul(uint256,uint256,uint256) (tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol#32-35) performs a multiplication on the result of a division: + - a = x / y (tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol#33) + - a * z (tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol#34) + diff --git a/tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol-0.8.20.zip b/tests/e2e/detectors/test_data/divide-before-multiply/0.8.20/divide_before_multiply_reassignment.sol-0.8.20.zip new file mode 100644 index 0000000000000000000000000000000000000000..19a51ed3d0ee59476a2d4e4b5b870b353d813230 GIT binary patch literal 4194 zcmbuDRX`ICp!GL;G)f2zrZgj@C8V2?qeQy9VU&^*5`r`;%?!%Xox%tKr5rK3k#0u# z{qOgFpYAyi=j}Y5m*3Hc;SreW(rx$dU@e4L@1+7~TRqV##Kb)48h5A`$9u~>pT z(5fp<+Fg=7ixTsuVFq)TCd>KbNE2%Hm=CE^|Jx#bRA4bganHtnx?mbfcuF1Hj!P7* z|FTis4cCN@dh02UdHc62Ix6Y(qcq!NZ!u$iN*99L!5M=l;#-v8a=$}u$@r?{$b0AY zdB`6&*Y5Ira1*}B%Zo1g_l@MPAQ-Z_N=exbKI}%xjEGGq=J=tf_9~iS0cX{OEO;L4 zCvt6=G4-Su-KsJg3{bZREl-^QHP6v2@tAV;Wtr@;sZHPCzn@=N*(k=Gucs9Erf#w` z8+d;%(Gxuu*jo2dDl^r*$*=tN{R5%u(;}?9;%@)Zwkx`0>W|Gb{v9Li#FtY;43cQ6 zxynMDNv2+8EKvz!8k%bwMUe+$Hi@2W!|1kzu4Sg|8vWE?AMQ~WJl6Ym6ru2XBS1%atmPrE0q~CO%9}Ge!nIzgUZ$Z8QjwLSRASB|+Yuo8gRB+m*nJ;Xf>8@JORt9(w;m(jNb%=YVd1 z1@xKl9mP;D>D8`1!0)D9!3Iu9{Gx&xyH@$SplxLM*bb0Wa=qQGuKQf2i}T&+ zu6x%B{9h!+Xjn=op)IiG(oAW&ftUIys=Xx9+}Zmwd!PvPod&RH1T1fMn6#MtsvLEoG zt0ds8xSM=O`zK4rjsxpX%hnMylOFR%<`UUBGv9bH+gAlGOn<1B=DzMkMZ+_L_=vTbxHT@VfZ+0VY$M}~E|F@a6$;@F8NGIKTGKvr1TAY_WE$$>Q zV#&C^zJ@&ZJn>E?Fj2B$Vi{rUPN2z) z)$S;^3L%}iGHXdY8j9o6UoMLRsqdTCr6dtu!I+|iq}&?eG;Lhkb*_xb`{ja(?!8B2 zB1h5yWS^1GzTt+h^@+Wet)Qk0qDLu;Nx3u3e3b+O=I>9QkqnCs_T$dLHMcPRWgHg zZ1s)3z)j`pjc+YO1e>?c&`op>TLak98srm-ej4fP(D5!wFJm$=+xZNr#TiOk;Gj&e z&h8~bC+!H54v)NEY=j_^DWQWxfl{J!6{-FBybHX8*fPMF%i9vQj(y3P=GOl0%?kir zWTO#8yynQRaQJMlIXZ%OS_RW$(G;|4k&ux|m&+{>rG4P^lkha}u}q81ZhA-=;aUlN&LthbtUfi27%;e8O=__WkFk$ND0Hp1dyJ@W&tMwLSFX`cv zsq*JXJDEqNn`$b^@eD0*N;b|FISy9s1A2j|lNAbtmo&~dsdj=|gr-#;WY$@^h%t(h7pe$#AxLHTl&|Ek z8jBWXH8}KuB4QEI2x*leQ6cjt#u9!1|vkp zknb6_9tuwU<@%(_7%gTU{JDo?Q!AJHCj>;Ns$>qH^TB7#OE&#*)M&m*;lF2Lu~;9P zKG}|x9$FC$+8|i-04S!zjQs95i}<&cRS@LB5mAcC`wv4g20*=~{O)LyUW&qkL&!%**pM z2qDJa+j3z4C|LhwRipGPD5FM5x}{ao{CTs!pQ*{|cJ{A=QDv6GlLlMv!yT$Mw9Ie*ssvE^q?i65Q67+~IhG)SoW zd}Rz8h(aawkl5^S3j!mUknI5Cgh%WAe&*^B{`v%WWrycfZTfBghr<_UV>yS!5%K5h zfe->uBHs3!Til^zki6k-XfYA*FTg-wU_{GBPiv77ugE$%B=^iKabx=2xMmi-%fn14 zj!lBVl2c`pNjh$9i`Ut;N$FWx0m|+x z<2p@ZXeMDie>f$A5W^#v!G_wcTH5Ug{88qTkPQ?KE<6!{o|bl(xnC;YvvpNZbLd6r_+bS?xBWux zg94@5P>Esh16Dx0<12PigG#Vvs_TboN$)STwDuFqAN&KlUSL(v{xY_|Y-V!&THp3J z<_9xmpzqJtZFz-@F9=z_|r4^?`eMf#aDQCj%XfUk#Ol&+5`yf`^Mbp-ae)_uSpM@2p zi~u7V>~8#L+ifEYt%%!T84gr6?73mvoE`t@$<#*fZtS}T^un#4*j0PEIXzp_UnuY8m3K#5j z4I*hDEfs$X@ynfO2s-0@R(6`h@|fX-vyxWyqGM|IFU&yiGp?3n6zbk)3qi4^j`9{W z-fa81eEP@*u8dgB=rrSzNXfW7=d4`Z=}ed_n6=AVr{MfkDP&(lC0Hr(+puraN*5+Q z0yvWwqke9umI=h3O=phGn0KbSS~_jms!Q*W!@|mDW~jeX`G{0swx*}x=LW@~TA=SD zxxF4+vx1uk8_8uKj>W?*SBy2kZziA07^n?w3iO_#TG_l#0C+#@$n;Zst-+&sdOck> zC>a-TEebLm;5wd;4erxP8mxaIFG@Hj%K|KQW_r>(^VXiJ<23C+jcS4ynk^foB{3*by1nAQYJNmGb}yrUqTM;}Mk83nPXx z$KJ1Q#fL3r%Bo;Bl{MTipnsUddZakzZM`KIqkCDSl_A8XoEn}uv)rN&+_|-j$jrFE z4#G)|OgaUoZyVXJ*yV_?8i8Qf4L$`LAbvW`<}wasqC}ufigx$4k$>Zir$ZE%09p(a z{@J>1@>9c6n_(WmdYJeIt`i-H3_4ZeKbFX`Vv{Vi!?hAz2T7ai;+Bu3y$DU0HouEq zBEDDv{m(V*>5G#QBCC#B3QoJmbw*XBDE-R7k`O`bu#)_o?{G%@g_n(|X^$8`J!r1{ zrZG7A7{knn;63K}TJ_*^Sz{T9ynD~~htWavKD2zh4H9#$4DWyhM&SxZ>vaWU8c z85juiZ9(r$WDxDEMjkl5Mw3XAs7dTD8jW%>bs#!bU;TcK2FRpPjuna&w2l*0{58Ay zZ~q>s8rry`WTWs;X2YkMw3+1hIp;en4XGfRGFGVm8AvYSCbOPk`+4o0W_U&EzXiXd zIM2yow&Y^m5xiQ2oKRDx$8$)!v8%z^GV)>P^rf8WTPipq4~9x7e?NX|*PqI*a+e5H z-bD5jE9#R?&vXUdcAllDAxh%MiU-