v0.7.0

docs/curve_vyper_reentrancy.md

@@ -0,0 +1,26 @@
+# Curve Readonly Reentrancy
+
+## Configuration
+
+- Check: `pess-curve-vyper-reentrancy`
+- Severity: `High`
+- Confidence: `High`
+
+## Description
+
+Finds if the code is compiled with vulnerable Vyper compiler version and contains non-reentrant modifiers. 
+Details:
+- [Curve exploit postmortem](https://hackmd.io/@LlamaRisk/BJzSKHNjn)
+- [Postmortem from Vyper team](https://hackmd.io/@vyperlang/HJUgNMhs2)
+
+## Vulnerable Scenario
+
+[test scenarios](../../tests/vyper/curve_vyper_reentrancy_test.vy)
+
+## Related Attacks
+
+- [Vyper compiler exploits](https://www.halborn.com/blog/post/explained-the-vyper-bug-hack-july-2023)
+
+## Recomendations
+
+- Upgrade the version of your Vyper compiler.

docs/nft_approve_warning.md

@@ -6,7 +6,7 @@
 * Confidence: `Low`
 
 ## Description
-The detector sees if a contract contains `erc721.[safe]TransferFrom(from, ...)` where `from` parameter is not related to `msg.sender`.
+The detector sees if a contract contains `erc721.[safe]TransferFrom(from, ...)` or `erc1155.safe[Batch]TransferFrom(from, ...)` where `from` parameter is not related to `msg.sender`.
 An attacker can steal any approved NFTs because `transferFrom` function does NOT check that the call is made by its owner. 
 
 ## Vulnerable Scenario
@@ -17,4 +17,4 @@ An attacker can steal any approved NFTs because `transferFrom` function does NOT
 [Unauthorized transfer_from Vulnerability](https://ventral.digital/posts/2022/8/18/sznsdaos-bountyboard-unauthorized-transferfrom-vulnerability)
 
 ## Recommendation
-Make sure that in `erc721.[safe]TransferFrom(from, ...)` functions `from` parameter is related to `msg.sender`.
+Make sure that in `erc721.[safe]TransferFrom(from, ...)` and `erc1155.safe[Batch]TransferFrom(from, ...)` functions `from` parameter is related to `msg.sender`.

docs/price_manipulation.md

@@ -0,0 +1,15 @@
+# Price Manipulation through token transfers
+
+## Configuration
+* Check: `pess-price-manipulation`
+* Severity: `High`
+* Confidence: `Low`
+
+## Description
+The detector finds calculations that depend on the balance and supply of some token. Such calculations could be manipulated through direct transfers to the contract, increasing its balance.
+
+## Vulnerable Scenario
+[test scenario](../tests/price_manipulation_test.sol)
+
+## Recommendation
+Avoid possible manipulations of calculations because of external transfers.

slitherin/__init__.py

@@ -33,6 +33,8 @@
 from slitherin.detectors.potential_arith_overflow import PotentialArithmOverflow
 from slitherin.detectors.curve.curve_readonly_reentrancy import CurveReadonlyReentrancy
 from slitherin.detectors.balancer.balancer_readonly_reentrancy import BalancerReadonlyReentrancy
+from slitherin.detectors.vyper.reentrancy_curve_vyper_version import CurveVyperReentrancy
+from slitherin.detectors.price_manipulation import PriceManipulationDetector
 
 artbitrum_detectors = [
     ArbitrumPrevrandaoDifficulty,
@@ -67,6 +69,8 @@
     PotentialArithmOverflow,
     CurveReadonlyReentrancy,
     BalancerReadonlyReentrancy
+    CurveVyperReentrancy,
+    PriceManipulationDetector
 ]
 plugin_printers = []
 

slitherin/detectors/nft_approve_warning.py

@@ -2,9 +2,10 @@
 from typing import List
 from slither.core.cfg.node import Node
 from slither.core.declarations import Function, SolidityVariableComposed
+from slither.core.declarations.function_contract import FunctionContract
 from slither.analyses.data_dependency.data_dependency import is_dependent
 
-
+SAFE_ERC20_LIB_SIG = "safeTransferFrom(address,address,address,uint256)"
 class NftApproveWarning(AbstractDetector):
     """
     Sees if contract contains erc721.[safe]TransferFrom(from, ...) where from parameter is not related to msg.sender
@@ -16,12 +17,14 @@ class NftApproveWarning(AbstractDetector):
     CONFIDENCE = DetectorClassification.LOW
 
     WIKI = 'https://ventral.digital/posts/2022/8/18/sznsdaos-bountyboard-unauthorized-transferfrom-vulnerability'
-    WIKI_TITLE = 'NFT Approve Warning'
+    WIKI_TITLE = 'Token Approve Warning'
     WIKI_DESCRIPTION = "In [safe]TransferFrom() from parameter must be related to msg.sender"
     WIKI_EXPLOIT_SCENARIO = '-'
     WIKI_RECOMMENDATION = 'from parameter must be related to msg.sender'
 
-    _signatures=["transferFrom(address,address,uint256)", "safeTransferFrom(address,address,uint256,bytes)", "safeTransferFrom(address,address,uint256)"]
+    _signatures=["transferFrom(address,address,uint256)", "safeTransferFrom(address,address,uint256,bytes)", "safeTransferFrom(address,address,uint256)", 
+                 SAFE_ERC20_LIB_SIG, # SafeERC20.safeTransferFrom
+                 "safeTransferFrom(address,address,uint256,uint256,bytes)", "safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)"] # ERC1155 
 
     def _detect_arbitrary_from(self, f: Function):
         all_high_level_calls = [
@@ -48,7 +51,8 @@ def _arbitrary_from(self, nodes: List[Node]):
                     and hasattr(ir.function, 'solidity_signature')
                     and ir.function.solidity_signature in self._signatures
                 ):
-                    is_from_sender = is_dependent(ir.arguments[0], SolidityVariableComposed("msg.sender"), node.function.contract)
+                    is_from_sender = ir.function.solidity_signature !=  SAFE_ERC20_LIB_SIG and is_dependent(ir.arguments[0], SolidityVariableComposed("msg.sender"), node.function.contract) or \
+                                    ir.function.solidity_signature ==  SAFE_ERC20_LIB_SIG and is_dependent(ir.arguments[1], SolidityVariableComposed("msg.sender"), node.function.contract)
                     # is_from_self = is_dependent(ir.arguments[0], SolidityVariable("this"), node.function.contract)
                     if (not is_from_sender): # and not is_from_self
                         irList.append(ir.node)

slitherin/detectors/potential_arith_overflow.py

@@ -86,7 +86,7 @@ def _find_vulnerable_expressions(self, fun: Function) -> list:
                         errors.extend(response_errors)
                     if has_problems:
                         final_results.append((node, str(irs[-1].lvalue._type), errors))
-                if len(irs) > 0 and isinstance(irs[-1], ops.Return) and len(fun.return_type) == 1 and str(fun.return_type[0]) in INT_TYPES: # @todo currently works only with single returns
+                if len(irs) > 0 and isinstance(irs[-1], ops.Return) and fun.return_type is not None and len(fun.return_type) == 1 and str(fun.return_type[0]) in INT_TYPES: # @todo currently works only with single returns
                     expected_bits_cut = str(fun.return_type[0]).removeprefix("uint").removeprefix("int")
                     expected_bits = int(256 if not expected_bits_cut else expected_bits_cut)
                     has_problems = False
@@ -115,4 +115,4 @@ def _detect(self) -> List[Output]:
                         for (op, op_ret_type) in op_with_ret_type:
                             info += ["\t\t`", str(op), f"` returns {op_ret_type}, but the type of the resulting expression is {node_final_type}.", "\n"]
                     res.append(self.generate_result(info))
-        return res
+        return res

slitherin/detectors/price_manipulation.py

@@ -0,0 +1,71 @@
+from typing import List
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.slithir.operations.high_level_call import HighLevelCall
+from slither.slithir.operations.internal_call import InternalCall
+from slither.slithir.operations.solidity_call import SolidityCall
+from slither.slithir.operations.binary import Binary
+from slither.analyses.data_dependency.data_dependency import is_dependent
+
+
+class PriceManipulationDetector(AbstractDetector):
+    ARGUMENT = 'pess-price-manipulation' # slither will launch the detector with slither.py --detect mydetector
+    HELP = 'Contract math can be manipulated through external token transfers.'
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.LOW
+
+    WIKI = 'https://github.com/pessimistic-io/slitherin/blob/master/docs/price_manipulation.md'
+    WIKI_TITLE = '# Price Manipulation through token transfers'
+    WIKI_DESCRIPTION = "Calculations could be manipulated through direct transfers to the contract, increasing its balance as they depends on these balances."
+    WIKI_EXPLOIT_SCENARIO = 'N/A'
+    WIKI_RECOMMENDATION = 'Avoid possible manipulations of calculations because of external transfers.'
+
+    def _detect(self) -> List[Output]:
+        all_balance_vars = []
+        all_supply_vars = []
+        all_binary_ops = []
+        for contract in self.contracts:
+            if not contract.is_interface:
+                for func in contract.functions:
+                    for n in func.nodes:
+                        for x in n.irs:
+                            if isinstance(x, SolidityCall):
+                                if x.function.name == "balance(address)" or x.function.name == "self.balance" or x.function.name == "this.balance()":
+                                    all_balance_vars.append((n, x._lvalue))
+                            if isinstance(x, HighLevelCall):
+                                if str(x.function_name).lower() == "balanceof":
+                                    all_balance_vars.append((n, x._lvalue))
+                                if "supply" in str(x.function_name).lower():
+                                    all_supply_vars.append((n, x._lvalue))
+                            if isinstance(x, InternalCall):
+                                if "supply" in str(x.function_name).lower():
+                                    all_supply_vars.append((n, x._lvalue))
+                            if isinstance(x, Binary):
+                                all_binary_ops.append((n, x))
+        results = []
+        for (balance_node, bal) in all_balance_vars:
+            for (supply_node, supply) in all_supply_vars:
+                for (node, bin_op) in all_binary_ops:
+                    l, r = bin_op.get_variable
+                    is_bal_dependent = is_dependent(l, bal, contract)
+                    is_supply_dependent = is_dependent(r, supply, contract)
+                    if is_bal_dependent and is_supply_dependent:
+                        results.append((node, balance_node, supply_node))
+        if not results:
+            return []
+
+
+        response = []
+        for issue_node, balance_node, supply_node in results:
+            res = []
+            res.append("Calculation ")
+            res.append(issue_node)
+            res.append(" depends on balance and token supply - these values could be manipulated through external calls.\n")
+            res.append("\tBalance dependency: ")
+            res.append(balance_node)
+            res.append("\n")
+            res.append("\tSupply dependency: ")
+            res.append(supply_node)
+            res.append("\n")
+            response.append(self.generate_result(res))
+        return response

slitherin/detectors/strange_setter.py

@@ -3,6 +3,8 @@
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.core.declarations import Function
 import slither.core.expressions.new_array as na
+import slither.core.expressions.new_contract as nc
+from slither.analyses.data_dependency.data_dependency import is_dependent
 
 
 class StrangeSetter(AbstractDetector):
@@ -51,9 +53,18 @@ def _is_strange_setter(self, fun: Function) -> bool:
             for external in fun.external_calls_as_expressions:
                 if isinstance(external._called, na.NewArray):
                     continue
+                if isinstance(external._called, nc.NewContract): # skip new contract calls, idk how to get arguments passed to creation
+                    continue
                 for arg in [*external.arguments, external._called._expression]:
                     if str(arg) == str(param):
                         used_params.add(param)
+        if fun.name == "constructor":
+            for base_call in fun.explicit_base_constructor_calls:
+                if not self._is_strange_constructor(base_call):
+                    for param_cur in fun.parameters:
+                        for param_base in base_call.parameters:
+                            if is_dependent(param_base, param_cur, base_call):
+                                used_params.add(param_cur)
         intersection_len = len(set(fun.parameters) & used_params)
         return intersection_len != len(fun.parameters)
 

slitherin/detectors/vyper/reentrancy_curve_vyper_version.py

@@ -0,0 +1,40 @@
+from typing import List
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations import Function
+from slither.slithir.operations.event_call import EventCall
+
+VULNERABLE_VERSIONS = ['0.2.15', '0.2.16', '0.3.0']
+class CurveVyperReentrancy(AbstractDetector):
+    ARGUMENT = 'pess-curve-vyper-reentrancy' # slither will launch the detector with slither.py --detect mydetector
+    HELP = f'Vyper compiler versions {", ".join(VULNERABLE_VERSIONS)} are vulnerable to malfunctioning re-entrancy guards. Upgrade your compiler version.'
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = 'https://github.com/pessimistic-io/slitherin/blob/master/docs/curve_vyper_reentrancy.md'
+    WIKI_TITLE = 'Vulnerable Vyper version'
+    WIKI_DESCRIPTION = "Some Vyper versions are vulnerable to malfunctioning re-entrancy guards."
+    WIKI_EXPLOIT_SCENARIO = 'https://hackmd.io/@LlamaRisk/BJzSKHNjn'
+    WIKI_RECOMMENDATION = 'Upgrade the version of your Vyper compiler.'
+
+    def _detect(self) -> List[Output]:
+        res = []
+        if not self.compilation_unit.is_vyper:
+            return res
+        compiler_version = self.compilation_unit.compiler_version.version
+        if compiler_version not in VULNERABLE_VERSIONS:
+            return res
+
+        for contract in self.contracts:
+            for f in contract.functions_entry_points:
+                for modifier in f.modifiers:
+                    if modifier.name.startswith("nonreentrant"):
+                        res += ["\t", modifier.name, " in ", f, " function.\n"]
+        if not res:
+            return res
+        return [self.generate_result(
+            [f"Vyper {compiler_version} compiler version is vulnerable to malfunctioning re-entrancy guards. Found vulnerable usages:\n",
+             *[x for x in res],
+             "\n"
+            ]
+        )]

tests/nft_approve_warning_test.sol

@@ -36,3 +36,56 @@ contract Test
     }
 }
 
+interface IERC1155 {
+    function safeTransferFrom(address from, address to, uint256 id, uint256 value, bytes memory data) external;
+    function safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory values,
+        bytes memory data
+    ) external;
+}
+
+contract TestERC1155 {
+    function vuln_safeTransferFrom(address target, address from, address to) external {
+        IERC1155(target).safeTransferFrom(from, to, 1, 1, "");
+    }
+
+    function ok_safeTransferFrom(address target, address to) external {
+        IERC1155(target).safeTransferFrom(msg.sender, to, 1, 1, "");
+    }
+
+    function vuln_safeBatchTransferFrom(address target, address from, address to) external {
+        IERC1155(target).safeBatchTransferFrom(from, to, new uint256[](2), new uint256[](2), "");
+    }
+
+    function ok_safeBatchTransferFrom(address target, address to) external {
+        IERC1155(target).safeBatchTransferFrom(msg.sender, to, new uint256[](2), new uint256[](2), "");
+    }
+}
+
+
+library SafeERC20 {
+    function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
+        // _callOptionalReturn(token, abi.encodeCall(token.transferFrom, (from, to, value)));
+    }
+}
+
+contract TestSafeERC20LibCall {
+    function vuln1_safeErc20TransferFrom(IERC20 token, address from, address to, uint256 value) external {
+        SafeERC20.safeTransferFrom(token, from, to, value);
+    }
+
+    function ok1_safeErc20TransferFrom(IERC20 token, address to, uint256 value) external {
+        SafeERC20.safeTransferFrom(token, msg.sender, to, value);
+    }
+
+    function vuln2_safeErc20TransferFrom(address from, address to, uint256 value) external {
+        SafeERC20.safeTransferFrom(IERC20(msg.sender), from, to, value);
+    }
+
+    function ok2_safeErc20TransferFrom(address to, uint256 value) external {
+        SafeERC20.safeTransferFrom(IERC20(msg.sender), msg.sender, to, value);
+    }
+}

tests/price_manipulation_test.sol

@@ -0,0 +1,47 @@
+interface IERC20 {
+    function totalSupply() external view returns (uint256);
+    function balanceOf(address account) external view returns (uint256);
+}
+contract Test1 {
+    IERC20 token;
+
+    function test_vuln_1() external returns(uint256 price) {
+        price = token.balanceOf(address(this)) / token.totalSupply();
+    }
+
+    function test_vuln_2() external returns(uint256 price) {
+        uint256 bal = token.balanceOf(address(this));
+        uint256 supply = token.totalSupply();
+        price = bal / supply; 
+    }
+
+    function test_vuln_3() external returns(uint256 price) {
+        uint256 bal = getBalance();
+        price = bal / token.totalSupply();
+    }
+
+    function test_vuln_4() external returns(uint256 price) {
+        uint256 bal = getBalance();
+        price = 10 + (bal / (token.totalSupply() * 5));
+    }
+
+    function test_vuln_5() external returns(uint256 price) {
+        price = getBalance() / mySupply();
+    }
+
+    function test_vuln_6() external returns(uint256 price) {
+        price = getBalance() + mySupply() + 1;
+    }
+
+    function test_vuln_7() external returns(uint256 price) {
+        price = address(token).balance / mySupply();
+    }
+
+    function getBalance() public returns(uint256 bal) {
+        bal = token.balanceOf(msg.sender);
+    }
+
+    function mySupply() public returns (uint256) {
+        return 100;
+    }
+}

tests/strange_setter_test.sol

@@ -87,3 +87,13 @@ contract OkConstructor {
         init = true;
     }
 }
+
+contract TestInheritance is StrangeSetter{
+    constructor(uint256 _toSet) StrangeSetter(_toSet) {}
+}
+
+contract TestNewContract {
+    constructor(uint256 _toSet) {
+        new TestInheritance(_toSet);
+    }
+}

tests/vyper/curve_vyper_reentrancy_test.vy

@@ -0,0 +1,15 @@
+# @version =0.2.15
+
+@external
+@nonreentrant("hello_lock")
+def helloWorld() -> String[24]:
+    return "Hello World!"
+
+@external
+@nonreentrant("another_lock")
+def another_reentrant_func() -> uint256:
+    return 1
+
+@external
+def normal_func() -> uint256:
+    return 0