Merge pull request #2239 from crytic/dev-msgvalue-loop-fps

msg-value-loop: Don't report if msg.value is in a conditional expression

slither/detectors/statements/msg_value_in_loop.py

@@ -8,6 +8,9 @@
 from slither.slithir.operations import InternalCall
 from slither.core.declarations import SolidityVariableComposed, Contract
 from slither.utils.output import Output
+from slither.slithir.variables.constant import Constant
+from slither.core.variables import Variable
+from slither.core.expressions.literal import Literal
 
 
 def detect_msg_value_in_loop(contract: Contract) -> List[Node]:
@@ -37,6 +40,21 @@ def msg_value_in_loop(
 
     for ir in node.all_slithir_operations():
         if in_loop_counter > 0 and SolidityVariableComposed("msg.value") in ir.read:
+            # If we find a conditional expression with msg.value and is compared to 0 we don't report it
+            if ir.node.is_conditional() and SolidityVariableComposed("msg.value") in ir.read:
+                compared_to = (
+                    ir.read[1]
+                    if ir.read[0] == SolidityVariableComposed("msg.value")
+                    else ir.read[0]
+                )
+                if (
+                    isinstance(compared_to, Constant)
+                    and compared_to.value == 0
+                    or isinstance(compared_to, Variable)
+                    and isinstance(compared_to.expression, Literal)
+                    and str(compared_to.expression.value) == "0"
+                ):
+                    continue
             results.append(ir.node)
         if isinstance(ir, (InternalCall)):
             msg_value_in_loop(ir.function.entry_point, in_loop_counter, visited, results)

tests/e2e/detectors/test_data/msg-value-loop/0.4.25/msg_value_loop.sol

@@ -26,4 +26,38 @@ contract C{
         }
     }
 
+    function good1(address[] memory receivers) public payable {
+        require(msg.value == 0);
+        for (uint256 i = 0; i < receivers.length; i++) {
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good2(address[] memory receivers) public payable {
+        uint zero = 0;
+        for (uint256 i = 0; i < receivers.length; i++) {
+            assert(msg.value == zero);
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good3(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            if (0 != msg.value) {
+                revert();
+            }
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good4(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            _g();
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function _g() internal {
+        require(msg.value == 0);
+    }
 }

tests/e2e/detectors/test_data/msg-value-loop/0.5.16/msg_value_loop.sol

@@ -26,4 +26,38 @@ contract C{
         }
     }
 
+    function good1(address[] memory receivers) public payable {
+        require(msg.value == 0);
+        for (uint256 i = 0; i < receivers.length; i++) {
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good2(address[] memory receivers) public payable {
+        uint zero = 0;
+        for (uint256 i = 0; i < receivers.length; i++) {
+            assert(msg.value == zero);
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good3(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            if (0 != msg.value) {
+                revert();
+            }
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good4(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            _g();
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function _g() internal {
+        require(msg.value == 0);
+    }
 }

tests/e2e/detectors/test_data/msg-value-loop/0.6.11/msg_value_loop.sol

@@ -26,4 +26,38 @@ contract C{
         }
     }
 
+    function good1(address[] memory receivers) public payable {
+        require(msg.value == 0);
+        for (uint256 i = 0; i < receivers.length; i++) {
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good2(address[] memory receivers) public payable {
+        uint zero = 0;
+        for (uint256 i = 0; i < receivers.length; i++) {
+            assert(msg.value == zero);
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good3(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            if (0 != msg.value) {
+                revert();
+            }
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good4(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            _g();
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function _g() internal {
+        require(msg.value == 0);
+    }
 }

tests/e2e/detectors/test_data/msg-value-loop/0.7.6/msg_value_loop.sol

@@ -26,4 +26,38 @@ contract C{
         }
     }
 
+    function good1(address[] memory receivers) public payable {
+        require(msg.value == 0);
+        for (uint256 i = 0; i < receivers.length; i++) {
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good2(address[] memory receivers) public payable {
+        uint zero = 0;
+        for (uint256 i = 0; i < receivers.length; i++) {
+            assert(msg.value == zero);
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good3(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            if (0 != msg.value) {
+                revert();
+            }
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good4(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            _g();
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function _g() internal {
+        require(msg.value == 0);
+    }
 }

tests/e2e/detectors/test_data/msg-value-loop/0.8.0/msg_value_loop.sol

@@ -26,4 +26,38 @@ contract C{
         }
     }
 
+    function good1(address[] memory receivers) public payable {
+        require(msg.value == 0);
+        for (uint256 i = 0; i < receivers.length; i++) {
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good2(address[] memory receivers) public payable {
+        uint zero = 0;
+        for (uint256 i = 0; i < receivers.length; i++) {
+            assert(msg.value == zero);
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good3(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            if (0 != msg.value) {
+                revert();
+            }
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function good4(address[] memory receivers) public payable {
+        for (uint256 i = 0; i < receivers.length; i++) {
+            _g();
+            balances[receivers[i]] += 1;
+        }
+    }
+
+    function _g() internal {
+        require(msg.value == 0);
+    }
 }