Merge pull request #91 from pessimistic-io/detector_balancer

Detector balancer

README.md

@@ -1,4 +1,4 @@
-#  Slitherin by Pessimistic.io
+# Slitherin by Pessimistic.io
 
 [![Blog](https://img.shields.io/badge/Blog-Link-blue?style=flat-square&logo=appveyor?logo=data:https://pessimistic.io/favicon.ico)](https://blog.pessimistic.io/)
 [![Our Website](https://img.shields.io/badge/By-pessimistic.io-green?style=flat-square&logo=appveyor?logo=data:https://pessimistic.io/favicon.ico)](https://pessimistic.io/)
@@ -99,6 +99,7 @@ Slitherin detectors are included into original Slither after the installation. Y
 | [Arbitrary Call](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/arbitrary_call/arbitrary_call.py)                        | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/arbitrary_call.md)                 | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/arbitrary_call_test.sol)                                                         | 0                                                                                                                |
 | [Elliptic Curve Recover](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/ecrecover.py)                        | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/ecrecover.md)                 | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/ecrecover.sol)                                                         | 0                                                                                                                |
 | [Public vs External](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/public_vs_external.py)                        | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/public_vs_external.md)                 | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/public_vs_external_test.sol)                                                         | 0                                                                                                                |
+| [Balancer Read-only Reentrancy](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/balancer/balancer_readonly_reentrancy.py)                        | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/balancer/readonly_reentrancy.md)                 | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/balancer/readonly_reentrancy_test.sol)                                                         | 0                                                                                                                |
 
 **Please note:**
 

docs/balancer/readonly_reentrancy.md

@@ -0,0 +1,24 @@
+# Balancer Readonly Reentrancy
+
+## Configuration
+
+- Check: `pess-balancer-readonly-reentrancy`
+- Severity: `High`
+- Confidence: `Medium`
+
+## Description
+
+Highlights the use of Balancer getter functions `getRate` and `getPoolTokens` (which are not checked for readonly reentrancy via `VaultReentrancyLib.ensureNotInVaultContext` or `IVault.manageUserBalance`), which return values that theoretically could be manipulated during the execution.
+
+## Vulnerable Scenario
+
+[test scenarios](../../tests/balancer/readonly_reentrancy_test.sol)
+
+## Related Attacks
+
+- [Sentimentxyz Exploit](https://quillaudits.medium.com/decoding-sentiment-protocols-1-million-exploit-quillaudits-f36bee77d376)
+- [Sturdy Exploit](https://blog.solidityscan.com/sturdy-finance-hack-analysis-bd8605cd2956)
+
+## Recommendation
+
+- [Official Balancer recomendation](https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#on-chain-price-evaluation)

package.json

@@ -1,5 +1,8 @@
 {
   "dependencies": {
-    "@openzeppelin/contracts": "^4.9.3"
+    "@openzeppelin/contracts": "^4.9.3",
+    "@balancer-labs/v2-interfaces": "^0.4.0",
+    "@balancer-labs/v2-pool-utils": "^4.0.0"
+
   }
 }

slitherin/__init__.py

@@ -32,6 +32,7 @@
 from slitherin.detectors.arbitrum.arbitrum_chainlink_price_feed import ArbitrumChainlinkPriceFeed
 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
 
@@ -67,6 +68,7 @@
     AAVEFlashloanCallbackDetector,
     PotentialArithmOverflow,
     CurveReadonlyReentrancy,
+    BalancerReadonlyReentrancy,
     CurveVyperReentrancy,
     PriceManipulationDetector
 ]

slitherin/detectors/balancer/balancer_readonly_reentrancy.py

@@ -0,0 +1,114 @@
+from typing import List
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations import Function, Contract
+from slither.core.cfg.node import Node
+
+
+class BalancerReadonlyReentrancy(AbstractDetector):
+    """
+    Sees if a contract has a beforeTokenTransfer function.
+    """
+
+    ARGUMENT = "pess-balancer-readonly-reentrancy"  # slither will launch the detector with slither.py --detect mydetector
+    HELP = "Balancer readonly-reentrancy"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/pessimistic-io/slitherin/blob/master/docs/balancer/readonly_reentrancy.md"
+    WIKI_TITLE = "Balancer Readonly Reentrancy"
+    WIKI_DESCRIPTION = "Check docs"
+    WIKI_EXPLOIT_SCENARIO = "-"
+    WIKI_RECOMMENDATION = "Check docs"
+
+    VULNERABLE_FUNCTION_CALLS = ["getRate", "getPoolTokens"]
+    visited = []
+    contains_reentrancy_check = {}
+
+    def is_balancer_integration(self, c: Contract) -> bool:
+        """
+        Iterates over all external function calls, and checks the interface/contract name
+        for a specific keywords to decide if the contract integrates with balancer
+        """
+        for (
+            fcontract,
+            _,
+        ) in c.all_high_level_calls:
+            contract_name = fcontract.name.lower()
+            if any(map(lambda x: x in contract_name, ["balancer", "ivault", "pool"])):
+                return True
+
+    def _has_reentrancy_check(self, node: Node) -> bool:
+        if node in self.visited:
+            return self.contains_reentrancy_check[node]
+
+        self.visited.append(node)
+        self.contains_reentrancy_check[node] = False
+
+        for c, n in node.high_level_calls:
+            if isinstance(n, Function):
+                if not n.name:
+                    continue
+                if (
+                    n.name == "ensureNotInVaultContext"
+                    and c.name == "VaultReentrancyLib"
+                ) or (
+                    n.name == "manageUserBalance"
+                ):  # TODO check if errors out
+                    self.contains_reentrancy_check[node] = True
+                    return True
+
+        has_check = False
+        for internal_call in node.internal_calls:
+            if isinstance(internal_call, Function):
+                has_check |= self._has_reentrancy_check(internal_call)
+                # self.contains_reentrancy_check[internal_call] |= has_check
+
+        self.contains_reentrancy_check[node] = has_check
+        return has_check
+
+    def _check_function(self, function: Function) -> list:
+        has_dangerous_call = False
+        dangerous_call = None
+        for n in function.nodes:
+            for c, fc in n.high_level_calls:
+                if isinstance(fc, Function):
+                    if fc.name in self.VULNERABLE_FUNCTION_CALLS:
+                        dangerous_call = n  # Saving only first dangerous call
+                        has_dangerous_call = True
+                        break
+
+        if has_dangerous_call and not any(
+            [self._has_reentrancy_check(node) for node in function.nodes]
+        ):
+            return [dangerous_call]
+        return []
+
+    def _detect(self) -> List[Output]:
+        """Main function"""
+        result = []
+        for contract in self.compilation_unit.contracts_derived:
+            if not self.is_balancer_integration(contract):
+                continue
+            res = []
+            for f in contract.functions_and_modifiers_declared:
+                function_result = self._check_function(f)
+                if function_result:
+                    res.extend(function_result)
+            if res:
+                info = [
+                    "Balancer readonly-reentrancy vulnerability detected in ",
+                    contract,
+                    ":\n",
+                ]
+                for r in res:
+                    info += [
+                        "\tThe answer of ",
+                        r,
+                        " call could be manipulated through readonly-reentrancy\n",
+                    ]
+                res = self.generate_result(info)
+                res.add(r)
+                result.append(res)
+
+        return result

test_balancer.bash

@@ -0,0 +1 @@
+slither tests/balancer/readonly_reentrancy_test.sol --detect pess-balancer-readonly-reentrancy --solc-remaps @=node_modules/@

tests/balancer/readonly_reentrancy_test.sol

@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.18;
+import {VaultReentrancyLib} from "@balancer-labs/v2-pool-utils/contracts/lib/VaultReentrancyLib.sol";
+import "@balancer-labs/v2-interfaces/contracts/vault/IVault.sol";
+
+interface IBalancerPool {
+    function getRate() external view returns (uint);
+}
+
+contract BalancerIntegration {
+    address balancerVault;
+
+    constructor() {}
+
+    function getPriceVulnerable(address vault) public returns (uint) {
+        return IBalancerPool(vault).getRate();
+    }
+
+    function getPriceVulnerable2(address vault) public {
+        bytes32 poolId = "0x123";
+        uint256[] memory balances = new uint256[](10);
+        (, balances, ) = IVault(balancerVault).getPoolTokens(poolId);
+    }
+
+    function _ensureNotReentrant() internal {
+        VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+    }
+
+    function _ensureNotReentrant2() internal {
+        IVault(balancerVault).manageUserBalance(new IVault.UserBalanceOp[](0));
+    }
+
+    function getPriceOk(address vault) public returns (uint) {
+        VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+        return IBalancerPool(vault).getRate();
+    }
+
+    function getPriceOk2(address vault) public returns (uint) {
+        _ensureNotReentrant();
+        return IBalancerPool(vault).getRate();
+    }
+
+    function getPriceOk3(address vault) public returns (uint) {
+        _ensureNotReentrant2();
+        return IBalancerPool(vault).getRate();
+    }
+
+    function getPriceOk4(address vault) public returns (uint) {
+        uint a = IBalancerPool(vault).getRate();
+        _ensureNotReentrant2();
+        return a;
+    }
+
+    function getPriceOk5(address vault) public {
+        VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+        bytes32 poolId = "0x123";
+        uint256[] memory balances = new uint256[](10);
+        (, balances, ) = IVault(balancerVault).getPoolTokens(poolId);
+    }
+}