Detect when ether is sent in Yul (#1909)

* Detect when ether is sent in Yul

* address pylint warning instead of supressing

* fix guard clause

* remove branch to appease pylint

* check that call can send eth prior to reading args

---------

Co-authored-by: alpharush <[email protected]>

slither/detectors/attributes/locked_ether.py

@@ -3,7 +3,7 @@
 """
 from typing import List
 
-from slither.core.declarations.contract import Contract
+from slither.core.declarations import Contract, SolidityFunction
 from slither.detectors.abstract_detector import (
     AbstractDetector,
     DetectorClassification,
@@ -17,7 +17,9 @@
     NewContract,
     LibraryCall,
     InternalCall,
+    SolidityCall,
 )
+from slither.slithir.variables import Constant
 from slither.utils.output import Output
 
 
@@ -68,8 +70,28 @@ def do_no_send_ether(contract: Contract) -> bool:
                         ):
                             if ir.call_value and ir.call_value != 0:
                                 return False
-                        if isinstance(ir, (LowLevelCall)):
-                            if ir.function_name in ["delegatecall", "callcode"]:
+                        if isinstance(ir, (LowLevelCall)) and ir.function_name in [
+                            "delegatecall",
+                            "callcode",
+                        ]:
+                            return False
+                        if isinstance(ir, SolidityCall):
+                            call_can_send_ether = ir.function in [
+                                SolidityFunction(
+                                    "delegatecall(uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                                SolidityFunction(
+                                    "callcode(uint256,uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                                SolidityFunction(
+                                    "call(uint256,uint256,uint256,uint256,uint256,uint256,uint256)"
+                                ),
+                            ]
+                            nonzero_call_value = call_can_send_ether and (
+                                not isinstance(ir.arguments[2], Constant)
+                                or ir.arguments[2].value != 0
+                            )
+                            if nonzero_call_value:
                                 return False
                         # If a new internal call or librarycall
                         # Add it to the list to explore

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_4_25_locked_ether_sol__0.txt

@@ -1,5 +1,10 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#37) has payable functions:
+	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#4-6)
+	But does not have a function to withdraw the ether
+
+Contract locking ether found:
+	Contract UnlockedAssembly (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#27-35) has payable functions:
 	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_5_16_locked_ether_sol__0.txt

@@ -1,5 +1,10 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#37) has payable functions:
+	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#4-6)
+	But does not have a function to withdraw the ether
+
+Contract locking ether found:
+	Contract UnlockedAssembly (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#27-35) has payable functions:
 	 - Locked.receive() (tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_6_11_locked_ether_sol__0.txt

@@ -1,5 +1,5 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#36) has payable functions:
 	 - Locked.receive_eth() (tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/snapshots/detectors__detector_LockedEther_0_7_6_locked_ether_sol__0.txt

@@ -1,5 +1,5 @@
 Contract locking ether found:
-	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#26) has payable functions:
+	Contract OnlyLocked (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#36) has payable functions:
 	 - Locked.receive_eth() (tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol#4-6)
 	But does not have a function to withdraw the ether
 

tests/e2e/detectors/test_data/locked-ether/0.4.25/locked_ether.sol

@@ -23,4 +23,15 @@ contract Unlocked is Locked, Send{
 
 }
 
+// Still reported because solidity < 0.6.0 doesn't have assembly in the AST
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.5.16/locked_ether.sol

@@ -23,4 +23,15 @@ contract Unlocked is Locked, Send{
 
 }
 
+// Still reported because solidity < 0.6.0 doesn't have assembly in the AST
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.6.11/locked_ether.sol

@@ -23,4 +23,14 @@ contract Unlocked is Locked, Send{
 
 }
 
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }

tests/e2e/detectors/test_data/locked-ether/0.7.6/locked_ether.sol

@@ -23,4 +23,14 @@ contract Unlocked is Locked, Send{
 
 }
 
+contract UnlockedAssembly is Locked{
+
+    function withdraw() public {
+        assembly {
+            let success := call(gas(), caller(),100,0,0,0,0)    
+        }
+    }
+
+}
+
 contract OnlyLocked is Locked{ }