Refactor Ether Send Module (#696)

* Refactor ether_send -> ether_thief and update tests

* Commit file change missed previously

* Remove unused import and fix formatting

* Fix expected output in Truffle test

mythril/analysis/modules/ether_thief.py

@@ -10,15 +10,21 @@ import logging
 """
 MODULE DESCRIPTION:
 
-Check for CALLs that send >0 Ether to either the transaction sender, or to an address provided as a function argument.
-If msg.sender is checked against a value in storage, check whether that storage index is tainted (i.e. there's an unconstrained write
-to that index).
+Search for cases where Ether can be withdrawn to a user-specified address. 
+
+An issue is reported ONLY IF:
+
+- The transaction sender does not match contract creator;
+- The sender has not previously sent any ETH to the contract account.
+
+This is somewhat coarse and needs to be refined in the future.
+
 """
 
 
 def execute(state_space):
 
-    logging.debug("Executing module: ETHER_SEND")
+    logging.debug("Executing module: ETHER_THIEF")
 
     issues = []
 
@@ -46,31 +52,42 @@ def _analyze_state(state, node):
         return []
 
     try:
+
+        """
+        FIXME: Instead of solving for call_value > 0, check whether call value can be greater than
+        the total value of all transactions received by the caller
+        """
+
         model = solver.get_model(
             node.constraints + not_creator_constraints + [call_value > 0]
         )
 
-        debug = "Transaction Sequence: " + str(
-            solver.get_transaction_sequence(
-                state, node.constraints + not_creator_constraints + [call_value > 0]
-            )
+        transaction_sequence = solver.get_transaction_sequence(
+            state, node.constraints + not_creator_constraints + [call_value > 0]
         )
 
+        # For now we only report an issue if zero ETH has been sent to the contract account.
+
+        for key, value in transaction_sequence.items():
+            if int(value["call_value"], 16) > 0:
+                return []
+
+        debug = "Transaction Sequence: " + str(transaction_sequence)
+
         issue = Issue(
             contract=node.contract_name,
             function_name=node.function_name,
             address=instruction["address"],
             swc_id=UNPROTECTED_ETHER_WITHDRAWAL,
-            title="Ether send",
+            title="Ether thief",
             _type="Warning",
             bytecode=state.environment.code.bytecode,
-            description="It seems that an attacker is able to execute an call instruction,"
-            " this can mean that the attacker is able to extract funds "
-            "out of the contract.".format(target),
+            description="Users other than the contract creator can withdraw ETH from the contract account"
+            + " without previously having sent any ETH to it. This is likely to be vulnerability.",
             debug=debug,
         )
         issues.append(issue)
     except UnsatError:
-        logging.debug("[UNCHECKED_SUICIDE] no model found")
+        logging.debug("[ETHER_THIEF] no model found")
 
     return issues

tests/cmd_line_test.py

@@ -29,7 +29,7 @@ class TruffleTestCase(BaseTestCase):
         command = "cd {}; truffle compile; python3 {} --truffle --max-transaction-count 1".format(
             truffle_project_root, MYTH
         )
-        self.assertIn("=== Ether send ====", output_of(command))
+        self.assertIn("=== Ether thief ====", output_of(command))
 
 
 class InfuraTestCase(BaseTestCase):

tests/testdata/outputs_expected/ether_send.sol.o.json

@@ -1 +1 @@
-{"error":null,"issues":[{"address":722,"contract":"Unknown","debug":"<DEBUG-DATA>","description":"It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.","function":"withdrawfunds()","swc-id":"105","title":"Ether send","type":"Warning"},{"address":883,"contract":"Unknown","debug":"<DEBUG-DATA>","description":"This binary add operation can result in integer overflow.\n","function":"invest()","swc-id":"101","title":"Integer Overflow","type":"Warning"}],"success":true}
+{"error": null, "issues": [{"address": 722, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.", "function": "withdrawfunds()", "swc-id": "105", "title": "Ether thief", "type": "Warning"}, {"address": 883, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "This binary add operation can result in integer overflow.\n", "function": "invest()", "swc-id": "101", "title": "Integer Overflow", "type": "Warning"}], "success": true}

tests/testdata/outputs_expected/ether_send.sol.o.markdown

@@ -1,6 +1,6 @@
 # Analysis results for test-filename.sol
 
-## Ether send
+## Ether thief
 - SWC ID: 105
 - Type: Warning
 - Contract: Unknown
@@ -9,7 +9,7 @@
 
 ### Description
 
-It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.
+Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.
 
 ## Integer Overflow
 - SWC ID: 101

tests/testdata/outputs_expected/ether_send.sol.o.text

@@ -1,10 +1,10 @@
-==== Ether send ====
+==== Ether thief ====
 SWC ID: 105
 Type: Warning
 Contract: Unknown
 Function name: withdrawfunds()
 PC address: 722
-It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.
+Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.
 --------------------
 
 ==== Integer Overflow ====

tests/testdata/outputs_expected/multi_contracts.sol.o.json

@@ -1 +1 @@
-{"error": null, "issues": [{"address": 142, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.", "function": "_function_0x8a4068dd", "swc-id": "105", "title": "Ether send", "type": "Warning"}], "success": true}
+{"error": null, "issues": [{"address": 142, "contract": "Unknown", "debug": "<DEBUG-DATA>", "description": "Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.", "function": "_function_0x8a4068dd", "swc-id": "105", "title": "Ether thief", "type": "Warning"}], "success": true}

tests/testdata/outputs_expected/multi_contracts.sol.o.markdown

@@ -1,6 +1,6 @@
 # Analysis results for test-filename.sol
 
-## Ether send
+## Ether thief
 - SWC ID: 105
 - Type: Warning
 - Contract: Unknown
@@ -9,4 +9,4 @@
 
 ### Description
 
-It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.
+Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.

tests/testdata/outputs_expected/multi_contracts.sol.o.text

@@ -1,9 +1,9 @@
-==== Ether send ====
+==== Ether thief ====
 SWC ID: 105
 Type: Warning
 Contract: Unknown
 Function name: _function_0x8a4068dd
 PC address: 142
-It seems that an attacker is able to execute an call instruction, this can mean that the attacker is able to extract funds out of the contract.
+Users other than the contract creator can withdraw ETH from the contract account without previously having sent any ETH to it. This is likely to be vulnerability.
 --------------------